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

feat: Add JournalInner #2311

Merged
merged 9 commits into from
Mar 26, 2025
Merged

feat: Add JournalInner #2311

merged 9 commits into from
Mar 26, 2025

Conversation

rakita
Copy link
Member

@rakita rakita commented Mar 25, 2025

Introduce JournalInner that extracts field and logic from journal

Copy link

codspeed-hq bot commented Mar 25, 2025

CodSpeed Performance Report

Merging #2311 will degrade performances by 5.28%

Comparing rakita/journal_inner (ef62acc) with main (6f38322)

Summary

❌ 2 regressions
✅ 6 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
analysis 58.6 µs 60.4 µs -2.95%
transfer 25 µs 26.4 µs -5.28%

rakita and others added 4 commits March 26, 2025 01:40
* add read_scalar, point_add and point_mul into the wrapper

* modify bn128.rs to use new api methods

* preserve previous semantics

* initial commit to add matter-labs wrapper

* feature gate matter-labs impl -- make substrate impl still default

* update revm and precompile cargo.toml file

* use cfg_if

* make bn an optional dependency

* cfg else -> else_if so that there is no silent fallback

* follow same cfg_if pattern as other precompiles

* fix optimism

* add back `self` import

* Push empty commit to trigger CI

* cargo fmt

* clippy fix

* make `bn` the default with revm and revme

* fix typo

* Update crates/revm/Cargo.toml

* Update crates/revm/Cargo.toml

* Update crates/precompile/src/lib.rs

* Update crates/optimism/Cargo.toml

* revert Cargo.toml formatting

* multi:

- remove bn as optional
- if default-features and matter-labs-eip1962 feature is enabled, then we choose matter-labs
- remove conditional configs in other  parts of code since bn is always available

* revert crates/optimism/src/precompiles.rs

* revert crates/optimism/src/evm.rs

* revert unnecessary changes in crates/precompile/src/lib.rs

* revert Cargo.toml changes

* revert automatic Cargo.toml formatting

* revert formatting on secp256k1 in Cargo.toml

* revert c-kzg formatting in Cargo.toml

* revert dev key in Cargo.toml

* revert c-kzg feature in Cargo.toml

* Update crates/precompile/Cargo.toml

* Apply suggestions from code review

* initial commit of arkworks file -- replace matter-labs

* add arkworks depedency

* grep replace matter-labs with arkworks

* make substrate-bn impl optional

* propagate arkworks/std

* add comment for substrate-bn impl

* Update crates/precompile/src/bn128/arkworks.rs

---------
* temp code preview

* add criterion to benchmarks

* added args to fn run() calls in bins/revme/

* modifed the code inside the benchmark tests

* left only .replay function inside bench_functions and added minor revisions to the rest of the code

* edited comment annotation in burntpix.rs file

* added comments and set different default value for warm_up_time
@rakita rakita force-pushed the rakita/journal_inner branch from 3e2bcf5 to 8bd0eb2 Compare March 26, 2025 00:40
@rakita rakita marked this pull request as ready for review March 26, 2025 11:05
@rakita
Copy link
Member Author

rakita commented Mar 26, 2025

There was no major changes to logic but perf degraded by few us. The part that is shown in codspeed as red, I would like to optimize in different PR, to clone precompiles only when spec is different. Both of regresions seems related to HashMap inserts but it does not show what part of code it is.

@rakita rakita merged commit 079ff3e into main Mar 26, 2025
28 of 29 checks passed
@rakita rakita deleted the rakita/journal_inner branch March 26, 2025 11:45
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.

4 participants