Skip to content

refactor(levm): move restore_cache_state() to call_frame.rs #2734

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

Closed
wants to merge 12 commits into from

Conversation

DiegoCivi
Copy link
Contributor

@DiegoCivi DiegoCivi commented May 9, 2025

Motivation

Declutter vm.rs

Description

The function restore_cache_state() was moved to call_frame.rs where it makes more sense.

Closes #2728

Copy link

github-actions bot commented May 9, 2025

Lines of code report

Total lines added: 66
Total lines removed: 59
Total lines changed: 125

Detailed view
+-----------------------------------------+-------+------+
| File                                    | Lines | Diff |
+-----------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/call_frame.rs | 206   | +66  |
+-----------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/vm.rs         | 277   | -59  |
+-----------------------------------------+-------+------+

Copy link

github-actions bot commented May 9, 2025

Benchmark Results Comparison

PR Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 248.0 ± 3.1 242.8 253.6 1.00
levm_Factorial 821.9 ± 14.9 807.7 854.6 3.31 ± 0.07

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.413 ± 0.073 1.338 1.554 1.00
levm_FactorialRecursive 12.990 ± 0.141 12.872 13.266 9.19 ± 0.49

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 213.7 ± 3.8 210.7 223.6 1.00
levm_Fibonacci 818.5 ± 8.5 807.0 834.9 3.83 ± 0.08

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.8 ± 0.1 8.7 8.9 1.00
levm_ManyHashes 17.1 ± 0.2 16.9 17.5 1.95 ± 0.02

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.235 ± 0.022 3.218 3.291 1.00
levm_BubbleSort 5.563 ± 0.094 5.442 5.722 1.72 ± 0.03

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 250.2 ± 2.1 247.8 253.7 1.00
levm_ERC20Transfer 495.0 ± 2.7 491.0 498.9 1.98 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 142.2 ± 1.0 141.2 144.3 1.00
levm_ERC20Mint 321.2 ± 4.3 316.6 328.5 2.26 ± 0.03

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.035 ± 0.008 1.025 1.048 1.00
levm_ERC20Approval 1.870 ± 0.010 1.857 1.885 1.81 ± 0.02

Main Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 240.1 ± 1.6 238.5 243.3 1.00
levm_Factorial 813.4 ± 7.0 803.5 827.6 3.39 ± 0.04

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.435 ± 0.090 1.335 1.565 1.00
levm_FactorialRecursive 12.914 ± 0.035 12.863 12.982 9.00 ± 0.56

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 212.1 ± 5.7 207.0 227.9 1.00
levm_Fibonacci 815.2 ± 11.5 804.9 838.0 3.84 ± 0.12

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.8 ± 0.1 8.7 9.2 1.00
levm_ManyHashes 17.1 ± 0.4 16.9 18.2 1.95 ± 0.06

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.239 ± 0.031 3.194 3.315 1.00
levm_BubbleSort 5.504 ± 0.029 5.453 5.540 1.70 ± 0.02

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 249.5 ± 3.8 246.4 259.2 1.00
levm_ERC20Transfer 494.8 ± 5.1 489.8 506.8 1.98 ± 0.04

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 141.2 ± 1.2 139.5 143.2 1.00
levm_ERC20Mint 316.3 ± 1.5 314.3 319.7 2.24 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.032 ± 0.009 1.023 1.057 1.00
levm_ERC20Approval 1.858 ± 0.007 1.847 1.871 1.80 ± 0.02

@DiegoCivi DiegoCivi marked this pull request as ready for review May 9, 2025 21:45
@DiegoCivi DiegoCivi requested a review from a team as a code owner May 9, 2025 21:45
@JereSalo
Copy link
Contributor

Maybe we could handle everything related to callframe backups in callframe.rs because gen_db.rs in itself is going to big enough so if we can remove some things from there it would be great.
I'd also move there merge_call_frame_backup_with_parent() and those kinds of functions maybe.
Main reason is that gen_db.rs is going to have an addition soon (It's being worked on this PR)

@JereSalo
Copy link
Contributor

Oh, I think I didn't explain myself clearly.
I wanted to move the function to call_frame.rs but still keep it as part of VM. Like do an impl VM in that file.
We can talk about this and decide what the best thing to do is later.

@DiegoCivi
Copy link
Contributor Author

DiegoCivi commented May 13, 2025

Ohh, my bad. When I was implementing this change, I thought it didn’t make much sense. Now that I understand, it does.

@DiegoCivi DiegoCivi changed the title refactor(levm): move restore_cache_state() to gen_db.rs refactor(levm): move restore_cache_state() to call_frame.rs May 13, 2025
@JereSalo
Copy link
Contributor

Closing this because it's being addressed in #2796

@JereSalo JereSalo closed this May 16, 2025
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.

Move restore_cache_state to call_frame
2 participants