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

evmrs: add benchmarks #821

Merged
merged 1 commit into from
Oct 1, 2024
Merged

evmrs: add benchmarks #821

merged 1 commit into from
Oct 1, 2024

Conversation

LorenzSchueler
Copy link
Collaborator

@LorenzSchueler LorenzSchueler commented Sep 30, 2024

This PR adds the benchmarks which contains benchmarks (currently only ffi_overhead) for calling from Rust via FFI into evmrs.

The benchmarks can be used in two ways:

  • with criterion to compare different features:
    cargo bench --package benchmarks --profile profiling [--features <feature1>,<feature2>,...]
  • as a standalone executable for profiling
    cargo build --package benchmarks --profile profiling [--features <feature1>,<feature2>,...]
    then use ./target/profiling/benchmarks with the profiler of your choice

Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

Unfortunately, it is still not clear why we need so many benchmark functions.
Can you write an example of the use of the main function in benchmarks/src/main.rs
I find difficult to justify different entry points to the program.

rust/benchmarks/benches/criterion.rs Outdated Show resolved Hide resolved
rust/benchmarks/src/main.rs Outdated Show resolved Hide resolved
rust/benchmarks/benches/criterion.rs Outdated Show resolved Hide resolved
@LorenzSchueler
Copy link
Collaborator Author

We need two entry points because with criterion be get statistics about the benchmarks (in particular accurate timing data), but criterion also has some overhead. main.rs executes the same benchmark function(s) but without criterion is therefore better suited for profiling e.g. with perf.

LuisPH3
LuisPH3 previously approved these changes Oct 1, 2024
rust/benchmarks/benches/criterion.rs Outdated Show resolved Hide resolved
@LorenzSchueler
Copy link
Collaborator Author

You could have multiple files but as long as they all use criterion that does not make too much sense because you can already filter the tests within one test binary. The only thing you would gain is that they are compiled as two separate executables. But they would both depend on benchmarks/src/lib.rs anyway.

@LorenzSchueler
Copy link
Collaborator Author

I renamed the file to interpreter.rs because all tests in here will test the whole interpreter and not just parts of it.

Copy link
Contributor

@LuisPH3 LuisPH3 left a comment

Choose a reason for hiding this comment

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

👍

@LorenzSchueler LorenzSchueler merged commit cb319d2 into main Oct 1, 2024
9 checks passed
@LorenzSchueler LorenzSchueler deleted the evmrs-benchmarks branch October 1, 2024 08:53
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