bench: tipset validation #6181
bench: tipset validation #6181
Conversation
WalkthroughThis PR introduces criterion-based benchmarking infrastructure for tipset validation. It adds a new optional criterion dependency, creates a benchmark-private feature, and implements a tipset-validation benchmark that measures state computation performance using cached snapshots from calibnet and mainnet. Changes
Sequence DiagramsequenceDiagram
participant Criterion as Criterion Benchmark
participant BTV as bench_tipset_validation()
participant GS as get_snapshot()
participant PV as prepare_validation()
participant V as validate()
Criterion->>BTV: Initialize tokio runtime
BTV->>GS: Download snapshot (calibnet/mainnet)
GS-->>BTV: Return cached snapshot path
BTV->>PV: Prepare validation (build DB, ChainStore, StateManager)
PV->>V: Warmup call to compute_tipset_state
V-->>PV: Warmup complete
PV-->>BTV: Return StateManager + Tipset
BTV->>V: Benchmark loop - validate(state_manager, tipset)
V-->>BTV: compute_tipset_state result
BTV-->>Criterion: Benchmark metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes The changes span multiple file types with mixed complexity: manifest and documentation updates are straightforward, while the tipset_validation.rs introduces meaningful logic involving async operations, snapshot management, database construction, and state setup. The coordination across Cargo.toml, new module structure, and the benchmark implementation requires verification of correctness and integration points. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
hanabi1224
force-pushed
the
hm/bench-tipset-validation
branch
from
2b5a46a to
83e0353
Compare
hanabi1224
requested review from
LesnyRumcajs,
elmattic and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Cargo.toml (1)
56-56: Unify criterion features across dep and dev-dep to avoid accidental default features.Right now, [dependencies] sets
default-features = false, but [dev-dependencies] likely enables default features implicitly. This can re-enable unwanted transitive features during benches due to feature unification. Recommend disabling default features in dev-deps too and listing only what you need.Apply this change in dev-dependencies:
-[dev-dependencies] -criterion = { version = "0.7", features = ["async_tokio", "csv_output"] } +[dev-dependencies] +criterion = { version = "0.7", default-features = false, features = ["async_tokio", "csv_output"] }benches/tipset-validation.rs (1)
4-6: Optional: note about plots to avoid surprise warning.You may add a hint like “Install gnuplot to enable plot rendering; otherwise plotters backend is used” to align with the observed output.
src/benchmark_private/mod.rs (1)
4-6: Scoped re-exports OK; consider narrowing later.Re-exporting
forestandcidworks for benches. Optionally re-export only the items benches require to keep the surface tighter.src/benchmark_private/tipset_validation.rs (3)
26-33: Limit visibility of CACHE_DIR to avoid leaking bench internalsMake this crate-private; it’s only used inside the crate and doesn’t need to be exposed publicly.
-pub static CACHE_DIR: LazyLock<PathBuf> = LazyLock::new(|| { +pub(crate) static CACHE_DIR: LazyLock<PathBuf> = LazyLock::new(|| {
65-66: Propagate warmup errors; keep unwrap only in the measured pathReturn Result from validate, use ? during warmup, and unwrap inside the Criterion closure to fail fast during measurement without hiding setup errors.
- // warmup - validate(state_manager.clone(), ts.clone()).await; + // warmup + validate(state_manager.clone(), ts.clone()).await?;-async fn validate(state_manager: Arc<StateManager<ManyCar>>, ts: Arc<Tipset>) { - state_manager - .compute_tipset_state(ts, crate::state_manager::NO_CALLBACK, VMTrace::NotTraced) - .await - .unwrap(); -} +async fn validate( + state_manager: Arc<StateManager<ManyCar>>, + ts: Arc<Tipset>, +) -> anyhow::Result<()> { + state_manager + .compute_tipset_state(ts, crate::state_manager::NO_CALLBACK, VMTrace::NotTraced) + .await?; + Ok(()) +}- b.to_async(&rt) - .iter(|| validate(black_box(state_manager.clone()), black_box(ts.clone()))) + b.to_async(&rt).iter(|| async { + validate(black_box(state_manager.clone()), black_box(ts.clone())) + .await + .unwrap() + })- b.to_async(&rt) - .iter(|| validate(black_box(state_manager.clone()), black_box(ts.clone()))) + b.to_async(&rt).iter(|| async { + validate(black_box(state_manager.clone()), black_box(ts.clone())) + .await + .unwrap() + })Also applies to: 69-74, 94-96, 106-108
18-23: Tune Criterion for mainnet to avoid “100 samples couldn’t complete in 5s”Split the chained benches so you can dial different settings; reduce sample_size or increase measurement_time for mainnet.
use criterion::Criterion; +use std::time::Duration;- let mut group = c.benchmark_group("tipset_validation"); - - group - .bench_function("calibnet@3111900", |b| { + let mut group = c.benchmark_group("tipset_validation"); + group.sample_size(50); + group.bench_function("calibnet@3111900", |b| { // ... - }) - .bench_function("mainnet@5427431", |b| { + }); + + // Heavier: fewer samples and longer measurement + group.sample_size(10); + group.measurement_time(Duration::from_secs(70)); + group.bench_function("mainnet@5427431", |b| { // ... - }); + });Also applies to: 82-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(3 hunks)benches/car-index.rs(1 hunks)benches/tipset-validation.rs(1 hunks)src/benchmark_private/mod.rs(1 hunks)src/benchmark_private/tipset_validation.rs(1 hunks)src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
benches/tipset-validation.rs (1)
src/benchmark_private/tipset_validation.rs (1)
bench_tipset_validation(76-111)
src/benchmark_private/tipset_validation.rs (3)
src/genesis/mod.rs (1)
read_genesis_header(21-44)src/utils/net/download_file.rs (1)
download_file_with_cache(30-89)src/networks/mod.rs (1)
from_chain(404-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
🔇 Additional comments (7)
benches/car-index.rs (1)
5-5: Doc update matches feature-gated benches.The invocation reflects the new benchmark-private gating and looks good.
Cargo.toml (2)
301-302: Feature wiring looks correct.
benchmark-private = ["dep:criterion"]cleanly pulls criterion only when needed.
325-329: Bench target registration is set up properly.
harness = falsewithrequired-features = ["benchmark-private"]matches the Criterion entry points in benches.src/lib.rs (1)
100-106: Public gate for benchmark internals is appropriate.
pub mod benchmark_private;behind#[cfg(feature = "benchmark-private")]+#[doc(hidden)]keeps the surface semver-exempt and opt-in. LGTM.benches/tipset-validation.rs (1)
8-12: Criterion wiring and import path are correct.
criterion_group!/criterion_main!withbench_tipset_validationcompiles under the gated module.src/benchmark_private/tipset_validation.rs (2)
35-37: No action required - NetworkChain Display already outputs correct CDN namingThe NetworkChain enum derives
displaydoc::Displayusing doc comments that are already lowercase ("mainnet", "calibnet", "butterflynet"). The URL construction on line 36 invokes Display via{chain}, which produces the exact lowercase tokens expected by the CDN. No explicit normalization or conversion is necessary.Likely an incorrect or invalid review comment.
34-43: Verify MD5 support with actual DigitalOcean Spaces snapshot URLs; confirm Resumable is appropriateYour concern is partially validated:
DigitalOcean Spaces is explicitly supported in
HOSTS_WITH_MD5_ETAG(".digitaloceanspaces.com"), and the cache logic extracts MD5 from the etag header. However, if the etag header is missing or unparseable as hex,get_content_md5_hash_from_urlreturnsNone, and the cache logic callsbail!()on None, causing a re-run to error rather than gracefully miss.The
DownloadFileOption::Resumableenum variant exists and is valid, so that suggestion is sound for large.car.zstfiles.The code assumes DigitalOcean Spaces provides MD5 in the etag header. You should verify this assumption by testing against the actual snapshot URLs to confirm they provide the expected etag format; otherwise, cache hits will fail on subsequent runs.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit