[Builtins] Only be 'Lazy' for the result of a builtin application #4607
Conversation
|
/benchmark plutus-benchmark:validation |
| @@ -51,7 +54,12 @@ instance NFData (RuntimeScheme n) where | |||
| -- argument of the denotation and calling 'makeKnown' over its result. | |||
| type ToRuntimeDenotationType :: GHC.Type -> Peano -> GHC.Type | |||
| type family ToRuntimeDenotationType val n where | |||
| ToRuntimeDenotationType val 'Z = MakeKnownM val | |||
| -- We use 'Lazy' here, because we don't want to compute the denotation when it's fully saturated | |||
| -- before figuring out what it's going to cost. An alternative is to be really careful and never | |||
There was a problem hiding this comment.
we don't want to compute the denotation when it's fully saturated before figuring out what it's going to cost
What does this mean? Do we sometimes not compute the result if the cost is too high?
There was a problem hiding this comment.
Do we sometimes not compute the result if the cost is too high?
Exactly. If something costs more than what the budget allows, we don't compute the builtin application and return an out-of-ex failure immediately.
| ~(ToRuntimeDenotationType val n) -- Must be lazy, because we don't want to compute the | ||
| -- denotation when it's fully saturated before figuring | ||
| -- out what it's going to cost. | ||
| (ToRuntimeDenotationType val n) |
There was a problem hiding this comment.
Do you have an example of this value being unnecessarily forced before? I didn't spot any.
There was a problem hiding this comment.
It would be very wrong to let people run what they don't have a budget for, so we have tests for that. If you remove that tilda on master and run cabal test plutus-core-test, you'll get
ExpensiveSucc: FAIL
untyped-plutus-core/test/Evaluation/Builtins/Definition.hs:200:
expected: Right (Right EvaluationFailure)
but got: Right (Left BuiltinErrorCall)
Use -p '/ExpensiveSucc/' to rerun this test only.
ExpensivePlus: FAIL
untyped-plutus-core/test/Evaluation/Builtins/Definition.hs:230:
expected: Right (Right EvaluationFailure)
but got: Right (Left BuiltinErrorCall)
Use -p '/ExpensivePlus/' to rerun this test only.
There was a problem hiding this comment.
Yeah but I was wondering why Lazy is helpful. The reason you added Lazy is because this value was unnecessarily forced to WHNF somewhere - right?
There was a problem hiding this comment.
Ah, yeah, see the competing PR that removes the unnecessary forcing. Somehow it was worse than the Lazy approach.
There was a problem hiding this comment.
Well maybe I need some more coffee 😂 but the competing PR simply avoids constructing and destructing BuiltinRuntime. But since the ~(ToRuntimeDenotationType val n) field is lazy, constructing and destructing BuiltinRuntime doesn't force it, does it?
There was a problem hiding this comment.
the competing PR simply avoids constructing and destructing BuiltinRuntime. But since the
~(ToRuntimeDenotationType val n)field is lazy
Constructing and deconstructing is what forces us to have that tilda there. The competing PR avoids the constructing and deconstructing, which allows it to remove the tilda (the first file in the diff).
There was a problem hiding this comment.
Does that mean the saving in the competing PR didn't come from avoiding unnecessary forcing (since there wasn't unnecessary forcing before either), it came from avoiding the construction and destruction? Then why would this PR help since it doesn't avoid construction/destruction, but just replace the tilda with Lazy?
There was a problem hiding this comment.
it came from avoiding the construction and destruction?
Avoiding the construction and deconstruction is why the competing PR should've been faster than the Lazy approach. However it's not the reason why the two PR exist. The point is that while we want to avoid premature forcing of the result, we don't want to avoid forcing of every intermediate partial application as keeping those thunks likely costs us some. I.e. we want to be lazy only for a fully saturated builtin and not for any partial application.
|
Comparing benchmark results of 'plutus-benchmark:validation' on '4af22f9fb' (base) and 'a2c72aa7d' (PR) Results table
|
|
Hm, Ok, I guess this somehow interferes with the |
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '4af22f9fb' (base) and 'fb6a43af7' (PR) Results table
|
|
Closing in favor of #4610. I like this one more, but the other one actually helps. |
effectfully
deleted the
effectfully/builtins/only-be-lazy-for-the-result-2
branch
effectfully
restored the
effectfully/builtins/only-be-lazy-for-the-result-2
branch
effectfully
deleted the
effectfully/builtins/only-be-lazy-for-the-result-2
branch
effectfully
restored the
effectfully/builtins/only-be-lazy-for-the-result-2
branch
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '4af22f9fb' (base) and '9674d36d7' (PR) Results table
|
|
-7.07%. The only thing I did is moving |
|
fyi: Looking around in hackage for something an established way of the |
|
-7% is worse than -8%, hence choosing #4610 over this one. |
effectfully
deleted the
effectfully/builtins/only-be-lazy-for-the-result-2
branch
Same as #4581 except the history is correct.