Remove immediate unlifting #4879
Conversation
plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Evaluation/Machine/Cek/Internal.hs
Show resolved
Hide resolved
...plutus-core/src/PlutusCore/Evaluation/Machine/MachineParameters/DeferredMachineParameters.hs
Show resolved
Hide resolved
|
I selfishly stole the joy of deleting code, sorry 😅 |
|
No, the benchmarking machine is totally busted still. |
|
/benchmark plutus-benchmark:validation |
1 similar comment
|
/benchmark plutus-benchmark:validation |
|
I cancelled the benchmarking jobs. I can log in and run things manually, but I think |
|
[Spurious benchmark results deleted] |
|
That doesn't look good... |
In fact it looks wrong. I ran the benchmarks again on both branches and got a 2-3% speedup. I'll do it once more just to check. |
|
To run the benchmarks manually you may want to make sure to follow the steps in the script, especially the |
There was a problem hiding this comment.
I looked at the generated Core and apparently it's now worse. None of the asConstant/fromConstant business gets inlined due to toBuitinMeaning getting specialized but not inlined. Despite all the pragmas and calls to inline being in the right places. Checked three times.
@michaelpj it's that time of year where I remind how I suggested not to create long inlining chains (do feel free to remind me why we're here in the first place).
|
Just checked, on the recent BTW, you lost in the process. |
There was a problem hiding this comment.
On the bright side, simply keeping DeferredMachineParameters allows us to preserve inlining. I'm not making this shit up, I swear.
So let's do that unless you feel like trying to figure out why the hell inlining only works properly there (it can be anything like DeferredMachineParameters doesn't export DefaultMachineParameters or the REWRITE rule unlike Default).
|
I've now manually run the benchmarks on both the master branch and this branch three times on the benchmarking machine. It looks as if I got something wrong the first time. Here are the figures for the third attempt, which are much more encouraging; the figures for the second one are very similar to these. I'm not sure what happened the first time. The results end like this The figures for this branch are pretty similar to those above, but those for the main branch are much faster. Maybe I just failed to clean up properly, as suggested by @zliu41 . |
|
I propose not to merge the PR until we have benchmarking results with the |
|
@michaelpj could you resolve the merge conflicts and so I can run the benchmark more properly? Or if you would prefer for me to do it let me know. Thanks! |
| @@ -2,6 +2,10 @@ | |||
| -- We keep them separate, because the function unfolds into multiple thousands of lines of Core and | |||
| -- we want to instantiate it in two different ways on top of that, which gives another ton of Core | |||
| -- that we need to inspect, hence we dedicate an entire folder to that. | |||
|
|
|||
| -- Core is tidier when the worker-wrapper optimization does not happen. | |||
| {-# OPTIONS_GHC -fno-worker-wrapper #-} | |||
There was a problem hiding this comment.
We shouldn't merge this, though. It's a good optimization.
michaelpj
force-pushed
the
mpj/remove-immediate-unlifting
branch
from
942edd2 to
65fc33b
Compare
|
Done |
|
@kwxm would you kindly benchmark this again just to double check what happens with the option turned on. Thanks. |
I can't! The new nix stuff doesn't work on the benchmarking machine (wrong nix version and I seem to be unable to update it), and the old version's gone now. |
|
Since I couldn't use the benchmarking machine I ran the benchmarks on desktop machine that I have that seems to be quite consistent (although it takes about twice as long as the usual machine). I benchmarked this branch and the current version of master twice each and couldn't see any significant difference.
|
|
There's that:
I'd recheck, but neither This branch is supposed to be a bit faster than Also I'm not really sure how we're supposed to handle issues like that without the benchmarking machine working, surely we can't ask Kenneth to run the benchmarks manually each time we change something. |
|
Yes will do that today. I just thought I'll have a quick check on the difference between the two files first. This wrapper option and the inline are the only differences. 🤔 Yeah sorry for the trouble Kenneth. I can benchmark locally too then. Not sure what else to do really. |
|
Core dump for |
|
Here it is. It's only around 27,000 lines of code. :P |
|
Here: With |
|
Just in case, here, with the line |
There was a problem hiding this comment.
Here: With {-# INLINE mkMachineParametersFor #-} removed it's only 13,000 line of core.
It all looks good to me, I do not see anything bad in that output. I don't know what causes your output to be different from mine, but giving that the benchmarking results are definitely on your side, I'm going to suggest that we merge this PR (without the INLINE pragma on mkMachineParametersFor, since it's no longer doing anything useful).
Running Seems kinda random. I'm fine with both including and not including the pragma. |
|
I'll do some more local benchmarking today before we merge this. |
|
I got this when I ran the benchmarking locally. Comparing master with this branch with the INLINE pragma removed. It's like what Kenneth found before. I'll do one with the pragma in.
|
|
This one is comparing with the INLINE pragma in against master.
|
|
This is comparing between with or without INLINE:
|
|
So, confirmed that the INLINE pragma isn't helping. I agree we should merge without the pragma (so can merge the PR as is). I would have done more benchmarking if the machine is working.... |
So we got, in chronological order: Not a particularly good progression! Anyway, it can be noise or just the benchmarking machine is being weird, which it can sometimes, so I personally don't care.
Could you leave a comment in the code? It's useful info. |
thealmarty
force-pushed
the
mpj/remove-immediate-unlifting
branch
3 times, most recently
from
5258fb0 to
e0d22d7
Compare
Since checking shows it is in fact unused pre-Vasil.
michaelpj
force-pushed
the
mpj/remove-immediate-unlifting
branch
from
e0d22d7 to
70648d1
Compare
|
/benchmark validation |
|
🎉 |
* Remove immediate unlifting
Since checking shows it is in fact unused pre-Vasil.
* Nits
* Remvoe BuiltinRuntimeOptions
* Remove {-# INLINE mkMachineParametersFor #-}.
* Noop commit
* again
* fix
Co-authored-by: Marty Stumpf <thealmartyblog@gmail.com>
Since checking shows it is in fact unused pre-Vasil.