feat(benches): PR-12 — snapshot hash microbench (Closes #43)

[flyingrobots]

Adds a snapshot hashing benchmark that measures Engine::snapshot() on a reachable chain graph.

What

• benches/snapshot_hash.rs in rmg-benches
• Sizes: 10, 100, 1000 nodes (+root), Throughput=elements (nodes)
• Uses only public rmg-core APIs (GraphStore, NodeRecord, EdgeRecord, Engine)

Why

• Establishes a stable microbench for the state-root hashing hot path as part of M1.

Scope

• Benches-only; no runtime changes. JSON artifacts/regression gates will land in PR‑14/PR‑15.

Base

• Stacked on PR‑11 so the benches crate diff stays minimal.

[coderabbitai]

Warning

Rate limit exceeded

@flyingrobots has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c0ec43f and c44c827.

⛔ Files ignored due to path filters (1)

• README.md is excluded by !*.md

📒 Files selected for processing (4)

• crates/rmg-benches/benches/README.md (1 hunks)
• docs/decision-log.md (1 hunks)
• docs/echo-total.md (5 hunks)
• docs/execution-plan.md (1 hunks)

Summary by CodeRabbit

• Tests

  • Added performance benchmarks for snapshot hashing operations across varying dataset sizes (10–1,000 nodes).
  • Added performance benchmarks for scheduler throughput across varying entity counts (10–1,000 entities).

• Documentation

  • Updated planning and decision documentation with benchmark configuration and methodology.
  • Updated math validation test guidance and tolerance documentation.

Summary by CodeRabbit

• Tests
  • Added two Criterion microbenchmarks: snapshot_hash and scheduler_drain.
• Chores
  • Added blake3 as a dev-dependency and registered bench targets in crates/rmg-benches/Cargo.toml.
  • Merged/sync notes and documented conflict resolution in decision logs.
• Documentation
  • Extended decision log, execution plan, and related docs describing the benches and merge details.

Walkthrough

Adds two new Criterion benchmark suites (snapshot hashing and scheduler drain), registers them in the rmg-benches Cargo manifest, pins blake3 as a dev-dependency, and updates several documentation files with bench rationale and merge-sync notes.

Changes

Cohort / File(s)	Summary
Benchmark manifest crates/rmg-benches/Cargo.toml	Added description, added dev-dependencies entry blake3 = "1.8", and registered three [[bench]] targets: motion_throughput (existing), snapshot_hash, scheduler_drain (both harness = false).
Snapshot-hash benchmark crates/rmg-benches/benches/snapshot_hash.rs	New Criterion benchmark that builds a chain graph, invokes Engine::snapshot to hash reachable subgraph for sizes 10/100/1000, records throughput and black-boxes the hash result.
Scheduler-drain benchmark crates/rmg-benches/benches/scheduler_drain.rs	New Criterion benchmark defining a no-op rewrite rule (rule id derived via blake3), builds engines with N entities, applies rule per-entity in a transaction, commits and measures scheduler/apply throughput for sizes 10/100/1000.
Docs / decision logs docs/decision-log.md, docs/echo-total.md, docs/execution-plan.md, docs/math-validation-plan.md	Added PR-12 benches narrative, rationale for blake3 pin, bench implementation notes (constants, batching, throughput semantics), merge-sync/resolve notes, and minor docs edits (spelling, test tasks).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Bench as Criterion harness
  participant Engine as Engine
  participant Graph as GraphStore
  note right of Bench #DDEEFF: snapshot_hash flow
  Bench->>+Graph: build chain nodes (n)
  Bench->>+Engine: attach Graph, create Engine
  Bench->>Engine: Engine::snapshot(root)
  Engine-->>-Bench: hash of reachable subgraph
  note right of Bench #F6F0E1: record throughput & black_box result

Loading

sequenceDiagram
  autonumber
  participant Bench as Criterion harness
  participant Engine as Engine
  participant Scheduler as Scheduler/RuleApplier
  note right of Bench #E8F6E1: scheduler_drain flow
  Bench->>+Engine: build engine with N entities
  Bench->>Engine: begin transaction
  loop per-entity
    Bench->>Scheduler: apply no-op rule to entity
    Scheduler-->>Bench: ApplicationResult (Applied)
  end
  Bench->>Engine: commit transaction
  Engine-->>-Bench: commit result
  note right of Bench #F6F0E1: throughput = number of rule applications

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Files are self-contained benches and docs; low risk to production code.
• Pay attention to:
  • blake3 version pin and whether dev-only usage matches CI tooling.
  • Bench harness flags (harness = false) and Criterion setup (batched sizing, throughput recording).
  • Any debug_asserts placed in hot loops (harmless for benches but worth checking).

Possibly related PRs

• feat(benches): PR-11 — rmg-benches crate skeleton (Closes #42) #112 — Extends the rmg-benches crate; directly related because it provides the bench crate scaffold and targets that this PR populates.
• feat/rmg core bootstrap #1 — Adds Criterion benches and exercises Engine primitives used by these benchmarks; high-level feature overlap.

Poem

⚙️ Chains and drains, a microbench fête,
Blake3 whispers ids precise and neat,
No-op rules march, commits in flight,
Throughput counted, hashes tight,
Docs note the merge — the benches meet.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "feat(benches): PR-12 — snapshot hash microbench (Closes #43)" is directly related to the primary deliverables in this changeset. The PR adds two significant new benchmark files (snapshot_hash.rs and scheduler_drain.rs) along with supporting configuration and documentation updates. The title specifically and accurately highlights the snapshot hash benchmark, which is a core component of the work. While the title does not mention the scheduler_drain.rs benchmark or documentation updates, it remains clear, specific, and focused on a real and substantial part of the change that aligns with the PR's stated objective to establish a stable microbench for the state-root hashing hot path.
Description Check	✅ Passed	The pull request description is clearly related to the changeset and provides relevant context. It accurately describes the snapshot_hash.rs benchmark addition, including specific implementation details (chain topology, sizes: 10/100/1000 nodes, throughput measurement), the public APIs used, the rationale (establishing a stable microbench for the state-root hashing hot path), and scope clarifications (benches-only, no runtime changes). While the description does not mention the scheduler_drain.rs benchmark or the extensive documentation updates, it is not vague, generic, or off-topic—it meaningfully conveys information about a central component of the changeset, which satisfies the lenient pass criterion.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/echo-total.md (1)

1-10: Rollup corrupted — regenerate immediately.

The docs/echo-total.md file is truncated, missing ~4500 lines including ISSUES_MATRIX.md and ROADMAP.md sections. Diff confirms large content deletion starting at line 771.

Regenerate locally:

scripts/gen-echo-total.sh
git add docs/echo-total.md
git commit -m "docs: refresh echo-total rollup for PR-113"

The gen-echo-total.sh script is well-formed and deterministic; running it will restore all missing sections.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0430c47 and b4b58e4.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

• crates/rmg-benches/Cargo.toml (1 hunks)
• crates/rmg-benches/benches/scheduler_drain.rs (1 hunks)
• crates/rmg-benches/benches/snapshot_hash.rs (1 hunks)
• docs/decision-log.md (1 hunks)
• docs/echo-total.md (5 hunks)
• docs/execution-plan.md (1 hunks)
• docs/math-validation-plan.md (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

crates/rmg-benches/benches/scheduler_drain.rs (2)

crates/rmg-core/src/ident.rs (2)

• make_node_id (35-40)
• make_type_id (27-32)

crates/rmg-core/src/demo/motion.rs (1)

• build_motion_demo_engine (110-127)

crates/rmg-benches/benches/snapshot_hash.rs (1)

crates/rmg-core/src/ident.rs (3)

• make_edge_id (51-56)
• make_node_id (35-40)
• make_type_id (27-32)

🪛 GitHub Actions: Docs Rollup Check

docs/echo-total.md

[error] 1-1: docs/echo-total.md is out of date. Run 'make echo-total' (or scripts/gen-echo-total.sh) and commit the result.

⏰ 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). (2)

• GitHub Check: Tests (musl)
• GitHub Check: Security Audit

🔇 Additional comments (7)

crates/rmg-benches/Cargo.toml (1)

18-24: Benches metadata looks correct (harness=false).

Entries for snapshot_hash and scheduler_drain align with the new files and Criterion’s macros.

docs/execution-plan.md (1)

36-46: Docs accurately mirror benches + metadata changes.

Matches Cargo and bench files; no nits.

docs/math-validation-plan.md (1)

45-51: Minor edits LGTM.

Tooling/tolerances/tasks updates read clean; nothing to fix.

Also applies to: 55-55, 63-70, 74-77

crates/rmg-benches/benches/snapshot_hash.rs (1)

46-61: Benchmark scaffolding is sound.

• Builds once per size; iterations measure Engine::snapshot() only.
• Throughput accounts for root (+1). Good.

crates/rmg-benches/benches/scheduler_drain.rs (2)

9-36: No-op rule scaffold is fine for isolating scheduler drain.

Deterministic rule id, empty left, default Footprint, and Abort policy are appropriate for a pure scheduler microbench.

---

61-74: No issues found — iter_batched correctly excludes setup time from measurement.

The code is correctly using iter_batched(...) with BatchSize::PerIteration. Criterion's iter_batched measures only the routine closure, not the setup closure, so build_engine_with_entities(n) is properly excluded from timing while the transaction rules and commit are measured. No changes needed.

docs/decision-log.md (1)

194-203: Decision entries mirror code changes.

cargo-deny modernization and PR-12 merge notes are consistent with this PR.

[flyingrobots]

@coderabbitai review pls

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the new benchmark files and Cargo.toml to perform initial review
echo "=== crates/rmg-benches/Cargo.toml ==="
cat crates/rmg-benches/Cargo.toml

echo -e "\n=== benches/snapshot_hash.rs ==="
cat crates/rmg-benches/benches/snapshot_hash.rs

echo -e "\n=== benches/scheduler_drain.rs ==="
cat crates/rmg-benches/benches/scheduler_drain.rs

Length of output: 5364

---

@flyingrobots: Understood. Initiating GOD-TIER NIGHTMARE pedantic review. Hold onto your keyboard.

---

🔥 crates/rmg-benches/benches/snapshot_hash.rs

CRITICAL Issues:

1. Line 36: Edge ID naming is MISLEADING

   let edge_id = make_edge_id(&format!("e-{}-{}", i, i + 1));

   This creates an edge from prev to id, but the format "e-{}-{}" with i and i+1 suggests indices rather than semantic connection. The edge connects node at index i to node at index... well, actually i since that's the current node. The i+1 is meaningless. This should be format!("edge-{}-to-{}", prev_idx, i) or something that accurately reflects the topology.

2. Line 54-56: Redundant closure parameter

   group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, &_n| {

   You pass &n as input but the closure ignores _n entirely because engine is already captured. This is wasteful and violates the principle of clarity. Either remove the input parameter or actually use it to construct the engine inside the benchmark.

3. Line 48: Throughput comment is imprecise

   group.throughput(Throughput::Elements(n as u64 + 1)); // +1 for root

   The comment says "+1 for root" but doesn't explain that this represents the total reachable node count. What's an "element" in this context? A node? Make it explicit: // Total nodes in reachable set (n entities + 1 root)

SEVERE Issues:

4. Lines 9-42: Magic string apocalypse

   • "root" (line 12)
   • "world" (line 13)
   • "entity" (line 27)
   • "link" (line 28)
   • "ent-{}" (line 32)
   • "e-{}-{}" (line 36)

   None of these are constants. If you change "entity" to "Entity" during refactoring, the benchmark silently measures something completely different. Define module-level constants:

   const ROOT_ID: &str = "root";
   const WORLD_TYPE: &str = "world";
   const ENTITY_TYPE: &str = "entity";
   const LINK_TYPE: &str = "link";

5. Line 30: Variable naming is lazy

   let mut prev = root;

   prev could mean "previous", "preview", "prevent". Use prev_node, source_node, or chain_tail.

6. Lines 46-51: Benchmark size rationale is absent

   for &n in &[10usize, 100, 1_000] {

   Why 10, 100, 1000? Why not 50, 500, 5000? Is this testing scaling behavior? L1/L2/L3 cache effects? Document the methodology. Add a comment explaining the size selection rationale.

MODERATE Issues:

7. File-level: Missing module documentation
   No //! module doc explaining:

   • What this benchmark measures (snapshot hash computation over reachable graph)
   • Why chain topology was chosen (linear dependency, worst-case for some algos, best-case for others)
   • Expected performance characteristics
   • How to interpret results

8. Line 56-60: Comment is ambiguous

   // Hash the reachable graph (engine.snapshot performs state_root hashing).
   let snap = engine.snapshot();
   criterion::black_box(snap.hash);

   Does snapshot() perform hashing or just return a pre-computed hash? The comment suggests it "performs" hashing but accessing .hash suggests it might be cached. Clarify: "Compute snapshot hash of reachable subgraph" or "Retrieve cached snapshot hash".

9. Line 10: Missing constructors validation
   The benchmark assumes Engine::new(), insert_node(), insert_edge() all succeed. If the graph construction is broken, the benchmark will measure garbage. Consider asserting the engine state before benchmarking.

---

💀 crates/rmg-benches/benches/scheduler_drain.rs

CRITICAL Issues:

1. Lines 12, 22, 62: DRY VIOLATION - String literal duplication

   const NAME: &str = "bench/noop";  // Line 12
   // ...
   name: NAME,                        // Line 22
   // ...
   let res = engine.apply(tx, "bench/noop", id).expect("apply");  // Line 62

   The string "bench/noop" appears TWICE in the code. If you rename the rule, you must update it in two places. This is a maintenance landmine. Export NAME as pub(crate) const BENCH_NOOP_RULE_NAME: &str or add a getter to the rule.

2. Line 14-18: Magic string "rule:" prefix

   h.update(b"rule:");
   h.update(NAME.as_bytes());

   The prefix "rule:" is a magic constant. If the rule ID construction scheme changes elsewhere in the codebase, this benchmark breaks silently. Extract to const RULE_ID_PREFIX: &[u8] = b"rule:"; or use a centralized utility function.

3. Line 63: Assertion in benchmarked section

   assert!(matches!(res, ApplyResult::Applied));

   This assertion is inside the iter_batched closure, meaning it's measured as part of the benchmark. This skews timings. Move validation outside the hot path or document why this is intentional.

SEVERE Issues:

4. Line 52: blake3 version pinning is inconsistent
   In Cargo.toml:

   blake3 = "1.8.2"

   The comment in Cargo.toml says version pinning is for cargo-deny wildcard bans, but that only applies to path dependencies. Why is blake3 pinned to a patch version? Should be "1.8" or "1" per semver best practices unless there's a critical bug fix in 1.8.2 specifically. Document the reason or loosen to minor version.

5. Lines 54, 58: Magic strings galore

   • "entity" (line 58)
   • "sched-ent-{}" (line 59)

   Same problem as snapshot_hash.rs. Define constants.

6. Lines 52, 62, 65: Weak panic messages in benchmarks

   .expect("register noop rule")
   .expect("apply")
   .expect("commit")

   If these panic, you get zero context about why. Benchmarks should either:

   • Return Result and propagate errors properly, or
   • Have descriptive panics: expect("Failed to register benchmark noop rule - engine state corrupted?")

MODERATE Issues:

7. Line 66: Unused snapshot not black_boxed

   let _snap = engine.commit(tx).expect("commit");

   The underscore prefix indicates intentional discard, but the optimizer might eliminate commit() entirely if it determines _snap is unused. Black box it: criterion::black_box(engine.commit(tx).expect("commit"));

8. File-level: No explanation of "drain"
   The benchmark is named scheduler_drain but there's no comment explaining what "drain" means. Is this draining a task queue? Flushing pending operations? Measuring scheduler throughput? Document the methodology.

9. Line 49: Throughput semantics unclear

   group.throughput(Throughput::Elements(n as u64));

   What's an "element"? A rule application? A node? A transaction? Make it explicit in a comment.

10. Line 24-26: Trivial matcher matches everything

    fn matcher(_s: &rmg_core::GraphStore, _n: &rmg_core::NodeId) -> bool {
        true
    }

    The comment says this is a "no-op rule to isolate scheduler overhead" but the matcher returns true unconditionally. Is this correct? Won't this match the root node, world nodes, and other entities you didn't intend? Should it check _n's type? Document why unconditional true is correct here.

11. Line 71: BatchSize::PerIteration is undocumented

    BatchSize::PerIteration,

    Why PerIteration instead of SmallInput or LargeInput? Add a comment explaining that setup cost (engine construction) should be excluded from timing.

---

⚡ crates/rmg-benches/Cargo.toml

SEVERE Issues:

1. Line 11: blake3 patch version pinning is unjustified

   blake3 = "1.8.2"

   Pinning to 1.8.2 specifically implies you need that exact patch. But the inline comment mentions cargo-deny concerns for rmg-core, not blake3. Either:

   • Document the reason for the specific patch pin, or
   • Loosen to "1.8" or "1" to allow compatible updates.

2. Line 11: blake3 default features

   blake3 = "1.8.2"

   You're pulling in default features. Does blake3's default features include things you don't need (e.g., std, rayon parallelism)? Benchmarks should be lean. Consider:

   blake3 = { version = "1.8", default-features = false }

MODERATE Issues:

3. Missing documentation in package metadata
   No description field. While publish = false means this won't hit crates.io, internal documentation is still valuable for cargo tree and teammate discovery.

---

🌐 GENERAL / CROSS-CUTTING ISSUES:

1. No benchmark documentation or README
   The rmg-benches crate has no README.md explaining:

   • How to run benchmarks (cargo bench)
   • How to generate flamegraphs
   • How to interpret results
   • What baseline performance is expected
   • Regression thresholds (mentioned in PR description as "planned for PR-14/15" but still)

2. No module-level docs in bench files
   Both snapshot_hash.rs and scheduler_drain.rs lack //! module docs. Benchmarks are executable documentation of performance characteristics. Treat them as such.

3. Criterion configuration is default
   No custom Criterion::default() configuration. Consider:

   • Warm-up time customization for microbenchmarks
   • Sample size tuning
   • Measurement time adjustment

4. No CI/regression testing setup visible
   PR description mentions "JSON artifacts and regression gates are planned for PR-14 and PR-15", but the current state has zero automation. At minimum, add a comment or TODO referencing those future PRs.

---

🎯 DEMANDS:

1. Extract ALL magic strings to module-level constants. No exceptions.
2. Add comprehensive module-level documentation to both benchmark files explaining what's measured, why, and how to interpret results.
3. Fix the DRY violation in scheduler_drain.rs (bench/noop duplication).
4. Clarify or fix blake3 version pinning in Cargo.toml.
5. Document throughput semantics (what's an "element"?) in both benchmarks.
6. Fix misleading edge ID format in snapshot_hash.rs.
7. Add comments explaining benchmark size rationale (why 10, 100, 1000?).
8. Black box the unused snapshot in scheduler_drain.rs or explain why it's safe.
9. Remove redundant closure parameter in snapshot_hash.rs bench_with_input.
10. Justify or remove the assertion in the hot path in scheduler_drain.rs.

This is a benches-only PR, so the blast radius is limited, but benchmarks are permanent performance documentation. They must be impeccable. Currently, they're riddled with maintainability landmines and documentation debt.

---

I'll now trigger the full automated review to catch anything I missed in my manual pass.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (1)

docs/echo-total.md (1)

782-809: Rollup content also mixes blake3 =1.8.2 and 1.8.

Normalize the version reference here to match Cargo.toml and the decision log after you fix them.

Also applies to: 263-293

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4b58e4 and c0ec43f.

📒 Files selected for processing (6)

• crates/rmg-benches/Cargo.toml (1 hunks)
• crates/rmg-benches/benches/scheduler_drain.rs (1 hunks)
• crates/rmg-benches/benches/snapshot_hash.rs (1 hunks)
• docs/decision-log.md (1 hunks)
• docs/echo-total.md (5 hunks)
• docs/execution-plan.md (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

crates/rmg-benches/benches/snapshot_hash.rs (1)

crates/rmg-core/src/ident.rs (3)

• make_edge_id (51-56)
• make_node_id (35-40)
• make_type_id (27-32)

crates/rmg-benches/benches/scheduler_drain.rs (2)

crates/rmg-core/src/ident.rs (2)

• make_node_id (35-40)
• make_type_id (27-32)

crates/rmg-core/src/demo/motion.rs (1)

• build_motion_demo_engine (110-127)

🪛 GitHub Actions: Docs Rollup Check

docs/echo-total.md

[error] 1-1: docs/echo-total.md is out of date. Run 'make echo-total' (or scripts/gen-echo-total.sh) and commit the result.

⏰ 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). (2)

• GitHub Check: Tests (musl)
• GitHub Check: Security Audit

🔇 Additional comments (3)

crates/rmg-benches/benches/snapshot_hash.rs (1)

50-57: Edge ID clarity LGTM; no redundant hashing.

Using <from>-to-<to> labels and hoisted link_ty keeps IDs readable and avoids per-iter recompute.

Also applies to: 63-67

crates/rmg-benches/benches/scheduler_drain.rs (1)

25-51: Rule construction is deterministic and minimal — good.

Stable rule id, empty left graph, and default footprint are appropriate for isolating scheduler overhead.

docs/echo-total.md (1)

1-8: Verify rollup regeneration locally — sandbox environment blocked verification.

The regeneration script uses process substitution (/dev/fd/63) which the sandbox environment doesn't support. I cannot verify whether the rollup is actually stale or confirm the regeneration resolves the CI failure.

Required manual verification:

scripts/gen-echo-total.sh && git diff docs/echo-total.md

Execute locally to:

1. Regenerate docs/echo-total.md deterministically
2. Confirm the rollup output changed (or verify it's already current)
3. Verify no version pin inconsistencies remain in the regenerated file
4. Commit if changes exist and CI passes