[Builtins] Unlift both ways #4516
Conversation
This reverts commit 19656b6.
…to effectfully/builtins/defer-readKnown-until-full-saturation
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'e59ade143' (base) and 'f07097902' (PR)
|
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'e59ade143' (base) and '50d93e7a6' (PR)
|
|
@michaelpj this adds customizable unlifting and propagates it all the way up to
I'm now very confused: I don't know how an I still need to polish the code and document it, but I once I do that, could you take if from there? |
| @@ -245,10 +227,10 @@ class uni ~ UniOf val => MakeKnownIn uni val a where | |||
| -- See Note [Cause of failure]. | |||
| -- | Convert a Haskell value to the corresponding PLC val. | |||
| -- The inverse of 'readKnown'. | |||
| makeKnown :: Maybe cause -> a -> ExceptT (ErrorWithCause MakeKnownError cause) Emitter val | |||
| makeKnown :: Maybe cause -> a -> ExceptT (ErrorWithCause ReadKnownError cause) Emitter val | |||
There was a problem hiding this comment.
This seems weird. Why does makeKnown throw a ReadKnownError? If we're just not distinguishing them any more, let's give it a better name.
There was a problem hiding this comment.
Because readKnown is now deferred to the same place where makeKnown happens. I'll change the name.
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExBudgetingDefaults.hs
Outdated
Show resolved
Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug.hs
Outdated
Show resolved
Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Debug.hs
Outdated
Show resolved
Hide resolved
| -> Maybe EvaluationContext | ||
| mkEvaluationContext newCMP = | ||
| EvaluationContext . inline Plutus.mkMachineParameters <$> Plutus.applyCostModelParams Plutus.defaultCekCostModel newCMP | ||
| mkEvaluationContextUnliftingImmediate :: Plutus.CostModelParams -> Maybe EvaluationContext |
There was a problem hiding this comment.
This distinction isn't the ledger team's problem, rather we will make the decision based on the protocol version and the language version. So this function would need to take a protocol version - I'll check that that's okay with the ledger team.
There was a problem hiding this comment.
For now, I think just put this back the way it was and pass defaultUnliftingMode and we (i.e. @nikos ;) ) will sort it out afterwards.
There was a problem hiding this comment.
In fact, I realised from asking Jared about it that probably what we should do for now is this:
- Put both the deferred and the immediate unlifting versions into the
EvaluationContext - Pull out the one that we need in
evaluateScriptbased on the protocol version
That way we avoid requiring a protocol version in mkEvaluationContext (although we may want one eventually anyway). Doable here because there are only two options!
There was a problem hiding this comment.
Great, makes sense to me.
There was a problem hiding this comment.
After reading these comments, now I understand why we need both unlifting modes: Are we planning to switch to another unlifting mode in new language version?
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExBudgetingDefaults.hs
Outdated
Show resolved
Hide resolved
|
|
||
| -- | A class that allows us to derive a polytype for a builtin. | ||
| class KnownPolytype (binds :: [Some TyNameRep]) val args res a | args res -> a, a -> res where | ||
| class KnownMonotype val args res a => |
There was a problem hiding this comment.
I was a little surprised by this! To be clear, this is the monotype that corresponds to the type after stripping off the polymorphic arguments, right?
There was a problem hiding this comment.
After stripping off the all constructors binding type variables. There's a comment about the naming:
We use two type classes for automatic derivation of type schemes: 'KnownMonotype' and
'KnownPolytype'. The terminology is due to https://en.wikipedia.org/wiki/Hindley%E2%80%93Milner_type_system#The_Hindley%E2%80%93Milner_type_system
I should remove the "monomorphic" (right down below the quote) word from that Note, though.
Now KnownMonotype is a superclass of KnownPolytype, because we don't need to have toImmediateF and toDeferredF in KnownPolytype, since for polytypes those are the same as for their corresponding monotypes. Hence I just added the constraint. KnownPolytype only allows us to all-bind type variables and after than it's just
instance KnownMonotype val args res a => KnownPolytype '[] val args res a where
knownPolytype = knownMonotype
knownPolyruntime = knownMonoruntime @val @args @resso no reason to thread toImmediateF and toDeferredF through KnownPolytype given that none of them is of any relevance to any all-bound type variables.
effectfully
force-pushed
the
effectfully/builtins/unlift-both-ways
branch
from
b3130e4 to
3b26af5
Compare
|
|
||
| -- | Once we've run out of term-level arguments, we return a 'TypeSchemeResult'. | ||
| instance (res ~ res', KnownTypeAst (UniOf val) res, MakeKnown val res) => | ||
| KnownMonotype val '[] res res' where | ||
| knownMonotype = TypeSchemeResult | ||
| knownMonoruntime = RuntimeSchemeResult | ||
|
|
||
| toImmediateF = makeKnown (Just ()) |
There was a problem hiding this comment.
I'll remove this Just () nonsense in a follow-up.
|
/benchmark plutus-benchmark:validation |
| defaultUnliftingMode :: UnliftingMode | ||
| defaultUnliftingMode = UnliftingImmediate | ||
|
|
There was a problem hiding this comment.
wait, i read in some comment above the 'deferred' is the default mode
There was a problem hiding this comment.
oh maybe i am reviewing a forced-push commit for benchmarking-only.
There was a problem hiding this comment.
Are you sure? There's a comment about it being "preferred", but it does not make it default quite yet.
There was a problem hiding this comment.
we want deferred to be the default but immediate matches the current behaviour
| toMachineParameters :: EvaluationContext -> DefaultMachineParameters | ||
| toMachineParameters = case defaultUnliftingMode of | ||
| UnliftingImmediate -> machineParametersImmediate | ||
| UnliftingDeferred -> machineParametersDeferred |
There was a problem hiding this comment.
in real-world (on-chain), do we have to keep both unlifting modes around? as long as we commit in a single one being the default, isn't that enough to keep around?
There was a problem hiding this comment.
I guess it does not matter that much since it is cached anyway.
There was a problem hiding this comment.
See Michael's comment. Basically, we have to keep both the versions and pick one depending on the language version. The "default" then is whatever the last language version uses. This PR doesn't implement the branching on the language version, it just lays out all the infrastructure to support that. @michaelpj will you implement the branching yourself in a follow-up? In any case, I think it should be done in a follow-up, so that we don't keep this one unmerged for too long, 'cause it blocks some other things.
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'e59ade143' (base) and 'e80dbb757' (PR)
|
|
Instead of |
|
I've optimized the immediate unlifting a little, now it's -5.88%, i.e. pretty much the same as the deferred one.
It's literally |
|
Comments addressed, ready for a final review. @michaelpj is the alignment fine now? All the infrastructure for branching on the language version is there, but that actual logic is not, we still use the default way of doing unlifting, which is the immediate one. |
|
|
||
| -- See Note [MakeKnownM and ReadKnownM being type synonyms]. | ||
| -- | The monad that 'makeKnown' runs in. | ||
| type MakeKnownM cause = ExceptT (ErrorWithCause KnownTypeError cause) Emitter |
There was a problem hiding this comment.
Is there an existing note where we explain that we only allow emitting effects at the end?
There was a problem hiding this comment.
This is described in the docs above the default sets of builtins, plus attempting to use Emitter in argument position gives you a nice custom type error saying you can't do that.
Technically speaking... For deferred unlifting we could as well allow Emitter in argument position due to unlifting happening in the same place where lifting happens anyway. I'm not sure if that could be useful and it most likely would slow things down a bit (unless! We give up on transformers, which is something that I'm going to try anyway).
| toDeferredF getF = \arg -> | ||
| -- The bang is very important: without it GHC thinks that the argument may not be needed in |
|
Ok, two approves, I'm pressing merging to keep me going. @kwxm sorry for not giving you enough time to review, please do that anyway and I'll address the comments separately. |
This adds customizable unlifting and propagates it all the way up to `EvaluationContext`. The old immediate unlifting gets faster by 5.88% and the new deferred unlifting is faster than `master` by about the same amount. We're practically inlining `readKnown` and `makeKnown` here without storing them in any data type, which is probably what causes the speedup. It is beneficial to do unlifting in the same place where the value is used, more opportunities for inlining, worker-wrapping etc. `EvaluationContext` now has two sets of parameters: one is with immediate unlifting and the other one is with deferred unlifting. We have to keep both of them, because depending on the language version either one has to be used or the other. We also compile them separately due to all the inlining and optimization that need to happen for things to be efficient.
Don't look here yet.