Skip to content

perf(levm): reuse stack pool #5179

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

perf(levm): reuse stack pool #5179

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3b636e
reuse stack pool
iovoid Nov 4, 2025
c8a22d7
Merge branch 'main' into reuse_stack_pool
jrchatruc Nov 6, 2025
6ae8184
Merge branch 'main' into reuse_stack_pool
lferrigno Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion crates/vm/backends/levm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use ethrex_common::{
},
};
use ethrex_levm::EVMConfig;
use ethrex_levm::call_frame::Stack;
use ethrex_levm::constants::{SYS_CALL_GAS_LIMIT, TX_BASE_COST};
use ethrex_levm::db::gen_db::GeneralizedDatabase;
use ethrex_levm::errors::{InternalError, TxValidationError};
Expand Down Expand Up @@ -99,6 +100,8 @@ impl LEVM {
) -> Result<BlockExecutionResult, EvmError> {
Self::prepare_block(block, db, vm_type)?;

let mut shared_stack_pool = Vec::with_capacity(1024);
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The magic number 1024 for the stack pool capacity is not explained. Consider either adding a comment explaining why this specific capacity was chosen, or extracting it as a named constant (e.g., STACK_POOL_CAPACITY) to improve code readability and maintainability.

Copilot uses AI. Check for mistakes.

let mut receipts = Vec::new();
let mut cumulative_gas_used = 0;

Expand All @@ -113,7 +116,14 @@ impl LEVM {
)));
}

let report = Self::execute_tx(tx, tx_sender, &block.header, db, vm_type)?;
let report = Self::execute_tx_in_block(
tx,
tx_sender,
&block.header,
db,
vm_type,
&mut shared_stack_pool,
)?;
LEVM::send_state_transitions_tx(&merkleizer, db, queue_length)?;

cumulative_gas_used += report.gas_used;
Expand Down Expand Up @@ -231,6 +241,26 @@ impl LEVM {
vm.execute().map_err(VMError::into)
}

pub fn execute_tx_in_block(
// The transaction to execute.
tx: &Transaction,
// The transactions recovered address
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Grammar issue in comment: "The transactions recovered address" should be "The transaction's recovered address" (possessive form).

Copilot uses AI. Check for mistakes.
tx_sender: Address,
// The block header for the current block.
block_header: &BlockHeader,
db: &mut GeneralizedDatabase,
vm_type: VMType,
stack_pool: &mut Vec<Stack>,
) -> Result<ExecutionReport, EvmError> {
Comment on lines +244 to +254
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

This new public function is missing a documentation comment. Consider adding a doc comment explaining the purpose of this function and how it differs from execute_tx, particularly the role of the stack_pool parameter in optimizing stack allocations across multiple transaction executions within a block.

Copilot uses AI. Check for mistakes.
let env = Self::setup_env(tx, tx_sender, block_header, db, vm_type)?;
let mut vm = VM::new(env, db, tx, LevmCallTracer::disabled(), vm_type)?;

std::mem::swap(&mut vm.stack_pool, stack_pool);
let result = vm.execute().map_err(VMError::into);
std::mem::swap(&mut vm.stack_pool, stack_pool);
Comment on lines +258 to +260
Copy link
Contributor

@pablodeymo pablodeymo Nov 6, 2025

Choose a reason for hiding this comment

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

I think it's better to use std::mem::take at the beggining and then take. We don't need to actually swap.

Comment on lines +258 to +260
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

If vm.execute() panics, the second std::mem::swap won't execute, potentially leaving the stack_pool in an inconsistent state. Consider using a guard pattern or RAII wrapper to ensure the swap-back happens even in case of panics, or document that panic safety is not guaranteed here.

Suggested change
std::mem::swap(&mut vm.stack_pool, stack_pool);
let result = vm.execute().map_err(VMError::into);
std::mem::swap(&mut vm.stack_pool, stack_pool);
struct StackPoolGuard<'a> {
a: &'a mut Vec<Stack>,
b: &'a mut Vec<Stack>,
}
impl<'a> Drop for StackPoolGuard<'a> {
fn drop(&mut self) {
std::mem::swap(self.a, self.b);
}
}
std::mem::swap(&mut vm.stack_pool, stack_pool);
let _guard = StackPoolGuard { a: &mut vm.stack_pool, b: stack_pool };
let result = vm.execute().map_err(VMError::into);

Copilot uses AI. Check for mistakes.
result
}

pub fn undo_last_tx(db: &mut GeneralizedDatabase) -> Result<(), EvmError> {
db.undo_last_transaction()?;
Ok(())
Expand Down
Loading