PLT-1432: [Builtins] Make 'geq' inlinable #5061
Conversation
effectfully
force-pushed
the
effectfully/builtins/evaluation/inlinable-geq
branch
from
9d41dd4 to
5cf0066
Compare
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'd1becb405' (base) and 'd1becb405' (PR) Results table
|
|
^ almost all +, then almost all - Suspicious. |
|
@michaelpj you can see here that benchmarks are being run even though the CI says there's an error. The CI seems to run something different from what we do. |
It's comparing the same branch. |
|
/benchmark plutus-benchmark:validation |
Oh, indeed, I didn't notice that! But then it's even more suspicious! Why would the first half be consistently slower and the second one consistently faster if we're comparing the same branch? It seems like the chance of this occurring randomly is negligible. |
|
Comparing benchmark results of 'plutus-benchmark:validation' on 'a12b99450' (base) and 'a12b99450' (PR) Results table
|
|
@zeme-iohk I think it's the same branch still. Also we can observe a very similar pattern in the numbers: noise, then multiple consecutive +, one of which is beyond what we used to consider the noise threshold (2%):
then lots of consecutive -:
then noise again. I believe it's extremely unlikely that this is happening by chance. I suspect there's a problem with either our benchmarks or the machine. |
effectfully
force-pushed
the
effectfully/builtins/evaluation/inlinable-geq
branch
from
5cf0066 to
758965f
Compare
|
/benchmark plutus-benchmark:validation |
My latest change seems to have broken benchmarking. Sorry about this, will fix asap |
|
@zeme-iohk no problem & no rush. |
|
/benchmark plutus-benchmark:validation |
|
I've fixed it manually here: Comparing benchmark results of ' plutus-benchmark:validation' on '41bfd20d7' (base) and '758965ff6' (PR) Results table
|
|
/benchmark nofib |
|
Click here to check the status of your benchmark. |
|
That's some good stuff right there. |
|
Comparing benchmark results of ' nofib' on 'f6963f009' (base) and '758965ff6' (PR) Results table
|
|
/benchmark plutus-benchmark:validation |
|
Click here to check the status of your benchmark. |
|
Eek, I should've refreshed the page before running the benchmarks again. These are going to be the same. Although I can use them to check that the machine is consistent, so it's not all waste. |
|
Comparing benchmark results of ' plutus-benchmark:validation' on '1843a7dab' (base) and '758965ff6' (PR) Results table
|
|
Only -1.16% on average. One of those cases where the numbers appear better than what they actually are. But it's kinda free and we see multiple 3+% improvements and even more 2+% improvements and all slowdowns are <=1.1%, i.e. below the noise threshold. Plus Core looks better and it may (likely won't) unlock more optimizations in future. Needs docs of course. @michaelpj do you have any opinion? |
|
(nofib is -1.6% on average) BTW, the reason we're not seeing a larger speedup is GHC weirdly inlining |
Sure, I was going to write some.
No idea. But it can't be worse than what we have now, because what we have isn't supposed to get inlined at all (even though it somehow does occasionally and inconsistently). And the approach of this PR is the best I can think of and I did spend a lot of time investigating other possible options (see #4462 and #4463) So I think this PR is our best bet and we should go with it even if we're not sure that GHC will continue to inline |
zliu41
changed the title
effectfully
force-pushed
the
effectfully/builtins/evaluation/inlinable-geq
branch
2 times, most recently
from
58f7850 to
918afb6
Compare
|
/benchmark plutus-benchmark:validation |
effectfully
force-pushed
the
effectfully/builtins/evaluation/inlinable-geq
branch
from
918afb6 to
4c085ec
Compare
|
Ready for review, but the benchmarking runner is not with us anymore. |
effectfully
force-pushed
the
effectfully/builtins/evaluation/inlinable-geq
branch
from
4c085ec to
91c1242
Compare
|
cc @zeme-iohk the benchmarking job seems to have got stuck in a queue? https://github.com/input-output-hk/plutus/actions/runs/4186578896 Is the runner accepting jobs? |
|
|
The runner has been offline all day I'm afraid |
@michaelpj made me learn about loop-breakers, hence this PR achieving more or less the same as #4462 and #4463 except without as much burden.
My quick-and-dirty benchmarking suggests that this does give us a speedup, plus the Core definitely looks better.
Manual benchmarking is a pain in the ass, so let's wait for automated benchmarking to arise from the ashes, hopefully that'll happen soonish. Unless of course somebody wants to volunteer.