Fix all regressions relative to v1.1 #116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that I tried to keep this PR minimal and only apply changes that had a significant impact in the benchmarks. That means, for example, that I didn't copy over the |
curiousleo
changed the title
|
Ready for review. |
lehins
approved these changes
Apr 29, 2020
Closed
curiousleo
added a commit
that referenced
this pull request
May 13, 2020
* Make bitmask-with-rejection non-recursive * INLINE some uniformRM implementations
curiousleo
added a commit
that referenced
this pull request
May 13, 2020
* Make bitmask-with-rejection non-recursive * INLINE some uniformRM implementations
lehins
pushed a commit
that referenced
this pull request
May 18, 2020
* Make bitmask-with-rejection non-recursive * INLINE some uniformRM implementations
curiousleo
added a commit
that referenced
this pull request
May 19, 2020
* Make bitmask-with-rejection non-recursive * INLINE some uniformRM implementations
Shimuuar
pushed a commit
to Shimuuar/random
that referenced
this pull request
Jan 6, 2025
…hortbytestring Improve uniform `ShortByteString`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This combines changes to the bitmask-rejection methods from #103 with a few benchmark-driven
INLINEpragmas.Context
In
interface-to-performanceright now, there are performance regressions relative to v1.1:This PR: no more regressions relative to v1.1
This PR speeds up the slower generators such that every benchmarked function runs faster than on v1.1 by 1400% or more.
Comparison between v1.1 (reference) and this branch:
Some regressions relative to
interface-to-performanceStrangely, while speeding up many generator functions significantly, this PR introduces some mild regressions relative to
interface-to-performance.Comparison between
interface-to-performance(reference) and this branch:As an additional observation, adding one more
INLINE, e.g. for Int8'suniformRM, actually makesnextWord32andnextWord64slower! My guess is that GHC has some sort of "inlining budget", and that adding anotherINLINEpragma somewhere leads to less inlining elsewhere. For this reason I've only addedINLINEpragmas where they improved the benchmark results significantly, and removed them if they led to slower generated code.Conclusion
While I can't fully explain the non-local effects I observed with respect to inlining, this PR does objectively remove all regression we previously had relative to v1.1, so I suggest we merge.