SCP-2462: Use BuiltinData in the ledger etc. #3539
Conversation
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
c0f1430 to
f6028a6
Compare
| deriving stock (Generic, Haskell.Show) | ||
| deriving anyclass (ToJSON, FromJSON) | ||
| newtype Context = Context BuiltinData | ||
| deriving (ToJSON, FromJSON) via PLC.Data |
There was a problem hiding this comment.
Could you derive Pretty this way too?
There was a problem hiding this comment.
Yes, do you think it's useful? Doesn't hurt, I guess.
|
I regenerated most of the |
|
Oh no!!!! |
|
The figures from the |
|
I ran the I also used |
|
That's sad, but it kinda makes sense. |
|
I think there may be a more obvious problem, looking into it now... |
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
766c9f0 to
2f390e2
Compare
|
So one issue was the |
plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExMemory.hs
Outdated
Show resolved
Hide resolved
|
hacked in some tallying of builtin counts: Going to try a few things. First one: replace null+ifThenElse with chooseList. |
|
Profiling suggested that we weren't obviously spending more time in builtin operations. I concluded that we were perhaps simply doing more work than we had been. Things I tried:
The current state has some code-bloat due to the fact that the non-unsafe I need to benchmark this properly but that requires me to regenerate the scripts again... |
|
Oh, and if this works, I should take more time to work out why the old way was so fast, I really don't know how it can have been! |
In the two examples I looked at up above it was calling |
So I think this mainly means we were generating |
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
b9a197c to
996e44b
Compare
Better, but still not great. |
Nice! |
I think I know what this is: it's got a manual use of the slow |
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
90c947d to
9833e2a
Compare
|
Didn't work 🤔 |
This propagates up the use of `BuiltinData` to script arguments. The payoff is in `Plutus.V1.Ledger.Api`, where we can now just wrap the arguments in constants instead of having to use `Lift`. I've opted to be painfully explicit about the difference between `Data` and `BuiltinData`. There are a few places where we need conversions, but overall not *too* many.
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
9833e2a to
42ae44c
Compare
|
Okay, I'm going to break with best practices and merge this. I'm planning to keep investigating the regressions, or generally, to optimize Also note that I'm not updating the benchmark programs in this PR. |
michaelpj
force-pushed
the
mpj/data-ledger
branch
from
42ae44c to
0fd4f47
Compare
| -- Note that we are using builtin ifThenElse here so this is *strict* application! So we need to do | ||
| -- the manual laziness ourselves. We could instead convert the boolean to a Haskell boolean and use | ||
| -- normal if, but we might as well use the builtins directly here. | ||
| go l = ifThenElse (null l) (\_ -> []) (\_ -> fromBuiltin (head l):go (tail l)) unitval | ||
| go l = chooseList (\_ -> []) (\_ -> fromBuiltin (head l):go (tail l)) l unitval |
| choosePlc :: Opaque term b -> Opaque term b -> SomeConstantOf uni [] '[a] -> EvaluationResult (Opaque term b) | ||
| choosePlc a b (SomeConstantOfArg _ (SomeConstantOfRes _ xs)) = case xs of | ||
| [] -> EvaluationSuccess a | ||
| _ : _ -> EvaluationSuccess b |
There was a problem hiding this comment.
You don't need EvaluationResult if you always return EvaluationSuccess. I.e. you can drop all of that.
There was a problem hiding this comment.
I don't know how to do that 🤔 I think I tried to write it like the other functions and it didn't work for some reason?
| ExMemory 2854 No newline at end of file | ||
| ExMemory 2170 |
There was a problem hiding this comment.
These are really impressive numbers.
This propagates up the use of
BuiltinDatato script arguments. Thepayoff is in
Plutus.V1.Ledger.Api, where we can now just wrap thearguments in constants instead of having to use
Lift.I've opted to be painfully explicit about the difference between
Dataand
BuiltinData. There are a few places where we need conversions, butoverall not too many.
Will need a corresponding ledger PR, there are a couple of small changes.
Pre-submit checklist:
Pre-merge checklist: