Fix space leak in BinIface.getSymbolTable
ClosedPublic

Authored by duog on Oct 24 2017, 9:25 PM.

Details

Summary

Replace a call to mapAccumR, which uses linear stack space, with a gadget that
uses constant space.

Remove an unused parameter from fromOnDiskName.

The tests T1292_imports and T4239 are now reporting imported names in a
different order. I don't completely understand why, but I presume it is because
the symbol tables are now read more strictly. The new order seems better in
T1792_imports, and equally random in T4239.

There are several performance test improvements.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
duog created this revision.Oct 24 2017, 9:25 PM
bgamari accepted this revision.Oct 24 2017, 10:14 PM

Good catch! I suspect the ordering changes are fine.

This revision is now accepted and ready to land.Oct 24 2017, 10:14 PM

@duog IIRC haddock uses the same logic to deserialise symbol tables! Could you port your patch over?

duog added a comment.Oct 25 2017, 1:00 AM

@alexbiehl: Yes I saw that while grepping and I do intend to port the patch once it validates.

Closed by commit rGHC1c15d8ed112b: Fix space leak in BinIface.getSymbolTable (authored by Douglas Wilson <douglas.wilson@gmail.com>, committed by bgamari). · Explain WhyOct 25 2017, 3:09 PM
This revision was automatically updated to reflect the committed changes.

That is embarrassing imperative code for something as nice as a mapAccumR. Is there no way nice functional way of implementing this?

duog added a comment.Oct 25 2017, 9:56 PM

That is embarrassing imperative code for something as nice as a mapAccumR. Is there no way nice functional way of implementing this?

I agree it's not great. I'll have another think about it.

In D4124#115386, @duog wrote:

That is embarrassing imperative code for something as nice as a mapAccumR. Is there no way nice functional way of implementing this?

I agree it's not great. I'll have another think about it.

If you extract all the ST and StateT stuff into a generic helper function, what is the type of that? That might be useful even if the implementation stays like this, as the type signature of the helper (arrayMapAccumR?) would help the reader understand this code.