Repository navigation
refactor(runtime): key window demand counters by enum - #506
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughPer-window demand-plan counters now use ChangesDemand reason counters
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified; the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🔵 Needs a closer look
The change is correct and well-tested, but it adds a new third-party dependency (and a second major version of syn to the tree), which is a supply-chain/dependency-policy decision warranting human sign-off.
0 open findings
What changed in this PR
This PR is a focused ergonomic refactor of per-window frame-demand attribution in the render-thread scheduler. It replaces the positional [u64; DemandReason::COUNT] plan counter, which required converting each DemandReason to an integer index on every update, with an EnumMap<DemandReason, u64> indexed directly by the reason. The public diagnostic snapshot shape (WindowFrameSnapshot.demand_reasons, reason names, ordering) and process-wide atomic counters are intentionally preserved, so downstream consumers are unaffected.
Changes:
- Derive
enum_map::EnumonDemandReasonand store per-window plans asEnumMap<DemandReason, u64>, indexing with the reason directly and converting to the fixed-size array only for diagnostic snapshots. - Add a regression test covering zero-initialization, diagnostic array/name alignment, count retention across demand reconciliation, and window-stats removal.
- Add the
enum-map3.1.0 /enum-map-derive1.0.1 dependencies (with correspondingproc-macro2/quotepatch bumps andsyn 3.0.4in the lockfile).
| File | Description |
|---|---|
| crates/neomacs-display-runtime/src/render_thread/frame_sched.rs | Adds enum_map::Enum to the DemandReason derive list in the defining macro. |
| crates/neomacs-display-runtime/src/render_thread/frame_stats.rs | Switches the per-window plans field to EnumMap, using reason-keyed indexing, default(), and into_array() for snapshots. |
| crates/neomacs-display-runtime/src/render_thread/tests/frame_sched_test.rs | New test validating zero-init, ordering, count retention, and removal of window demand stats. |
| crates/neomacs-display-runtime/Cargo.toml | Adds the enum-map workspace dependency to the crate. |
| Cargo.toml | Declares enum-map = "3.1.0" in the workspace dependency table. |
| Cargo.lock | Locks enum-map/enum-map-derive and the induced proc-macro2/quote/syn version updates. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Per-window frame-demand counters used a positional array and required converting
DemandReasonto an integer index on each update. Store them asEnumMap<DemandReason, u64>and index directly with the reason.Derive
enum_map::Enumon the existing enum. Default new windows to zero counters, retain counts during demand reconciliation, and convert to the existing fixed-size array only when producing diagnostic snapshots. Keep the public snapshot fields, reason names, ordering, and process-wide atomic counters unchanged.Add enum-map 3.1.0 and enum-map-derive 1.0.1. The derive crate requires proc-macro2 1.0.107 and quote 1.0.47, updating those two locked patch versions; other existing package versions are unchanged.
Validation:
cargo nextest run -p neomacs-display-runtime --lib -E 'test(render_thread::frame_sched::frame_sched_test::)'passes all 46 scheduler tests in parallel. An added regression test assigns a distinct count to every reason and verifies zero initialization, diagnostic array/name alignment, count retention during reconciliation, and removal of window stats. Formatting and diff checks pass. Tests remain inrender_thread/tests/frame_sched_test.rs.