Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance ghc 9 and bench #101

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Apr 16, 2021

Benchmarks now consume the resulting generator produced by computation that is being benchmarks, thus it can no longer be optimized away by the ghc. Fixing benchmarks brings them to more realistic and consistent runtimes.

Fixed benchmarks still pointed out to regressions due to worse inliner heuristics in ghc-9 In order to workaround that problem we just need to add INLINE for all functions and just call it a day. Which is also done in this PR

Getting rid of problems with inliner we are still left with a few regressions with ghc-9, albeit not as drastic (only ~60-70%). I think it will be good to follow up on those with a separate investigation another PR.

@lehins lehins requested a review from Bodigrim April 16, 2021 00:11
@Bodigrim
Copy link
Contributor

Bodigrim commented Apr 16, 2021

Sorry, I'm AFK until Tuesday, so cannot take a close look at the moment.

Let's separate making benchmarks meaningful and forcing inlining into separate PRs. The reason is that I've had a similarly looking inlining regression in text and there is an intention to fix this on GHC side. We can opt for forcing inlining anyways later, but for now it would be useful to provide GHC team with unamended source code.

See https://gitlab.haskell.org/ghc/ghc/-/issues/19557 and https://gitlab.haskell.org/ghc/ghc/-/merge_requests/5547


Does the claim about 1000x speed up remains valid?

Is it possible to setup env once and thread the same generator through all benchmarks?

@lehins
Copy link
Contributor Author

lehins commented Apr 16, 2021

Sorry, I'm AFK until Tuesday, so cannot take a close look at the moment.

Take you time, I don't expect mass adoption of ghc-9 by Tuesday 😄

Let's separate making benchmarks meaningful and forcing inlining into separate PRs.

They are already implemented in two separate commits, which is good enough for separating the concerns.

Does the claim about 1000x speed up remains valid?

Yes, that claim did not use those benchmarks but instead we used legacy benchmarks that were included in old random as well. I personally didn't even use this benchmark suite for anything because I had my own repo that I used when working on random.
(I prefer criterion over gauge, so I am really happy with this switch to tasty-bench ;)

Is it possible to setup env once and thread the same generator through all benchmarks?

Possible, but it doesn't make a difference. And if it doesn't make a difference, I am not gonna waste my time on doing the work.

@lehins lehins force-pushed the fix-performance-ghc-9-and-bench branch from 7f8cc7f to c9471d4 Compare April 19, 2021 21:35
@lehins lehins merged commit a5d6b73 into master Apr 20, 2021
@lehins lehins deleted the fix-performance-ghc-9-and-bench branch September 8, 2021 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants