Compare results of compress by hashing
ClosedPublic

Authored by sgraf on Dec 21 2018, 6:55 AM.

Details

Reviewers
AndreasK
nomeata
osa1
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rNOFIB042cf0be9e6f: Compare results of compress by hashing
Summary

We don't want the result in the repo as it's a sizeable binary file that doesn't compress well.

Storing the output file in the repository becomes infeasible for
large inputs. There are two possible remedies:

  1. Generate the result files during make boot (Phab:D5426). We discovered some drawbacks (like missing dependency files to build compress during boot) to this approach which make it infeasible.
  2. Shrink the output files, for example by hashing the string that we would normally output and compare that instead.

This patch implements the second alternative. This somewhat distorts
the runtime profile, so we might want to consider doing hashing within the
benchmark runner in the future.

Test Plan

make boot

Diff Detail

Repository
rNOFIB nofib
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sgraf created this revision.Dec 21 2018, 6:55 AM
Owners added a reviewer: Restricted Owners Package.Dec 21 2018, 6:55 AM
AndreasK requested changes to this revision.Dec 21 2018, 1:37 PM

Comparing a hash is certainly a cleaner solution.

real/compress/Main.hs
24

Why not foldl'?

This revision now requires changes to proceed.Dec 21 2018, 1:37 PM
sgraf added a comment.Dec 23 2018, 5:55 AM

Will fix this as soon as I find the time to do so.

real/compress/Main.hs
24

For no reason, actually. Perhaps embarassingly, I didn't really measure the performance impact of foldr vs. foldl' until now and using foldl' seems much better.

sgraf added a comment.Dec 23 2018, 6:02 AM

A bit unrelated to the foldl' refactoring, but not less concerning: I realised that one problem of this approach is that something (I think it's cat or >) seems to work differently in MinGW than on Linux, probably inserting CRs. mode=fast works just fine, but mode=norm seems to compute a different hash.

In D5469#151064, @sgraf wrote:

A bit unrelated to the foldl' refactoring, but not less concerning: I realised that one problem of this approach is that something (I think it's cat or >) seems to work differently in MinGW than on Linux, probably inserting CRs. mode=fast works just fine, but mode=norm seems to compute a different hash.

As far as I know putStrLn/print behave differently for the two in regards to line endings. Just using putStr (show hash) instead should work.

sgraf added a comment.Dec 23 2018, 9:27 AM

As far as I know putStrLn/print behave differently for the two in regards to line endings. Just using putStr (show hash) instead should work.

I thought so, but the printed hashes are different, not just some additional CR/whitespace in the output. I did some digging and it's not cat or >, but because I had a checkout where there were CRLF line endings in source files. That's strange, given that I had set core.autocrlf to input for some time now. Removing and checking out all files get rid of the issue. That's something to keep in mind, though and one more argument to do this in a final post-processing step in the benchmark runner.

osa1 added a subscriber: osa1.Dec 23 2018, 11:26 PM

I'm confused

Invoking compress during boot to generate the stdout file doesn't work,
because at that point the dependency file for building compress might not have
been created yet, resulting in make boot failures.

Why would you invoke compress during boot? Isn't compress a benchmark that
needs to be run _after_ boot?

In D5469#151068, @osa1 wrote:

I'm confused

Invoking compress during boot to generate the stdout file doesn't work,
because at that point the dependency file for building compress might not have
been created yet, resulting in make boot failures.

Why would you invoke compress during boot? Isn't compress a benchmark that
needs to be run _after_ boot?

The idea was we don't want the result in the repo as it's a sizeable binary file that doesn't compress well.

So we either have to shrink it (this patch) or generate it during boot (state before this patch).
The easiest way to generate the output file during boot is by using compress itself, so that's what I did initially.
In hindsight it was a bad solution for more than one reason, hence why sgraf is changing it.

osa1 added a comment.Dec 24 2018, 5:06 AM
In D5469#151068, @osa1 wrote:

I'm confused

Invoking compress during boot to generate the stdout file doesn't work,
because at that point the dependency file for building compress might not have
been created yet, resulting in make boot failures.

Why would you invoke compress during boot? Isn't compress a benchmark that
needs to be run _after_ boot?

The idea was we don't want the result in the repo as it's a sizeable binary file that doesn't compress well.

So we either have to shrink it (this patch) or generate it during boot (state before this patch).
The easiest way to generate the output file during boot is by using compress itself, so that's what I did initially.
In hindsight it was a bad solution for more than one reason, hence why sgraf is changing it.

Great. Could you update the description with this information?

sgraf edited the summary of this revision. (Show Details)Dec 24 2018, 9:57 AM
sgraf added a comment.Dec 25 2018, 4:53 AM

I just realized what might have been going on: Every time we change one of the source files, the output will differ, because the input file depends on *.hs source files. Well, let's fix that another time.

sgraf updated this revision to Diff 19239.Dec 25 2018, 4:59 AM
  • Use foldl' instead of foldr
sgraf updated this revision to Diff 19240.Dec 25 2018, 7:15 AM
sgraf marked 2 inline comments as done.
  • Remove compress2.stdin
osa1 accepted this revision.Dec 25 2018, 7:21 AM

