Streaming appends: tail-only GPU uploads, coalesced tier refines, throttled reopen re-sync - #188
Merged
Merged
Conversation
Greptile SummaryThis PR optimizes streaming appends and adds coverage for the new client-side behavior. The main changes are:
Confidence Score: 5/5Safe to merge with low risk. The append fast path is guarded by conservative fallback checks, the protocol and design docs were updated consistently, and focused Python and browser smoke tests cover the new behavior. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (2): Last reviewed commit: "Keep append encodings stable with split ..." | Re-trigger Greptile |
Merging this PR will degrade performance by 23.12%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_select_lasso_message_1m |
90.5 ms | 117.7 ms | -23.12% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/trace-append-perf-sbf211 (2a2a2e6) with main (aa464a8)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
Alek99
force-pushed
the
claude/trace-append-perf-sbf211
branch
from
July 23, 2026 18:40
d88e55c to
664a29b
Compare
…loads, coalesced tier refines, throttled reopen re-sync Streaming appends paid three per-tick O(N) costs beyond the wire payload itself: - The client destroyed and rebuilt every affected GPU trace with fresh bufferData allocations per tick. Now kernel encode offsets stay sticky across appends (Column.suggest_offset keeps the last shipped offset while every value stays within one span of it, at most 1 f32 mantissa bit vs a fresh midpoint), so consecutive append payloads keep byte-identical prefixes, and the client extends a direct scatter/line in place: the same buffer objects (VAO attachments stay valid) grow a capacity-doubling data store, mirroring Column.append, and each tick uploads only the appended tail via bufferSubData. Any encoding change (re-centered offset, expanded color/size domain, tier/style/keys change) falls back to the full rebuild. - Every append re-requested the current view with delay 0, so a streaming decimated line paid a full M4 round trip per tick. Appends now coalesce through the existing debounce machinery (delay 120 ms, maxWait 300 ms), and an at-home stream skips the decimated re-request entirely when the payload's recorded decimation_px (new spec field, §28) already covers the plot width. - FigureWidget.append re-synced the spec/buffers traits on every tick beside the custom message, shipping every payload twice. Live clients ignore trait updates, so the reopen-state re-sync now coalesces to at most one per second (leading edge immediate; trailing flush on the running event loop so the final streamed state always lands). Measured in headless Chromium (scripts/append_stream_smoke.py, 20k rows x 6 buffers, 200-row ticks): steady-state GPU upload drops from ~485 KB and 6 buffer reallocations per tick to 4.8 KB tail-only (~100x, scaling with N); at-home view round trips 5/5 -> 0/5, zoomed burst 6/6 -> 2/6; widget wire bytes per tick halve. The smoke also pins pixel-equivalence between N in-place appends and a fresh render of the final payload, and runs in CI beside the other stdlib gates.
Alek99
force-pushed
the
claude/trace-append-perf-sbf211
branch
from
July 24, 2026 17:27
664a29b to
d42c735
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStreaming append handling now preserves compatible payload prefixes, grows direct WebGL traces through tail uploads, rebuilds incompatible traces, records decimation width, coalesces refinement requests, and adds Chromium smoke coverage across local verification and CI. ChangesStreaming append
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StreamSource
participant ChartView
participant WebGL
participant ViewScheduler
StreamSource->>ChartView: append complete payload
ChartView->>WebGL: extend compatible trace tail
ChartView->>ChartView: rebuild incompatible trace
ChartView->>ViewScheduler: schedule coalesced refinement
ViewScheduler-->>ChartView: refined view payload
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/append_stream_smoke.py`:
- Line 247: Update both request-count filters in scripts/append_stream_smoke.py
at lines 247-247 and 276-276 to match messages where m.type is "density_view"
instead of "view", including the filters used by the home and zoomed assertions.
- Around line 270-273: Update the phase B loop in the append-stream smoke test
to generate six payloads whose row ranges continue monotonically after phase A’s
21,000 rows, rather than reusing the existing ticks via i%ticks.length. Keep
each message as type:"append" and preserve the current wait and append
invocation so the test exercises consecutive appends and refinement coalescing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e902dec3-3886-4f04-9535-46962815b50f
📒 Files selected for processing (13)
.github/workflows/ci.ymlCLAUDE.mdjs/src/54_kernel.tspython/xy/_payload.pypython/xy/columns.pyscripts/append_stream_smoke.pyscripts/render_smoke_nonumpy.pyscripts/verify_local.pyspec/design-dossier.mdspec/design/rust-engine.mdspec/design/wire-protocol.mdspec/process/contributing.mdtests/test_streaming.py
| await wait(60); | ||
| }} | ||
| await wait(1000); // any coalesced refine timer would have fired by now | ||
| const viewSendsHome=comm.sent.filter(m=>m.type==="view").length; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count density_view refinement messages. Both filters look for view, but client refinement requests use density_view; home re-requests are invisible and the zoomed assertion observes zero requests.
scripts/append_stream_smoke.py#L247-L247: filtercomm.sentusingm.type === "density_view".scripts/append_stream_smoke.py#L276-L276: filtercomm.sentusingm.type === "density_view".
📍 Affects 1 file
scripts/append_stream_smoke.py#L247-L247(this comment)scripts/append_stream_smoke.py#L276-L276
🤖 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 `@scripts/append_stream_smoke.py` at line 247, Update both request-count
filters in scripts/append_stream_smoke.py at lines 247-247 and 276-276 to match
messages where m.type is "density_view" instead of "view", including the filters
used by the home and zoomed assertions.
Comment on lines
+270
to
+273
| for(let i=0;i<6;i++){{ | ||
| const t=ticks[i%ticks.length]; | ||
| v._onKernelMsg({{type:"append",affected:[0,1],spec:t.spec}},[b64(t.blob)]); | ||
| await wait(60); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a monotonically growing payload sequence for phase B.
Phase A ends at 21,000 rows, but this replays 20,200–21,000-row payloads under type:"append". That exercises shrink/replacement behavior, not six consecutive appends, so refinement coalescing is not meaningfully validated.
Proposed fix
+ zoom_ticks = []
+ for k in range(TICKS + 1, TICKS + 7):
+ spec, blob = build_payload(N0 + k * M, x_off, y_off)
+ zoom_ticks.append({"spec": spec, "blob": base64.b64encode(blob).decode()})
+
-const ticks={json.dumps(ticks)};
+const ticks={json.dumps(ticks)};
+const zoomTicks={json.dumps(zoom_ticks)};
...
- for(let i=0;i<6;i++){{
- const t=ticks[i%ticks.length];
+ for(let i=0;i<6;i++){{
+ const t=zoomTicks[i];📝 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.
Suggested change
| for(let i=0;i<6;i++){{ | |
| const t=ticks[i%ticks.length]; | |
| v._onKernelMsg({{type:"append",affected:[0,1],spec:t.spec}},[b64(t.blob)]); | |
| await wait(60); | |
| for(let i=0;i<6;i++){{ | |
| const t=zoomTicks[i]; | |
| v._onKernelMsg({{type:"append",affected:[0,1],spec:t.spec}},[b64(t.blob)]); | |
| await wait(60); |
🤖 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 `@scripts/append_stream_smoke.py` around lines 270 - 273, Update the phase B
loop in the append-stream smoke test to generate six payloads whose row ranges
continue monotonically after phase A’s 21,000 rows, rather than reusing the
existing ticks via i%ticks.length. Keep each message as type:"append" and
preserve the current wait and append invocation so the test exercises
consecutive appends and refinement coalescing.
Alek99
added a commit
that referenced
this pull request
Jul 24, 2026
The tail-only append path (#188) extends an existing interleaved style buffer, whose stroke_width rows are baked in device pixels at the dpr in force when they were written. `_resize` moves `this.dpr` on browser zoom or a monitor swap *without* rebuilding traces, so points appended after such a change were scaled at the new dpr while the prefix kept the old one — a visible outline-width step inside a single trace. Record the dpr the style buffer was built at and fall back to the full rebuild when it no longer matches; the rebuild renormalizes every row. Two latent traps in the same path, neither reachable today: - The per-point buffer guards live inside the `kind === "scatter"` branch, so a future per-point channel on a line would be silently skipped rather than rejected. Refuse any non-scatter mark that carries one. - `tier_update` reallocates x/y/base data stores without clearing the append path's capacity bookkeeping. Unreachable because `decimate_view` skips exactly the traces the fast path accepts, but a stale cap would mean a tail `bufferSubData` past the end of the store if that drifts. Harden the append smoke, which was passing partly for the wrong reasons: - Phase B replayed phase-A payloads, shipping *fewer* rows than were already resident. That fails the `len >= old_len` prefix check, so it measured rebuild-path coalescing, not fast-path coalescing. Stream six fresh monotonic payloads instead. - DECIMATED_PX was 2048 against a ~620 px plot, so the at-home skip predicate `decimation_px >= plot.w` held with ~3x of slack — a unit or dpr error on either side would not have been caught. Tighten to 640. - Add a probe for the bug above: append once at a fixed dpr (asserting the fast path is taken), then move devicePixelRatio and append again (asserting a rebuild). Verified to fail with the fix reverted. Finally, give the log-family scale test one home (`lod.pins_offset_to_zero`) instead of a copy in `_payload.ship` and another in `geometry_offset`; if those drift, `ship` starts pinning a sticky midpoint on an axis whose shader transform requires the zero origin.
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the streaming-append audit: fixes items #3–#5 (the client-side O(N)-per-tick costs). The wire-level delta path (#2) and the multi-trace multiplier (#163) remain future work; with these three fixes the per-tick client cost drops from O(N) to O(rows appended) everywhere except the payload bytes themselves.
What changed
#3 — GPU buffers grow in place instead of realloc-per-tick (
js/src/54_kernel.ts,python/xy/columns.py)Column.suggest_offsetis now sticky across appends — once shipped, a column keeps its encode offset while every value stays within one span of it (≤1 f32 mantissa bit vs a fresh midpoint; a right-growing stream never exceeds that bound, and §16 deep-zoom re-centering is untouched). This makes consecutive append payloads byte-identical prefixes of each other.offset/scale/dtype, same channel shapes/domains, same style, no transition keys, nocurve:"smooth"/step) extends its existing GPU buffers with a tail-onlybufferSubData. Buffer objects are retained so VAO attachments stay valid; data stores grow with doubling capacity, mirroringColumn.append. Any precondition failure (re-centered offset, expanded continuous color/size domain, tier flip past a threshold, etc.) falls back to the full rebuild, which is always correct. The client derives prefix-compatibility entirely from the previous vs fresh spec — no wire-protocol change.#4 — tiered refines coalesce to burst cadence instead of one kernel round trip per tick (
js/src/54_kernel.ts,python/xy/_payload.py)_applyAppendno longer re-requests the view withdelay: 0; it uses the existing debounce machinery withdelay: 120 / maxWait: 300, so a continuous stream pays at most one M4/density round trip per 300 ms.decimation_px(§28: every decimation decision is recorded). Parked at home, the append payload was already M4-decimated over exactly that view — when the recorded px covers the plot width, the client skips the decimated re-request entirely.#5 — the widget no longer ships every append payload twice (
python/xy/widget.py)FigureWidget.appendused to re-sync thespec/bufferstraits and send the same spec+blob as the custom message every tick. Live clients ignore trait updates (they exist only for notebook-reopen state), so the trait re-sync now coalesces to at most one per second: leading edge immediate, trailing flush armed on the running event loop so the final streamed state always lands. Without a running loop (plain scripts, tests) every append flushes immediately — the old behavior.Measured
Headless-Chromium smoke (
scripts/append_stream_smoke.py, new CI gate: 20k rows, 6 per-point buffers, 200-row ticks, run before/after on the same harness):bufferDatareallocsWidget wire accounting (CI run on this branch,
Figure.scatter+ 1-row appends):append_datakernel timeTests / verification
scripts/append_stream_smoke.py(stdlib-only, likerender_smoke_nonumpy): asserts tail-only uploads after warm-up, pixel-equivalence between incremental appends and a fresh render, zero at-home re-requests, and burst coalescing. Wired intoci.yml,make check-browser, and the contributing table.render_smoke_nonumpystream probe updated to the new contract: in-place growth when encoding is unchanged, rebuild when a payload re-encodes (new offset).tests/test_streaming.py: sticky offsets + byte-identical prefixes across appends (geometry and stable-domain channels), expanding channel domain rewrites only that channel, re-centering guard,decimation_pxrecorded.tests/test_widget.py: throttled reopen re-sync (burst parks, trailing flush lands the final state) and immediate-flush behavior without an event loop.static/*.jsbundles were regenerated with the pinned vite toolchain).wire-protocol.md§4/§5,rust-engine.md§5, design-dossier §4, contributing gate table.Known scope limits (documented in the spec text): the fast path covers direct scatter/line — smooth/step-expanded lines, keyed transitions, and expanding continuous color/size domains rebuild as before (a shader-side normalization that would make channel domains tail-stable belongs to the #2 delta-frame redesign).
Generated by Claude Code
Summary by CodeRabbit