[Builtins] Optimize 'MakeKnownM' #4587
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 plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'ffa31787c' (base) and 'baaddd21c' (PR) Results table
|
|
This is just getting silly now... |
|
-8.27% on average. |
|
Disgusting. |
|
I'm kind of scandalized that even with a concrete monad transformer stack this is faster! |
There are two things at play here. The obvious one is that the new monad is fully strict (now that I say that, I'm tempted to make |
|
Okay, well I don't think we can say no to 8%, so I guess tidy it up ship it.
Can you do crazy stuff like this? |
michaelpj
reviewed
May 9, 2022
| fmap _ (MakeKnownFailure logs err) = MakeKnownFailure logs err | ||
| fmap f (MakeKnownSuccess a) = MakeKnownSuccess (f a) | ||
| fmap f (MakeKnownSuccessWithLogs logs a) = MakeKnownSuccessWithLogs logs (f a) | ||
| {-# INLINE fmap #-} |
There was a problem hiding this comment.
All these INLINE annotations seem a little risky to me. If you have any substantial do block in MakeKnownM you're going to be at risk of exponential code bloat. Maybe we should just do INLINABLE?
There was a problem hiding this comment.
Why exponential? It's a constant factor: every call to >>= becomes, what, six pattern matches. Not too much of a deal, given that we don't do much in MakeKnownM and we want all of it to be efficient, apart from stuff in tests, but it's still not that much.
There was a problem hiding this comment.
Yes, you're right, it's not exponential. It would only become so if GHC got happy with case-of-case, which it's probably too smart to do. I'd be interested to know if performance is actually worse with INLINABLE.
There was a problem hiding this comment.
Yes, you're right, it's not exponential. It would only become so if GHC got happy with case-of-case, which it's probably too smart to do.
Hm, there's something to worry about here indeed. So let's say we didn't have MakeKnownSuccessWithLogs, then
(a >>= f) >>= gwould expand into
case (case a of
MakeKnownFailure logs err -> MakeKnownFailure logs err
MakeKnownSuccess x -> f x) of
MakeKnownFailure logs err -> MakeKnownFailure logs err
MakeKnownSuccess y -> g ywhich would be turned by case-of-case into
case a of
MakeKnownFailure logs err ->
case MakeKnownFailure logs err of
MakeKnownFailure logs err -> MakeKnownFailure logs err
MakeKnownSuccess y -> g y
MakeKnownSuccess x ->
case f x of
MakeKnownFailure logs err -> MakeKnownFailure logs err
MakeKnownSuccess y -> g ywhich would be turned by case-of-known-constructor into
case a of
MakeKnownFailure logs err ->
MakeKnownFailure logs err
MakeKnownSuccess x ->
case f x of
MakeKnownFailure logs err -> MakeKnownFailure logs err
MakeKnownSuccess y -> g ywhich is still perfectly linear.
But we do have MakeKnownSuccessWithLogs and so it is indeed very much possible we'll get duplication in the two success cases.
But then
- we do want the duplication, since it's important for this stuff to be as efficient as possible
- we won't actually get it, because our
fandgare going to be known and so thecase-of-known-constructorwill save us
Really, I don't think we should worry about this. Especially compared to the ridiculous code bloats with meanings of builtins inlining all the way through the pipeline at each and every step until the very construction of EvaluationContext (we won't have any problems with MakeKnownM at any step, because the denotations have always been perfectly linear due to everything being fully known).
I'd be interested to know if performance is actually worse with
INLINABLE.
It probably isn't any worse, but we're building quite a house of cards with all the optimizations here, I don't want to make it any more shaky unless we have to.
No, I mean |
…to effectfully/builtins/optimize-MakeKnownM
effectfully
force-pushed
the
effectfully/builtins/optimize-MakeKnownM
branch
from
baaddd2 to
83bbac6
Compare
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '20eadc688' (base) and '83bbac61f' (PR) Results table
|
|
-8.11% on average. Ready for review. I'm going to try to get rid of |
zliu41
reviewed
May 10, 2022
michaelpj
approved these changes
May 10, 2022
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '20eadc688' (base) and '54665b928' (PR) Results table
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '20eadc688' (base) and 'f6845efe8' (PR) Results table
|
This reverts commit c27e088.
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '20eadc688' (base) and 'f93d9df52' (PR) Results table
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '20eadc688' (base) and 'f93d9df52' (PR) Results table
|
|
I've done a bunch of experimentation, mostly related to the review comments, but also this: default makeKnown :: KnownBuiltinType val a => a -> MakeKnownM val
-- Everything on evaluation path has to be strict in production, so in theory we don't need to
-- force anything here. In practice however all kinds of weird things happen in tests and @val@
-- can be non-strict enough to cause trouble here, so we're forcing the argument. Looking at the
-- generated Core, the forcing amounts to pulling a @case@ out of the 'fromConstant' call,
-- which doesn't affect the overall cost and benchmarking results suggest the same.
--
-- Note that the value is only forced to WHNF, so care must be taken to ensure that every value
-- of a type from the universe gets forced to NF whenever it's forced to WHNF.
makeKnown x = pure . fromConstant . someValue $! xAll of that is finished, merging. |
|
@michaelpj ci/hydra-eval has been pending for two hours. |
effectfully
force-pushed
the
effectfully/builtins/optimize-MakeKnownM
branch
from
909627e to
bd8071f
Compare
|
It's being weird, but it does seem to have passed. Force merging. |
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.
Don't look here yet.