[Builtins] Drop 'RuntimeScheme' #4778
Conversation
|
/benchmark plutus-benchmark:validation |
1 similar comment
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '75592da88' (base) and '4490cb4c1' (PR) Results table
|
|
^ -4%
OK, I don't understand how our benchmarking infrastructure works. I was getting different results in a PR that didn't have any |
|
I don't quite understand the question. Which two situations are we comparing? The benchmarking compares the base of the PR to the tip of the PR, if you pick a different base you might get different results... |
|
/benchmark plutus-benchmark:validation |
|
^ @michaelpj stuck again? |
|
Looks like it triggered an auto-gc and then it took too long so it got cancelled?? |
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '75592da88' (base) and '4490cb4c1' (PR) Results table
|
|
^ still -4%
I cancelled it after 5 hours, 'cause I wasn't sure what it was doing there. |
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '75592da88' (base) and 'ee9837130' (PR) Results table
|
effectfully
force-pushed
the
effectfully/builtins/drop-RuntimeScheme-2
branch
from
ae13d53 to
e4b8a5f
Compare
effectfully
force-pushed
the
effectfully/builtins/drop-RuntimeScheme-2
branch
from
ca3a7a5 to
c57f732
Compare
effectfully
force-pushed
the
effectfully/builtins/drop-RuntimeScheme-2
branch
from
c57f732 to
4c001d3
Compare
effectfully
force-pushed
the
effectfully/builtins/drop-RuntimeScheme-2
branch
from
e53af8a to
eb3467d
Compare
…to effectfully/builtins/drop-RuntimeScheme-2
effectfully
force-pushed
the
effectfully/builtins/drop-RuntimeScheme-2
branch
from
d5c2aef to
602e30d
Compare
| class ExMemoryUsage a where | ||
| memoryUsage :: a -> ExMemory -- ^ How much memory does 'a' use? | ||
|
|
||
| instance (ExMemoryUsage a, ExMemoryUsage b) => ExMemoryUsage (a, b) where | ||
| memoryUsage (a, b) = 1 <> memoryUsage a <> memoryUsage b | ||
| {-# INLINE memoryUsage #-} |
There was a problem hiding this comment.
Adding those pragmas alone causes a slowdown: #4765.
I've verified it multiple times that adding them here causes a 1-2% speedup (because now they are actually inlined in the right place). Unfortunately, a git rebase wiped out the whole commit history from existence on GitHub, and so there's no way I could prove it apart from redoing the experiments, which I'm tired of.
There was a problem hiding this comment.
In the other PR calls to memoryUsage don't get inlined for specific types, because in the current version of the builtin application machinery we only check memory usages of the whole CekValue provided as an argument. Meaning GHC sees the argument as a CekValue with an arbitrarily typed constant inside and thus GHC can't inline much.
With this PR we only feed properly unlifted values to costing functions, so that GHC knows the type of each of those values and can pick an appropriate ExMemoryUsage instance at runtime and inline the call to memoryUsage away.
So I don't know what causes the slowdown, but I know what causes the speedup (it was the whole point of this refactoring).
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExBudgetingDefaults.hs
Show resolved
Hide resolved
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek.hs
Show resolved
Hide resolved
|
Ready for review. |
| {-# INLINE toExMemory #-} -- It probably gets inlined anyway, but an explicit pragma | ||
| -- shouldn't hurt. | ||
| -- See Note [ExMemoryUsage instances for non-constants]. | ||
| instance (Closed uni, uni `Everywhere` ExMemoryUsage) => ExMemoryUsage (CekValue uni fun) where |
There was a problem hiding this comment.
I feel a bit weird about this, but I have no good reason for it.
There was a problem hiding this comment.
Why? It's the same old function, except as a class method now.
I would kinda like to forbid costing of CekValues in the first place and just say that if a function accepts a term whose type is a type variable, the costing function is supposed to either ignore the ExMemory of that argument or to unlift the argument and only then look at the ExMemory of the unlifted value, but I have no idea how to express that and even if we could, I think it would be too much of a complication to worth this static guarantee.
Right now I'm just keeping the old behavior and optimizing the machinery.
There was a problem hiding this comment.
I think the reason is that before it was more explicit whereas now it feels like we could accidentally use the typeclass method on something that's not a CekValue and not notice. But I don't think this is really a problem.
There was a problem hiding this comment.
now it feels like we could accidentally use the typeclass method on something that's not a
CekValue
But that's exactly what we do! We call the costing functions over the ExMemory of unlifted values now. It's just that we occasionally unlift into Opaque for polymorphic builtins, which is a wrapper around CekValue, hence why we need the instance. For Integer, Bool, SomeConstant etc we call memoryUsage directly without going through CekValue twice (once for unlifting, once for costing). It's the whole point of this PR!
There was a problem hiding this comment.
Apparently I have no idea what the point of this PR is ;)
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/CostingFun/Core.hs
Show resolved
Hide resolved
| (ToCostingType n) | ||
| -- Evaluators that ignore the entire concept of costing (e.g. the CK machine) may of course force | ||
| -- the result of the builtin application unconditionally. | ||
| data BuiltinRuntime val |
There was a problem hiding this comment.
I've never really been sure why this is called BuiltinRuntime ...
There was a problem hiding this comment.
'Cause it's the builtin-related stuff that is used at runtime. Name suggestions are always very welcome!
| -- 'BuiltinRuntime', so that we don't need to store it here, but somehow doing so was | ||
| -- consistently slowing evaluation down by half a percent. Might be noise, might be not, but | ||
| -- at least we know that removing this @fun@ is not helpful anyway. See this commit reversing | ||
| -- the change: https://github.com/input-output-hk/plutus/pull/4778/commits/86a3e24ca3c671cc27c6f4344da2bcd14f961706 | ||
| (Term NamedDeBruijn uni fun ()) | ||
| -- ^ This must be lazy. It represents the fully discharged partial application of the builtin | ||
| -- function that we're going to run when it's fully saturated. We need the 'Term' to be able |
There was a problem hiding this comment.
Isn't the comment at line 213 wrong now? It says (apropos BuiltinRuntime) "The partial application and its costing function.", but that's the Term argument, no? The Term is the stuff we've got so far and the BuiltinRuntime is the stuff we're still waiting for.
There was a problem hiding this comment.
Oh, that's hard to express...
The
Termis the stuff we've got so far and theBuiltinRuntimeis the stuff we're still waiting for.
So yes, Term is the partial application in Plutus and BuiltinRuntime is the stuff we're waiting for, but the previous stuff that went into BuiltinRuntime constitutes the partial application of the denotation of the builtin, it's what is stored in there. To put it differently, if you have a partial application, it means you're still waiting for more stuff to be able to compute it.
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Internal.hs
Show resolved
Hide resolved
| class ExMemoryUsage a where | ||
| memoryUsage :: a -> ExMemory -- ^ How much memory does 'a' use? | ||
|
|
||
| instance (ExMemoryUsage a, ExMemoryUsage b) => ExMemoryUsage (a, b) where | ||
| memoryUsage (a, b) = 1 <> memoryUsage a <> memoryUsage b | ||
| {-# INLINE memoryUsage #-} |
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/CostingFun/Core.hs
Show resolved
Hide resolved
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/CostingFun/Core.hs
Show resolved
Hide resolved
Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
|
Comments addressed (I think). @michaelpj do you still find something in here confusing? Should I update the docs or something? |
|
Wait, it seems like I forgot to push. One sec. |
Done. |
Same as #4757 (at the time of writing). Just making sure I didn't screw up any git-related stuff.