Opened 3 years ago

Closed 2 years ago

#7702 closed bug (fixed)

Memory Leak in CoreM (CoreWriter)

Reported by: afarmer Owned by:
Priority: normal Milestone: 7.8.1
Component: Compiler Version: 7.6.2
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

While running HERMIT on extended examples, we noticed that the CoreWriter component of CoreM is building up unevaluated SimplCount expressions. Attached is a one line patch to make the field in the CoreWriter record strict, which fixes the leak.

Also attached are heap profiles before and after to verify the fix. (You'll notice we have other memory leaks in HERMIT to address, the important bit being the big CoreWriter peak is gone in after.pdf.) These were generated by running a large number of HERMIT transformations in a batch, with HERMIT calling ghc with +RTS -hT -RTS.

I'm not sure how this affects compilation time or allocation when SimplCount is not the VerySimplCount constructor. Would the nofib suite answer these questions?

Attachments (7)

before.pdf (12.6 KB) - added by afarmer 3 years ago.
Heap Profile Before Patch
after.pdf (13.0 KB) - added by afarmer 3 years ago.
Heap Profile After Patch
Fix-memory-leak-in-CoreWriter-Trac-7702.patch (716 bytes) - added by afarmer 3 years ago.
Patch
before-second-patch.pdf (13.5 KB) - added by afarmer 2 years ago.
after-second-patch.pdf (21.4 KB) - added by afarmer 2 years ago.
Fix-thunk-leak-in-CoreM-s-CoreWriter-7702.patch (912 bytes) - added by afarmer 2 years ago.
second patch, fixing thunk leak
Performance-test-for-Trac-7702.patch (6.8 KB) - added by afarmer 2 years ago.
Patch to testsuite that adds a performance test for this space leak

Download all attachments as: .zip

Change History (24)

Changed 3 years ago by afarmer

Heap Profile Before Patch

Changed 3 years ago by afarmer

Heap Profile After Patch

Changed 3 years ago by afarmer

Patch

comment:1 Changed 3 years ago by afarmer

  • Status changed from new to patch

comment:2 Changed 2 years ago by anfarmer@…

commit 8bac590554087adb89d863dd55501c5e7aa25c2c

Author: Andrew Farmer <[email protected]>
Date:   Mon Feb 18 03:32:35 2013 -0600

    Fix memory leak in CoreWriter (Trac #7702)

 compiler/simplCore/CoreMonad.lhs |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

comment:3 Changed 2 years ago by simonpj@…

commit 458c653a795ea06e7cbd24872e9961711f7044e8

Author: Simon Peyton Jones <[email protected]>
Date:   Fri Mar 1 17:55:07 2013 +0000

    Comment the fix to Trac #7702

 compiler/simplCore/CoreMonad.lhs |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

comment:4 Changed 2 years ago by simonpj

  • difficulty set to Unknown
  • Resolution set to fixed
  • Status changed from patch to closed

Thanks very much for the fix! I've just committed it.

Simon

Changed 2 years ago by afarmer

Changed 2 years ago by afarmer

comment:5 Changed 2 years ago by afarmer

  • Resolution fixed deleted
  • Status changed from closed to new

Turns out that the thunk leak in after.pdf (and before-second-patch.pdf) was also due to CoreM. Attached is another patch to fix this. Running the same test as before, you can see that the thunk leak disappears in after-second-patch.pdf.

I also tested with _only_ the second patch, and the leak is still present, so both are necessary.

comment:6 Changed 2 years ago by afarmer

  • Status changed from new to patch

comment:7 Changed 2 years ago by ezyang

In the second patch, you should either use liftIO $ evaluate w3, or move the seq inside the return, on the principle of #5129. I think both would eliminate the space leak but I'm not sure which is more semantically correct in this case.

comment:8 Changed 2 years ago by simonpj

Would you like to update the patch in the light of Edward's suggestion, check that it works, and re-submit?

Also if either or both of you (or someone else) would like to improve http://www.haskell.org/haskellwiki/Performance, notably the "space leak" sub-page (deserves a top level heading of its own), that would be great. Specifically, the lore of #5129 belongs there, not buried in a closed ticket.

I'm sure there are other articles about looking for space leaks that could usefully be linked to from that space leak page, like Neil Mitchell's recent blog post.

Thanks!

Simon

comment:9 Changed 2 years ago by afarmer

Both:

    let w3 = w1 `plusWriter` w2
    return $ w3 `seq` (y, s'', w3)

and:

    let w3 = w1 `plusWriter` w2
    liftIO $ evaluate w3
    return (y, s'', w3)

will fix the leak. I'm inclined to the former, as the latter requires adding an extra import for evaluate itself. Let me know which is preferable and I'll upload a new patch.

Can the testsuite check for things like maximum residency? If so, I can also contribute a minimal test case to avoid future regressions.

Edward: could you address the documentation that Simon mentioned? I admit I found this mostly with lots of trial and error, and feel like you may be better at explaining #5129 than me. ;-)

comment:10 Changed 2 years ago by simonpj

I don't mind which.

Re testsuite, yes... look in tests/perf. Look under "Performance tests" on http://hackage.haskell.org/trac/ghc/wiki/Building/RunningTests/Adding.

Simon

Changed 2 years ago by afarmer

second patch, fixing thunk leak

comment:11 Changed 2 years ago by afarmer

Sorry for the delay. Here is the patch with the seq inside the return, as recommended by ezyang. This is confirmed to fix the leak (in conjunction with the first patch you already merged) in our HERMIT use cases.

I'm working on a minimal performance test for the testsuite... I'll post here when I have something.

comment:12 Changed 2 years ago by afarmer

Note about the testsuite patch: I developed this on a 64-bit machine and picked the max allocation numbers accordingly. If someone has access to a 32-bit machine, the baseline/deviation in all.T should probably be altered for the 32-bit case.

Changed 2 years ago by afarmer

Patch to testsuite that adds a performance test for this space leak

comment:13 Changed 2 years ago by simonpj

  • Owner set to igloo

Great, thanks. Ian can you check and commit?

comment:14 Changed 2 years ago by igloo

  • Milestone set to 7.8.1

comment:15 Changed 2 years ago by igloo

  • Resolution set to fixed
  • Status changed from patch to closed

Applied, thanks!

comment:16 Changed 2 years ago by afarmer

  • Owner igloo deleted
  • Resolution fixed deleted
  • Status changed from closed to new

It doesn't look like all of the performance test got committed. Only the changes to tests/simplCore/should_compile/all.T appear in the commit in the testsuite. None of the new files (like those in the T7702plugin directory) appear.

comment:17 Changed 2 years ago by igloo

  • Resolution set to fixed
  • Status changed from new to closed

Thanks; they should all be added now.

Note: See TracTickets for help on using tickets.