LGTM.

Once the binary is in the repo this won't buy us much, but I guess this helps when doing shallow clone (which I never do, but perhaps others do).

AndreasK accepted this revision.Dec 26 2018, 6:36 AM
In D5469#151127, @osa1 wrote:

Once the binary is in the repo this won't buy us much, but I guess this helps when doing shallow clone (which I never do, but perhaps others do).

As far as I know the large binary isn't in the repo yet, so at least that's not an issue.

Also LGTM thanks for cleaning up nofib sgraf!

I just realized what might have been going on: Every time we change one of the source files, the output will differ, because the input file depends on *.hs source files. Well, let's fix that another time.

I thought about generating them from noise or deterministic patterns using a small script back when, but it doesn't seem worth it since we would then benchmark a very unrealistic usecase and add more complexity to the whole thing.

But you seem good at coming up with solutions to these problems, so if you can think of a nifty solution I'm all for it!

This revision is now accepted and ready to land.Dec 26 2018, 6:36 AM
sgraf added a comment.Dec 26 2018, 6:42 AM

I thought about generating them from noise or deterministic patterns using a small script back when, but it doesn't seem worth it since we would then benchmark a very unrealistic usecase and add more complexity to the whole thing.

But you seem good at coming up with solutions to these problems, so if you can think of a nifty solution I'm all for it!

Thanks for putting you trust in me :)

Actually, I already have a solution in https://phabricator.haskell.org/D5468#change-ebwukeRjdffL: Use a prefix of enwik8 as test input. But I'll probably fix that as part of the two monster diffs.

sgraf updated this revision to Diff 19241.Dec 26 2018, 6:56 AM
  • Actually do the left fold. Don't know how this compiled earlier
sgraf updated this revision to Diff 19242.Dec 26 2018, 6:58 AM
  • Remove and ignore the stdout file
sgraf updated this revision to Diff 19243.Dec 26 2018, 6:59 AM
  • Nevermind, got confused. Revert "Remove and ignore the stdout file"
This revision was automatically updated to reflect the committed changes.

Hmm, this does not actually fix the problem that makes perf.haskell.org fail, as shown in this log (which contains this fix, I believe):
https://raw.githubusercontent.com/nomeata/ghc-speed-logs/8fbffea2c4c63826e79778ffb828c5516e8f64bc/d2a0f3ab8438b80663f7977bb6b474c02baffa5c.log.broken

sgraf added a comment.Jan 1 2019, 4:39 PM

That's strange. Are you following https://gitlab.haskell.org/ghc/nofib? Because CI there went green just today for make mode=fast. In addition to compress, I also had to fix compress2 but that's about it. The boot failures in particular should be gone.

Are you following https://gitlab.haskell.org/ghc/nofib?

I wasn’t (am now) but the other sources are mirrored, so that is not the problem. The above log was for

commit d2a0f3ab8438b80663f7977bb6b474c02baffa5c
Author: Peter Trommler <ptrommler@acm.org>
Date:   Mon Dec 31 16:46:22 2018 +0100

    Rewrite comment on C calling convention

Note that I am running make boot -j8, i.e. with parallelism. Is the Makefile buggy in that respect?

sgraf added a comment.Jan 2 2019, 3:16 AM
Are you following https://gitlab.haskell.org/ghc/nofib?

I wasn’t (am now) but the other sources are mirrored, so that is not the problem. The above log was for

commit d2a0f3ab8438b80663f7977bb6b474c02baffa5c
Author: Peter Trommler <ptrommler@acm.org>
Date:   Mon Dec 31 16:46:22 2018 +0100

    Rewrite comment on C calling convention

Strange, I can't find that commit anywhere in git log.

Note that I am running make boot -j8, i.e. with parallelism. Is the Makefile buggy in that respect?

Since nofib uses recursive make, -j8 will have no effect, or shouldn't have. That's also indicated by the warnings it generates. See https://www.mail-archive.com/ghc-devs@haskell.org/msg16324.html

Hmm, this does not actually fix the problem that makes perf.haskell.org fail, as shown in this log (which contains this fix, I believe):
https://raw.githubusercontent.com/nomeata/ghc-speed-logs/8fbffea2c4c63826e79778ffb828c5516e8f64bc/d2a0f3ab8438b80663f7977bb6b474c02baffa5c.log.broken

The nofib submodule hasn't been bumped since end of november. So unless you checkout nofib's master explicitly this won't contain this fix yet.

Strange, I can't find that commit anywhere in git log.

Oh, this might be a branch, sorry if that’s the source of confusion… And I broke something else this morning. Will report back once it actually tried to build latest master.

But Andreas is right, this is the last bump of nofib:

commit 51e56dd5e299a6d7a0a3b0b97afd33dbd3485279
Author: klebinger.andreas@gmx.at <klebinger.andreas@gmx.at>
Date:   Mon Dec 3 19:59:09 2018 +0100

    Bump nofib submodule
    
    Test Plan: run nofib
    
    Reviewers: bgamari, tdammers
    
    Reviewed By: tdammers
    
    Subscribers: rwbarton, carter
    
    Differential Revision: https://phabricator.haskell.org/D5396