Skip to content

refactor(levm): moved retdata from vm to callframe#2694

Merged
JereSalo merged 9 commits into
mainfrom
retdata-in-callframe
May 13, 2025
Merged

refactor(levm): moved retdata from vm to callframe#2694
JereSalo merged 9 commits into
mainfrom
retdata-in-callframe

Conversation

@SDartayet

@SDartayet SDartayet commented May 7, 2025 •
edited by JereSalo
Loading

Copy link
Copy Markdown
Contributor

Motivation

Make code more readable by coupling related behaviour.

Description

Some fields in retdata were repeated in the callframe, and the retdata was always being popped alongside the current call frame. This PR deletes the retdata struct, and moves the fields that weren't redundant to the call frame, refactoring the code where appropriate.

It also removes gas_used from CallFrame::new() because we were always setting it to zero.

Closes #2571, Closes #2720

@github-actions

github-actions Bot commented May 7, 2025 •
edited
Loading

Copy link
Copy Markdown

Lines of code report

Total lines added: 7
Total lines removed: 21
Total lines changed: 28

Detailed view 
+-----------------------------------------------------+-------+------+
| File                                                | Lines | Diff |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/call_frame.rs             | 140   | +7   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/opcode_handlers/system.rs | 779   | -11  |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/vm.rs                     | 415   | -10  |
+-----------------------------------------------------+-------+------+

@github-actions

github-actions Bot commented May 7, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmark Results Comparison

PR Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 232.5 ± 2.4 230.9 239.2 1.00
levm_Factorial 824.5 ± 5.5 819.3 838.5 3.55 ± 0.04

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.446 ± 0.086 1.311 1.554 1.00
levm_FactorialRecursive 13.116 ± 0.237 12.947 13.523 9.07 ± 0.56

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 207.1 ± 0.8 206.2 208.4 1.00
levm_Fibonacci 822.7 ± 11.9 812.2 853.5 3.97 ± 0.06

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.5 ± 0.1 8.5 8.7 1.00
levm_ManyHashes 17.1 ± 0.5 16.9 18.5 2.00 ± 0.06

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.200 ± 0.046 3.161 3.305 1.00
levm_BubbleSort 5.534 ± 0.124 5.450 5.858 1.73 ± 0.05

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 246.5 ± 1.2 244.8 248.0 1.00
levm_ERC20Transfer 497.7 ± 4.0 491.8 506.8 2.02 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 139.5 ± 0.9 138.8 141.7 1.00
levm_ERC20Mint 315.4 ± 1.3 313.4 318.0 2.26 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.078 ± 0.013 1.056 1.102 1.00
levm_ERC20Approval 1.882 ± 0.008 1.870 1.893 1.75 ± 0.02

Main Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 241.7 ± 1.1 240.7 244.2 1.00
levm_Factorial 822.3 ± 8.8 814.1 841.8 3.40 ± 0.04

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.575 ± 0.075 1.448 1.668 1.00
levm_FactorialRecursive 13.096 ± 0.128 12.986 13.385 8.31 ± 0.41

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 212.9 ± 0.7 211.5 213.9 1.00
levm_Fibonacci 828.2 ± 10.4 813.4 842.9 3.89 ± 0.05

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.8 ± 0.1 8.7 9.1 1.00
levm_ManyHashes 17.2 ± 0.1 17.2 17.4 1.95 ± 0.02

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.237 ± 0.014 3.226 3.264 1.00
levm_BubbleSort 5.533 ± 0.037 5.487 5.589 1.71 ± 0.01

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 253.3 ± 4.4 249.4 264.1 1.00
levm_ERC20Transfer 501.4 ± 4.7 497.4 511.5 1.98 ± 0.04

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 142.6 ± 1.6 141.0 145.4 1.00
levm_ERC20Mint 320.4 ± 3.9 317.0 329.7 2.25 ± 0.04

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.051 ± 0.013 1.034 1.070 1.00
levm_ERC20Approval 1.885 ± 0.010 1.871 1.897 1.79 ± 0.02

@SDartayet SDartayet marked this pull request as ready for review May 8, 2025 21:00
@SDartayet SDartayet requested a review from a team as a code owner May 8, 2025 21:00
@SDartayet SDartayet marked this pull request as draft May 8, 2025 21:10
@SDartayet SDartayet force-pushed the retdata-in-callframe branch from 3164e05 to d6b70c2 Compare May 9, 2025 14:38
@SDartayet SDartayet marked this pull request as ready for review May 9, 2025 14:40
JereSalo
JereSalo approved these changes May 12, 2025
View reviewed changes

@JereSalo JereSalo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like that we don't need RetData anymore

rodrigo-o
rodrigo-o approved these changes May 13, 2025
View reviewed changes

@rodrigo-o rodrigo-o left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good! Just a small nit

Comment thread crates/vm/levm/src/opcode_handlers/system.rs
@JereSalo JereSalo enabled auto-merge May 13, 2025 15:11
@JereSalo JereSalo added this pull request to the merge queue May 13, 2025
Merged via the queue into main with commit ce61df3 May 13, 2025
34 checks passed
@JereSalo JereSalo deleted the retdata-in-callframe branch May 13, 2025 15:22
fmoletta pushed a commit that referenced this pull request May 15, 2025
@SDartayet @JereSalo @fmoletta
refactor(levm): moved retdata from vm to callframe (#2694)
de4ff43 
**Motivation**

Make code more readable by coupling related behaviour.

**Description**

Some fields in retdata were repeated in the callframe, and the retdata
was always being popped alongside the current call frame. This PR
deletes the retdata struct, and moves the fields that weren't redundant
to the call frame, refactoring the code where appropriate.

It also removes `gas_used` from `CallFrame::new()` because we were
always setting it to zero.

Closes #2571, Closes #2720

---------

Co-authored-by: JereSalo <jeresalo17@gmail.com>
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
@SDartayet @JereSalo
refactor(levm): moved retdata from vm to callframe (lambdaclass#2694)
59470f1 
**Motivation**

Make code more readable by coupling related behaviour.

**Description**

Some fields in retdata were repeated in the callframe, and the retdata
was always being popped alongside the current call frame. This PR
deletes the retdata struct, and moves the fields that weren't redundant
to the call frame, refactoring the code where appropriate.

It also removes `gas_used` from `CallFrame::new()` because we were
always setting it to zero.

Closes lambdaclass#2571, Closes lambdaclass#2720

---------

Co-authored-by: JereSalo <jeresalo17@gmail.com>
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JereSalo JereSalo JereSalo approved these changes

+1 more reviewer

@rodrigo-o rodrigo-o rodrigo-o approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LEVM: Simplify RetData and move it to CallFrame LEVM: see if StateBackup and/or RetData can be call frame attributes

3 participants

@SDartayet @rodrigo-o @JereSalo