[#14037] Allow fusion for zip7 and related
AbandonedPublic

Authored by rockbmb on Oct 20 2018, 6:32 PM.
rockbmb created this revision.Oct 20 2018, 6:32 PM
rockbmb added a comment.EditedOct 21 2018, 4:15 PM

Good news, it seems the current set of changes, when make fasttest -j8 is run, produce (on my machine, of course):

Unexpected stat failures:
   perf/compiler/T5631.run         T5631 [stat too good] (normal)
   perf/haddock/haddock.base.run   haddock.base [stat too good] (normal)
   perf/haddock/haddock.Cabal.run  haddock.Cabal [stat too good] (normal)
   perf/should_run/T9203.run       T9203 [stat too good] (normal)

There are several steps to go through, still, such as:

  • running tests for every commit since one individual change might introduce a regression that the other commits overcome with increased performance
  • Use make slowtest instead of make fasttest since running all tests might reveal more information about whether this change is positive or not.
rockbmb added a comment.EditedOct 21 2018, 4:26 PM

For T9203:

Expected    T9203(normal) bytes allocated:  98360576 +/-5%
Lower bound T9203(normal) bytes allocated:  93442547 
Upper bound T9203(normal) bytes allocated: 103278605 
Actual      T9203(normal) bytes allocated:  46371896 
Deviation   T9203(normal) bytes allocated:     -52.9 %

For T5631:

Expected    T5631(normal) bytes allocated: 1161885448 +/-5%
Lower bound T5631(normal) bytes allocated: 1103791175 
Upper bound T5631(normal) bytes allocated: 1219979721 
Actual      T5631(normal) bytes allocated: 1055392736 
Deviation   T5631(normal) bytes allocated:       -9.2 %

For haddock.base:

Expected    haddock.base(normal) bytes allocated: 25913205656 +/-5%
Lower bound haddock.base(normal) bytes allocated: 24617545373 
Upper bound haddock.base(normal) bytes allocated: 27208865939 
Actual      haddock.base(normal) bytes allocated: 24007020952 
Deviation   haddock.base(normal) bytes allocated:        -7.4 %

For haddock.Cabal:

Expected    haddock.Cabal(normal) bytes allocated: 27520214496 +/-5%
Lower bound haddock.Cabal(normal) bytes allocated: 26144203771 
Upper bound haddock.Cabal(normal) bytes allocated: 28896225221 
Actual      haddock.Cabal(normal) bytes allocated: 25275864688 
Deviation   haddock.Cabal(normal) bytes allocated:        -8.2 %

.

rockbmb updated the Trac tickets for this revision.Oct 21 2018, 4:27 PM
osa1 added a reviewer: dfeuer.Oct 22 2018, 1:24 AM
osa1 added subscribers: dfeuer, osa1.

I found it surprising that T9203 allocates -53% now, as it doesn't even use lists. Do you know why it's effected by these changes?

@dfeuer may want to review library parts.

@rockbmb updating the diff description with the problem and how you decided to add NOINLINE [1] vs. NOINLINE would be good.

There are a number of changes in this patch, but none of them are commented in any way. So maybe someone will change them all back in a while! It would really help to document the intent, rather than just change the code.

Sometimes a single Note, which can be pointed to from several places, is the easiest way to explain how all the moving parts fit together.
W
I've made a few notes to that effects in the individual places.

compiler/utils/MonadUtils.hs
94

Why this change? Does it fuse better? Why?

libraries/base/Data/OldList.hs
735

Why? How does inlining help?

770

Why NOINLINE?

(Incidentally, since it's recursive the pragma will do nothing.)

libraries/base/GHC/List.hs
1016

Why? Please explain.

There are a number of changes in this patch, but none of them are commented in any way. So maybe someone will change them all back in a while! It would really help to document the intent, rather than just change the code.

Sometimes a single Note, which can be pointed to from several places, is the easiest way to explain how all the moving parts fit together.
W
I've made a few notes to that effects in the individual places.

Sorry, I tend to add comments near the end of my contributions, I can certainly do that now.

Hm, it seems I spoke too soon. I am not sure of the reason, but it seems that the tests that output stat too good do so in the latest master branch as well, and with identical results. Can anyone confirm this? I had expected that all performance tests in the latest master would pass without being too bad or too good, at least when ran with make test, but it seems I was wrong, both on that and about these changes being strictly positive. I cannot claim they are when the same improvement appears without them.
I'll continue trying things out.

Hm, it seems I spoke too soon. I am not sure of the reason, but it seems that the tests that output stat too good do so in the latest master branch as well, and with identical results. Can anyone confirm this? I had expected that all performance tests in the latest master would pass without being too bad or too good, at least when ran with make test, but it seems I was wrong, both on that and about these changes being strictly positive. I cannot claim they are when the same improvement appears without them.
I'll continue trying things out.

make test on master had some result too good tests for me today. But I haven't kept the log around to confirm it's the same tests. But it seems like a reasonable assumption.

bgamari requested changes to this revision.Oct 28 2018, 11:14 AM

Great work @rockbmb!

As suggested earlier, this certainly needs more explanation and perhaps characterisation. Bumping out of the review queue for now.

Let us know if there's anything we can do to help.

This revision now requires changes to proceed.Oct 28 2018, 11:14 AM
rockbmb added a comment.EditedDec 18 2018, 2:21 PM

@bgamari Is it alright if I close this, and reopen it in GHC's Gitlab instance? Or would you rather have me complete this issue here?

EDIT: I just read the first paragraph from [this page], I'll follow those instructions and finish this patch here.

rockbmb updated this revision to Diff 19296.Feb 5 2019, 7:46 PM
  • Allow fusion for unzip4
  • Allow fusion for unzip5
  • Allow fusion for unzip6
  • Allow fusion for unzip7
  • Add comment explaining INLINE for unzips
  • Add inline rule to mapAndUnzip3M
  • Add inline rule to mapAndUnzip4M
  • Add inline rule to mapAndUnzip5M
  • Add explanation to mapAndUnzipM changes
  • Add inline for zipWith3M
  • Add inline for zipWith3M_
  • Add inline for zipWith4M
  • Add comment explaining zipWithM inline
rockbmb added a comment.EditedFeb 5 2019, 7:49 PM

This differential has been updated to reflect the changes from D5241 (some of the previous changes here have already been implemented by that diff), along with some comments explaining the changes.

@bgamari this is a low priority issue, but could you take a look when possible?
@simonpj the changes I've now introduced should address your requests for comment. I apologize for taking so long to update a simple issue.

It seems CI has been disabled for Phabricator as part of the move to GitLab.
In any case, I forked GHC there, performed these changes on my fork and pushed them here. Every required build passes.

rockbmb updated this revision to Diff 19298.Feb 8 2019, 5:39 PM
  • Fix reference in commit to differential with unzip fusion rules
rockbmb marked 3 inline comments as done.Feb 9 2019, 4:22 PM
rockbmb added inline comments.
libraries/base/Data/OldList.hs
735

Removed this change as it was addressed in D5241.

770

Removed this change too as it was also done in D5241.

rockbmb marked 4 inline comments as done.Feb 9 2019, 4:26 PM
rockbmb added inline comments.
compiler/utils/MonadUtils.hs
94

I added a comment right above this set of functions to answer your question.

rockbmb marked an inline comment as done.Feb 9 2019, 4:27 PM
rockbmb edited reviewers, added: simonpj; removed: hvr, dfeuer.Feb 15 2019, 4:01 PM

I apologise for my insistence on this issue as well (I ask for some tolerance based on point 7.) - can someone review this?

I don't like having comments like this! We should fix the original problem.

The inline principle for @zipWith3M@ and @zipWith3M_@ is the same as for
zipWithM' and 'zipWithM_' in "Control.Monad".
No comment or explanation is given in that module as to why 'zipWith'
has an @INLINE@ pragma, but it stands to reason that if it was added in the
 past and not removed or addressed in any way, it is overall useful (but still
undocumented, which would be nice to address).

I'll push a patch and you can then change this one to refer to the new Notes in my patch

@simonpj sorry, but what do you specifically mean by this:

I don't having like comments like this

?

There could be many things wrong with the comments: they're too long, they're not concise enough, they're too numerous and should therefore be condensed into fewer comments, or just one in an easily accessible location, or all of the previous things together.

Sorry not to be clear! I meant that I don't like a comment that says "I don't know why this code is right, but it's the same as that code over there, which also lacks any comment". Clearly the right thing is to add the missing comment which I have done in
https://gitlab.haskell.org/ghc/ghc/merge_requests/394

@simonpj you're right - I forgot to raise that point in this discussion so that someone could add those comments. I didn't do it myself since I'm not that familiar with the codebase yet.
Thank you that MR. As soon as it's merged and lands on this instance's master, I'll fix this Diff and push the changes.

rockbmb updated this revision to Diff 19299.Feb 19 2019, 9:20 PM
  • Fix comment regarding 'unzip' inlining
  • Fix comment regarding 'mapAndUnzipM' inlining
  • Fix comment regarding zipWithNM functions [skip ci]

@simonpj This diff should be more acceptable now.

rockbmb updated this revision to Diff 19300.Feb 19 2019, 9:23 PM
  • Fix comment regarding 'mapAndUnzipM' inlining
  • Fix comment regarding zipWithNM functions [skip ci]

Lovely. Some minor suggestions.

compiler/utils/MonadUtils.hs
68

Is this Note referred to? I think not. Perhaps it'd make sense to refer to this note from functions defined in this module; and this Note can refer on to the Note in List.hs?

72

Can you refer directly to the Note?

124

Same comment here

libraries/base/Data/OldList.hs
938

Is there a Note in GHC..List to refer to? If so, do; if not, fine.

simonpj accepted this revision.Feb 20 2019, 3:39 AM

Accepting -- use your judgement in deciding how/whether to respond to my suggestions.

rockbmb updated this revision to Diff 19301.Feb 20 2019, 9:11 AM
  • Fix comments after latest review [skip ci]
rockbmb updated this revision to Diff 19302.Feb 20 2019, 9:15 AM
  • Fix comments after latest review [skip ci]
rockbmb updated this revision to Diff 19303.Feb 20 2019, 9:16 AM
  • Fix comments after latest review [skip ci]
rockbmb updated this revision to Diff 19304.Feb 20 2019, 9:18 AM
  • Fix comments after latest review [skip ci]
rockbmb marked 5 inline comments as done.Feb 20 2019, 9:28 AM

@simonpj should be even more acceptable now :)

Thanks! Go for it!

@simonpj I do not have write access to this repository, and I believe it is needed in order to merge this. Would you mind doing so for me?

I'm not sure what the protocol is these days. Can you ask for help on ghc-devs?

@simonpj I've sent the email. Unfortunately, Microsoft has suspended my Outlook email account because it classified my email as "suspicious activity", but I sent it with another address. Can you confirm reception?

Sigh. There has to be a better way than this. Send mail to me personally at simonpj@microsoft.com and I will act as your personal mail forwarding agent and fwd it to ghc-devs

I've just sent you an email.

I apologise for this whole situation, although I'd like to point out that this happened because of circumstances beyond my control, and also that it has happened before - I could not write to libraries@haskell.org a few months ago, so I asked a colleague to write for me. It seems to have been a problem with Outlook blacklisting some types of messages.

rockbmb abandoned this revision.Feb 24 2019, 4:01 PM

This can be closed now that the GitLab MR has been merged.
Thank you to all who lent their time to guide me.