Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove gas blocks #391

Merged
merged 9 commits into from
Mar 1, 2023

Conversation

onbjerg
Copy link
Collaborator

@onbjerg onbjerg commented Feb 27, 2023

Removes gas blocks as a concept and replaces analysis with a jump map-only analysis.

Closes #388, also closes #389 using bitvec

@onbjerg onbjerg marked this pull request as ready for review February 27, 2023 16:12
@gakonst
Copy link
Collaborator

gakonst commented Feb 27, 2023

Nice! What does the flamegraph look like before/after @onbjerg? This doesn't reduce perf right?

@onbjerg
Copy link
Collaborator Author

onbjerg commented Feb 27, 2023

The benchmarks I ran from this repo fluctuated wildly for me, both on this branch and master, but they seemed to be about the same. There are probably some optimizations we can do here, but the most important thing is that this unlocks saving the analysis in reth for us, so it is basically a ~30% perf boost there regardless if this is a bit slower.

cc @rakita maybe you can run the benchmarks and tell if it is slower or not

The analysis benchmark in revm-test was faster for this (a lot), the other test (snailtracer) was about the same, but that one fluctuated +-500ms total for both branches (which is a lot considering the entire test was ~2s)

@gakonst
Copy link
Collaborator

gakonst commented Feb 28, 2023

could we also run https://github.com/ziyadedher/evm-bench?

@rakita
Copy link
Member

rakita commented Feb 28, 2023

Few runs,
Snailtracer old:

Snailtracer Host+Interpreter benchmark (2.0s) ...  40_966_324.283 ns/iter (0.996 R²)
Snailtracer Interpreter benchmark (2.2s) ...  39_801_445.345 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.1s) ...  41_884_049.233 ns/iter (0.997 R²)
Snailtracer Interpreter benchmark (2.2s) ...  39_596_354.867 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  42_948_784.517 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.2s) ...  40_548_198.630 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  43_424_133.383 ns/iter (0.997 R²)
Snailtracer Interpreter benchmark (2.2s) ...  40_452_607.309 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  41_005_224.767 ns/iter (0.996 R²)
Snailtracer Interpreter benchmark (2.1s) ...  38_424_680.479 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.4s) ...  42_549_231.861 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.2s) ...  39_077_422.206 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.1s) ...  43_233_361.267 ns/iter (0.997 R²)
Snailtracer Interpreter benchmark (2.2s) ...  39_191_841.606 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.4s) ...  42_033_109.533 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.2s) ...  39_420_830.715 ns/iter (1.000 R²)

Snailtracer new:


Snailtracer Host+Interpreter benchmark (2.0s) ...  41_591_448.633 ns/iter (0.992 R²)
Snailtracer Interpreter benchmark (2.3s) ...  41_239_868.818 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  42_691_983.833 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.2s) ...  40_330_841.697 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  43_282_036.567 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  40_963_185.897 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  43_517_364.250 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  40_984_911.158 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  43_236_385.667 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  41_115_102.461 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  42_307_366.900 ns/iter (0.998 R²)
Snailtracer Interpreter benchmark (2.3s) ...  41_097_052.018 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.1s) ...  43_448_364.767 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  41_287_393.467 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  43_662_096.233 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  41_245_110.279 ns/iter (1.000 R²)

Snailtracer Host+Interpreter benchmark (2.0s) ...  42_780_813.617 ns/iter (0.999 R²)
Snailtracer Interpreter benchmark (2.3s) ...  40_727_529.442 ns/iter (1.000 R²)

it is around 1-2ms slower around 3-5% for snailtracer which is heavy on interpreter. The slowdown is expected, and I am okay with it.

Analysis old:


Raw elapsed time: 367.316519ms
Checked elapsed time: 365.075158ms
Analysed elapsed time: 144.711801ms

Raw elapsed time: 380.111668ms
Checked elapsed time: 373.040297ms
Analysed elapsed time: 146.85897ms

Raw elapsed time: 377.883329ms
Checked elapsed time: 373.885083ms
Analysed elapsed time: 147.505902ms

Raw elapsed time: 378.708501ms
Checked elapsed time: 370.666095ms
Analysed elapsed time: 148.429065ms

Raw elapsed time: 377.108566ms
Checked elapsed time: 369.936979ms
Analysed elapsed time: 144.669355ms

Analysis new:


Raw elapsed time: 322.746998ms
Checked elapsed time: 316.67977ms
Analysed elapsed time: 124.731712ms

Raw elapsed time: 385.438729ms
Checked elapsed time: 352.804895ms
Analysed elapsed time: 144.460865ms

Raw elapsed time: 375.978933ms
Checked elapsed time: 369.375501ms
Analysed elapsed time: 147.478046ms

Raw elapsed time: 375.241596ms
Checked elapsed time: 372.077402ms
Analysed elapsed time: 147.898902ms

Raw elapsed time: 377.414059ms
Checked elapsed time: 368.542605ms
Analysed elapsed time: 145.513484ms

Raw elapsed time: 375.037675ms
Checked elapsed time: 368.56894ms
Analysed elapsed time: 150.668685ms

Analysis seems similar but I would expect a little better performance for new way, will check that part of the code a bit. better.

PR looks great!

@rakita
Copy link
Member

rakita commented Mar 1, 2023

Optimized new analysis:

Raw elapsed time: 195.979682ms
Checked elapsed time: 187.656551ms
Analysed elapsed time: 145.363039ms

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nits, but it lgtm

@rakita rakita merged commit f91d5f9 into bluealloy:main Mar 1, 2023
@onbjerg onbjerg deleted the onbjerg/remove-gas-blocks branch March 2, 2023 14:37
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
This was referenced Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compact jump table Remove block gas optimization
3 participants