sizeExpr: fix a bug in the size calculation
ClosedPublic

Authored by simonmar on Feb 10 2016, 3:54 AM.

Details

Summary

There were two bugs here:

  • We weren't ignoring Cast in size_up_app
  • An application of a non-variable wasn't being charged correctly

The result was that some things looked too cheap. In my case I had
things like

((f x) `cast` ...) y

which was given size 21 instead of 30, and this had knock-on effects
elsewhere that caused some large code bloat.

Test Plan
  • nofib runs (todo)
  • validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
ghci-ccs
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8506
Build 10551: arc lint + arc unit
simonmar updated this revision to Diff 6602.Feb 10 2016, 3:54 AM
simonmar retitled this revision from to sizeExpr: fix a bug in the size calculation.
simonmar updated this object.
simonmar edited the test plan for this revision. (Show Details)
simonmar added a reviewer: simonpj.
simonmar updated the Trac tickets for this revision.
simonmar updated this object.Feb 10 2016, 3:55 AM
simonmar edited edge metadata.
simonmar updated this revision to Diff 6603.Feb 10 2016, 3:56 AM

remove debugging code

simonmar added inline comments.Feb 10 2016, 3:58 AM
compiler/coreSyn/CoreUnfold.hs
581

Adding 1 and multiplying by 10 was missing here

587

subtracing voids was missing here

nofib results:

--------------------------------------------------------------------------------
        Program           Size    Allocs   Runtime   Elapsed  TotalMem
--------------------------------------------------------------------------------
      cacheprof          +0.8%     -0.1%     -5.5%     -5.4%     -1.8%
   k-nucleotide          +0.4%   +106.7%     +3.4%     +3.3%      0.0%
--------------------------------------------------------------------------------
            Min          +0.4%     -0.1%    -11.5%    -11.6%    -25.0%
            Max          +1.0%   +106.7%     +9.6%     +9.9%     +2.9%
 Geometric Mean          +0.8%     +0.7%     -2.4%     -2.5%     -0.3%

There was one major regression: k-nucleotide. I'll look into that.

Binary sizes look to be up by 1%, but my guess is that since it's so consistent, this is a single .o file in a package that got pulled in because we didn't inline something from it, and it would go away with -split-objs.

The runtime differences are probably noise, I'll check.

simonpj accepted this revision.Feb 10 2016, 10:36 AM
simonpj edited edge metadata.

Good bug!

Gotta check k-nucleotide, indeed. And worth knowing where that 1% size increase comes from, or at least if split-objs makes it go away.

compiler/coreSyn/CoreUnfold.hs
666

Better to pass n_voids to callSize and do the subtraction here; that way the caller can't forget, as indeed happened above! Saves duplication too.

This revision is now accepted and ready to land.Feb 10 2016, 10:36 AM

I checked k-nucleotide, and it's too convoluted for me to figure out what changed. I did ticky-ticky runs and dumped the core output:

  • Ticky with HEAD: P92
  • Ticky with this patch: P93
  • Core with HEAD: P90
  • Core with this patch: P91

If you use -funfolding-use-threshold=130 (default is 60) then performance goes back to where it was before.

I'm not inclined to worry too much if this program was depending on this bug to get good performance. It already has plenty of INLINE pragmas, maybe it just needs more.

Binary sizes go down to +0.4% with -split-objs. I think this is probably because we're pulling in some whole objects rather than inlining small bits of them, which is ok. Allocations generally go down a tiny bit apart from k-nucleotide.

simonmar updated this revision to Diff 6613.Feb 10 2016, 4:24 PM
simonmar edited edge metadata.

@simonpj's suggestion

simonmar updated this revision to Diff 6614.Feb 10 2016, 4:25 PM
simonmar edited edge metadata.

remove spurious change

I'm not inclined to worry too much if this program was depending on this bug to get good performance. It already has plenty of INLINE pragmas, maybe it just needs more.

OK fair enough. I certainly don't have time to dig into this right now, and the current code is clearly wrong.

bgamari accepted this revision.Feb 11 2016, 5:09 AM
bgamari edited edge metadata.

In reference to,

We weren't ignoring Cast in size_up_app... The result was that some things looked too cheap.

I'm not sure I understand how this would result in sizes being too small. On first glance I would have thought this would have inflated sizes.

Regardless, if it's good enough for @simonpj it's good enough for me.

simonmar marked an inline comment as done.Feb 11 2016, 7:06 AM

I'm not sure I understand how this would result in sizes being too small. On first glance I would have thought this would have inflated sizes.

Cast expressions were falling through to the final wildcard case which was under-charging by a lot. But fixing the under-charging alone would result in *over*-charging for Cast unless we deal with it explicitly, which is what this patch does.

This revision was automatically updated to reflect the committed changes.