[Builtins] Monomorphize 'readKnown' #4307
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
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and '916535235' (PR)
|
|
WAT |
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and '916535235' (PR)
|
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and '916535235' (PR)
|
effectfully
force-pushed
the
effectfully/builtins/performance/monomorphize-readKnown
branch
from
9165352 to
1db9fbb
Compare
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and '1db9fbbd2' (PR)
|
|
I'm quite puzzled as to how this branch is so ridiculously (and consistently) faster than the baseline. Below is the comparison of changes in generated Core that I was able to recognize. In
this PR: In
this PR: And that's it. Like yeah, this PR clearly compiles better than Anyway, I don't want to spend more time digging through Core, so I'll write some docs, ask for reviews and merge this. |
|
One possible explanation would be that we somehow end up forcing the call to |
michaelpj
approved these changes
Jan 4, 2022
| coerceVia _ = coerce | ||
| {-# INLINE coerceVia #-} | ||
|
|
||
| coerceArg :: Coercible a b => (a -> r) -> b -> r |
There was a problem hiding this comment.
you hate eta-expansion so much? :p
There was a problem hiding this comment.
I very much do when it comes to performance-critical code. It's pretty common to call coerce over the entire function instead of just its argument, because that does not create any closures and in some cases makes the whole definition more sharing-friendly. I'll make a comment.
| let runtime' = BuiltinRuntime schB (f x) . exF $ toExMemory arg | ||
| res <- evalBuiltinApp fun term' env runtime' | ||
| returnCek unbudgetedSteps ctx res | ||
| TypeSchemeArrow _ schB -> case readKnown (Just argTerm) arg of |
There was a problem hiding this comment.
I'd be curious to see what happens if you do it this way:
do
x <- liftEither $ readKnown ...
...
That might force us back into ST (if it doesn't get optimized away, which it probably will), which might be enlightening. And if it does get optimized away, it's arguably better style.
There was a problem hiding this comment.
Great suggestion, thank you.
|
But actually, I can't think why it would matter. We replace one pattern match (to get the state token) with another one (to take apart the Either and call throwError). So it seems like no win. Maybe it's just that |
effectfully
force-pushed
the
effectfully/builtins/performance/monomorphize-readKnown
branch
from
1db9fbb to
cc30b67
Compare
|
/benchmark |
effectfully
force-pushed
the
effectfully/builtins/performance/monomorphize-readKnown
branch
from
cc30b67 to
3d1f229
Compare
|
Comparing benchmark results of 'c8c5183f7' (base) and 'cc30b671b' (PR)
|
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and '2a500e25b' (PR)
|
This reverts commit 2a500e2.
|
/benchmark |
|
Comparing benchmark results of 'c8c5183f7' (base) and 'eb1706059' (PR)
|
effectfully
deleted the
effectfully/builtins/performance/monomorphize-readKnown
branch
|
Oops, @kwxm, sorry, I forgot to ask for your review. Doing that now. I'll address comments in another PR. |
|
I don't have much to say, except that the performance changes seem rather mysterious. Maybe we should try some detailed profiling on one of the examples with a big speedup to see if anything jumps out. |
|
Also, looking at the top of It's a pity we can't tell it which extensions we don't want... |
|
HLS told me that we don't need |
|
@thealmarty I'm using it in #4312, which I'm planning to get merged next, so don't bother. |
MaximilianAlgehed
pushed a commit
to Quviq/plutus
that referenced
this pull request
Mar 3, 2022
This gave us a speedup of up to 19% for unclear reasons.
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.
Do not look here yet.