Toxaris / pts

Interpreter for functional pure type systems.
BSD 3-Clause "New" or "Revised" License
21 stars 7 forks source link

Memory leak #72

Closed Blaisorblade closed 9 years ago

Blaisorblade commented 9 years ago

As discussed with @Toxaris, we leak memory during processing. Relevant data should now be tracked in this issue.

Blaisorblade commented 9 years ago

I'm going with bisection.

Testing Blaisorblade/pts@1c19757fdc6d156d326ae9b2dfb1c7a84c4f37a1 confirms that there was no memory leak on stlc.lfo when we last worked on it.

Blaisorblade commented 9 years ago

@Toxaris, bisection proved that the leak was introduced in 392d3ed3fc0ea4af7f651b5b1549adf95a475952 — "Execute processStmt in the Writer monad to collect bindings".

I think the next step should be to use heap profiling there (or nearby) to figure out the case of the leak. To compile it, start from my wip/old-bisect-profile-7 branch, it includes some extra fixes to make it work. (You can compare it with wip/old-bisect-profile-9, which does not exhibit the leak here).

However, some of the facts we found don't hold there: -hb reports that the leak is in VOID state, not in LAG state. This makes sense, because this commit does not yet use the extra state computed with the writer monad (in fact, it does not even write it).

Blaisorblade commented 9 years ago

Apparently Writer.Strict just doesn't work:

-- Although the output is built strictly, it is not possible to
-- achieve constant space behaviour with this transformer: for that,
-- use "Control.Monad.Trans.State.Strict" instead.
Blaisorblade commented 9 years ago

@Toxaris, is that enough for a fix? We tried switching to Writer.Strict, but the comment suggest that might not work. The comment is a bit too terse for me. I'm reading up on this, but can't say anything intelligent.

My best guess is that the second component of the pair is never forced. In fact, AFAIK the only difference between http://code.haskell.org/~ross/transformers/Control/Monad/Trans/Writer/Lazy.hs and http://code.haskell.org/~ross/transformers/Control/Monad/Trans/Writer/Strict.hs is that the pair itself is matched lazily, not that the second component is forced via seq.

A (different?) variant of the problem appears here.

Toxaris commented 9 years ago

Cool. Looking into it. Sounds like we should use StateT to collect the bindings for the module, not WriterT.

Blaisorblade commented 9 years ago

That might not be enough. From State.Strict: -- In this version, sequencing of computations is strict (but computations -- are not strict in the state unless you force it with 'seq' or the like).

What do you think of the following idea for Writer.Stricter? This would evaluate mappend, instead of building a thunk for it.

    m >>= k  = WriterT $ do
        (a, w)  <- runWriterT m
        (b, w') <- runWriterT (k a)
-       return (b, w `mappend` w')
+       let w'' = w `mappend` w'
+       return $ w'' `seq` (b, w'')
Toxaris commented 9 years ago

I don't understand the proposed code. Do you mean return (w''seq(b, w'')) or w''seqreturn (b, w'') or yet another thing?

Blaisorblade commented 9 years ago

The first; I have now fixed the code (with a dollar).

Toxaris commented 9 years ago

The proposed implementation seems to be not tail recursive, but I think the following implementation of Writer in terms of a state monad would be:

newtype WriterT m w a = W {runWriterT :: w -> m (a, w)}

return :: (Monad m, Monoid w) => a -> WriterT m w a
return x = WriterT $ \w0 -> return (x, w0)

(>>=) :: Monad m => WriterT m w a -> (a -> WriterT m w b) -> WriterT m w b
m >>= k = WriterT $ \w0 -> do
    (a, w1) <- runWriterT m w0
    runWriterT (k a) w1

tell :: (Monad m, Monoid w) => w -> WriterT m w ()
tell w = WriterT $ \w0 -> return (w0 `mappend` w, ())

Note that this code only calls mappend once per tell, not once per >>=.

Blaisorblade commented 9 years ago

I guess yes, but it's still too lazy. Adapting from the stackoverflow link from before:

tell :: (MonadState w m, Monoid w) => w -> m ()
tell w = get >>= \w0 -> put $! w0 `mappend` w
Toxaris commented 9 years ago

Why is it too lazy? What's wrong with building a thunk that is linear in the number of exported bindings? (ok, it is not perfect, but it is not a big deal either).

Blaisorblade commented 9 years ago

We agreed that's different from building a thunk which is linear in the number of executed statements.