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

tests: migrate revme benchmarks to Criterion #1569

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

Conversation

JacekGlen
Copy link

@JacekGlen JacekGlen commented Jun 26, 2024

This PR migrates the existing revme evm --bench tool to Criterion. The benefits of doing so include:

  • more stable results
  • detailed reports
  • automatic performance degradation detection

The main motivation for this change is that the tool can be used in gas-cost-estimator project, but its purpose goes beyond it. You can learn more about the project from this short presentation, or visit the wiki Stage 4 Scope.

In the future, I'll extend the tool to run on a preloaded state, which is useful for testing scenarios with opcodes like DELEGATECALL or SLOAD.

@rakita
Copy link
Member

rakita commented Jun 27, 2024

There is already a cli that allows you to run bytecode in revme: https://github.com/bluealloy/revm/blob/b3f31fc6f2fa0c15f7287ac83ba608a8513ffef1/bins/revme/src/cmd.rs#L21C5-L21C25

and even has a --bench flag.

if self.bench {
// Microbenchmark
let bench_options = microbench::Options::default().time(Duration::from_secs(3));
microbench::bench(&bench_options, "Run bytecode", || {
let _ = evm.transact().unwrap();
});
return Ok(());
}

@rakita
Copy link
Member

rakita commented Jun 27, 2024

Wouldn't mind replacing it with criterion

@JacekGlen
Copy link
Author

There is already a cli that allows you to run bytecode in revme: https://github.com/bluealloy/revm/blob/b3f31fc6f2fa0c15f7287ac83ba608a8513ffef1/bins/revme/src/cmd.rs#L21C5-L21C25

and even has a --bench flag.

if self.bench {
// Microbenchmark
let bench_options = microbench::Options::default().time(Duration::from_secs(3));
microbench::bench(&bench_options, "Run bytecode", || {
let _ = evm.transact().unwrap();
});
return Ok(());
}

I missed that, this is cool!

I'll update my PR to use criterion in revme, as suggested. This gives much more stable results.

@JacekGlen JacekGlen changed the title tests: add benchmark for bytecode execution tests: migrate revme benchmarks to Criterion Jul 3, 2024
@JacekGlen
Copy link
Author

Wouldn't mind replacing it with criterion

Hi @rakita , I've updates the existing revme benchmark with Criterion. Have a look at the change now, please.

Comment on lines 128 to 136
let evm = Evm::builder()
.with_db(BenchmarkDB::new_bytecode(Bytecode::new()))
.modify_tx_env(|tx| {
tx.caller = address!("1000000000000000000000000000000000000000");
tx.transact_to = TxKind::Call(address!("0000000000000000000000000000000000000000"));
})
.build();

let host = DummyHost::new(*evm.context.evm.env.clone());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let evm = Evm::builder()
.with_db(BenchmarkDB::new_bytecode(Bytecode::new()))
.modify_tx_env(|tx| {
tx.caller = address!("1000000000000000000000000000000000000000");
tx.transact_to = TxKind::Call(address!("0000000000000000000000000000000000000000"));
})
.build();
let host = DummyHost::new(*evm.context.evm.env.clone());
let mut env = Env::default();
env.tx.caller = address!("1000000000000000000000000000000000000000");
env.tx.transact_to = TxKind::Call(address!("0000000000000000000000000000000000000000"));
let host = DummyHost::new(env);

Dont need builder here.

criterion_group.bench_function("bytecode", |b| {
b.iter(|| {
Interpreter::new(contract.clone(), u64::MAX, false).run(
EMPTY_SHARED_MEMORY,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EMPTY_SHARED_MEMORY,
SharedMemory::new();

Shared memory is preallocated and shared between multiple calls, so having SharedMemory::new() will have an initialization hit but it will be slightly faster on memory expansion.

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.

left few nits. lgtm otherwise

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.

lgtm

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.

2 participants