rmg-core/scheduler: fail-fast drain; u32 radix histogram; keep PendingTx private #121
Conversation
…icy across execution-plan, decision-log, rollup
…apshot_hash, scheduler_drain). Reads estimates.json and charts time/iter vs n; includes tables + CI guidance.
…ile targets vendor-d3, bench-report, bench-serve, bench-open; include vendor/.gitkeep
…llback to baseline estimates.json, clearer guidance when results are missing
…ll; avoid subshell block to prevent syntax errors; ensure target directory exists
…on); regenerate rollup
…stogram; keep PendingTx private; docs follow-up
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Add doc backticks for /, . - Split into helpers (too_many_lines) without changing behavior. - Tests: move helper above statements; remove println!/unwrap!; capture fmt args. - Benches: tidy reserve_scaling placeholder imports/param name. - Docs Guard: update execution-plan + decision-log; regenerate echo-total. Determinism and invariants preserved; no algorithmic or ordering changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Resolve README.md, execution-plan.md, echo-total.md conflicts. - Keep scheduler lint-only changes; no runtime logic edits. - Regenerate docs/echo-total.md per Docs Guard.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
WalkthroughDeterministic scheduler refactor replaces BTreeMap draining with a radix-based, O(n) drain and generation-stamped conflict tracking; adds PendingTx/PendingRewrite/GenSet/ActiveFootprints and new scheduler APIs (enqueue, drain_for_tx, reserve, finalize_tx). Adds comprehensive benchmarking infra (Makefile targets, D3 dashboards, bench_bake.py), new/updated Criterion benches, a blake3 dev-dep pin, and many LaTeX/math docs. Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"primaryColor":"#2b6cb0","secondaryColor":"#d69e2e","tertiaryColor":"#2f855a"}}}%%
sequenceDiagram
participant Engine
participant Scheduler
participant ActiveFP as ActiveFootprints
Engine->>Scheduler: enqueue(tx, PendingRewrite)
note right of Scheduler `#dceefc`: store thin/fat in PendingTx (dedupe: last‑wins)
Engine->>Scheduler: drain_for_tx(tx)
Scheduler->>Scheduler: drain_in_order (radix or small-sort)
Scheduler-->>Engine: Vec<PendingRewrite> (deterministic order)
loop per rewrite
Engine->>Scheduler: reserve(tx, &mut PendingRewrite)
Scheduler->>ActiveFP: has_conflict / conflict_or_mark
alt no conflict
ActiveFP-->>Scheduler: ok
Scheduler-->>Engine: reserved = true
else conflict
ActiveFP-->>Scheduler: conflict
Scheduler-->>Engine: reserved = false
end
end
Engine->>Scheduler: finalize_tx(tx)
Scheduler->>Scheduler: clear PendingTx, update telemetry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring careful attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
- Additive API only; no behavior change. - Update docs (execution-plan, decision-log) and regenerate echo-total.
There was a problem hiding this comment.
Actionable comments posted: 20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockREADME.mdis excluded by!*.mddocs/benchmarks/vendor/d3.v7.min.jsis excluded by!**/*.min.jsrmg-math/main.logis excluded by!**/*.logrmg-math/main.outis excluded by!**/*.outrmg-math/main.pdfis excluded by!**/*.pdf
📒 Files selected for processing (38)
Makefile(2 hunks)crates/rmg-benches/Cargo.toml(1 hunks)crates/rmg-benches/benches/README.md(2 hunks)crates/rmg-benches/benches/reserve_scaling.rs(1 hunks)crates/rmg-benches/benches/scheduler_drain.rs(2 hunks)crates/rmg-benches/benches/snapshot_hash.rs(3 hunks)crates/rmg-core/Cargo.toml(1 hunks)crates/rmg-core/src/engine_impl.rs(1 hunks)crates/rmg-core/src/footprint.rs(1 hunks)crates/rmg-core/src/scheduler.rs(5 hunks)docs/benchmarks/index.html(1 hunks)docs/benchmarks/report-inline.html(1 hunks)docs/benchmarks/vendor/.gitkeep(1 hunks)docs/decision-log.md(3 hunks)docs/execution-plan.md(4 hunks)docs/notes/scheduler-optimization-followups.md(1 hunks)docs/notes/scheduler-radix-optimization-2.md(1 hunks)docs/notes/scheduler-radix-optimization.md(1 hunks)docs/rmg-confluence-appendix.tex(1 hunks)docs/rmg-diagrams.tex(1 hunks)docs/rmg-hypergraphs-encoding.tex(1 hunks)docs/rmg-macros.sty(1 hunks)docs/rmg-math-claims.md(1 hunks)docs/rmg-rulial-distance.tex(1 hunks)rmg-math/aux/rmg-diagrams.tex(1 hunks)rmg-math/build.sh(1 hunks)rmg-math/chapters/appendix-scheduler.tex(1 hunks)rmg-math/chapters/confluence.tex(1 hunks)rmg-math/chapters/embedding.tex(1 hunks)rmg-math/chapters/rulial-distance.tex(1 hunks)rmg-math/main.aux(1 hunks)rmg-math/main.bbl(1 hunks)rmg-math/main.blg(1 hunks)rmg-math/main.tex(1 hunks)rmg-math/main.toc(1 hunks)rmg-math/refs.bib(1 hunks)rmg-math/sty/rmg-macros.sty(1 hunks)scripts/bench_bake.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/rmg-benches/benches/scheduler_drain.rs (2)
crates/rmg-core/src/scheduler.rs (2)
new(36-44)new(476-481)crates/rmg-core/src/engine_impl.rs (1)
new(71-84)
crates/rmg-benches/benches/reserve_scaling.rs (1)
crates/rmg-core/src/scheduler.rs (3)
h(536-540)new(36-44)new(476-481)
crates/rmg-core/src/scheduler.rs (3)
crates/rmg-core/src/telemetry.rs (1)
conflict(60-62)crates/rmg-core/src/ident.rs (2)
make_node_id(35-40)make_edge_id(51-56)crates/rmg-core/src/tx.rs (1)
from_raw(30-32)
🪛 checkmake (0.2.2)
Makefile
[warning] 106-106: Missing required phony target "all"
(minphony)
[warning] 106-106: Missing required phony target "clean"
(minphony)
[warning] 106-106: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: CI
crates/rmg-core/src/footprint.rs
[error] 64-64: method iter not found for this struct. (During: cargo doc -p rmg-geom --no-deps)
crates/rmg-core/src/engine_impl.rs
[error] 1-1: Cargo clippy reported compilation errors; build failed with exit code 101
crates/rmg-benches/Cargo.toml
[warning] 1-1: panic setting is ignored for bench profile
crates/rmg-core/Cargo.toml
[warning] 1-1: panic setting is ignored for bench profile
crates/rmg-core/src/scheduler.rs
[error] 197-197: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
[error] 202-202: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
[error] 227-227: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
[error] 230-230: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
🪛 LanguageTool
docs/notes/scheduler-optimization-followups.md
[style] ~16-~16: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...serts that drain_in_order() returns exactly the same sequence from both implementations > ...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[grammar] ~200-~200: Ensure spelling is correct
Context: ...e n - Cons: - 91x slower at n=10 (687µs vs 7.5µs) - Fixed 5MB zeroing cost do...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~223-~223: To elevate your writing, try using a synonym here.
Context: ...ort_unstable` is extremely optimized; hard to beat with more passes - Would need...
(HARD_TO)
docs/notes/scheduler-radix-optimization.md
[style] ~36-~36: This phrase is redundant. Consider writing “moments” or “times”.
Context: ...e the simulation as version control for moments in time. In this mental model, a frame is like ...
(MOMENT_IN_TIME)
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... snapshot. No prediction. No rollback. No "authoritative correction." Just one pu...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/notes/scheduler-radix-optimization-2.md
[style] ~50-~50: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...orld′ ```` No prediction. No rollback. No arbitration. If two machines disagree, ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/rmg-math-claims.md
[style] ~44-~44: To elevate your writing, try using an alternative expression here.
Context: ...am, not a banner claim. ## 2) Why this matters for Echo (and why the Ruliad reference ...
(MATTERS_RELEVANT)
🪛 markdownlint-cli2 (0.18.1)
docs/notes/scheduler-optimization-followups.md
89-89: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/2/3
(MD029, ol-prefix)
90-90: Ordered list item prefix
Expected: 2; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
91-91: Ordered list item prefix
Expected: 3; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
147-147: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
159-159: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
221-221: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
docs/notes/scheduler-radix-optimization.md
6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
36-36: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
36-36: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
94-94: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
242-242: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
242-242: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
304-304: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
304-304: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
322-322: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
322-322: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
374-374: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
377-377: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
380-380: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
383-383: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
386-386: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
444-444: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
444-444: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🪛 Ruff (0.14.3)
scripts/bench_bake.py
1-1: Shebang is present but file is not executable
(EXE001)
59-59: Abstract raise to an inner function
(TRY301)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Do not catch blind exception: Exception
(BLE001)
82-83: Explicitly concatenated string should be implicitly concatenated
Remove redundant '+' operator to implicitly concatenate
(ISC003)
🪛 Shellcheck (0.11.0)
rmg-math/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (38)
docs/rmg-confluence-appendix.tex (2)
20-20: Verify \DPO macro usage matches its definition.Line 20 uses
\DPO{K}{L}{R}{D}{G}{H}, passing 6 arguments to the\DPOmacro defined inrmg-math/aux/rmg-diagrams.tex.From the earlier review, the
\DPOmacro's parameter semantics are undocumented, making it impossible to verify this usage is correct. The standard DPO diagram is:L ← K → R ↓ ↓ ↓ G ← D → HBut the macro implementation (lines 6-8 of rmg-diagrams.tex) suggests a side-by-side double diagram layout, not a standard single DPO square.
Verification needed:
- Compile this document and verify the rendered diagram matches the intended DPO square
- Cross-reference with the
\DPOmacro definition to ensure parameter order is correct- Consider adding a comment clarifying the parameter mapping:
% \DPO renders: L←K→R over G←D→H \[\DPO{K}{L}{R}{D}{G}{H}.\]This issue is linked to the earlier review comment on
rmg-diagrams.texabout documenting parameter semantics.
4-5: LaTeX dependencies verified—no fixes required.The verification confirms both required files exist and are accessible:
rmg-macros.sty→./docs/rmg-macros.sty(found;\usepackage{rmg-macros}resolves correctly)rmg-diagrams.tex→./docs/rmg-diagrams.tex(found;\input{rmg-diagrams.tex}resolves correctly)Both files reside in the
docs/directory alongsidermg-confluence-appendix.tex, so the relative references work without adjustment. The dependency chain is sound; no compilation failures expected.docs/notes/scheduler-radix-optimization-2.md (1)
339-339: The GitHub link is publicly accessible—review comment is incorrect.The verification confirms the URL
https://github.com/flyingrobots/echoreturns HTTP 200, meaning the repository is public and reachable. No action required on this line. The link is functioning correctly and requires no modification.Likely an incorrect or invalid review comment.
rmg-math/chapters/appendix-scheduler.tex (1)
1-10: The review comment is incorrect and should be disregarded.All referenced macros are properly defined in the included
rmg-macros.stypackage:
\Del,\Use,\im, and\Haloare all declared via\DeclareMathOperatorin./rmg-math/sty/rmg-macros.sty(which is correctly included in main.tex at line 5)\Halo_ris valid:\Halois defined, and the subscriptris standard LaTeX math notation\mathrm{Touch}is not a custom macro—it uses standard LaTeX's\mathrm{}command, not a custom\TouchmacroThe appendix compiles without undefined macro errors. No verification or fixes required.
Likely an incorrect or invalid review comment.
crates/rmg-benches/benches/snapshot_hash.rs (3)
75-79: LGTM: Stabilizes CI benchmark runs.Explicit
warm_up_time,measurement_time, andsample_sizeconfiguration prevents environment-dependent variance. These parameters are appropriate for CI stability.
80-80: Expanded input range increases runtime by ~10×.The new range
[10, 100, 1_000, 3_000, 10_000, 30_000]extends coverage to 30K nodes, which will significantly increase total benchmark duration. Ensure your CI budget accommodates this; if runs exceed timeouts, consider making the extended range opt-in via a feature flag or environment variable.
91-91: Correct:PerIterationisolates hashing measurement.Switching from
BatchSize::SmallInputtoBatchSize::PerIterationensuresbuild_chain_engineruns for every iteration, excluding setup cost from the timed section. This is the correct mode for this benchmark.crates/rmg-benches/benches/README.md (2)
57-59: LGTM: Exact blake3 pin avoids non-determinism.Pinning
blake3 = { version = "=1.8.2", default-features = false, features = ["std"] }and disabling default features prevents accidental parallelism or SIMD variance across environments. This is essential for reproducible benchmark results.
41-45: Documentation verified. Makefile targets confirmed.Both
bench-report(line 67) andbench-bake(line 109) targets exist in the Makefile exactly as documented. The README accurately describes the benchmarking workflow with clear, actionable instructions.rmg-math/main.bbl (1)
1-41: Generated artifact: BibTeX bibliography output.This
.bblfile is a BibTeX compiler artifact containing formatted bibliography entries. It's generated fromrefs.biband should not be manually edited. The entries appear well-formed and correspond to standard citations in graph transformation theory.Note: Consider adding
*.bblto.gitignoreif you prefer to exclude generated LaTeX artifacts from version control.docs/rmg-hypergraphs-encoding.tex (1)
1-38: LGTM: Formal embedding documentation.This LaTeX document formalizes the embedding of typed open hypergraphs into the RMG DPOI framework. The structure (categories, functors, propositions, theorems) follows standard mathematical conventions. The references to
rmg-macrosandrmg-diagrams.texare consistent with the PR's addition of LaTeX infrastructure.Minor observation: Line 19 defines
T^\staras a disjoint union; ensure the corresponding implementation (if any) respects this type separation to avoid runtime type confusion.rmg-math/main.blg (1)
1-46: Generated artifact: BibTeX processing log.This
.blgfile is BibTeX compiler output showing processing statistics. It's a build artifact and should not be manually edited.Suggestion: Add
*.blgto.gitignoreto exclude BibTeX logs from version control, keeping the repository clean of build artifacts.docs/rmg-diagrams.tex (1)
7-13: Reject the original review comment—no action required.The script output reveals these macros are intentionally different, not duplicates:
- rmg-math/aux/rmg-diagrams.tex: Wraps
\DPOoutput in\begin{center}...\end{center}and configures[ampersand replacement=\&]for tikzcd- docs/rmg-diagrams.tex: Strips the centering wrapper and uses standard
&ersandsThe docs files (
rmg-hypergraphs-encoding.tex,rmg-confluence-appendix.tex) correctly include the docs version, which is the uncentered variant appropriate for inline documentation. Consolidating these would break one context or the other. Each implementation serves its architectural purpose.crates/rmg-core/src/engine_impl.rs (1)
160-170: The original review comment is factually incorrect.The code in engine_impl.rs (lines 160-170) is syntactically and semantically sound. The refactor from direct map insertion to
scheduler.enqueue()is valid:
- The
enqueuemethod signature matches the call site exactly:fn enqueue(&mut self, tx: TxId, rewrite: PendingRewrite)- All six fields of
PendingRewriteare present and correctly named:rule_id,compact_rule,scope_hash,scope,footprint,phase- The enum variant
RewritePhase::Matchedexists and is valid- Struct field initialization order doesn't matter in Rust
The claimed pipeline compilation failure cannot be verified in the sandbox environment and appears to be a false positive or unrelated environmental issue. The code itself requires no fixes.
Likely an incorrect or invalid review comment.
crates/rmg-core/src/footprint.rs (1)
40-43: ---The original review's pipeline failure concerns are unfounded and should be disregarded.
The codebase verification reveals:
- Only one
IdSetdefinition exists, incrates/rmg-core/src/footprint.rs:29. No shadowing or conflicts.rmg-geomhas zero dependency onIdSet. Zero references to footprint or IdSet inrmg-geom/src/lib.rs.IdSetis not re-exported from rmg-core's public API. Onlypack_port_key,Footprint, andPortKeyare publicly re-exported (line 69 of lib.rs).The disconnect between the error ("rmg-geom") and the changed file ("rmg-core/footprint.rs") is a red flag that the pipeline failure is unrelated to this change—not an indication of a hidden cross-crate issue. All three hypotheses in the original comment are definitively ruled out by inspection.
The
iter()implementation is trivially correct.BTreeSet<Hash>::iter()exists and works as expected.Likely an incorrect or invalid review comment.
rmg-math/main.toc (1)
1-12: TOC updates are coherent.Entries line up with the added sections/paragraphs, so the build will cite the right anchors.
rmg-math/main.aux (1)
1-41: Aux sync looks solid.Citations and labels all point to definitions present in refs.bib and the new chapters, so LaTeX won’t choke.
rmg-math/refs.bib (1)
1-52: Bibliography entries check out.All cited keys resolve and the metadata is sufficient for the alpha style already in use.
rmg-math/chapters/confluence.tex (1)
1-41: Chapter content integrates cleanly.Definitions and theorems align with the cited DPO literature, and every reference is backed by the new bibliography entries.
docs/decision-log.md (3)
23-24: LGTM: Comprehensive decision entries for Clippy cleanup.The entries clearly document the scheduler cleanup (fail-fast drain, histogram, privacy) and lint fixes with appropriate rationale and consequences. The technical details align with the PR objectives.
201-209: Verify: Exact-patch pinning strategy for blake3 in benches.The decision to pin
blake3 = "=1.8.2"with exact-patch syntax prevents even patch-level updates. While this ensures deterministic benchmarks, it also blocks minor security or bug fixes in the dependency.Consider whether:
- The exact-patch pin
=1.8.2is intentional for reproducibility (acceptable for dev-dependency)- A periodic review process exists to update pinned bench dependencies
- A comment in
Cargo.tomldocuments the pinning rationaleIf exact-patch pinning is a deliberate choice for benchmark stability, document the trade-off (reproducibility vs. security updates) in the decision log or in
crates/rmg-benches/Cargo.toml.
236-244: LGTM: Benches DX decision is well-documented.The entry comprehensively documents the new benchmark workflow with nohup server, bench_bake script, and inline HTML reports. The rationale for offline sharing and the two-path approach (live server + baked report) are clear.
docs/rmg-macros.sty (4)
1-9: LGTM: Standard LaTeX package structure.The package header follows LaTeX conventions with proper format requirements, version dating, and appropriate dependencies. The lightweight dependency set (amsmath, amssymb, amsthm, mathtools, enumitem, hyperref) is suitable for math macros.
10-25: LGTM: Category and arrow macros are well-defined.The macro definitions follow LaTeX conventions:
\Cat{...}provides a consistent category formatting wrapper- Category variants (
\GraphT,\OGraphT, etc.) compose cleanly- Arrow macros use standard extensible arrows appropriately
26-31: LGTM: Math operators declared correctly.The
\DeclareMathOperatordeclarations follow best practices for defining custom operators. The operator names (Del,Use,Halo,im) are appropriate for mathematical notation.
32-46: LGTM: Theorem environments and list formatting.The theorem-like environments are properly styled and sequenced:
- Plain style for theorems/lemmas/propositions/corollaries
- Definition style for definitions
- Remark style for remarks
The
\setlist{nosep}setting provides compact vertical spacing, which is a deliberate design choice for dense mathematical documents.docs/benchmarks/index.html (6)
1-62: LGTM: Clean HTML structure and theme-aware CSS.The HTML5 structure is semantic with proper meta tags. The CSS custom properties enable consistent theming, and the header provides clear context about what's being measured and why it matters (O(n) scaling, 60 FPS budget).
63-79: LGTM: D3 loader with sensible fallback strategy.The dynamic script loader correctly tries the local vendor copy first before falling back to the CDN. The error handling provides actionable guidance for users.
97-107: LGTM: Well-structured benchmark configuration.The GROUPS array provides clear metadata (key, label, color, dash pattern) for each benchmark series. The INPUT sizes span multiple orders of magnitude suitable for detecting algorithmic complexity. The configurable ROOT path supports flexible deployment.
134-159: LGTM: Clean formatter and data loading logic.The
fmtNsfunction provides human-readable time formatting with appropriate unit thresholds. Therun()function correctly prioritizes inline data (offline mode) over fetch-based loading and gracefully aggregates results and errors.
161-287: LGTM: Solid D3 visualization with threshold marker.The render function creates a professional log-log chart with:
- Proper scale domains and ranges
- Grid lines and axis labels
- Threshold marker at n=1024 documenting the sort algorithm transition
- CI bands and interactive tooltips
Data is rendered via D3's data binding (
.text()and.attr()), which provides automatic escaping and prevents XSS.
289-345: LGTM: Complete visualization with legend and error guidance.The legend clearly shows line styles matching the chart. The 2x2 stat-card grid provides detailed tabular data. The missing-results panel gives actionable guidance for troubleshooting. All rendering is XSS-safe via D3's data binding methods.
Makefile (4)
5-5: LGTM: Configurable bench port variable.The
BENCH_PORTvariable follows the same pattern as the existingPORTvariable, allowing users to override the default 8000 port viamake bench-report BENCH_PORT=8080.
47-58: LGTM: Idempotent D3 vendoring with proper curl flags.The
vendor-d3target correctly checks for existence before downloading and uses appropriate curl flags (-fsSL) for error handling and redirect following. The HTTPS source (unpkg.com) is a trusted CDN.
88-104: LGTM: Robust PID-based server lifecycle management.The
bench-statustarget safely checks both file existence and process liveness viaps. Thebench-stoptarget correctly suppresses errors when the process doesn't exist (|| true), making it idempotent.
106-118: LGTM: Clean inline report baking workflow.The
bench-baketarget correctly sequences: vendor D3 → run benches → bake inline report → open file. Thebench-open-inlinetarget provides a convenient shortcut.Note: Same
opencommand portability concern applies here (Lines 115, 118) as mentioned in the previous comment.scripts/bench_bake.py (1)
91-137: LGTM: Clean HTML baking logic with robust injection.The
bake_htmlfunction systematically:
- Validates the template exists (early exit)
- Collects results and missing entries via
load_estimate- Injects the inline script via marker search with a
</body>fallback- Writes the output with proper path handling
The
main()function uses argparse correctly and handles both absolute and relative output paths. The script is well-structured for its utility purpose.docs/benchmarks/report-inline.html (1)
346-351: LGTM: Properly embedded benchmark data.The inline
<script>contains well-formed JSON data with 24 benchmark results (4 groups × 6 input sizes). Each entry includesgroup,n,mean,lb, andubfields. The empty__CRITERION_MISSING__array indicates all benchmarks succeeded.This is the expected output from
scripts/bench_bake.pythat enables offline viewing viafile://protocol.
| fn make_hash(val: u8) -> Hash { | ||
| let mut h = [0u8; 32]; | ||
| h[0] = val; | ||
| h | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication: reuse existing hash helper.
The make_hash function duplicates the h function from crates/rmg-core/src/scheduler.rs lines 535-539. This violates DRY and creates maintenance burden.
Solution: Extract the hash helper to a shared test utility module, or expose it through a #[cfg(test)] pub(crate) module in rmg-core that benches can access.
If the h function in scheduler.rs is moved to a test_utils module:
use rmg_core::test_utils::make_test_hash;
// Remove local make_hash, use shared versionAlternatively, define once in a benches/common/mod.rs:
// benches/common/mod.rs
pub fn make_hash(val: u8) -> rmg_core::Hash {
let mut h = [0u8; 32];
h[0] = val;
h
}Then import: use common::make_hash;
🤖 Prompt for AI Agents
In crates/rmg-benches/benches/reserve_scaling.rs around lines 23 to 27, the
local make_hash duplicates the h helper from crates/rmg-core/src/scheduler.rs;
remove this duplication by reusing a single helper: either move the helper into
a shared test utility (e.g. add a test_utils module in rmg-core and expose a
make_test_hash function via #[cfg(test)] pub(crate) so benches can import
rmg_core::test_utils::make_test_hash) or create a benches/common/mod.rs with a
public make_hash that returns rmg_core::Hash and import it in
reserve_scaling.rs; then delete the local make_hash and update the use/import to
reference the shared helper.
| // Note: This benchmark requires access to DeterministicScheduler which is crate-private. | ||
| // Moving this to rmg-core/src/scheduler.rs tests module or using a pub(crate) test harness. | ||
|
|
||
| fn bench_reserve_scaling(_c: &mut Criterion) { | ||
| // This is a placeholder - the actual benchmark needs to be in rmg-core | ||
| // where we can access the scheduler directly. | ||
|
|
||
| // TODO: Implement this properly by either: | ||
| // 1. Adding a test-only public API to DeterministicScheduler | ||
| // 2. Moving this benchmark into rmg-core as a test module | ||
| // 3. Using Engine API indirectly (less precise) | ||
|
|
||
| let _ = ( | ||
| BenchmarkId::new("placeholder", "reserve_scaling"), | ||
| Throughput::Elements(1), | ||
| make_hash(0), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incomplete benchmark provides no value and pollutes the bench suite.
This placeholder benchmark:
- Doesn't measure anything (
bench_reserve_scalingis empty) - Executes on every
cargo bench, wasting CI time - Produces misleading benchmark output (timing of no-op)
- Contains only TODO comments
While scaffolding has value during development, merged code should be functional. The extensive comments indicate this work is blocked on API design decisions (exposing DeterministicScheduler internals).
Options:
-
Recommended: Remove this file entirely and track the work in issue rmg-core: scheduler radix-drain performance tuning #122. Add it back when the implementation path is clear.
-
Use
#[ignore]attribute to prevent execution:
#[ignore = "Waiting for scheduler test API - see issue #122"]
fn bench_reserve_scaling(_c: &mut Criterion) {
// ...
}- Implement a minimal version using the Engine API (even if less precise), documenting the limitations.
Which approach aligns with your team's practices?
If you'd like, I can draft a minimal implementation using the Engine API approach, or generate an issue template for tracking this work.
🤖 Prompt for AI Agents
crates/rmg-benches/benches/reserve_scaling.rs lines 29-46: the file contains a
no-op placeholder benchmark that runs on every cargo bench and pollutes CI;
remove the file entirely (preferred) or, if you must keep it, mark the benchmark
to not run by adding an ignore attribute with a clear reason referencing issue
#122 (e.g., #[ignore = "Waiting for scheduler test API - see issue #122"]) and
leave a short TODO comment pointing to the issue; ensure CI and bench docs are
updated if you remove the file and add an issue reference in the PR description.
| # Exact pin and trimmed features to avoid rayon/parallelism in benches. | ||
| blake3 = { version = "=1.8.2", default-features = false, features = ["std"] } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exact version pin creates maintenance burden; document rationale or use tilde requirement.
The change from blake3 = "1.8" to blake3 = { version = "=1.8.2", ... } uses an exact pin (=1.8.2), which:
- Prevents security patches: If
1.8.3fixes a vulnerability, it won't be pulled automatically - Requires manual updates: Every blake3 release needs a deliberate Cargo.toml edit
- Conflicts with other deps: If another crate requires
blake3 >= 1.8.3, resolution fails
The comment says "exact pin", but doesn't explain why exact rather than tilde (~1.8.2) or caret with minor (^1.8.2).
Recommendations by priority:
-
If only avoiding rayon is the goal, use tilde for patch flexibility:
blake3 = { version = "~1.8.2", default-features = false, features = ["std"] }
This allows
1.8.xpatches while staying on minor1.8. -
If patch-level stability is critical (e.g., for benchmark reproducibility), document why:
# Exact pin ensures bit-identical hashing across benchmark runs for reproducibility. # Update deliberately when validating new blake3 releases don't change bench characteristics. blake3 = { version = "=1.8.2", default-features = false, features = ["std"] }
-
Verify the pin is current:
What is the latest version of the blake3 Rust crate?
What's the specific rationale for the exact pin here?
| serde = { version = "1.0", features = ["derive"], optional = true } | ||
| serde_json = { version = "1.0", optional = true } | ||
| once_cell = "1.19" | ||
| rustc-hash = "2.0" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify rustc-hash version is current and optimal for deterministic use.
The dependency specifies rustc-hash = "2.0" using semver caret syntax, allowing automatic minor/patch updates. Given the PR's emphasis on deterministic scheduling, confirm that:
- Version 2.0 is the latest stable release
- Default features provide the required determinism guarantees for
FxHashMap/FxHashSet
🌐 Web query:
What is the latest stable version of the rustc-hash crate and what are its default features?
💡 Result:
Latest stable on crates.io: rustc-hash 2.1.1 (published 2025-02-05). Default features: a single default feature "std" (i.e., the "std" feature is enabled by default). [1][2]
Sources:
[1] docs.rs rustc-hash 2.1.1 (version + publish date).
[2] docs.rs rustc-hash feature flags (shows "default" -> "std").
Upgrade rustc-hash to latest stable (2.1.1) for optimal dependency currency.
The dependency rustc-hash = "2.0" specifies version 2.0, but the latest stable is 2.1.1 (published 2025-02-05). While the caret syntax allows automatic minor/patch updates to 2.1.1, explicitly pinning an outdated version is sloppy. Upgrade to rustc-hash = "2.1.1" to:
- Eliminate ambiguity about which version was verified
- Capture the latest bug fixes and performance improvements in 2.1.1
- Confirm determinism: the "std" default feature is properly enabled and provides the required FxHashMap/FxHashSet guarantees
🤖 Prompt for AI Agents
In crates/rmg-core/Cargo.toml around line 22 the dependency is pinned as
rustc-hash = "2.0"; update this to rustc-hash = "2.1.1" to explicitly depend on
the latest stable release, ensuring you pick up bug fixes and performance
improvements; if your code relies on the standard feature set, keep the default
features (no change needed) or explicitly add features = ["std"] only if you
previously disabled defaults.
| for port_key in pr.footprint.b_in.iter() { | ||
| if active.ports.contains(*port_key) { | ||
| return true; | ||
| } | ||
| } | ||
| for port_key in pr.footprint.b_out.iter() { | ||
| if active.ports.contains(*port_key) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Compilation broken: PortSet has no .iter().
The four loops calling .iter() on pr.footprint.b_in / b_out won't even build—CI is already screaming with no method named 'iter' found for struct 'PortSet'. Switch to the actual traversal API that PortSet exposes (e.g. its keys()/for_each helper) and adjust both the conflict check and the marker path accordingly so we stop compiling against phantom methods.
Also applies to: 227-233
🧰 Tools
🪛 GitHub Actions: CI
[error] 197-197: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
[error] 202-202: no method named iter found for struct PortSet in the current scope. hint: method not found in PortSet
🤖 Prompt for AI Agents
crates/rmg-core/src/scheduler.rs around lines 197-205 (and likewise 227-233):
the code calls .iter() on PortSet which does not exist; replace those .iter()
loops by using PortSet's actual traversal API (e.g., call keys() to get an
iterator or use its for_each helper) to visit each port key and test membership
in active.ports, and update the marker-path logic to use the same traversal so
the conflict checks and marker adjustments operate on the actual keys returned
by keys()/for_each rather than a phantom iterator method. Ensure the replacement
preserves early-return on first match and mirrors the original loop behavior.
| pdflatex main.tex # generates main.aux | ||
| bibtex main # builds main.bbl from refs.bib | ||
| pdflatex main.tex # incorporates citations | ||
| pdflatex main.tex # final pass for cross-refs |
There was a problem hiding this comment.
Add shebang and fail-fast error handling.
The script lacks a shebang line and error handling, creating two critical issues:
- Missing shebang: Shell type is ambiguous (bash/sh/dash), leading to portability issues and preventing static analysis tools from providing shell-specific advice.
- Silent failures: Commands continue executing even after errors, potentially producing incomplete/corrupt output files (e.g., if
pdflatexfails,bibtexstill runs on stale.aux).
Apply this diff:
+#!/usr/bin/env bash
+set -euo pipefail
+
pdflatex main.tex # generates main.aux
bibtex main # builds main.bbl from refs.bib
pdflatex main.tex # incorporates citations
pdflatex main.tex # final pass for cross-refsThe set -euo pipefail ensures:
-e: Exit immediately if any command fails-u: Treat unset variables as errors-o pipefail: Pipeline fails if any command in it fails
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pdflatex main.tex # generates main.aux | |
| bibtex main # builds main.bbl from refs.bib | |
| pdflatex main.tex # incorporates citations | |
| pdflatex main.tex # final pass for cross-refs | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| pdflatex main.tex # generates main.aux | |
| bibtex main # builds main.bbl from refs.bib | |
| pdflatex main.tex # incorporates citations | |
| pdflatex main.tex # final pass for cross-refs |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In rmg-math/build.sh around lines 1 to 4, the script lacks a shebang and
fail-fast options so the shell is ambiguous and errors are silently ignored; add
a bash shebang as the first line (#!/usr/bin/env bash) and enable strict error
handling by adding set -euo pipefail immediately after it so the script exits on
any command failure, treats unset variables as errors, and fails pipelines if
any element fails; no other logic changes required.
| \NeedsTeXFormat{LaTeX2e} | ||
| \ProvidesPackage{rmg-macros}[2025/11/06 RMG math macros] | ||
| \RequirePackage{amsmath,amssymb,amsthm,mathtools,enumitem,hyperref} | ||
| \newcommand{\Cat}[1]{\mathbf{#1}} | ||
| \newcommand{\GraphT}{\Cat{Graph}_T} | ||
| \newcommand{\OGraphT}{\Cat{OGraph}_T} | ||
| \newcommand{\HypT}{\Cat{Hyp}_T} | ||
| \newcommand{\OHypT}{\Cat{OHyp}_T} | ||
| \newcommand{\mono}{\hookrightarrow} | ||
| \newcommand{\To}{\Rightarrow} | ||
| \newcommand{\iso}{\cong} | ||
| \DeclareMathOperator{\Del}{Del} | ||
| \DeclareMathOperator{\Use}{Use} | ||
| \DeclareMathOperator{\Halo}{Halo} | ||
| \DeclareMathOperator{\im}{im} | ||
| \theoremstyle{plain}\newtheorem{theorem}{Theorem} | ||
| \newtheorem{lemma}{Lemma}\newtheorem{proposition}{Proposition} | ||
| \theoremstyle{definition}\newtheorem{definition}{Definition} | ||
| \theoremstyle{remark}\newtheorem{remark}{Remark} | ||
| \setlist{nosep}\hypersetup{hidelinks} |
There was a problem hiding this comment.
Inconsistency: Missing corollary environment compared to docs/rmg-macros.sty.
This file is a condensed version of docs/rmg-macros.sty, but Line 17 omits the \newtheorem{corollary}{Corollary} environment that exists in the docs version (docs/rmg-macros.sty:37).
If both files should provide the same API, add the missing environment:
\theoremstyle{plain}\newtheorem{theorem}{Theorem}
-\newtheorem{lemma}{Lemma}\newtheorem{proposition}{Proposition}
+\newtheorem{lemma}{Lemma}\newtheorem{proposition}{Proposition}\newtheorem{corollary}{Corollary}
\theoremstyle{definition}\newtheorem{definition}{Definition}Alternatively, if these files serve different purposes (different document types), document the intentional divergence.
Optional style note: The ultra-condensed formatting (multiple commands per line) sacrifices readability for brevity. Consider whether maintainability is more important than saving ~26 lines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \NeedsTeXFormat{LaTeX2e} | |
| \ProvidesPackage{rmg-macros}[2025/11/06 RMG math macros] | |
| \RequirePackage{amsmath,amssymb,amsthm,mathtools,enumitem,hyperref} | |
| \newcommand{\Cat}[1]{\mathbf{#1}} | |
| \newcommand{\GraphT}{\Cat{Graph}_T} | |
| \newcommand{\OGraphT}{\Cat{OGraph}_T} | |
| \newcommand{\HypT}{\Cat{Hyp}_T} | |
| \newcommand{\OHypT}{\Cat{OHyp}_T} | |
| \newcommand{\mono}{\hookrightarrow} | |
| \newcommand{\To}{\Rightarrow} | |
| \newcommand{\iso}{\cong} | |
| \DeclareMathOperator{\Del}{Del} | |
| \DeclareMathOperator{\Use}{Use} | |
| \DeclareMathOperator{\Halo}{Halo} | |
| \DeclareMathOperator{\im}{im} | |
| \theoremstyle{plain}\newtheorem{theorem}{Theorem} | |
| \newtheorem{lemma}{Lemma}\newtheorem{proposition}{Proposition} | |
| \theoremstyle{definition}\newtheorem{definition}{Definition} | |
| \theoremstyle{remark}\newtheorem{remark}{Remark} | |
| \setlist{nosep}\hypersetup{hidelinks} | |
| \NeedsTeXFormat{LaTeX2e} | |
| \ProvidesPackage{rmg-macros}[2025/11/06 RMG math macros] | |
| \RequirePackage{amsmath,amssymb,amsthm,mathtools,enumitem,hyperref} | |
| \newcommand{\Cat}[1]{\mathbf{#1}} | |
| \newcommand{\GraphT}{\Cat{Graph}_T} | |
| \newcommand{\OGraphT}{\Cat{OGraph}_T} | |
| \newcommand{\HypT}{\Cat{Hyp}_T} | |
| \newcommand{\OHypT}{\Cat{OHyp}_T} | |
| \newcommand{\mono}{\hookrightarrow} | |
| \newcommand{\To}{\Rightarrow} | |
| \newcommand{\iso}{\cong} | |
| \DeclareMathOperator{\Del}{Del} | |
| \DeclareMathOperator{\Use}{Use} | |
| \DeclareMathOperator{\Halo}{Halo} | |
| \DeclareMathOperator{\im}{im} | |
| \theoremstyle{plain}\newtheorem{theorem}{Theorem} | |
| \newtheorem{lemma}{Lemma}\newtheorem{proposition}{Proposition}\newtheorem{corollary}{Corollary} | |
| \theoremstyle{definition}\newtheorem{definition}{Definition} | |
| \theoremstyle{remark}\newtheorem{remark}{Remark} | |
| \setlist{nosep}\hypersetup{hidelinks} |
🤖 Prompt for AI Agents
In rmg-math/sty/rmg-macros.sty around lines 1–20, the package is missing the
\newtheorem{corollary}{Corollary} declaration present in docs/rmg-macros.sty
(docs version line ~37); to fix, add the \newtheorem{corollary}{Corollary}
declaration alongside the existing theorem/lemma/proposition declarations (same
theoremstyle block) so both files expose the same API, or if the omission is
intentional, add a brief comment noting the deliberate divergence.
| #!/usr/bin/env python3 | ||
| """ | ||
| Bake Criterion results into a self-contained HTML report that works over file:// | ||
| Reads estimates from target/criterion for known groups and injects them into | ||
| docs/benchmarks/index.html, producing docs/benchmarks/report-inline.html with | ||
| `window.__CRITERION_DATA__` and `window.__CRITERION_MISSING__` prepopulated. | ||
| Usage: | ||
| python3 scripts/bench_bake.py [--out docs/benchmarks/report-inline.html] | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import json | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| ROOT = Path(__file__).resolve().parents[1] | ||
| CRITERION = ROOT / "target" / "criterion" | ||
| TEMPLATE = ROOT / "docs" / "benchmarks" / "index.html" | ||
| DEFAULT_OUT = ROOT / "docs" / "benchmarks" / "report-inline.html" | ||
|
|
||
| # Only bake groups the dashboard renders by default | ||
| GROUPS = [ | ||
| ("snapshot_hash", "Snapshot Hash"), | ||
| ("scheduler_drain", "Scheduler Drain"), | ||
| ("scheduler_drain/enqueue", "Scheduler Enqueue"), | ||
| ("scheduler_drain/drain", "Scheduler Drain Phase"), | ||
| ] | ||
| INPUTS = [10, 100, 1000, 3000, 10000, 30000] | ||
|
|
There was a problem hiding this comment.
File permissions: Script not executable despite shebang.
The script has a shebang (#!/usr/bin/env python3) but lacks executable permissions.
Apply this fix:
chmod +x scripts/bench_bake.pyOr remove the shebang if the script is only meant to be invoked via python3 scripts/bench_bake.py.
The constants and imports look clean. The GROUPS list matches the dashboard configuration.
🧰 Tools
🪛 Ruff (0.14.3)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In scripts/bench_bake.py around lines 1-32, the file includes a Python shebang
but is not marked executable; make the script executable in the repository (set
the executable bit on the file) so it can be run directly via
./scripts/bench_bake.py and commit that permission change so the executable mode
is tracked in git; alternatively, if you prefer not to support direct execution,
remove the shebang and leave invocation via python3 explicit—apply one of these
fixes and commit the change.
| def load_estimate(group: str, n: int): | ||
| base = CRITERION / group / str(n) | ||
| for kind in ("new", "base", "change"): | ||
| p = base / kind / "estimates.json" | ||
| if p.exists(): | ||
| try: | ||
| obj = json.loads(p.read_text()) | ||
| mean = ( | ||
| obj.get("mean", {}).get("point_estimate") | ||
| if isinstance(obj.get("mean"), dict) | ||
| else None | ||
| ) | ||
| if mean is None and isinstance(obj.get("Mean"), dict): | ||
| mean = obj["Mean"].get("point_estimate") | ||
| lb = ( | ||
| obj.get("mean", {}) | ||
| .get("confidence_interval", {}) | ||
| .get("lower_bound") | ||
| ) | ||
| ub = ( | ||
| obj.get("mean", {}) | ||
| .get("confidence_interval", {}) | ||
| .get("upper_bound") | ||
| ) | ||
| if mean is None: | ||
| raise ValueError("missing mean.point_estimate") | ||
| return { | ||
| "ok": True, | ||
| "path": str(p.relative_to(ROOT)), | ||
| "mean": float(mean), | ||
| "lb": float(lb) if lb is not None else None, | ||
| "ub": float(ub) if ub is not None else None, | ||
| } | ||
| except Exception as e: | ||
| return { | ||
| "ok": False, | ||
| "path": str(p.relative_to(ROOT)), | ||
| "error": f"parse error: {e}", | ||
| } | ||
| return { | ||
| "ok": False, | ||
| "path": str((base / "new" / "estimates.json").relative_to(ROOT)), | ||
| "error": "not found (tried new/base/change)", | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Code quality: Exception handling could be more specific.
Lines 40-72 use a broad except Exception catch-all, and Line 59 raises a generic ValueError with a dynamic message. These are flagged by Ruff (BLE001, TRY003, TRY301).
While the current error handling works for this utility script, consider these improvements for better debugging:
- Catch specific exceptions instead of bare
Exception:
except (json.JSONDecodeError, KeyError, TypeError, ValueError) as e:
return {"ok": False, "path": str(p.relative_to(ROOT)), "error": f"parse error: {e}"}- Use a custom exception or keep ValueError but avoid dynamic messages:
if mean is None:
return {"ok": False, "path": str(p.relative_to(ROOT)), "error": "missing mean.point_estimate"}The function logic is sound; these are style improvements for production-grade code.
🧰 Tools
🪛 Ruff (0.14.3)
59-59: Abstract raise to an inner function
(TRY301)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In scripts/bench_bake.py around lines 34 to 78, the function load_estimate uses
a broad except Exception and raises a dynamic ValueError; change the error
handling to catch specific exceptions (e.g., json.JSONDecodeError, KeyError,
TypeError, ValueError) in the except clause and return the same {"ok": False,
"path": ..., "error": ...} structure instead of raising, and replace the raise
ValueError("missing mean.point_estimate") with an immediate return of the error
dict ("missing mean.point_estimate") to avoid throwing and to make failures
explicit and typed.
| def build_inline_script(results, missing) -> str: | ||
| return ( | ||
| "<script>\n" | ||
| + "window.__CRITERION_DATA__ = " | ||
| + json.dumps(results, separators=(",", ":")) | ||
| + ";\nwindow.__CRITERION_MISSING__ = " | ||
| + json.dumps(missing, separators=(",", ":")) | ||
| + ";\n</script>\n" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Style: Use implicit string concatenation or f-string.
Lines 81-87 use explicit + operators for string concatenation, which Ruff flags as ISC003.
Refactor using implicit concatenation:
def build_inline_script(results, missing) -> str:
return (
"<script>\n"
"window.__CRITERION_DATA__ = " + json.dumps(results, separators=(",", ":")) + ";\n"
"window.__CRITERION_MISSING__ = " + json.dumps(missing, separators=(",", ":")) + ";\n"
"</script>\n"
)Or use an f-string:
def build_inline_script(results, missing) -> str:
data_json = json.dumps(results, separators=(",", ":"))
missing_json = json.dumps(missing, separators=(",", ":"))
return (
f"<script>\n"
f"window.__CRITERION_DATA__ = {data_json};\n"
f"window.__CRITERION_MISSING__ = {missing_json};\n"
f"</script>\n"
)🧰 Tools
🪛 Ruff (0.14.3)
82-83: Explicitly concatenated string should be implicitly concatenated
Remove redundant '+' operator to implicitly concatenate
(ISC003)
🤖 Prompt for AI Agents
In scripts/bench_bake.py around lines 80 to 89, the function build_inline_script
uses explicit '+' string concatenation (triggering ISC003); change it to either
implicit string literal concatenation or an f-string: compute JSON strings for
results and missing first (e.g., data_json, missing_json) then return a single
parenthesized multi-line string that embeds those variables via implicit
concatenation or f-strings so no '+' operators are used.
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 (3)
crates/rmg-core/src/footprint.rs (2)
157-165: pack_port_key: OK for tests, but collision‑prone; keep it far from prod paths.Using only 32 bits of the digest for the node fingerprint risks collisions; docs say “tests/demos,” which is fine. Suggest adding a brief caution to the docstring and a
#[cfg(debug_assertions)]assert that this helper isn’t used in non‑test modules./// This is sufficient for tests and demos; production code may adopt a /// different stable scheme as long as equality and ordering are preserved. +/// +/// Caution: only 32 bits of the node fingerprint are used; do not rely on this +/// helper for collision‑sensitive production keys. #[inline] pub fn pack_port_key(node: &NodeId, port_id: u32, dir_in: bool) -> PortKey {
167-201: Add a tiny test for iter() order and PortSet/IdSet IntoIterator.Ensures deterministic ordering stays covered as APIs evolve.
#[test] fn idset_iter_is_sorted() { let mut s = IdSet::default(); let a = Hash::from(blake3::hash(b"a").into()); let b = Hash::from(blake3::hash(b"b").into()); // Insert in reverse s.0.insert(b); s.0.insert(a); let collected: Vec<_> = (&s).into_iter().cloned().collect(); assert_eq!(collected, vec![a, b]); }docs/echo-total.md (1)
1702-3316: Fix markdownlint violations across docs and re-run the rollupSummary: the lint run reports ~3,687 errors across docs/**/*.md. Required actions (apply in source files under docs/, then re-run the provided script):
- Add blank lines around headings, lists and fenced code blocks (MD022 / MD031 / MD032).
- Annotate every fenced code block with a language (
rust,shell, ```ts, etc.) (MD040).- Convert setext titles to ATX and ensure exactly one top-level H1 per file (MD003 / MD025).
- Address long-line complaints (MD013) by wrapping text or adding targeted markdownlint-disable for unavoidable long lines (CLI output, long tables, URLs).
- Remove duplicate headings (MD024), collapse multiple blank lines (MD012), trim trailing spaces (MD009), and fix list indentation (MD005 / MD007).
- Fix inline HTML usage (MD033) and table pipe style (MD055) where present.
High-priority starting points: docs/architecture-outline.md, docs/echo-total.md, docs/decision-log.md (each contains many violations).
After changes: run the original check script and attach the new lint output (or push to CI). I will re-check the results and close the comment when lint is clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/rmg-core/src/footprint.rs(2 hunks)docs/decision-log.md(3 hunks)docs/echo-total.md(9 hunks)docs/execution-plan.md(4 hunks)
🧰 Additional context used
🪛 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.
🪛 markdownlint-cli2 (0.18.1)
docs/echo-total.md
1704-1704: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1710-1710: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1715-1715: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1720-1720: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1726-1726: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1731-1731: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1737-1737: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1743-1743: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1750-1750: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1755-1755: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1774-1774: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1785-1785: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1794-1794: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1805-1805: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1812-1812: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1819-1819: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1828-1828: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1829-1829: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
1837-1837: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
1843-1843: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1851-1851: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2085-2085: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
2085-2085: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
2109-2109: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2113-2113: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2118-2118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2122-2122: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2125-2125: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2128-2128: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2131-2131: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2132-2132: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
2132-2132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2153-2153: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
2153-2153: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
2431-2431: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2445-2445: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2462-2462: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2480-2480: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2498-2498: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2515-2515: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2531-2531: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2549-2549: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2591-2591: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2595-2595: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
2644-2644: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2678-2678: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2701-2701: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
2701-2701: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2703-2703: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
2704-2704: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/1/1
(MD029, ol-prefix)
2714-2714: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2761-2761: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2766-2766: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
2767-2767: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
2768-2768: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
2800-2800: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
2800-2800: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
2808-2808: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
2808-2808: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
3076-3076: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3170-3170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3271-3271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3310-3310: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
3313-3313: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
3316-3316: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (3)
crates/rmg-core/src/footprint.rs (1)
111-143: Independence check looks correct and ordered (mask → ports → edges → nodes).Good symmetry and early exits. No action needed.
docs/echo-total.md (1)
1-1: Script environment lacks process substitution (/dev/fd) — unable to verify regeneration locally, but infrastructure is sound.The
gen-echo-total.shscript failed in the sandbox environment at thefind | sort -zpipeline stage due to missing/dev/fdsupport (a sandbox limitation, not a code issue). However, the entire infrastructure—workflow, script, Makefile target—is correctly configured and will execute successfully in the actual GitHub Actions environment (ubuntu-latest).Your review comment is structurally correct: If CI's
echo-total-check.ymlworkflow detected a mismatch and generated the error message you cited, then regenerating withmake echo-total(orscripts/gen-echo-total.sh) and committing is the right fix. The file has already been regenerated once on this branch (visible in git history and diff), so the developer likely just needs to verify docs are current and re-run the command before merge.However, definitively confirming whether the file is currently stale in this branch requires either:
- Running the full CI check in an environment with proper bash capabilities
- Manually verifying no doc source changes occurred since the last regeneration
docs/decision-log.md (1)
25-25: Clarify scope: is PortSet::iter() part of this PR or a gated follow-up?Line 25 introduces
PortSet::iter()as an "additive API" fix to "Unblock Clippy/build on GH."But execution-plan line 39 (Today's Intent) claims "no API surface changes intended" for the scheduler Clippy work, and line 48 later calls out PortSet::iter() as a "CI follow-up."
Verdict: Is PortSet::iter() scoped to commits
b29f168/9223f27in this PR, or is it in a separate follow-up commit? If in this PR, update the execution-plan to reflect the API addition. If separate, clarify the line 25 date/commit boundary.
| /// Returns an iterator over the identifiers in the set. | ||
| pub fn iter(&self) -> impl Iterator<Item = &Hash> { | ||
| self.0.iter() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nice: deterministic iteration exposure for IdSet.
Consider also implementing IntoIterator for &IdSet so for h in &id_set works idiomatically.
#[derive(Debug, Clone, Default)]
-pub struct IdSet(BTreeSet<Hash>);
+pub struct IdSet(BTreeSet<Hash>);
+
+impl<'a> IntoIterator for &'a IdSet {
+ type Item = &'a Hash;
+ type IntoIter = std::collections::btree_set::Iter<'a, Hash>;
+ fn into_iter(self) -> Self::IntoIter { self.0.iter() }
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/rmg-core/src/footprint.rs around lines 40–43, add an implementation of
IntoIterator for &IdSet so users can write `for h in &id_set`; implement
`impl<'a> IntoIterator for &'a IdSet` with Item = &Hash and IntoIter = the
iterator type returned by the internal set (use the concrete iter type, e.g. the
BTreeSet::Iter or equivalent) and have into_iter simply call the existing iter()
method, ensuring lifetimes and types match the file's imports.
| /// Returns an iterator over the port keys in the set. | ||
| pub fn iter(&self) -> impl Iterator<Item = &PortKey> { | ||
| self.0.iter() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Likewise for PortSet: add IntoIterator for &PortSet.
This improves ergonomics without exposing internals.
#[derive(Debug, Clone, Default)]
pub struct PortSet(BTreeSet<PortKey>);
impl PortSet {
@@
pub fn iter(&self) -> impl Iterator<Item = &PortKey> {
self.0.iter()
}
}
+
+impl<'a> IntoIterator for &'a PortSet {
+ type Item = &'a PortKey;
+ type IntoIter = std::collections::btree_set::Iter<'a, PortKey>;
+ fn into_iter(self) -> Self::IntoIter { self.0.iter() }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Returns an iterator over the port keys in the set. | |
| pub fn iter(&self) -> impl Iterator<Item = &PortKey> { | |
| self.0.iter() | |
| } | |
| /// Returns an iterator over the port keys in the set. | |
| pub fn iter(&self) -> impl Iterator<Item = &PortKey> { | |
| self.0.iter() | |
| } | |
| } | |
| impl<'a> IntoIterator for &'a PortSet { | |
| type Item = &'a PortKey; | |
| type IntoIter = std::collections::btree_set::Iter<'a, PortKey>; | |
| fn into_iter(self) -> Self::IntoIter { self.0.iter() } | |
| } |
🤖 Prompt for AI Agents
In crates/rmg-core/src/footprint.rs around lines 71 to 74, add an implementation
of IntoIterator for &PortSet so taking a reference can be used in for-loops and
iterator contexts; implement IntoIterator for &'_ PortSet with type Item = &'_
PortKey and IntoIter = std::slice::Iter<'_, PortKey> (or the appropriate
iterator produced by self.0.iter()), and have into_iter(self) return self.iter()
to forward to the existing iter() method without exposing internals.
| ## Recent Decisions (2025-10-28 onward) | ||
|
|
||
| The following entries use a heading + bullets format for richer context. | ||
| | 2025-11-06 | rmg-core scheduler Clippy cleanup | Make pre-commit pass without `--no-verify`: fix `doc_markdown`, `similar_names`, `if_not_else`, `option_if_let_else`, `explicit_iter_loop`; change `RewriteThin.handle` to `usize`; keep radix `counts16` as `Vec<u32>` (low bandwidth) with safe prefix-sum/scatter; fail fast in drain with `unreachable!` instead of `expect()` or silent drop; make `pending` field private (keep `PendingTx` private). | Preserve determinism and ordering while satisfying strict `clippy::pedantic` and `-D warnings`. Avoid truncation casts and private interface exposure. | Determinism preserved; panic on invariant violation; histogram remains 256 KiB on 64‑bit; pre-commit unblocked. |
There was a problem hiding this comment.
Clarify fail-fast mechanics and bucket count in histogram.
Line 23 Decision packs substantial scope into one cell. Three precision issues:
-
Fail-fast language: Decision says "fail fast in drain with
unreachable!instead ofexpect()or silent drop" — good detail. But Consequence says "panic on invariant violation" which is colloquial. Be explicit: useunreachable!()macro to panic loudly when invariants fail (missing payload, out-of-range handle, etc.), replacing prior silent drops. -
Histogram specificity: Decision mentions "keep radix
counts16asVec<u32>(low bandwidth)" but omits bucket count (65,536) and memory size (~256 KiB). These are key to the PR objective. Recommend:keep radix histogram `counts16` as `Vec<u32>` (65,536 buckets, ~256 KiB; low bandwidth prefix-sum/scatter preserved) -
Rationale clarity: Rationale mentions "Avoid truncation casts" but doesn't name the driver (
RewriteThin.handle: u32 → usize). Recommend:Promote `RewriteThin.handle` from `u32` to `usize` to eliminate truncation casts in hot paths...
| | 2025-11-06 | rmg-core scheduler Clippy cleanup | Make pre-commit pass without `--no-verify`: fix `doc_markdown`, `similar_names`, `if_not_else`, `option_if_let_else`, `explicit_iter_loop`; change `RewriteThin.handle` to `usize`; keep radix `counts16` as `Vec<u32>` (low bandwidth) with safe prefix-sum/scatter; fail fast in drain with `unreachable!` instead of `expect()` or silent drop; make `pending` field private (keep `PendingTx` private). | Preserve determinism and ordering while satisfying strict `clippy::pedantic` and `-D warnings`. Avoid truncation casts and private interface exposure. | Determinism preserved; panic on invariant violation; histogram remains 256 KiB on 64‑bit; pre-commit unblocked. | ||
| | 2025-11-06 | rmg-core test + benches lint fixes | Clean up `clippy::pedantic` failures blocking commit: (1) add backticks to doc comments for `b_in`/`b_out` and `GenSet(s)`; (2) refactor `DeterministicScheduler::reserve` into helpers to satisfy `too_many_lines`; (3) move inner test function `pack_port` above statements to satisfy `items_after_statements`; (4) remove `println!` and avoid `unwrap()`/`panic!` in tests; (5) use captured format args and `u64::from(...)`/`u32::from(...)` idioms; (6) fix `rmg-benches/benches/reserve_scaling.rs` imports (drop unused `CompactRuleId` et al.) and silence placeholder warnings. | Align tests/benches with workspace lint policy while preserving behavior; ensure CI and pre-commit hooks pass uniformly. | Clippy clean on lib + tests; benches compile; commit hook no longer blocks. |
There was a problem hiding this comment.
Resolve apparent contradiction: panic removal (tests) vs. addition (production drain).
Line 24 states: "(4) remove println! and avoid unwrap()/panic! in tests."
Line 23 states: "fail fast in drain with unreachable! instead of expect() or silent drop" (which adds panic-like behavior).
Readers may wonder: are we removing panics or adding them? The contexts differ (test hygiene vs. production invariant assertion), but this should be made explicit to avoid confusion.
Recommend clarifying the rationale in both entries:
- Line 24 Rationale: "Keep tests free of diagnostic print/panic calls; use assertion/test harness patterns instead."
- Line 23 Rationale: "...Production code must panic loudly on invariant violations (e.g., missing payload) rather than silently dropping, ensuring deterministic failure." (Separate from test hygiene.)
🤖 Prompt for AI Agents
In docs/decision-log.md around lines 23 to 24, clarify the apparent
contradiction between removing panics in tests and introducing a panic in
production drain: update the two entries to explicitly state that test changes
remove println!/unwrap!/panic! for test hygiene and deterministic reporting (use
assertions/test-harness patterns instead), while the scheduler change
intentionally introduces a loud panic (unreachable!/panic!) in production drain
to fail fast on invariant violations and preserve determinism; reword both lines
to include those rationales so readers understand the different contexts.
| - Changes: | ||
| - Doc lint: add backticks in `scheduler.rs` docs for `b_in`/`b_out` and `GenSet(s)`. | ||
| - Reserve refactor: split `DeterministicScheduler::reserve` into `has_conflict`, `mark_all`, `on_conflict`, `on_reserved` (fix `too_many_lines`). | ||
| - Tests hygiene: move inner `pack_port` helper above statements (`items_after_statements`), remove `println!`, avoid `unwrap()`/`panic!`, use captured format args. | ||
| - Numeric idioms: replace boolean→int and lossless casts with `u64::from(...)` / `u32::from(...)`. | ||
| - Benches: drop unused imports in `reserve_scaling.rs` to avoid workspace clippy failures when checking all targets. | ||
| - Expected behavior: identical drain order and semantics; minor memory increase for counts on 64‑bit. |
There was a problem hiding this comment.
Missing histogram details; vague on type-change scope.
The "Changes" bullets omit two core work items from the PR objectives:
- Histogram retained as
Vec<u32>(65,536 buckets ≈ 256 KiB) with wrapping prefix-sum/scatter RewriteThin.handle: u32 → usizeto avoid truncation casts
Line 44 vaguely states "Numeric idioms: replace boolean→int and lossless casts" but doesn't capture the explicit type-promotion change or histogram retention. This makes the plan read as though the numeric work is only about style cleanup, not structural changes.
Additionally, line 46 claims "minor memory increase for counts on 64‑bit" which contradicts the PR objective stating "memory...unchanged vs prior u32 approach (~256 KiB)." Clarify: are you noting that vec metadata grows on 64-bit, or is the claim that the PR itself increases memory footprint?
Recommend revising line 40 to name the concrete changes:
- Changes:
- - Doc lint: add backticks in `scheduler.rs` docs for `b_in`/`b_out` and `GenSet(s)`.
- - Reserve refactor: split `DeterministicScheduler::reserve` into `has_conflict`, `mark_all`, `on_conflict`, `on_reserved` (fix `too_many_lines`).
+ - Histogram: retain `Vec<u32>` with 65,536 buckets (~256 KiB); no memory/bandwidth change vs prior.
+ - Handle type: promote `RewriteThin.handle` from `u32` → `usize` to eliminate truncation casts.
+ - Doc lint: add backticks in `scheduler.rs` docs for `b_in`/`b_out` and `GenSet(s)`.
+ - Reserve refactor: split `DeterministicScheduler::reserve` into `has_conflict`, `mark_all`, `on_conflict`, `on_reserved` (fix `too_many_lines`).And clarify line 46 to align with the PR objective:
- Expected behavior: identical drain order and semantics; minor memory increase for counts on 64‑bit.
+ Expected behavior: identical drain order and semantics; memory footprint unchanged (histogram ~256 KiB; handle usize promotion localized).🤖 Prompt for AI Agents
docs/execution-plan.md lines 40–46: the "Changes" bullets omit two key PR items
and contradict the memory note; update the list to explicitly state that the
histogram is retained as a Vec<u32> with 65,536 buckets (~256 KiB) using a
wrapping prefix-sum/scatter implementation, and that RewriteThin.handle is
promoted from u32 to usize to avoid truncation, and revise the memory sentence
to say that per-count storage remains ~256 KiB (u32 buckets) while overall
allocation/metadata sizes change on 64-bit due to usize-sized pointers/lengths
rather than implying counts increased.
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/BENCHMARK_GUIDE.md(1 hunks)docs/benchmarks/RESERVE_BENCHMARK.md(1 hunks)docs/scheduler-reserve-complexity.md(1 hunks)docs/scheduler-reserve-validation.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/BENCHMARK_GUIDE.md
24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
68-68: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
91-91: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
100-100: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
101-101: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/benchmarks/RESERVE_BENCHMARK.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
80-80: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Security Audit
- GitHub Check: Tests (musl)
🔇 Additional comments (6)
docs/BENCHMARK_GUIDE.md (2)
30-66: Excellent Criterion.rs best practices—setup/teardown and black_box patterns are correct.Your use of
black_box()to prevent compiler constant-folding and placement of setup (data creation) outside the timing closure (lines 43–44) align precisely with official Criterion.rs guidance. The multi-input pattern withbench_with_input(),Throughputtracking, andBenchmarkIdalso matches Criterion's input-driven benchmarking standard.
127-149: All referenced infrastructure verified—documentation is accurate.Every referenced file, script, and Makefile target exists and is correctly configured. The benchmark infrastructure described in lines 127-149 is fully in place:
scripts/bench_bake.pywithGROUPSlist, all six Makefile targets, dashboard output files, and cross-referenced documentation files. No gaps or inconsistencies detected.docs/benchmarks/RESERVE_BENCHMARK.md (1)
1-127: Reject this entire documentation file—implementations do not exist.The referenced benchmark implementation does not exist, and the actual file in the codebase is unregistered and incomplete:
crates/rmg-benches/benches/reserve_independence.rsdoes not exist (the documentation is wrong about the filename)crates/rmg-benches/benches/reserve_scaling.rsexists but is an empty placeholder with TODOs, not the complete implementation described- Cargo.toml registration is missing — no
[[bench]]entry for eitherreserve_independenceorreserve_scaling- Dashboard integration is missing — no references to either benchmark in
index.htmlorbench_bake.py- Results table (lines 40–47) claims specific benchmark measurements that cannot exist from unimplemented code
The file's entire narrative—comprehensive benchmarking, dashboard integration, results validation, interpretation—is based on work that was never completed. The code stub explicitly states: "This is a placeholder - the actual benchmark needs to be in rmg-core" with TODO items for proper implementation.
This documentation should either be removed or completely rewritten to acknowledge its draft/placeholder status. Do not merge this file.
Likely an incorrect or invalid review comment.
docs/scheduler-reserve-validation.md (3)
87-110: Loop-counting restatement presented as verified, but lacks actual code verification.Lines 92–107 list "12 for-loops" (6 in Phase 1 checking, 6 in Phase 2 marking), but this is just a restatement of complexity-analysis.md's self-corrected count. Without the actual implementation, I cannot verify:
Nested vs sequential loops: The pseudo-code shows if-blocks within loops. Are these truly independent loops, or nested conditionals? Line 40 shows
if conflict { return false; }inside the loop—is this early return counted as a "loop" or a branch?Actual loop boundaries: Do all 12 loops execute unconditionally, or do any depend on earlier state? If Phase 1 returns early, Phase 2 never runs—so they're not always 12 iterations.
Footprint definition: Line 115 sums |n_write|, |n_read|, etc., but doesn't define whether these are always present, can be empty, or are optional. This affects whether m is well-defined.
Recommendation: Either embed the actual code or explicitly state these are assumptions that must be verified against the implementation.
197-226: Fast path IS implemented; however, performance claim remains unvalidated and merge recommendation is premature.The review comment contains a critical factual error and a valid structural concern:
❌ INCORRECT: Line 205 claims "Current implementation doesn't use factor_mask early exit." In fact,
crates/rmg-core/src/footprint.rslines 119–120 implement this exact optimization in theindependent()function:if (self.factor_mask & other.factor_mask) == 0 { return true; }This fast path is already in the codebase and used during conflict checking.
✅ VALID CONCERNS REMAIN:
- Benchmark is unimplemented:
crates/rmg-benches/benches/reserve_scaling.rsis a TODO stub (lines 32–44). The placeholder explicitly states: "TODO: Implement this properly." No direct old-vs-new comparison exists.- Stress testing limited: Scaling test maxes out at k=100, m=100.
- Merge recommendation contradicts stated gaps: Recommending merge "with caveats" when benchmarking—the primary validation method—is missing (not deferred) is inconsistent.
Recommendation: Correct the documentation's false claim about the missing fast path, then either:
- Downgrade merge readiness to "blocked until benchmarks are implemented," OR
- Upgrade the commit message caveats to explicitly flag the unimplemented benchmark.
14-55: Reject this review comment entirely—it's factually incorrect and undermines legitimate documentation.The original reviewer's "credibility gap" accusation is baseless:
✅ Test EXISTS and IS PROVIDED:
reserve_is_atomic_no_partial_marking_on_conflictis implemented at scheduler.rs:829–902 in the actual codebase. It's not pseudo-code; it's real, auditable test code.✅ Pseudo-code MATCHES REALITY: The documented test design precisely mirrors the actual test implementation:
- Rewrite 1 writes node A (succeeds)
- Rewrite 2 reads A (conflict!) and writes B—fails immediately, never marks B
- Rewrite 3 writes B (succeeds)—proving rewrite 2 didn't partially mark
✅ Two-phase protocol is REAL: The implementation at scheduler.rs:117–241 demonstrates the exact pattern described:
has_conflict()checks all resources; returns false on conflict (no marking)mark_all()marks only if phase 1 succeeds- Atomic guarantee: zero partial state on failure
✅ Line references are AUDITABLE: Developers can navigate to scheduler.rs:830 and verify the test structure themselves. (Minor nitpick: reference should cite line 829 for the
#[test]decorator, but this is trivial.)The real issue: The reviewer confused "test code in documentation" with "test code not existing." The documentation IS empirical proof because the test is in the repository, verifiable, and accurately described.
Likely an incorrect or invalid review comment.
| ## Step-by-Step Guide | ||
|
|
||
| ### 1. Create the Benchmark File | ||
|
|
||
| Create a new benchmark in `crates/rmg-benches/benches/`: | ||
|
|
||
| ```rust |
There was a problem hiding this comment.
Fix missing blank line and language specifier for fenced code block.
Criterion.rs benchmarks require harness = false in Cargo.toml, and your Rust snippet (lines 25–59) should follow markdown formatting standards. Per markdownlint-cli2 (MD031, MD040), fenced code blocks must be surrounded by blank lines and specify a language.
### 1. Create the Benchmark File
Create a new benchmark in `crates/rmg-benches/benches/`:
~ ```rust
// crates/rmg-benches/benches/my_feature.rs
use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
use rmg_core::*; // Import what you need
fn bench_my_feature(c: &mut Criterion) {
let mut group = c.benchmark_group("my_feature");
// Configure measurement
group.sample_size(50); // Statistical samples
group.measurement_time(std::time::Duration::from_secs(8));
// Test multiple input sizes to validate complexity
for &n in &[10, 100, 1_000, 3_000, 10_000, 30_000] {
// Set throughput for per-operation metrics
group.throughput(Throughput::Elements(n as u64));
group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, &n| {
// Setup (outside timing)
let data = create_test_data(n);
// Measured operation
b.iter(|| {
let result = my_feature(black_box(&data));
black_box(result); // Prevent optimization
});
});
}
group.finish();
}
criterion_group!(benches, bench_my_feature);
criterion_main!(benches);
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/BENCHMARK_GUIDE.md around lines 19 to 25, the fenced Rust code block is
missing a blank line before and after the block and does not specify the
language; also mention Criterion's requirement to set harness = false in
Cargo.toml. Fix by inserting a blank line before the opening rust fence and use rust as the opening fence, ensure the block is closed with ``` and
followed by a blank line, and add a short note telling users to set harness =
false in crates/rmg-benches/Cargo.toml (or the workspace Cargo.toml) so
Criterion benchmarks run correctly.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| ### 2. Register in Cargo.toml | ||
|
|
||
| Add to `crates/rmg-benches/Cargo.toml`: | ||
|
|
||
| ```toml | ||
| [[bench]] | ||
| name = "my_feature" | ||
| harness = false # Required for Criterion | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Fix missing blank line and language specifier for Cargo.toml code block.
Per markdownlint-cli2 (MD031, MD040), this TOML configuration block must be surrounded by blank lines and include a language specifier.
### 2. Register in Cargo.toml
Add to `crates/rmg-benches/Cargo.toml`:
+ ```toml
[[bench]]
name = "my_feature"
harness = false # Required for Criterion
- ```
+ ```
+🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
68-68: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/BENCHMARK_GUIDE.md around lines 67 to 76, the Cargo.toml fenced code
block is missing a leading/trailing blank line and a language specifier; update
the block so there is a blank line before and after the fenced section and
change the opening fence to specify the language as "toml" (```toml) so the
snippet is properly fenced and tools like markdownlint-cli2 recognize it.
| Verify the JSON artifacts exist: | ||
| ```bash | ||
| ls -la target/criterion/my_feature/*/new/estimates.json | ||
| ``` | ||
|
|
||
| ### 4. Integrate with Dashboard | ||
|
|
||
| #### 4a. Add to `docs/benchmarks/index.html` | ||
|
|
||
| Find the `GROUPS` array and add your benchmark: | ||
|
|
||
| ```javascript | ||
| const GROUPS = [ | ||
| // ... existing benchmarks ... | ||
| { | ||
| key: 'my_feature', // Must match group name | ||
| label: 'My Feature Description', // Display name | ||
| color: '#7dcfff', // Hex color (pick unique) | ||
| dash: '2,6' // Line style: null or '2,6' or '4,4' or '8,4' | ||
| }, | ||
| ]; | ||
| ``` | ||
|
|
||
| **Color Palette (already used):** | ||
| - `#bb9af7` - Purple (snapshot_hash) | ||
| - `#9ece6a` - Green (scheduler_drain) | ||
| - `#e0af68` - Yellow (scheduler_enqueue) | ||
| - `#f7768e` - Red (scheduler_drain/drain) | ||
| - `#7dcfff` - Cyan (reserve_independence) | ||
|
|
||
| **Pick a new color or use available:** | ||
| - `#ff9e64` - Orange | ||
| - `#73daca` - Teal | ||
| - `#c0caf5` - Light blue | ||
|
|
||
| **Dash Patterns:** | ||
| - `null` - Solid line | ||
| - `'2,6'` - Short dashes (dotted) | ||
| - `'4,4'` - Medium dashes | ||
| - `'8,4'` - Long dashes | ||
|
|
||
| #### 4b. Add to `scripts/bench_bake.py` |
There was a problem hiding this comment.
Convert emphasis-based section markers to proper heading syntax.
Per markdownlint-cli2 (MD036), emphasis (**text**) must not be used in place of heading syntax. Lines 90, 100, and 110 use bold text where markdown headings are required.
ls -la target/criterion/my_feature/*/new/estimates.json-
4a. Add to
docs/benchmarks/index.html
-
4a. Add to
docs/benchmarks/index.htmlFind the
GROUPSarray and add your benchmark: -
const GROUPS = [ // ... existing benchmarks ... { key: 'my_feature', // Must match group name label: 'My Feature Description', // Display name color: '#7dcfff', // Hex color (pick unique) dash: '2,6' // Line style: null or '2,6' or '4,4' or '8,4' }, ];
- Color Palette (already used):
-
Color Palette (already used)
#bb9af7- Purple (snapshot_hash)#9ece6a- Green (scheduler_drain)#e0af68- Yellow (scheduler_enqueue)#f7768e- Red (scheduler_drain/drain)#7dcfff- Cyan (reserve_independence)
- Pick a new color or use available:
-
Pick a new color or use available
#ff9e64- Orange#73daca- Teal#c0caf5- Light blue
- Dash Patterns:
-
Dash Patterns
null- Solid line'2,6'- Short dashes (dotted)'4,4'- Medium dashes'8,4'- Long dashes
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
---
91-91: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
100-100: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
---
101-101: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
docs/BENCHMARK_GUIDE.md lines 86-127: the file uses bold/strong text in place of
actual Markdown headings (MD036) at the indicated spots and the fenced code
block boundaries need to be correct; replace the emphasis-based section markers
(the ... lines around the color palette, "Pick a new color..." and "Dash
Patterns") with proper heading syntax (e.g., "### Color Palette (already used)",
"### Pick a new color or use available", "### Dash Patterns"), ensure the
surrounding code fence for the JavaScript snippet is correctly opened and
closed, and keep the existing list items unchanged so the section renders as
intended.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| ### 3. Results | ||
|
|
||
| Benchmark results for reserve() with n rewrites (each checking against k-1 prior): | ||
|
|
||
| | n (rewrites) | Mean Time | Time per Reserve | Throughput | | ||
| |--------------|-----------|------------------|------------| | ||
| | 10 | 8.58 µs | 858 ns | 1.17 M/s | | ||
| | 100 | 81.48 µs | 815 ns | 1.23 M/s | | ||
| | 1,000 | 827 µs | 827 ns | 1.21 M/s | | ||
| | 3,000 | 3.37 ms | 1.12 µs | 894 K/s | | ||
| | 10,000 | 11.30 ms | 1.13 µs | 885 K/s | | ||
| | 30,000 | 35.57 ms | 1.19 µs | 843 K/s | | ||
|
|
||
| **Analysis:** | ||
| - **Per-reserve time remains roughly constant** (~800-1200 ns) across all scales | ||
| - This proves O(m) complexity, **independent of k** (# prior reserves) | ||
| - Slight slowdown at larger scales likely due to: | ||
| - Hash table resizing overhead | ||
| - Cache effects | ||
| - Memory allocation | ||
|
|
||
| **Comparison to Theoretical O(k×m):** | ||
| - If reserve were O(k×m), the n=30,000 case would be ~900× slower than n=10 | ||
| - Actual: only 4.1× slower (35.57ms vs 8.58µs) | ||
| - **Validates O(m) claim empirically** |
There was a problem hiding this comment.
Clarify what "time per reserve" metric represents; avoid ambiguity in O(k×m) comparison.
Lines 40–47: The table shows "Time per Reserve" but the benchmark design (lines 13–22) measures "full apply+commit cycle with k-1 prior reserves for kth rewrite." This means:
- Total time includes enqueue + drain + reserve + execute for each rewrite, not reserve() in isolation
- Dividing by n conflates reserve() cost with executor and drain overhead
Lines 57–60: The O(k×m) vs O(m) comparison claims "if reserve were O(k×m), n=30,000 case would be ~900× slower." This reasoning assumes:
- Constant work per reserve (W) such that O(k×m) = 900× O(m)
- But mixing full-cycle timing with reserve()-only complexity claims is imprecise
Recommendation: Add a clarifying note or separate micro-benchmark (reserve()-only, no execute) to isolate GenSet overhead from the full commit cycle. Document the assumption that enqueue+drain+execute overhead is constant relative to reserve work.
| ```bash | ||
| # Option 1: Open inline report (works with file://) | ||
| open docs/benchmarks/report-inline.html | ||
|
|
||
| # Option 2: Serve and view live (fetches from target/criterion) | ||
| make bench-serve # Serves on http://localhost:8000 | ||
| # Then open http://localhost:8000/docs/benchmarks/index.html | ||
| ``` |
There was a problem hiding this comment.
Add language identifier and blank lines to code fence; fix heading spacing.
Multiple markdown linting violations detected:
- The code block at lines 75–82 lacks a language identifier (should be
bash) - Missing blank line before heading at line 74
- Missing blank line after heading before code block at line 75
Additionally, lines 80 and 90 have similar violations.
Apply this diff to fix spacing and add language identifiers:
-### View Dashboard
-```
+### View Dashboard
+
+```bash
# Option 1: Open inline report (works with file://)
open docs/benchmarks/report-inline.html
# Option 2: Serve and view live (fetches from target/criterion)
make bench-serve # Serves on http://localhost:8000
# Then open http://localhost:8000/docs/benchmarks/index.html
Similarly, ensure all remaining code blocks have language specifiers and proper blank-line spacing per MD031 and MD040.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
79-79: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
80-80: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
---
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/benchmarks/RESERVE_BENCHMARK.md around lines 75 to 82, the markdown has
lint violations: add a blank line before the heading at line 74 and insert a
blank line after the heading, replace the existing heading with "### View
Dashboard" (ensure a leading blank line), and update the code fence to include
the language identifier "bash" (```bash) and a blank line before and after the
code block; then scan the file for other code fences (notably around lines ~80
and ~90) and add appropriate language specifiers and surrounding blank lines to
satisfy MD031 and MD040.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| 1. **Hash Table Complexity:** | ||
| - GenSet uses `FxHashMap` which is O(1) average case | ||
| - Worst case with pathological hash collisions: O(log n) or O(n) | ||
| - In practice with good hashing: **O(1) amortized** | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Hash table worst-case complexity insufficiently emphasized for deterministic scheduler context.
Line 64 acknowledges O(n) worst case "with pathological hash collisions" but then dismisses it in line 65 with "in practice with good hashing: O(1) amortized." This is a dangerous assumption for a deterministic scheduler, where:
- Worst-case latency is critical (not just average case)
- Adversarial inputs (even unintentional) could trigger collisions
- "In practice" is not a guarantee
Questions:
- Which hasher is actually used? (Document says FxHashMap but doesn't confirm if it's Fx or xxHash or something else)
- Has collision behavior been tested with real workloads or adversarial inputs?
- What is the actual worst-case guarantee, if any?
Recommendation: Explicitly state that complexity analysis assumes good hashing behavior, and recommend benchmarking under pathological hash inputs to validate the O(m) claim holds.
🏁 Script executed:
#!/bin/bash
# Read the actual file to verify the quoted lines and surrounding context
if [ -f "docs/scheduler-reserve-complexity.md" ]; then
sed -n '55,75p' docs/scheduler-reserve-complexity.md
else
find . -name "scheduler-reserve-complexity.md" -type f
fiLength of output: 838
🏁 Script executed:
#!/bin/bash
# Search for hash collision testing, benchmarking, or worst-case analysis in codebase
rg -nP --type=md --type=rs -C5 "(collision|benchmark|adversarial|pathological|worst.*case)" \
--glob="*scheduler*" --glob="*complexity*" --glob="*hash*"Length of output: 26
🏁 Script executed:
#!/bin/bash
# Find references to FxHashMap to understand which hasher is actually used
rg -nP --type=rs "FxHashMap|use.*hash|hasher" -C3 | head -100Length of output: 26
🌐 Web query:
FxHashMap Rust hasher O(1) worst case collision behavior
💡 Result:
Short answer: No — FxHashMap (the fxhash crate) uses a fast, non-keyed FxHasher that is NOT collision-resistant; an attacker (or unlucky inputs) can produce many collisions and force quadratic/worst‑case O(n) behavior in the map. Use the default RandomState (std::collections::HashMap) or a keyed hasher like aHash for DOS resistance. [1][2][3]
Sources:
- fxhash crate docs (disclaimer: non-cryptographic, not designed to prevent collision attacks). [1]
- std::collections::HashMap docs (default uses a randomized, DOS‑resistant hasher via RandomState). [2]
- aHash docs (example of a fast, keyed alternative intended to avoid HashDoS). [3]
🏁 Script executed:
#!/bin/bash
# Fix: Search for FxHashMap in Rust files without the --type flag issue
rg -n "FxHashMap" --glob="*.rs" -C4Length of output: 2870
🏁 Script executed:
#!/bin/bash
# Search for GenSet definition and usage
rg -n "GenSet" --glob="*.rs" -C3Length of output: 5275
🏁 Script executed:
#!/bin/bash
# Search for adversarial, DOS, collision, or security-related comments around hash usage
rg -n "adversarial|DOS|collision|HashDoS|security" --glob="*.rs" --glob="*.md"Length of output: 9808
FxHashMap collision risk is REAL and inadequately disclosed in complexity documentation.
Your concerns are substantiated. Evidence:
- FxHashMap is "non-cryptographic, not designed to prevent collision attacks" and can be forced into O(n) per lookup.
- GenSet uses FxHashMap at the core (crates/rmg-core/src/scheduler.rs, line 471; deployed 5 times in ActiveFootprints struct lines 24–32).
- The codebase already acknowledges this gap: docs/notes/scheduler-optimization-followups.md explicitly states "Assumes no adversarial input targeting hash collisions" and recommends "Could switch to ahash or SipHash if collision attacks are a concern" (lines 95–97).
- The promised mitigation does not exist: The recommended benchmark
crates/rmg-benches/benches/scheduler_adversarial.rs(line 165) is not present in the codebase.
Consequence: Claimed O(m) complexity collapses to O(m × n) worst-case under adversarial inputs, violating the deterministic scheduler's latency guarantees.
Fixes required:
- Update docs/scheduler-reserve-complexity.md lines 62–66 to explicitly state: "Assumes no adversarial input targeting hash collisions; collision-resistant hasher (aHash/SipHash) evaluation recommended for production."
- Create the missing adversarial collision benchmark (crates/rmg-benches/benches/scheduler_adversarial.rs) to validate O(m) holds under stress.
- Evaluate hasher swap (aHash or SipHash) as a blocking item for release.
🤖 Prompt for AI Agents
docs/scheduler-reserve-complexity.md lines 62-66: The complexity blurb
understates FxHashMap collision risk; update those lines to explicitly note
"Assumes no adversarial input targeting hash collisions; collision-resistant
hasher (aHash/SipHash) evaluation recommended for production." Additionally add
a new benchmark file at crates/rmg-benches/benches/scheduler_adversarial.rs that
reproduces adversarial collision workloads to validate O(m) behavior under
stress, and add a follow-up task in
docs/notes/scheduler-optimization-followups.md to evaluate and, if needed, swap
to a collision-resistant hasher (aHash or SipHash) as a release-blocking item.
| ## Previous Implementation (Vec<Footprint>-based) | ||
|
|
||
| ### Code Structure | ||
| ``` | ||
| reserve(tx, pending_rewrite): | ||
| for prev_footprint in reserved_footprints: // k iterations | ||
| if !footprint.independent(prev_footprint): | ||
| return false | ||
| reserved_footprints.push(footprint.clone()) | ||
| ``` | ||
|
|
||
| ### Footprint::independent() Complexity (footprint.rs:114-138) | ||
|
|
||
| ``` | ||
| independent(a, b): | ||
| if (a.factor_mask & b.factor_mask) == 0: // O(1) - fast path | ||
| return true | ||
| if ports_intersect(a, b): // O(min(|a.ports|, |b.ports|)) | ||
| return false | ||
| if edges_intersect(a, b): // O(min(|a.e_*|, |b.e_*|)) | ||
| return false | ||
| if nodes_intersect(a, b): // O(min(|a.n_*|, |b.n_*|)) | ||
| return false | ||
| ``` | ||
|
|
||
| **Set intersection uses dual-iterator on sorted BTrees:** | ||
| - Complexity: O(min(|A|, |B|)) per intersection | ||
| - 4 intersection checks per `independent()` call | ||
|
|
||
| ### Total Complexity | ||
|
|
||
| **Best case (factor_mask disjoint):** O(k) | ||
|
|
||
| **Worst case (overlapping masks, no intersections):** | ||
| - k iterations × 4 intersection checks × O(m) per check | ||
| - **O(k × m)** where m is average footprint size | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Previous implementation analysis lacks verification, masks optimization opportunity.
Line 88 references footprint.rs:114-138 for the independent() complexity, but implementation is not provided. Additionally:
-
Missing fast-path analysis: Line 92–93 shows
factor_mask & b.factor_maskcheck that can return early with O(1), but the analysis jumps to worst case O(k × m). What percentage of calls trigger the fast path? This could dramatically change the practical complexity. -
Dual-iterator intersection assumption: Line 105 assumes "sorted BTrees" for O(min(|A|, |B|)) intersection, but doesn't verify this is the actual implementation.
-
No empirical comparison data: Both the old and new approaches are analyzed theoretically, but the section lacks any measurement showing the old approach was actually slow.
Strengthen this section by:
- Providing or referencing actual code snippets
- Analyzing factor_mask fast-path trigger frequency
- Including empirical measurements (if available) comparing old vs new
🤖 Prompt for AI Agents
In docs/scheduler-reserve-complexity.md around lines 77 to 116, the analysis of
the previous Vec<Footprint> implementation is incomplete: it references
footprint.rs:114-138 without showing the actual code, omits a quantitative
analysis of the factor_mask fast-path frequency, assumes dual-iterator on sorted
BTrees without verifying the data structure/algorithm used, and provides no
empirical timing/benchmark data. Fix by: include the exact independent() code
snippet (or a link to it) in the doc, add a short empirical stat showing what
percentage of independent() calls hit the (factor_mask & ...) fast-path (obtain
via instrumenting or sampling production/test runs), confirm and document the
actual set intersection implementation (BTree vs HashSet and iterator-based
intersection complexity) with code references, and add concise benchmark results
comparing old vs new approach (wall-clock and average per-call cost) with
environment and input-size notes.
| ### Example: k=100, m=20 | ||
|
|
||
| **Old approach:** | ||
| - 100 iterations × 4 intersections × ~10 comparisons = **~4,000 operations** | ||
|
|
||
| **New approach:** | ||
| - 20 hash lookups (checking) + 20 hash inserts (marking) = **~40 operations** | ||
|
|
||
| **Theoretical speedup: ~100x** | ||
|
|
||
| But actual speedup depends on: | ||
| - Cache effects (hash table vs sorted BTree) | ||
| - Early exit frequency | ||
| - Hash collision rate |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Hand-wavy operation counting undermines theoretical speedup claim.
Lines 140–146 claim ~100× speedup based on:
- Old: 100 iterations × 4 intersections × ~10 comparisons = ~4,000 operations
- New: 20 lookups + 20 inserts = ~40 operations
Problems:
- The "~10 comparisons" per intersection is unjustified. What if sets are small (1–3 elements)? Then it's 100 × 4 × 2 = 800, not 4,000.
- Assumes NO early-exit in old implementation. Realistically, first conflict exits immediately.
- Counts hash operations as unit cost, ignoring that hashing itself is non-trivial (especially for complex types like edges/ports).
- Example uses k=100, m=20. What if k=10,000 or m=100? Sensitivity analysis missing.
Recommendation: Parameterize the example with realistic ranges (k, m) and show how speedup varies. Or remove the "~100×" claim and let empirical benchmarks speak.
🏁 Script executed:
#!/bin/bash
# Locate and examine the scheduler-reserve-complexity.md file
find . -type f -name "*scheduler-reserve-complexity*" 2>/dev/null | head -20Length of output: 38
🏁 Script executed:
#!/bin/bash
# If file found, display lines 130-160 to capture full context
if [ -f "docs/scheduler-reserve-complexity.md" ]; then
echo "=== File found, displaying lines 130-160 ==="
sed -n '130,160p' "docs/scheduler-reserve-complexity.md" | cat -n
echo ""
echo "=== Total line count ==="
wc -l "docs/scheduler-reserve-complexity.md"
fiLength of output: 1109
🏁 Script executed:
#!/bin/bash
# Search for parameter definitions and complexity analysis context
if [ -f "docs/scheduler-reserve-complexity.md" ]; then
echo "=== Searching for 'k=' and 'm=' parameter definitions ==="
grep -n "k=" "docs/scheduler-reserve-complexity.md" | head -20
echo ""
echo "=== Searching for 'sensitivity' or 'varies' ==="
grep -n -i "sensitivity\|varies\|depends\|range" "docs/scheduler-reserve-complexity.md" | head -30
fiLength of output: 170
Quantitative speedup claim lacks analytical backing across parameter ranges; add sensitivity analysis or justify example representativeness.
The "~100x speedup" at lines 140–146 rests on one arbitrary data point (k=100, m=20) chosen from stated ranges (k: 10-1000, m: 5-50). The calculation—100 × 4 × ~10 = 4,000—is hand-waved:
- The "~10 comparisons" per intersection is unjustified. At m=5, expect fewer; at m=50, expect more.
- No early-exit quantification despite acknowledging it as a factor (line 148).
- Zero sensitivity analysis: speedup at k=10, m=50 vs k=1000, m=5 could differ by orders of magnitude.
Fix: Either (a) add a table/graph showing speedup across realistic (k, m) combinations, or (b) explicitly state why k=100, m=20 is representative and expected to hold across the range. Caveats about benchmarking (line 155–156) do not excuse incomplete analytical rigor upfront.
| ## 4. Performance Claims: Measured Results | ||
|
|
||
| ### Test: `reserve_scaling_is_linear_in_footprint_size` (scheduler.rs:982-1084) | ||
|
|
||
| **Methodology:** | ||
| 1. Reserve k=100 independent rewrites (creates active set) | ||
| 2. Measure time to reserve rewrites with varying footprint sizes | ||
| 3. All new rewrites are independent → k shouldn't affect timing | ||
|
|
||
| **Results (on test machine):** | ||
|
|
||
| | Footprint Size (m) | Time (µs) | Ratio to m=1 | | ||
| |--------------------|-----------|--------------| | ||
| | 1 | 4.4 | 1.0× | | ||
| | 10 | 20.1 | 4.6× | | ||
| | 50 | 75.6 | 17.2× | | ||
| | 100 | 244.2 | 55.5× | | ||
|
|
||
| **Analysis:** | ||
| - Roughly **linear scaling** with footprint size | ||
| - Not quadratic (which would show 100² = 10,000× for m=100) | ||
| - If it were O(k×m) with k=100, the m=100 case would be ~100× slower than m=1, not 56× | ||
| - Superlinear growth (56× vs 100×) likely due to: | ||
| - Hash table resizing overhead | ||
| - Cache effects with larger working sets | ||
| - Allocation costs | ||
|
|
||
| ### Theoretical vs Empirical | ||
|
|
||
| **Claimed:** "10-100x faster" | ||
|
|
||
| **Reality:** | ||
| - **Theoretical speedup** for k=100, m=20: ~100× | ||
| - **Empirical measurement needed** to compare old vs new directly | ||
| - Current test shows **O(m) scaling confirmed** | ||
| - Independence from k is proven by design | ||
|
|
||
| **Honest Assessment:** | ||
| - ✅ O(m) complexity confirmed empirically | ||
| - ✅ Independence from k proven by algorithm | ||
| - ⚠️ "10-100x" claim is extrapolated, not measured against old code | ||
| - ✅ For k=100, speedup should be ~100× in the limit | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Critical mathematical error in performance analysis; measurement rigor gaps; misleading speedup claims.
Line 161 contains a fundamental error:
"If it were O(k×m) with k=100, the m=100 case would be ~100× slower than m=1, not 56×"
This is mathematically incorrect. If complexity is O(k×m) with k=100 (constant), then:
- Time at m=1: t₁ = C × 100 × 1 = 100C
- Time at m=100: t₁₀₀ = C × 100 × 100 = 10,000C
- Ratio: t₁₀₀ / t₁ = 100×
The measured 56× actually fits O(k×m) better than pure O(m)! If it were truly O(m) with overhead, the ratio should approach 100× as m increases, not plateau at 56×. The superlinear effects mentioned in lines 162–164 suggest the model is more complex.
Measurement Rigor Gaps (line 148–156):
- No confidence intervals or error bars. Performance measurements have variance. Is the difference between 20.1 µs and 75.6 µs statistically significant?
- Sampling strategy unclear. Single run per data point? Warmed up? Cache cleared? JIT compiled?
- "On test machine" is vague. Hardware details matter.
- No baseline. How does new approach compare to old approach in real numbers, not theory?
Misleading Conclusion (line 173):
"O(m) complexity confirmed empirically"
The data does NOT confirm O(m). It shows superlinear scaling (56× for 100× input). This could indicate:
- O(m log m) or O(m¹·⁵) due to cache misses
- O(k×m) with overlapping overhead
- Hash table resizing during the benchmark
Recommendation:
- Fix the mathematical reasoning in line 161
- Re-analyze the empirical data with proper statistical rigor (error bars, multiple runs, confidence intervals)
- Revise the conclusion to reflect actual observed complexity (not O(m) if 56× for 100× input)
- Compare old vs new with actual benchmark code (not theoretical)
Generate shell script to validate test existence and performance measurement methodology:
🏁 Script executed:
#!/bin/bash
# Verify test functions and measurement methodology
# 1. Check if the referenced test functions exist in scheduler.rs
echo "=== Checking for referenced test functions ==="
rg -n 'fn reserve_is_atomic_no_partial_marking_on_conflict|fn reserve_determinism_same_sequence_same_results|fn reserve_scaling_is_linear_in_footprint_size' crates/rmg-core/src/
# 2. Verify GenSet is actually FxHashMap-based
echo "=== Checking GenSet implementation ==="
rg -n 'struct GenSet|type GenSet|impl.*GenSet' crates/rmg-core/src/
# 3. Look for hash table implementation details
echo "=== Checking for FxHashMap usage ==="
rg -n 'FxHashMap' crates/rmg-core/src/
# 4. Check if reserve() implementation exists and matches documented signature
echo "=== Checking reserve() implementation ==="
rg -A 20 'fn reserve.*pending_rewrite' crates/rmg-core/src/scheduler.rs | head -50
# 5. Verify the loop counts match what's documented
echo "=== Counting for-loops in reserve() Phase 1 & 2 ==="
rg -B 5 -A 100 'reserve.*tx.*pending_rewrite' crates/rmg-core/src/scheduler.rs | grep -E '^\s*for ' | head -20Length of output: 1020
---
🏁 Script executed:
```shell
#!/bin/bash
# Find actual reserve() function and test implementation
echo "=== Search for reserve function (any variant) ==="
rg -n 'fn reserve' crates/rmg-core/src/scheduler.rs | head -20
echo "=== Extract test_reserve_scaling_is_linear_in_footprint_size implementation ==="
sed -n '992,1100p' crates/rmg-core/src/scheduler.rs
echo "=== Look for timing/benchmark code patterns ==="
rg -n 'Instant::now|elapsed|Duration|time|bench' crates/rmg-core/src/scheduler.rs | head -20
echo "=== Check for test utilities (assert_le, assert_eq on timing) ==="
rg -n -A 5 'reserve_scaling_is_linear_in_footprint_size' crates/rmg-core/src/scheduler.rs | head -40
Length of output: 6180
---
🏁 Script executed:
```shell
#!/bin/bash
# Find if there are external benchmarks or where timing numbers come from
echo "=== Search for benchmark files ==="
find . -type d -name 'benches' -o -name 'bench' 2>/dev/null
echo "=== Look for criterion configuration or benchmark runner ==="
rg -l 'criterion|Criterion|bencher|bench_' --type toml --type rs | head -20
echo "=== Check Cargo.toml for [[bench]] sections ==="
rg -A 5 '\[\[bench' crates/rmg-core/Cargo.toml
echo "=== Search where the specific timing numbers appear in repo ==="
rg '4\.4|20\.1|75\.6|244\.2' . 2>/dev/null
Length of output: 280695
Critical mathematical error and insufficient measurement rigor—speedup claim unvalidated.
Line 161 contains a logical fallacy. If reserve() has O(k×m) complexity with k fixed at 100, then:
- Time at m=1: ∝ 100 × 1
- Time at m=100: ∝ 100 × 100
- Measured ratio should be ~100×, not 56×
The 56× result actually contradicts O(k×m) and suggests the scaling relationship is more complex than stated.
Measurement rigor is insufficient for validation:
- No error bars, confidence intervals, or multiple runs per size
- Test code (line 992) acknowledges "timing is noisy in tests" yet performs single measurements
- Scheduler is rebuilt between iterations, adding unknown overhead
- Only assertions are
success,!empty, andlast >= first—no threshold validation
The conclusion misrepresents the data:
Line 173 claims "O(m) complexity confirmed empirically," but 56× scaling for 100× input is inconsistent with pure O(m). The measured superlinearity (56× vs. claimed 100× for O(k×m)) indicates either:
- The complexity model is wrong
- Overhead and cache effects dominate, making the functional form unclear
- Noise/variance masks the true trend
Required fixes:
- Correct the mathematical reasoning in line 161 or remove it
- Use a proper benchmarking tool (criterion, etc.) with statistical rigor
- Acknowledge that the actual complexity class is indeterminate from this data
- Explicitly state whether doc numbers are from this test or external measurement
🤖 Prompt for AI Agents
In docs/scheduler-reserve-validation.md around lines 139 to 181, fix the
incorrect mathematical reasoning and overconfident conclusions: replace/remove
the claim at line 161 that implies O(k×m) would yield 100× but observed 56×
without acknowledging variance or overhead, and change the assertion at line 173
("O(m) complexity confirmed empirically") to a qualified statement that the data
suggests O(m)-like scaling but is insufficient to prove complexity class. Update
the methodology/results section to state these numbers are from this single
noisy test (or clearly mark if external), and add a short callout recommending
proper benchmarking (e.g., Criterion), multiple runs with error bars/confidence
intervals, and controlling rebuild/overhead before asserting speedup magnitudes.
| ## Conclusion | ||
|
|
||
| **Devil's Advocate Assessment:** | ||
|
|
||
| ✅ **Atomic reservation:** Proven with test | ||
| ✅ **Determinism:** Proven with test | ||
| ✅ **Time complexity:** O(m) confirmed empirically | ||
| ✅ **12 for-loops:** Counted and documented | ||
| ⚠️ **"10-100x faster":** Extrapolated from theory, not benchmarked | ||
|
|
||
| **Recommendation:** The implementation is correct and has good complexity. The performance claim is theoretically sound but should be validated with actual benchmarks comparing old vs new before being stated as fact. | ||
|
|
There was a problem hiding this comment.
Merge recommendation is premature given performance-claim validation gaps.
Line 224 correctly marks performance claim as "Extrapolated from theory, not benchmarked" (
Problem: If the PR title or objectives state "10-100x faster" and the merge is approved, readers will see that claim in:
- PR description
- Commit messages
- Potentially release notes
- Issues/discussions linking this PR
By the time they dig into docs/scheduler-reserve-validation.md and find the caveat, the claim has already propagated. This is a form of inadvertent marketing of unvalidated claims.
Recommendation: Upgrade the merge condition:
- If "10-100x faster" is removed from PR title/objectives: Acceptable to merge with caveats
- If "10-100x faster" remains prominent: Merge must be blocked until benchmarks validate the claim
Either way, the disclaimer in this document should also appear prominently in the PR description or commit message.
🤖 Prompt for AI Agents
In docs/scheduler-reserve-validation.md around lines 216–227, the merge
recommendation is premature given the unvalidated "10-100x faster" performance
claim; either remove that claim from the PR title/objectives or block merging
until empirical benchmarks validating the claim are added, and ensure the
cautionary disclaimer is copied verbatim into the PR description and the commit
message so it appears prominently; update the document to state this merge
gating policy explicitly (remove/validate claim OR block merge) and add a
checklist item requiring benchmark artifacts before marking the PR as mergeable.
Summary
Changes
Determinism/Correctness
Performance
Testing and Lints
Docs Guard
Risk
Issue
Commits