UniqFM: Use foldl' instead of fold
AbandonedPublic

Authored by AndreasK on Jun 14 2018, 9:57 AM.

Details

Summary

This improves compiler performance for quick builds where O0 is used but makes no difference at O2.

At O0:

Compile Allocations

-------------------------------------------------------------------------------
        Program            logMaster          logFold
-------------------------------------------------------------------------------
        -1 s.d.                -----            -0.3%
        +1 s.d.                -----            -0.1%
        Average                -----            -0.2%
Test Plan

ci

sjakobi created this revision.Jun 14 2018, 9:57 AM
AndreasK requested changes to this revision.Jun 14 2018, 10:04 AM

This currently increases allocation. sjakobi was so nice to upload the patch so I can check why.

This revision now requires changes to proceed.Jun 14 2018, 10:04 AM

It would be interesting, if a bit time-consuming, to evaluate the effect of each of these changes individually. That might help narrow down the cause of the regression.

[14-Jun-18 16:07:59] <sjakobi> With the strictness changes in UniqFM, I'm actually getting a few "stat not good enough" failures for
[14-Jun-18 16:07:59] <sjakobi> TEST="T12227 T12545 T3064 T5030 T5321Fun T5631 T9872a T9872b T9872c"

AndreasK added inline comments.Jun 14 2018, 11:58 AM
compiler/utils/UniqFM.hs
108

This is at least part of the issue.

listToUFM_Directly is not inlined and neither is M.fromList.
So we end up allocating the updated tuples before passing them to listToUFM_directly.

Down the line this produces some weird code like this tuple repacking:

listToUFM2
  = \ @ elt_aark ds1_abvm ->
      (case ds1_abvm of { (x_abvp, y_abvq) -> x_abvp `cast` <Co:1> },
       case ds1_abvm of { (x_abvu, y_abvv) -> y_abvv })

Which as far as I can tell should just be a coercion instead?

AndreasK updated this revision to Diff 17024.Jun 20 2018, 4:57 AM
  • Revert back to using a fold for list insertion
AndreasK accepted this revision.Jun 20 2018, 4:58 AM
This revision is now accepted and ready to land.Jun 20 2018, 4:58 AM
AndreasK retitled this revision from UniqFM: Be more strict in several functions to UniqFM: Use foldl' instead of fold.Jun 20 2018, 4:59 AM
AndreasK edited the summary of this revision. (Show Details)
dfeuer requested changes to this revision.Jun 20 2018, 9:01 PM
dfeuer added a subscriber: dfeuer.
dfeuer added inline comments.
compiler/utils/UniqFM.hs
78

This import doesn't seem to be used.

This revision now requires changes to proceed.Jun 20 2018, 9:01 PM
AndreasK commandeered this revision.Jul 1 2018, 7:00 AM
AndreasK edited reviewers, added: sjakobi; removed: AndreasK.
AndreasK updated this revision to Diff 17148.Jul 1 2018, 7:07 AM
  • Remove unused import
AndreasK edited the summary of this revision. (Show Details)Jul 1 2018, 7:12 AM
AndreasK marked an inline comment as done.
AndreasK edited the summary of this revision. (Show Details)Jul 1 2018, 7:16 AM
AndreasK edited the test plan for this revision. (Show Details)
sjakobi accepted this revision.Jul 2 2018, 10:53 AM

LGTM. Using foldl' instead of foldl seems less confusing/surprising to me.

bgamari accepted this revision.Jul 2 2018, 3:35 PM

This looks good to me.

Maybe check out how this relates to D4929? There doesn't seem to be any overlap between the two patches, but D4929 puts foldl' in GHC.Prelude, which make the Data.List import redundant here.

AndreasK abandoned this revision.EditedJul 4 2018, 3:25 PM

Abandoned in favour of D4929

Maybe check out how this relates to D4929? There doesn't seem to be any overlap between the two patches, but D4929 puts foldl' in GHC.Prelude, which make the Data.List import redundant here.

Good catch.
I didn't want to combine them initially as this one has already been accepted.

But it probably makes more sense to abandon this one and roll the changes into the other (larger) patch.