Skip to content

chore(levm): improve storage_original_values hashmap and gen_db field names #3261

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

Merged
merged 2 commits into from
Jun 23, 2025

Conversation

edg-l
Copy link
Contributor

@edg-l edg-l commented Jun 23, 2025

Motivation

We can avoid a double lookup with storage_original_values by using a tuple.

I found gen_db field names to be a bit confusing and not giving useful information, "immutable_cache" is not really a cache but a store for initial account states, so i changed it. Same with cache.

Description

Closes #issue_number

@edg-l edg-l requested a review from a team as a code owner June 23, 2025 06:02
Copy link

Lines of code report

Total lines added: 7
Total lines removed: 12
Total lines changed: 19

Detailed view
+--------------------------------------------------------------+-------+------+
| File                                                         | Lines | Diff |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/block_producer/payload_builder.rs | 328   | -6   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs                        | 514   | +5   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/gen_db.rs                       | 261   | -6   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/utils.rs                           | 455   | +2   |
+--------------------------------------------------------------+-------+------+

Copy link

github-actions bot commented Jun 23, 2025

Benchmark Results Comparison

PR Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 232.3 ± 0.8 231.7 234.4 1.00
levm_Factorial 721.0 ± 3.9 717.6 731.5 3.10 ± 0.02

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.620 ± 0.022 1.581 1.643 1.00
levm_FactorialRecursive 3.815 ± 0.033 3.776 3.884 2.36 ± 0.04

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 207.3 ± 0.8 206.5 208.9 1.00
levm_Fibonacci 726.4 ± 6.6 720.2 738.8 3.50 ± 0.03

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.7 ± 0.0 8.7 8.8 1.00
levm_ManyHashes 15.6 ± 0.1 15.5 15.8 1.80 ± 0.01

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.189 ± 0.008 3.181 3.203 1.00
levm_BubbleSort 5.356 ± 0.028 5.323 5.413 1.68 ± 0.01

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 243.7 ± 2.5 241.4 248.6 1.00
levm_ERC20Transfer 488.7 ± 2.6 485.0 493.3 2.00 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 137.0 ± 0.6 136.4 138.1 1.00
levm_ERC20Mint 319.9 ± 2.6 317.4 325.5 2.34 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.038 ± 0.003 1.033 1.042 1.00
levm_ERC20Approval 1.879 ± 0.102 1.835 2.169 1.81 ± 0.10

Main Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 235.2 ± 1.1 234.1 237.6 1.00
levm_Factorial 721.9 ± 5.9 716.3 737.6 3.07 ± 0.03

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.612 ± 0.032 1.553 1.647 1.00
levm_FactorialRecursive 3.836 ± 0.067 3.758 3.967 2.38 ± 0.06

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 211.5 ± 2.3 209.9 217.9 1.00
levm_Fibonacci 735.2 ± 21.3 722.3 789.1 3.48 ± 0.11

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.6 ± 0.0 8.6 8.7 1.00
levm_ManyHashes 15.7 ± 0.1 15.5 15.9 1.82 ± 0.02

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.211 ± 0.019 3.195 3.250 1.00
levm_BubbleSort 5.378 ± 0.093 5.329 5.631 1.68 ± 0.03

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 244.4 ± 1.9 241.7 247.6 1.00
levm_ERC20Transfer 487.6 ± 4.0 482.9 494.7 2.00 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 138.0 ± 0.6 137.2 139.2 1.00
levm_ERC20Mint 319.9 ± 2.0 317.5 323.8 2.32 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.045 ± 0.013 1.036 1.081 1.00
levm_ERC20Approval 1.842 ± 0.006 1.833 1.850 1.76 ± 0.02

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

LGTM. That said, I'd prefer if PRs from now on change only one thing at a time.

Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

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

By looking at the benchmark I don't see any improvement. I actually see a 0.4% deterioration of performance. Do we have any noticeable improvements in imports? If not, I'd close this PR.

Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

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

Approving this PR because of readability and no changes to performance.

@edg-l edg-l added this pull request to the merge queue Jun 23, 2025
Merged via the queue into main with commit 939e95f Jun 23, 2025
37 checks passed
@edg-l edg-l deleted the levm_improve_name branch June 23, 2025 13:17
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.

4 participants