chore(l1): move all benchmarks to benches/ folder#6472
chore(l1): move all benchmarks to benches/ folder#6472azteca1998 wants to merge 1 commit intomainfrom
Conversation
- Move RLP decode benchmarks to benches/benches/rlp_decode.rs - Move RLP encode benchmarks to benches/benches/rlp_encode.rs - Move P2P decode benchmarks to benches/benches/p2p_decode.rs - Update benches/Cargo.toml with new dependencies and benchmark targets - Remove benchmark entries from crates/common/rlp/Cargo.toml - Remove benchmark entry from crates/networking/p2p/Cargo.toml - Delete old benchmark directories from crates/ This consolidates all benchmarks in a single location for easier maintenance and avoids cluttering individual crate directories.
🤖 Claude Code ReviewPR Review:
|
azteca1998
changed the title
🤖 Kimi Code ReviewReview: Benchmark Centralization & Criterion Upgrade This PR consolidates benchmarks into a dedicated Critical Issues1. Potential Broken Imports in Moved Benchmarks (
Medium Risk2. Criterion API Breaking Changes (0.5 → 0.7) (
3. Orphaned Unit Tests in Source Crates (
Minor Issues4. Unused Dependency Check ( 5. Criterion Dependency Classification ( Positive Observations
SummaryThe PR is architecturally sound but requires verification that:
Recommendation: Run Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. This PR is benchmark-only: it moves the RLP and P2P Criterion benches into the workspace Residual risk: I could not fully compile the benches in this sandbox because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewOverall Assessment: This is a straightforward refactoring PR that consolidates benchmarks from individual crates into a centralized Issues to Address: 1. Import Path Compatibility in Moved Benchmarks (Critical)
2. Remaining Test Dependencies (High)
3. Dependency Version Consistency (Medium)
Positive Aspects:
Verification Checklist:
The refactoring pattern is sound, but requires validation that the moved code doesn't rely on Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo material findings. This PR is bench-only plumbing: it moves the RLP/P2P benchmark targets into the workspace One non-blocking maintainability note: benches/Cargo.toml:23 introduces Testing gap: I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough context for a thorough review. Here are my findings: PR Review: chore(l1): move all benchmarks to benches/ folderOverall: This is a clean consolidation with no correctness issues. There are a few dependency placement concerns worth addressing.
|
Greptile SummaryThis PR consolidates three benchmark files — previously spread across Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| benches/Cargo.toml | Adds ethrex-rlp, ethrex-trie, ethrex-p2p, rand 0.9.2, and once_cell as [dependencies] (not [dev-dependencies]); upgrades criterion to 0.7.0; adds three new [[bench]] targets. rand 0.9.2 duplicates the workspace rand 0.8.5, and both rand + once_cell belong in [dev-dependencies] for a bench-only crate. |
| benches/benches/p2p_decode.rs | Moved from crates/networking/p2p/benches/decode.rs; uses rand 0.8 API (thread_rng(), r#gen()) against the rand 0.9.2 dependency declared in benches/Cargo.toml, producing deprecation warnings as noted by the author. |
| benches/benches/rlp_decode.rs | Moved from crates/common/rlp/benches/decode.rs; cleanly uses rand 0.9 API throughout, no issues found. |
| benches/benches/rlp_encode.rs | Moved from crates/common/rlp/benches/encode.rs; cleanly uses rand 0.9 API, all bench functions are registered in criterion_group!, no issues found. |
| crates/common/rlp/Cargo.toml | Benchmark-related dev-dependencies (criterion, rand, once_cell, ethrex-trie) removed as expected; only hex-literal remains for unit tests. |
| crates/networking/p2p/Cargo.toml | Benchmark [[bench]] entry and associated dev-dependencies removed; no functional dependencies affected. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before (scattered)"]
A["crates/common/rlp/benches/decode.rs"]
B["crates/common/rlp/benches/encode.rs"]
C["crates/networking/p2p/benches/decode.rs"]
end
subgraph After["After (consolidated)"]
D["benches/benches/rlp_decode.rs"]
E["benches/benches/rlp_encode.rs"]
F["benches/benches/p2p_decode.rs"]
G["benches/benches/build_block_benchmark.rs"]
end
subgraph Deps["benches/Cargo.toml dependencies added"]
H["ethrex-rlp (workspace)"]
I["ethrex-trie (workspace)"]
J["ethrex-p2p (workspace)"]
K["rand 0.9.2"]
L["once_cell 1"]
M["criterion 0.7.0 (upgraded)"]
end
A -->|moved| D
B -->|moved| E
C -->|moved| F
H --> D & E
I --> D & E
J --> F
K --> D & E & F
L --> D & E
M --> D & E & F & G
Comments Outside Diff (1)
-
benches/benches/p2p_decode.rs, line 23 (link)Deprecated rand 0.8 API used with rand 0.9 dependency
rand::thread_rng()andr#gen()are deprecated in rand 0.9 (superseded byrand::rng()andrandom()respectively). Since the PR author already acknowledged these warnings, consider updating these calls to the 0.9 API at the same time to keep the benchmark consistent withrlp_decode.rsandrlp_encode.rs.And replace each
.r#gen()call with.random().Prompt To Fix With AI
This is a comment left during a code review. Path: benches/benches/p2p_decode.rs Line: 23 Comment: **Deprecated rand 0.8 API used with rand 0.9 dependency** `rand::thread_rng()` and `r#gen()` are deprecated in rand 0.9 (superseded by `rand::rng()` and `random()` respectively). Since the PR author already acknowledged these warnings, consider updating these calls to the 0.9 API at the same time to keep the benchmark consistent with `rlp_decode.rs` and `rlp_encode.rs`. And replace each `.r#gen()` call with `.random()`. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: benches/Cargo.toml
Line: 23-24
Comment:
**rand and once_cell belong in `[dev-dependencies]`**
`rand` and `once_cell` are only used in benchmark files, so they should live under `[dev-dependencies]` alongside `criterion`. Placing them in `[dependencies]` makes them compile-time dependencies of the crate for all targets. The workspace already declares `rand = "0.8.5"`; using `rand.workspace = true` in `[dev-dependencies]` (or bumping the workspace version to 0.9) would also avoid carrying two semver-incompatible copies of rand in the build graph.
```suggestion
rand = { version = "0.9.2" }
once_cell = "1"
```
Move both lines into `[dev-dependencies]`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: benches/benches/p2p_decode.rs
Line: 23
Comment:
**Deprecated rand 0.8 API used with rand 0.9 dependency**
`rand::thread_rng()` and `r#gen()` are deprecated in rand 0.9 (superseded by `rand::rng()` and `random()` respectively). Since the PR author already acknowledged these warnings, consider updating these calls to the 0.9 API at the same time to keep the benchmark consistent with `rlp_decode.rs` and `rlp_encode.rs`.
```suggestion
let mut rng = rand::rng();
```
And replace each `.r#gen()` call with `.random()`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore: move all benchmarks to benches/ f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rand = "0.9.2" | ||
| once_cell = "1" |
There was a problem hiding this comment.
[dev-dependencies]
rand and once_cell are only used in benchmark files, so they should live under [dev-dependencies] alongside criterion. Placing them in [dependencies] makes them compile-time dependencies of the crate for all targets. The workspace already declares rand = "0.8.5"; using rand.workspace = true in [dev-dependencies] (or bumping the workspace version to 0.9) would also avoid carrying two semver-incompatible copies of rand in the build graph.
| rand = "0.9.2" | |
| once_cell = "1" | |
| rand = { version = "0.9.2" } | |
| once_cell = "1" |
Move both lines into [dev-dependencies].
Prompt To Fix With AI
This is a comment left during a code review.
Path: benches/Cargo.toml
Line: 23-24
Comment:
**rand and once_cell belong in `[dev-dependencies]`**
`rand` and `once_cell` are only used in benchmark files, so they should live under `[dev-dependencies]` alongside `criterion`. Placing them in `[dependencies]` makes them compile-time dependencies of the crate for all targets. The workspace already declares `rand = "0.8.5"`; using `rand.workspace = true` in `[dev-dependencies]` (or bumping the workspace version to 0.9) would also avoid carrying two semver-incompatible copies of rand in the build graph.
```suggestion
rand = { version = "0.9.2" }
once_cell = "1"
```
Move both lines into `[dev-dependencies]`.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Summary
Consolidates all benchmarks into the
benches/folder for easier maintenance and cleaner crate structure.Changes
Moved benchmark files:
crates/common/rlp/benches/decode.rs→benches/benches/rlp_decode.rscrates/common/rlp/benches/encode.rs→benches/benches/rlp_encode.rscrates/networking/p2p/benches/decode.rs→benches/benches/p2p_decode.rsUpdated
benches/Cargo.toml:ethrex-rlp,ethrex-trie,ethrex-p2p,ethereum-types,rand = "0.9.2",once_cell = "1"criterionto version0.7.0(to match RLP benchmarks)rlp_decode,rlp_encode, andp2p_decodeCleaned up crate Cargo.tomls:
crates/common/rlp/Cargo.tomlcrates/networking/p2p/Cargo.tomlBenefits
benches/structureTest Plan
cargo check -p ethrex-benches --benchescompiles cleanlyNotes
The
p2p_decode.rsfile has deprecation warnings due to using old rand 0.8 API. This is expected and doesn't affect functionality. We can update it later to use the newer rand 0.9 API for consistency.