Fix GHCi space leaks (#15111)
ClosedPublic

Authored by simonmar on May 2 2018, 5:19 AM.

Details

Summary

There were a number of leaks causing previously loaded modules to be
retained after a new :load. This fixes enough leaks to get the
tests to pass from D4658.

Test Plan

See new tests in D4658

simonmar created this revision.May 2 2018, 5:19 AM
simonmar requested review of this revision.May 2 2018, 6:59 AM
bgamari added inline comments.May 2 2018, 8:38 PM
compiler/typecheck/TcRnTypes.hs
271

I'll admit I'm a bit unclear as to why precisely this set of fields must be banged. Was this set arrived at empirically or is there are a deeper reason why the others can't be problematic that I am missing?

simonmar added inline comments.May 3 2018, 4:23 AM
compiler/typecheck/TcRnTypes.hs
271

It's impossible to explain why this set of fields needs to be strict, because the rationale is of the form "the field points to a thunk from line N of TcRnDriver.hs which refers to a thunk from line Y of HscTypes.hs which refers to the HscEnv". To find these I used gdb with the findPtr() utility in rts/Printer.c. Retainer profiling is supposed to solve this kind of problem but in practice it doesn't.

So, while scattering some strict fields around clearly isn't ideal, it could be worse.

  • A strict field rules out a class of space leaks, which is better than adding bang patterns or strictness in key places.
  • We have tests to tell us when it breaks.

An alternative would be to do what we do in the Core-to-core passes and deeseq the whole thing, but this is a complex set of datatypes and writing manual seq functions for all of them would be a pain, not to mention the overhead of traversing everything.

bgamari accepted this revision.May 3 2018, 4:21 PM

Whatever works :)

compiler/typecheck/TcRnTypes.hs
271

Right, this is what I feared.

Fair enough.

This revision is now accepted and ready to land.May 3 2018, 4:21 PM
simonmar updated this revision to Diff 16347.May 9 2018, 7:46 AM
  • rebase
  • accept new stats result for T13701

I wanted to check the impact on compiler perf, so full nofib results: P180 (ignore the runtime results, I think my laptop was busy doing other things during one of the runs)

Bottom line: compiler allocations +0.1%, I think that's OK. It had an adverse effect on one of the compiler perf tests (T13701, +13% allocs) but I think that test is non-idiomatic in that it generates a lot of modules with no content, so it was probably benefiting from some of the lazily evaluated things that were made strict here. The time to run the test wasn't measurably different after this diff.

simonmar added inline comments.
compiler/iface/LoadIface.hs
445–471

@ezyang @simonpj I'd like your thoughts here. Hopefully it's clear from the comment above what I'm trying to achieve - this space leak is a complete show-stopper for us and I need to fix it somehow, but the fix here breaks a few Backbpack tests, e.g. see build results B45428. I think the reason is (please correct me if I'm wrong):

  • An interface in another package might refer to holes
  • When we're compiling and we read an interface with holes, the modules instantiating those holes are in the *home* package, ie. in the HPT. So we have interfaces in the EPS that need interfaces from the HPT, which breaks the assumption I was making in the fix here.

Any ideas on how we can modify this fix so that it doesn't break Backpack? Thoughts I had:

  • filter out from the HPT all the modules that are not instantiating holes. But filtering is an extra cost, and I'm not sure how to know whether a module is instantiating a hole or not.
  • Just don't do this if we're doing something Backback-ish. But that's a horrible solution and will reintroduce the space leak for some use cases. I don't even know if we can easily decide whether we're doing something Backpack-ish.

I'm pretty unfamiliar with the Backpack implementation so I'd really appreciate any pointers you have. Thanks!

simonmar updated this revision to Diff 16382.May 13 2018, 2:49 AM

add a hack to fix backpack

simonmar updated this revision to Diff 16386.May 13 2018, 3:20 PM

update haddock perf stats

simonmar updated this revision to Diff 16404.May 14 2018, 2:43 AM

comment fix

This revision was automatically updated to reflect the committed changes.