SCP-2289: Try making a special "for evaluation" term type #3340
Conversation
|
Or maybe I just can't get my head around the idea that a simple recursive traversal of the term could take 136% of the time to evaluate it. Surely not??? |
|
I guess I can try pulling it out of the benchmarked part and see... |
There was a problem hiding this comment.
We do a traversal before, and potentially a traversal at the end or when we make errors, but all of that should be lazy.
Doesn't look lazy to me. Everything is spine-strict, so forcing to WHNF is forcing to NF.
What benefits do we get by introducing ETerm? Previously, we'd only once pattern match on every AST node of a Term and the rest of the time deal with CekValues. Now we match on each AST node of a Term once and then on each AST node of a ETerm once and the rest is the same. What's the point?
Maybe Edward meant that we should change the on-chain representation? Then it would take us less time to construct the original Term with all the junk in it and we wouldn't need one more traversal.
| | VDelay (Term Name uni fun ()) (CekValEnv uni fun) | ||
| | VLamAbs Name (Term Name uni fun ()) (CekValEnv uni fun) | ||
| | VDelay (ETerm uni fun) (CekValEnv uni fun) | ||
| | VLamAbs !Unique (ETerm uni fun) (CekValEnv uni fun) |
There was a problem hiding this comment.
Do you need a completely new type of terms to replace names with uniques?
There was a problem hiding this comment.
Well, this way GHC knows statically that the name is a Unique and can unpack it and also just pass it directly to IntMap functions. I think it's doing that automatically (or at least, adding UNPACK pragmas here made no difference).
|
BTW, your stats look too shaky, are you sure there's no crazy variance in there? |
Yeah, but we always return them in a lazy
That's not quite true.
If it's truly the case that it's the initial traversal that causes this then yeah, we'd have to do something like that.
I ran it both yesterday and today on the benchmarking machine and got the same results. They're just quite weird! |
|
So overall what I thought we might get is:
I thought this might be a small improvement at best, I'm just baffled that it's instead much slower! |
I thought you meant
Yeah, you're right.
How about adding |
|
I tried the Marlowe benchmarks on my laptop and got this: and then I removed the exclamation marks from the (master on the left in both cases), so making it strict seems harmful (I tried that with the existing I suggest moving the |
michaelpj
force-pushed
the
mpj/term-for-eval
branch
2 times, most recently
from
5a4ec29
to
d69ad7f
Compare
|
/benchmark |
😲 |
|
Whoooooops, really sorry about retriggering the benchmarks! |
|
Maybe only trigger the benchmarks if that command is the only thing appearing in the comment? |
|
Haha, amazing. You do have to say it with the slash, so it should mostly be safe if you don't literally quote someone else saying it :p |
|
(It's broken still, working on it) /benchmark |
michaelpj
force-pushed
the
mpj/term-for-eval
branch
2 times, most recently
from
1d56eb8
to
1d331a6
Compare
|
/benchmark |
michaelpj
force-pushed
the
mpj/term-for-eval
branch
from
1d331a6
to
9fd1fa3
Compare
|
/benchmark |
1 similar comment
|
/benchmark |
Another one of Ed's suggestions. I got rid of annotations and replaced names with `Unique`s directly, which we can even unpack into the term. This seems to have better Core: indeed, it gets rid of some wrapping and unwrapping when looking up or inserting variable mappings. But the result is overall much worse.
michaelpj
force-pushed
the
mpj/term-for-eval
branch
from
dbb8fdf
to
655fe60
Compare
|
/benchmark |
[kwxm: edited to add backticks for better legibility] |
|
|
So what's going on here? The original benchmarks for this PR showed that it made things much worse, but now it's an improvement and there have been no other commits in the interim. |
|
Also some in the game state machine. Quite odd. |
No, I squashed some stuff in. In particular I pulled the conversion to |
Oh, right. I should have re-read the other comments! |
Those are exactly the same scripts that were anomalous in your frames PR last week. I'd like to know what's going on there. I spent about 3 hours talking to @bezirg about the similar thing with |
|
Random guess: the ones that regress are ones that are quite fast already. If they're scripts that do something like (say) look through the list of outputs to verify that someone is paid appropriately, then maybe reordering so that the output they're looking for is later could have an effect. |
| @@ -165,3 +171,47 @@ erase (TPLC.Error ann _) = Error ann | |||
| -- | Erase a Typed Plutus Core Program to its untyped counterpart. | |||
| eraseProgram :: TPLC.Program tyname name uni fun ann -> Program name uni fun ann | |||
| eraseProgram (TPLC.Program a v t) = Program a v $ erase t | |||
|
|
|||
| {-| | |||
| A specialized version of 'Term' for evaluation. The 'name' and 'a' parameters are specialized to 'Name' ('Unique') and '()' (omitted). | |||
There was a problem hiding this comment.
Hasn't a disappeared entirely?
There was a problem hiding this comment.
That's what I was getting at with "... specialized to () (omitted)". I can make it clearer.
| encode = \case | ||
| EVar n -> encodeTerm 0 <> encode n | ||
| EDelay t -> encodeTerm 1 <> encode t | ||
| ELamAbs n t -> encodeTerm 2 <> encode n <> encode t |
There was a problem hiding this comment.
No Binder optimization here? :(
There was a problem hiding this comment.
Disregard what I said, we need to encode n since we are currently evaluating using uniques in cek environment
There was a problem hiding this comment.
Yes, this is not a debruijn ETerm!
| deriving stock (Show, Functor, Generic) | ||
| deriving anyclass (NFData) | ||
|
|
||
| termToETerm :: Term TPLC.Name uni fun () -> ETerm uni fun |
There was a problem hiding this comment.
why not more generalized argument: Term Name uni fun a?
There was a problem hiding this comment.
Because that throws away information silently, this does not. Here you have to explicitly opt-in to throwing away any annotations yourself before calling this.
More generally: ETerm uni fun === Term Name uni fun (), and these functions witness that.
| @@ -416,7 +417,7 @@ throwingDischarged | |||
| -> t | |||
| -> CekValue uni fun | |||
| -> CekM uni fun s x | |||
| throwingDischarged l t = throwingWithCause l t . Just . dischargeCekValue | |||
| throwingDischarged l t = throwingWithCause l t . Just . etermToTerm . dischargeCekValue | |||
There was a problem hiding this comment.
What do we gain by going back to Term here? Wouldn't it simpler that throwingwithcause operated directly on ETerms?
There was a problem hiding this comment.
Yeah, this is a leftover from when I switched to making the interface to the CEK machine just use ETerms.
| @@ -141,4 +141,7 @@ readKnownCek | |||
| => MachineParameters CekMachineCosts CekValue uni fun | |||
| -> Term Name uni fun () | |||
| -> Either (CekEvaluationException uni fun) a | |||
| readKnownCek params = evaluateCekNoEmit params >=> readKnown | |||
| readKnownCek params t = do | |||
There was a problem hiding this comment.
i think this readknowncek are used only in testing. what if we maded the KnownType class work on ETerm's directly?
|
/benchmark |
|
|
I just want to have a look at the benchmark logs for this... /benchmark |
|
crowdfunding/crowdfunding-success-1 695.2 μs 628.5 μs -9.6% |
|
@michaelpj I'm going to close this as part of my campaign to get rid of zombie PRs. This one's been open since June 2021and it says "This branch has conflicts that must be resolved". Yeah, I bet it does. Oh, it also says "Some checks haven’t completed yet"! |
Another one of Ed's suggestions. I got rid of annotations and replaced
names with
Uniques directly, which we can even unpack into the term.This seems to have better Core: indeed, it gets rid of some wrapping and
unwrapping when looking up or inserting variable mappings.
But the result is overall much worse.
Again, I am baffled. We do a traversal before, and potentially a traversal at the end or when we make errors, but all of that should be lazy. So I don't understand how we can be causing such massive slowdowns. Ideas welcome, again!
Pre-submit checklist:
Pre-merge checklist: