refactor(moq-json): gate group rolls on already-written deltas#1909
Conversation
The delta-vs-snapshot budget check ran after computing the merge patch (and, on the Rust compressed path, after compressing it into the DEFLATE window), so a roll discarded all of that work. Move the gate ahead of the diff/patch/compression: compare only the deltas already written against ratio * snapshot, so rolling a new group costs no wasted CPU. Because the gate excludes the frame about to be written, the delta that tips a group past its budget still lands, so a group overshoots by at most one delta before rolling. Acceptable for an 8x default and well under the 256-frame cap. Also unifies the Rust and JS producers, which previously diverged: Rust compressed measured compressed sizes while Rust uncompressed and JS always measured raw against the current value's snapshot. Both now gate on the accumulated written bytes (compressed when compressing, raw otherwise) against the group's stored snapshot frame size. JS gains a #snapshotLen field mirroring Rust's snapshot_len. No wire-format change: frames are byte-identical, only the timing of the roll decision shifts. Tests updated for the one-frame overshoot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe JS and Rust JSON producers now gate delta emission using accumulated delta bytes and the current snapshot frame size, including compressed output sizes when compression is enabled. The delta-selection flow now allows the delta that crosses the ratio threshold and rolls the group afterward. The related tests were updated to expect the later rollovers and reconstructed values. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/json/src/json.test.ts (1)
154-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd compressed rollover coverage for the new byte-accounting path.
These tests cover plaintext budgeting, but the PR also changes compressed budgeting to use wire frame sizes. A compressed
deltaRatiotest would catch regressions where#snapshotLenor#deltaBytesaccidentally use raw lengths.As per coding guidelines, tests should cover critical functionality.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/json/src/json.test.ts` around lines 154 - 178, Add a compressed rollover test alongside the existing Producer deltaRatio coverage in json.test.ts to exercise the new byte-accounting path. Use the Producer<Value> flow with compression enabled, then assert rollover behavior using a deltaRatio that forces the gate to rely on wire frame sizes rather than raw lengths. This should specifically validate the byte counters in Producer and its `#snapshotLen/`#deltaBytes accounting so regressions in compressed budgeting are caught.Source: Coding guidelines
rs/moq-json/src/lib.rs (1)
647-678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover compressed delta-ratio rollover too.
The updated tests validate the new overshoot timing for raw frame sizes only. Since
delta()now budgets compressed groups by compressedslice.len(), add a compressed rollover test so raw-size accounting can’t regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-json/src/lib.rs` around lines 647 - 678, Add a compressed rollover test to cover the new `delta()` budgeting path, since the current tests only exercise raw frame-size accounting. Extend the existing test area near `deltas_stay_within_ratio_times_snapshot` by adding a case that uses compressed groups and verifies rollover based on compressed `slice.len()`, so the `delta()` logic cannot regress back to raw-size measurements unnoticed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/json/src/producer.ts`:
- Around line 21-26: The public documentation for deltaRatio in the producer
logic is still misleading about when a snapshot group rolls: update the comment
near the delta accumulation logic in producer.ts to explain that the check is
based only on already-written bytes, so the delta that first pushes the total
past the budget is still emitted before a new snapshot group starts. Make the
wording in the deltaRatio and compression notes consistent with the actual
behavior in the producer frame grouping logic.
---
Nitpick comments:
In `@js/json/src/json.test.ts`:
- Around line 154-178: Add a compressed rollover test alongside the existing
Producer deltaRatio coverage in json.test.ts to exercise the new byte-accounting
path. Use the Producer<Value> flow with compression enabled, then assert
rollover behavior using a deltaRatio that forces the gate to rely on wire frame
sizes rather than raw lengths. This should specifically validate the byte
counters in Producer and its `#snapshotLen/`#deltaBytes accounting so regressions
in compressed budgeting are caught.
In `@rs/moq-json/src/lib.rs`:
- Around line 647-678: Add a compressed rollover test to cover the new `delta()`
budgeting path, since the current tests only exercise raw frame-size accounting.
Extend the existing test area near `deltas_stay_within_ratio_times_snapshot` by
adding a case that uses compressed groups and verifies rollover based on
compressed `slice.len()`, so the `delta()` logic cannot regress back to raw-size
measurements unnoticed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 091a89d8-9d14-470d-8eb2-88226b0718f4
📒 Files selected for processing (3)
js/json/src/json.test.tsjs/json/src/producer.tsrs/moq-json/src/lib.rs
The deltaRatio doc still said deltas "stay within ratio times the snapshot; otherwise a new group is started", implying the budget is never exceeded. The new gate checks only already-written deltas, so the delta that first crosses the budget still lands before the group rolls. Reword both the Rust and JS docs to state this. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rollover tests only exercised raw frame-size accounting. Since the budget is measured on compressed slice sizes when compression is on (snapshot_len / delta_bytes in Rust, #snapshotLen / #deltaBytes in JS), add a compressed rollover test in each language: a tight ratio over many distinct updates must roll at least one group, and a late joiner must still reconstruct the final value across the compressed group boundary. Guards against the compressed budget regressing to raw lengths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the two nitpick findings (compressed rollover coverage): added a compressed delta-ratio rollover test in both languages in commit be356bc — 🤖 Addressed by Claude Code |
1 participant
Summary
The snapshot/delta producer rolls a new group once accumulated deltas exceed
delta_ratio × snapshot. The budget check ran after computing the merge patch (and, on the Rust compressed path, after compressing it into the per-group DEFLATE window), so a roll threw all of that work away.This moves the gate ahead of the diff/patch/compression. It now compares only the deltas already written against
ratio × snapshot, so rolling a new group costs no wasted CPU (no merge patch, no inflate).Because the gate excludes the frame about to be written, the delta that tips a group past its budget still lands, so a group overshoots by at most one delta before rolling. That's negligible for the
8default and stays well under the 256-frame cap.Why
The old compressed path had to run
encoder.frame(&patch)purely to learn the delta's wire size, mutating the DEFLATE window before the decision and discarding it on a roll. Gating on the running total of frames we actually wrote removes that throwaway work entirely. The compressed measurement is kept (it's the right signal: later frames in a shared DEFLATE stream compress progressively smaller, which a raw-size heuristic would miss).Unifies Rust and JS
The two producers previously diverged on what they measured:
Both now gate on the accumulated written bytes (compressed when compressing, raw otherwise) against the group's stored snapshot frame size. JS gains a
#snapshotLenfield mirroring Rust'ssnapshot_len, and its write helpers return the bytes actually written.Compatibility
delta_ratio/deltaRatioand their defaults are unchanged. Doc comments were tightened, and the JSdeltaRatiodoc gained the "compressed frame sizes" note to match Rust now that JS measures compressed too.rs/moq-json→js/jsonboth updated.Test plan
cargo test -p moq-json— 32 pass (updatedtight_ratio_rolls_snapshots,deltas_stay_within_ratio_times_snapshot,newer_group_supersedes_in_progress_reconstructionfor the one-frame overshoot)bun run --filter='@moq/json' test— 47 pass (mirrored test updates; the cross-implvectors.test.tsfixture only covers diff/patch shape, unaffected)cargo fmt --check(via nix)bun run --filter='@moq/json' check(tsc + biome)(Written by Claude)