feat(benches): PR-11 — rmg-benches crate skeleton (Closes #42) #112
Conversation
…y refresh cadence; regen rollup
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Summary by CodeRabbitRelease Notes
WalkthroughAdds a new benchmarking crate ( Changes
Sequence Diagram(s)sequenceDiagram
participant Criterion
participant Bench as motion_throughput
participant Engine
participant EngineAPI
rect rgb(230,245,255)
note over Criterion,Bench: Per-iteration batching (setup vs measured)
Criterion->>Bench: iter_batched(setup_fn, bench_fn)
end
rect rgb(240,250,240)
note over Bench,Engine: Setup: construct demo engine & labels
Bench->>Engine: build_engine_with_n_entities(n)
Engine-->>Bench: (engine, labels)
end
rect rgb(255,245,230)
note over Bench,EngineAPI: Measured: start txn, apply rule per entity, commit
Bench->>EngineAPI: start_transaction()
loop for each entity label
EngineAPI->>EngineAPI: apply(MOTION_RULE_NAME)
EngineAPI-->>Bench: Applied | NoMatch
end
Bench->>EngineAPI: commit_transaction()
Bench->>EngineAPI: decode_payload(first_entity)
end
Bench-->>Criterion: record throughput
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
15-19: Tune benchmark profile for speed (not size).Release profile is size‑optimized; benches should use a speed‑optimized profile to produce stable, representative numbers.
Apply:
resolver = "2" [workspace.dependencies] rmg-core = { version = "0.1.0", path = "crates/rmg-core" } [profile.release] opt-level = "s" lto = true codegen-units = 1 strip = true + +[profile.bench] +# Derive from release but optimize for throughput and faster builds +opt-level = 3 +lto = false +codegen-units = 16 +debug = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)crates/rmg-benches/Cargo.toml(1 hunks)crates/rmg-benches/benches/motion_throughput.rs(1 hunks)docs/ROADMAP.md(1 hunks)docs/decision-log.md(1 hunks)docs/echo-total.md(3 hunks)docs/execution-plan.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/rmg-benches/benches/motion_throughput.rs (3)
crates/rmg-core/src/payload.rs (2)
decode_motion_payload(33-44)encode_motion_payload(17-23)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.
🔇 Additional comments (6)
crates/rmg-benches/Cargo.toml (1)
7-13: Criterion features: confirm harness support; consider re-enabling defaults.With
default-features = false, Criterion’scargo_bench_supportmay be disabled, which can breakcriterion_main!/criterion_group!on some toolchains. If you hit build/link errors, drop the override or add the needed feature explicitly.If compilation fails, apply this change:
-criterion = { version = "0.5", default-features = false, features = ["html_reports"] } +criterion = { version = "0.5", features = ["html_reports"] }docs/execution-plan.md (1)
312-315: LGTM — benches entry is clear and scoped.docs/ROADMAP.md (2)
9-29: LGTM — milestone targets read well and align with perf plan.
34-60: LGTM — issue table refresh note is helpful.docs/decision-log.md (1)
150-155: LGTM — decision entry documents scope and impact crisply.docs/echo-total.md (1)
539-544: Manual verification required; script generation failed in sandbox.The rollup regeneration script (
scripts/gen-echo-total.sh) fails in this environment due to bash process substitution limitations (/dev/fd/63unsupported in sandbox). The output is truncated (~4000 lines removed, including ISSUES_MATRIX.md and ROADMAP.md sections), though those source files exist in the repo.Cannot definitively verify whether running
make echo-totallocally will produce correct results. Developer must:
- Run
make echo-total(orscripts/gen-echo-total.sh) in your local environment- Verify the regenerated rollup includes all expected sections (confirm ISSUES_MATRIX.md and ROADMAP.md are present)
- Ensure the diff reflects intentional updates, not truncation
- Commit if correct, or investigate script breakage if truncated
…\n- Precompute NodeId in setup; no hashing in measured loop\n- Use debug_assert! in hot path\n- Decode + validate payload and black_box result\n- Set Criterion group sample_size(50) and warm_up_time(3s)\n- Add [profile.bench] tuned for throughput\n\nRefs PR-11
There was a problem hiding this comment.
Actionable comments posted: 5
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)
226-234: Regenerate docs/echo-total.md before merge — file is auto-generated and stale per CI.This file is explicitly marked as auto-generated (
This file is generated by scripts/gen-echo-total.sh). Do not hand-edit; runmake echo-total, stage the output, and re-push.Verify locally:
make echo-total git diff -- docs/echo-total.mdAlso applies to: 547-551, 707-712, 737-749
♻️ Duplicate comments (1)
crates/rmg-benches/benches/motion_throughput.rs (1)
1-8: Nice: stabilized bench harness and side-effect retention.Precomputing
NodeId, usingdebug_assert!in the hot path, andblack_boxfor decoded payloads address prior stability concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Cargo.toml(2 hunks)crates/rmg-benches/Cargo.toml(1 hunks)crates/rmg-benches/benches/motion_throughput.rs(1 hunks)deny.toml(1 hunks)docs/decision-log.md(2 hunks)docs/echo-total.md(5 hunks)docs/execution-plan.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/rmg-benches/benches/motion_throughput.rs (3)
crates/rmg-core/src/payload.rs (2)
decode_motion_payload(33-44)encode_motion_payload(17-23)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.
🔇 Additional comments (6)
crates/rmg-benches/Cargo.toml (2)
9-11: Pin or confirm Criterion version wrt cargo-deny wildcards.With
bans.wildcards = "deny", confirm thatcriterion = "0.5"won’t trip bans. If CI flags it, pin exact patch (e.g., "0.5.x") for reproducible benches.If bans complain, apply:
-criterion = { version = "0.5", default-features = false, features = ["html_reports"] } +criterion = { version = "0.5.1", default-features = false, features = ["html_reports"] }
13-15: Good: bench harness wiring.
[[bench]] ... harness = falseis correct for Criterion; path+version pin onrmg-coresatisfies deny policy.deny.toml (1)
19-25: Validate cargo-deny version and SPDX identifiers.
- Ensure CI uses cargo-deny ≥ 0.14.21 so these deprecations are recognized.
- Double-check
Unicode-3.0is a valid SPDX ID; many projects useUnicode-DFS-2016. Replace if deny reports “unknown license id.”docs/execution-plan.md (1)
36-43: Scope and CI notes read clean.Benches-only scope and deny modernization are clearly documented; matches workspace changes.
Cargo.toml (1)
7-9: Workspace membership updated correctly.docs/decision-log.md (1)
150-155: Decision entry matches code changes.PR-11 benches crate rationale and consequences are crisp and aligned.
… noise_threshold; update README Benchmarks; regen docs rollup
Adds a minimal benches crate with Criterion and a motion throughput benchmark using public rmg-core APIs.
Why
What changed
How to run
Scope
Follow-ups