feat(bench): add additional benchmarks#1240
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Criterion-based benchmark targets and multiple new benchmark binaries across many crates (Cargo.toml dev-dependency and [[bench]] entries) and adds new Criterion benchmark source files under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/reward-curve/benches/reward_bench.rs`:
- Around line 30-35: The benchmark case name "large_t_underflow" is misleading
because at 240 years the decay factor is tiny but positive (decay_fp18 <
TOKEN_SCALE) so no underflow occurs; rename the case string "large_t_underflow"
to a clearer name such as "large_t" or "far_future" wherever it appears in the
benches (the tuple entry containing SECS_PER_YEAR, TOKEN_SCALE, decay_fp18 and
the string label), and update any test assertions, logs, or match arms that
reference "large_t_underflow" so they match the new name while leaving the
benchmark logic (use of SECS_PER_YEAR, safe_sub, decay_fp18) unchanged.
In `@crates/types/benches/pricing_bench.rs`:
- Around line 17-19: The helper function make_config() currently returns
ConsensusConfig::testnet(), which violates the guideline to use
ConsensusConfig::testing() for test/test-bench consistency; update make_config()
to return ConsensusConfig::testing() instead (replace ConsensusConfig::testnet()
with ConsensusConfig::testing() in the make_config function), unless testnet()
was intentionally chosen for production-like benchmarks—if so, add an inline
comment explaining the intent.
In `@crates/types/benches/solution_hash_bench.rs`:
- Line 9: Replace the hardcoded CHUNK_SIZE constant in the benchmark with the
canonical MAX_CHUNK_SIZE exported from the merkle module: remove or replace the
existing const CHUNK_SIZE: u64 = 256 * 1024; and import merkle::MAX_CHUNK_SIZE
(or the correct path to MAX_CHUNK_SIZE) and use that value (cast to u64 if
necessary) so the benchmark stays synchronized with production chunk size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d73704d1-524e-4f9b-860e-434dfdfc847a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/efficient-sampling/Cargo.tomlcrates/efficient-sampling/benches/sampling_bench.rscrates/irys-reth/Cargo.tomlcrates/irys-reth/benches/shadow_tx_bench.rscrates/packing/Cargo.tomlcrates/packing/benches/packing_bench.rscrates/reward-curve/Cargo.tomlcrates/reward-curve/benches/reward_bench.rscrates/types/Cargo.tomlcrates/types/benches/merkle_bench.rscrates/types/benches/pricing_bench.rscrates/types/benches/solution_hash_bench.rscrates/vdf/benches/vdf_bench.rs
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/efficient-sampling/benches/sampling_bench.rs`:
- Around line 61-70: Replace the current b.iter_custom loop with Criterion's
b.iter_batched so allocation of the Ranges buffer is done in the setup closure
and only ranges.reconstruct(&step_seeds, &partition_hash) is timed;
specifically, in the replacement use a setup closure that creates let ranges =
Ranges::new(64_840) (and captures or clones step_seeds/partition_hash as needed)
and a bench closure that calls ranges.reconstruct(&step_seeds, &partition_hash)
(or mutates a passed-in Ranges) and returns the measured result, using an
appropriate BatchSize so the allocation cost from Ranges::new is not included in
the measured reconstruct time.
- Around line 36-45: The benchmark currently uses b.iter_custom with
per-iteration setup calling Ranges::new and then timing
ranges.next_recall_range, which still incurs allocation/cache noise; replace
this pattern with Criterion's b.iter_batched (or iter_batched_ref) to separate
setup from timing: prepare Ranges::new in the setup closure and return it to the
timed closure that calls ranges.next_recall_range(1, &seeds[0],
&partition_hash), choosing SmallInput or LargeInput as appropriate and adjusting
batch size so allocations are not measured; update the bench function to call
b.iter_batched/setup and move creation of Ranges into the setup closure while
keeping only next_recall_range inside the timed closure.
In `@crates/irys-reth/benches/shadow_tx_bench.rs`:
- Around line 23-64: Bench closures in
bench_encode/bench_decode_roundtrip/bench_detect_and_decode currently omit
criterion::black_box and don't consume reject-path results which lets the
optimizer elide work and hides failures; update the closures that call
encode_prefixed_input, try_decode_prefixed, and detect_and_decode_from_parts
(and the reject-path benches) to wrap inputs and outputs with
criterion::black_box and to explicitly consume/assert results (use expect/unwrap
for success paths and assert_err or unwrap_err/black_box the Err for reject
paths) so the compiler cannot optimize away the measured work and failures are
surfaced; reference functions/values: bench_encode, bench_decode_roundtrip,
bench_detect_and_decode, encode_prefixed_input, try_decode_prefixed,
detect_and_decode_from_parts, make_block_reward_tx, SHADOW_TX_DESTINATION_ADDR.
In `@crates/packing/benches/packing_bench.rs`:
- Around line 71-83: The benchmark bench_packing_xor_owned calls
packing_xor_vec_u8 and ignores its return which allows the optimizer to elide
work; fix by passing the function's return through criterion::black_box so the
result is observed (use black_box around the return value inside the bench.iter
closure). Update the bench_packing_xor_owned closure to consume
packing_xor_vec_u8(...) via black_box to prevent optimization; reference the
functions bench_packing_xor_owned and packing_xor_vec_u8.
- Around line 181-182: The C-batch benchmark currently allocates and mutates the
same Vec<Vec<u8>> via let mut data: Vec<Vec<u8>> = (0..batch_size).map(|_|
vec![0_u8; CHUNK_SIZE]).collect(); which can bias results; change this benchmark
to use Criterion's iter_batched (or iter_batched_ref) pattern similar to the
other batch benchmark: create the fresh data in the setup closure (allocating
batch_size buffers of CHUNK_SIZE) and then mutate/process it in the test
closure, referencing the same identifiers (data, CHUNK_SIZE, batch_size) so the
allocation and mutation are separated between setup and measured phases and the
code path mirrors the other benchmark for consistent results.
- Around line 153-154: The benchmark currently creates `data: Vec<Vec<u8>>` once
(using `CHUNK_SIZE` and `batch_size`) and then calls
`capacity_pack_range_with_data`, which mutates the buffers, leading to
subsequent iterations operating on already-packed data; modify the benchmark to
produce fresh zero-filled `data` per iteration (either by using Criterion's
`iter_batched`/`iter_batched_ref` to allocate `data` for each run or by
resetting/zeroing each `Vec<u8>` inside the iteration closure) so each call to
`capacity_pack_range_with_data` gets unmodified buffers; update the code
locations that create `data` (the `data` binding and its use with
`capacity_pack_range_with_data`) accordingly so the benchmark measures
consistent, fresh inputs.
In `@crates/types/benches/merkle_bench.rs`:
- Around line 42-55: The benchmark bench_generate_data_root currently measures
chunk→root by regenerating leaves inside the loop (generate_leaves_from_chunks +
generate_data_root), so update it to be unambiguous: either rename the benchmark
group from "merkle/generate_data_root" to something like
"merkle/generate_leaves_and_data_root" to reflect the measured work, or split
into two benchmarks—keep bench_generate_data_root that only calls
generate_data_root on precomputed leaves and add a separate
bench_generate_leaves that measures generate_leaves_from_chunks—ensure you
reference the existing functions generate_leaves_from_chunks and
generate_data_root and adjust the group.bench_function/BenchmarkId names
accordingly.
- Around line 48-57: Benchmarked calls in the closure use fixed inputs/unused
outputs and can be optimized away; wrap the inputs and results with
criterion::black_box to prevent optimizer elision: when invoking
generate_leaves_from_chunks and generate_data_root inside the Benchmark closure
that iterates over chunks (see generate_leaves_from_chunks, generate_data_root,
and chunks), pass chunks.iter().map(|c| Ok(black_box(c.clone()))) into
generate_leaves_from_chunks and wrap the final generate_data_root(...) call in
black_box(...) (or wrap its return) so both inputs and outputs are protected.
In `@crates/types/benches/pricing_bench.rs`:
- Around line 28-31: Bench calls in the benchmark closure (the b.iter in the
group.bench_function) are missing black_box, which lets the optimizer skew
timings; wrap all inputs to calculate_term_fee (e.g., bytes, epochs, &config,
irys_price) with std::hint::black_box and also wrap the final result after
.expect(...) with black_box so the call and its return value cannot be
constant-folded or eliminated; update the closure passed to
group.bench_function/BenchmarkId::from_parameter to use black_box for inputs and
the expect result inside b.iter.
In `@crates/types/benches/solution_hash_bench.rs`:
- Around line 20-28: Wrap the compute_solution_hash invocations in the
Criterion::black_box to prevent the optimizer from eliding or rearranging the
calls: in the closure passed to group.bench_function for both "256KB_chunk" and
"empty_chunk", call black_box(compute_solution_hash(black_box(&chunk_256k),
black_box(offset), black_box(&seed))) (and similarly for the empty slice) so
that both the function return value and its inputs are black-boxed during
measurement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 405b7ef7-473d-4271-96ca-71a1b85006ec
📒 Files selected for processing (6)
crates/efficient-sampling/benches/sampling_bench.rscrates/irys-reth/benches/shadow_tx_bench.rscrates/packing/benches/packing_bench.rscrates/types/benches/merkle_bench.rscrates/types/benches/pricing_bench.rscrates/types/benches/solution_hash_bench.rs
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
crates/irys-reth/benches/shadow_tx_bench.rs (1)
57-64:⚠️ Potential issue | 🟠 MajorReject-path benches should assert
Ok(None)rather than black-boxing raw results.Lines 58 and 63 currently discard the contract outcome by black-boxing
Result<Option<_>, _>. This can hide regressions (ErrorSome) while still producing timings.Suggested patch
let non_shadow_addr = Some(Address::from([0x01; 20])); group.bench_function(BenchmarkId::from_parameter("non_shadow_reject"), |b| { - b.iter(|| black_box(detect_and_decode_from_parts(non_shadow_addr, &encoded))); + b.iter(|| { + let is_rejected = detect_and_decode_from_parts(non_shadow_addr, &encoded) + .expect("non_shadow_reject should not error") + .is_none(); + black_box(is_rejected) + }); }); let non_shadow_data = vec![0_u8; 100]; group.bench_function(BenchmarkId::from_parameter("no_prefix_reject"), |b| { - b.iter(|| black_box(detect_and_decode_from_parts(shadow_addr, &non_shadow_data))); + b.iter(|| { + let is_rejected = detect_and_decode_from_parts(shadow_addr, &non_shadow_data) + .expect("no_prefix_reject should not error") + .is_none(); + black_box(is_rejected) + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/irys-reth/benches/shadow_tx_bench.rs` around lines 57 - 64, The benchmark currently black-boxes the raw Result from detect_and_decode_from_parts (called in the closures for BenchmarkId "non_shadow_reject" and "no_prefix_reject"), which can hide regressions; change the bench closures to invoke detect_and_decode_from_parts(non_shadow_addr, &encoded) and detect_and_decode_from_parts(shadow_addr, &non_shadow_data) and assert that the outcome equals Ok(None) (e.g. with assert_eq! or matches!) inside the b.iter closure instead of black_box(...), keeping the same identifiers (non_shadow_addr, encoded, shadow_addr, non_shadow_data, detect_and_decode_from_parts) so the benches fail on Err or Some and only measure the intended reject path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/efficient-sampling/benches/sampling_bench.rs`:
- Around line 38-40: The closure currently discards the result of
ranges.next_recall_range(1, &seeds[0], &partition_hash) by ending the statement
with a semicolon, causing the closure to return () and defeating Criterion's
black_box protection; fix it by removing the trailing semicolon so the closure
returns the computed value from next_recall_range (the call on ranges) instead.
- Line 30: The benchmark currently calls make_seeds(1000) but only uses
seeds[0], which wastes allocation; change the call to make_seeds(1) (or
otherwise produce a single seed) and update any dependent uses to index that
single element, i.e., adjust where the variable seeds and its indexing are used
(referencing make_seeds and the seeds variable) so the benchmark only allocates
the minimal number of seeds.
- Around line 85-88: Replace the `.expect("valid range")` call inside the
benchmark closure with `.unwrap()` to avoid the extra panic-message formatting
overhead on the hot path; specifically, update the invocation of
recall_range_is_valid(...) inside the b.iter(|| { ... }) loop so it uses
unwrap() instead of expect() to reduce benchmark noise.
- Around line 60-62: The call to ranges.reconstruct currently has a trailing
semicolon which discards its return value so Criterion's black_box cannot
observe the computed result; remove the semicolon so the closure returns the
value from ranges.reconstruct (the same fix applied in bench_next_recall_range).
Locate the closure where ranges.reconstruct(&step_seeds, &partition_hash) is
invoked and make it return that expression instead of ending with a semicolon,
ensuring the benchmark receives the reconstructed value for proper black-boxing.
In `@crates/irys-reth/benches/shadow_tx_bench.rs`:
- Around line 52-54: The benchmark currently only asserts the Result from
detect_and_decode_from_parts but not the Option inside it; change the call in
the closure so that the positive path requires Ok(Some(_)) — i.e., call
detect_and_decode_from_parts(shadow_addr, &encoded), assert the Result is Ok,
then assert the Option is Some and unwrap it (e.g. expect or match to require
Some) before passing to black_box; reference detect_and_decode_from_parts and
the bench closure so the benchmark measures the decode work and not an Ok(None)
reject path.
In `@crates/packing/benches/packing_bench.rs`:
- Around line 27-55: The "testing" and "testnet" PackingTier entries in
build_packing_tiers() are exercising the same packing workload because only
iterations and chain_id are used by packing and ConsensusConfig::testing() and
::testnet() currently share those values; fix by removing the redundant
"testing" tier or by making the two tiers differ in a field that actually
affects packing (e.g., change the iterations or chain_id for the testnet entry),
updating the array in build_packing_tiers() (and the PackingTier entries) so
each tier maps to a distinct packing workload.
In `@crates/types/benches/merkle_bench.rs`:
- Line 70: Replace the direct index access proofs[0] with
proofs.first().expect(...) to avoid panics with unclear messages: call
proofs.first().expect("bench setup returned no proofs") and adjust the
destructuring to handle the borrowed tuple (e.g. dereference the & returned by
first() or clone as needed) so you bind proof_bytes and target safely; update
the let binding that currently uses (ref proof_bytes, target) to use the value
from proofs.first().expect(...) in merkle_bench.rs.
In `@crates/types/benches/pricing_bench.rs`:
- Around line 44-58: The term_fee is computed once with a hard-coded proof count
(10) but the benchmark loop varies proofs (1, 10, 100), so compute term_fee
inside the loop using the current proofs value to keep inputs consistent; move
the call to calculate_term_fee(bytes, config.epoch.submit_ledger_epoch_length,
&config, proofs, irys_price).expect("term fee") into the group.bench_function
closure (or immediately before calling calculate_perm_fee_from_config) and then
pass that per-case term_fee into calculate_perm_fee_from_config(bytes, &config,
proofs, irys_price, term_fee).
---
Duplicate comments:
In `@crates/irys-reth/benches/shadow_tx_bench.rs`:
- Around line 57-64: The benchmark currently black-boxes the raw Result from
detect_and_decode_from_parts (called in the closures for BenchmarkId
"non_shadow_reject" and "no_prefix_reject"), which can hide regressions; change
the bench closures to invoke detect_and_decode_from_parts(non_shadow_addr,
&encoded) and detect_and_decode_from_parts(shadow_addr, &non_shadow_data) and
assert that the outcome equals Ok(None) (e.g. with assert_eq! or matches!)
inside the b.iter closure instead of black_box(...), keeping the same
identifiers (non_shadow_addr, encoded, shadow_addr, non_shadow_data,
detect_and_decode_from_parts) so the benches fail on Err or Some and only
measure the intended reject path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c652daa1-94d7-422a-a440-6e5b8c980971
📒 Files selected for processing (6)
crates/efficient-sampling/benches/sampling_bench.rscrates/irys-reth/benches/shadow_tx_bench.rscrates/packing/benches/packing_bench.rscrates/types/benches/merkle_bench.rscrates/types/benches/pricing_bench.rscrates/types/benches/solution_hash_bench.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/domain/benches/block_tree_bench.rs`:
- Around line 59-69: The benchmark currently measures repeated mark_tip calls on
an already-marked tip (tree.mark_tip on the same block inserted in the setup),
so it measures steady-state re-marking instead of the cost of transitioning a
fresh tip; change the bench so each iteration creates or inserts a new block
(use extend_chain/random_block, seal_block, tree.add_block) and then calls
tree.mark_tip exactly once on that newly-inserted block (do not pre-mark it in
setup), ensuring mark_tip is invoked on a fresh state per iteration to capture
the true new-tip transition cost for mark_tip and related paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59a212b0-3fdd-433e-8e34-ed7feb7cdd92
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/database/Cargo.tomlcrates/database/benches/block_index_bench.rscrates/database/benches/chunk_cache_bench.rscrates/database/benches/ingress_proof_bench.rscrates/domain/Cargo.tomlcrates/domain/benches/block_tree_bench.rs
c7249f0 to
b61876d
Compare
6ae203b to
6606381
Compare
…bench): allocate fresh entropy buffer per benchmark iteration
eaa2999 to
5016fc9
Compare
Describe the changes
A clear and concise description of what the bug fix, feature, or improvement is.
Related Issue(s)
Please link to the issue(s) that will be closed with this PR.
Checklist
Additional Context
Add any other context about the pull request here.
Summary by CodeRabbit
Tests
Chores