[Builtins] Monomorphize 'makeKnown' #4421
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
…to effectfully/builtins/performance/monomorphize-makeKnown
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'be641e9e6' (PR)
|
|
Wow, much better this time around. Ready for review. |
michaelpj
reviewed
Feb 21, 2022
| @@ -284,7 +285,7 @@ defaultSlippage :: Slippage | |||
| defaultSlippage = 200 | |||
|
|
|||
| -- | The CEK machine is parameterized over an emitter function, similar to 'CekBudgetSpender'. | |||
| type CekEmitter uni fun s = Text -> CekM uni fun s () | |||
| type CekEmitter uni fun s = DList Text -> CekM uni fun s () | |||
There was a problem hiding this comment.
It's a bit sad that the monomorphization of readKnown means we have to commit to Writer (DList Text) and then that leaks up here too. But it would be annoying to traverse the list of logs to emit them in the machine's emitter. That said, that happens anyway in e.g. some of the log emitters for the CEK machine that assume they can post-process each log line individually. So maybe it's a false saving and it would be cleaner to just traverse the DList of logs and emit them again (given that most such lists should be small).
There was a problem hiding this comment.
Thinking about it, I feel like it was actually stupid to add DList here. A call to traverse_ has to be faster than a call to ?cekEmitter and in the vast majority of cases we're going to end up with no logs returned from the builtin application machinery. I'll remove that DList.
There was a problem hiding this comment.
Also in the CK machine for consistency?
There was a problem hiding this comment.
Can you add a comment here regardless of what you do?
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and '3156484b4' (PR)
|
|
Hmm, did that really make it slower? |
|
Whoa, let's check. |
This reverts commit 3156484.
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and '55a88822f' (PR)
|
This reverts commit 55a8882.
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'c894010be' (PR)
|
It seems it did. I consider myself challenged. |
|
/benchmark plutus-benchmark:validation |
This reverts commit 9a19955.
|
/benchmark plutus-benchmark:validation |
This reverts commit 668fa12.
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and '668fa122e' (PR)
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'f7df4676f' (PR)
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'cda727926' (PR)
|
This reverts commit cda7279.
|
/benchmark plutus-benchmark:validation |
1 similar comment
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'cda727926' (PR)
|
…to effectfully/builtins/performance/monomorphize-makeKnown
|
Here's a comparison of how RuntimeSchemeResult co1_ap8V $dKnownTypeIn1_sFnd -> | RuntimeSchemeResult co1_ap8W $dKnownTypeIn1_sF8h ->
let { sat_sFne = BBuiltinApp fun2_sFmy } in | let { sat_sF8i = BBuiltinApp fun2_sF7C } in
case (((ds_sFdI `cast` <Co:4>) | case (((ds_sEYM `cast` <Co:4>)
sat_sFne (dt15_sFmO `cast` <Co:5>)) | sat_sF8i (dt15_sF7S `cast` <Co:5>))
`cast` <Co:9>) | `cast` <Co:9>)
w2_sFm6 | w2_sF7a
of | of
{ (# ipv6_sFng, _ #) -> | { (# ipv6_sF8k, _ #) ->
let { | let {
sat_sFnm | sat_sF8q
= ((f4_sFmE `cast` <Co:9>) x_sFmN) | = ((f4_sF7I `cast` <Co:9>) x_sF7R)
`cast` <Co:7> } in | `cast` <Co:7> } in
let { | let {
sat_sFnk | sat_sF8o
= case term1_sFmz of dt16_sFni { __DEFAULT -> | = case term1_sF7D of dt16_sF8m { __DEFAULT ->
case argTerm_sFmG of dt17_sFnj { __DEFAULT -> | case argTerm_sF7K of dt17_sF8n { __DEFAULT ->
Apply () dt16_sFni dt17_sFnj | Apply () dt16_sF8m dt17_sF8n
} | }
} } in | } } in
let { sat_sFnl = Just sat_sFnk } in | let { sat_sF8p = Just sat_sF8o } in
case (makeKnown | case (makeKnown
($dKnownTypeIn1_sFnd `cast` <Co:8>) | ($dKnownTypeIn1_sF8h `cast` <Co:8>)
sat_sFnl | sat_sF8p
sat_sFnm) | sat_sF8q)
`cast` <Co:44> | `cast` <Co:44>
of | of
{ (ipv8_sFno, ipv9_sFnp) -> | { (ipv8_sF8s, ipv9_sF8t) ->
case (ipv9_sFnp `cast` <Co:2>) [] of sat_sFnE | case ((ds16_sEYT ipv9_sF8t)
| `cast` <Co:9>) -- *
{ __DEFAULT -> | ipv6_sF8k
join { | of
$w$j_sFnq w3_sFnr | { (# ipv10_sF8v, _ #) ->
= case ipv8_sFno of { | case ipv8_sF8s of {
Left err_sFnt -> | Left err_sF8y ->
jump exit7_sFhy w3_sFnr err_sFnt; | jump exit7_sF2C ipv10_sF8v err_sF8y;
Right res1_sFnu -> | Right res1_sF8z ->
jump $wreturnCek_sFj2 | jump $wreturnCek_sF46
ww_sFm3 ctx_sFm9 res1_sFnu w3_sFnr | ww_sF77 ctx_sF7d res1_sF8z ipv10_sF8v
} } in | }
joinrec { | }
go1_sFnv ds22_sFnw eta_sFnx | }
= case ds22_sFnw of { | }
[] -> jump $w$j_sFnq eta_sFnx; |
: y_sFnz ys_sFnA -> |
case ((ds16_sFdP y_sFnz) -- * |
`cast` <Co:9>) |
eta_sFnx |
of |
{ (# ipv10_sFnC, _ #) -> |
jump go1_sFnv ys_sFnA ipv10_sFnC |
} |
}; } in |
jump go1_sFnv sat_sFnE ipv6_sFng |
} |
} |
} |Nothing unexpected. The former has a couple more join points and jumps, one additional matching and one (a few?) regular applications. Could we optimize some of that away? Yep, we could just use Church-encoded lists directly to avoid the matching and the recursion, see this commit. It does fill the performance gap for the most part, but using I tried making type CekEmitter uni fun s =
DList Text -> CekM uni fun s (CekValue uni fun) -> CekM uni fun s (CekValue uni fun)so that we can have So... why am I even spending time here? Let's just make it |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '0397f83b8' (base) and 'f3133745b' (PR)
|
I agree, sounds fine. |
|
It was suspicious, you investigated, and it turns out to be fine. Good enough for me. |
michaelpj
approved these changes
Mar 7, 2022
Of course. I just wanted to get your approval first before documenting the solution. |
…to effectfully/builtins/performance/monomorphize-makeKnown
|
Comments addressed, merging. |
effectfully
deleted the
effectfully/builtins/performance/monomorphize-makeKnown
branch
kwxm
pushed a commit
that referenced
this pull request
Mar 15, 2022
This monomorphizes `makeKnown` like `readKnown` was monomorphized previously. This gives a speedup of 4.87% on average.
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.
Attempt №2.
Do not look here yet.