Rebuild streamed traces when the dpr moved under them - #266
Merged
Conversation
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.
|
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 (8)
📝 WalkthroughWalkthroughStreaming append logic now tracks style-buffer DPR, rejects unsafe tail uploads, and resets capacity metadata after reallocations. Geometry offset pinning uses a shared log-family policy. Browser smoke coverage and protocol documentation validate DPR changes, zoomed-history requests, and rebuild behavior. ChangesStreaming and geometry correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ChartView
participant WebGLBuffers
Browser->>ChartView: Change devicePixelRatio and resize
Browser->>ChartView: Request streaming append
ChartView->>ChartView: Compare stored and current DPR
ChartView->>WebGLBuffers: Rebuild trace when DPR differs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will degrade performance by 29%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_select_lasso_message_1m |
89.3 ms | 125.7 ms | -29% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing alek/append-fastpath-dpr-and-smoke-hardening (3d6bdad) with main (9b08085)
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. ↩
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 #188. One real defect, two latent traps in the same path, and a
round of hardening on the smoke that was covering it.
The bug
The tail-only append path extends the interleaved style buffer in place. Those
rows bake
stroke_widthin device pixels, at the dpr in force when theywere written (
_buildInstanceStyleChannels)._resizeupdatesthis.dpronbrowser zoom or a monitor swap and does not rebuild traces. So:
stroke_width * 1._resizesetsdpr = 2, traces untouched.stroke_width * 2.Result: a visible outline-width step partway through a single trace. Before
#188 the append rebuilt the trace, which renormalized every row, so this is
newly reachable.
Fix: record
g._styleDprwhen the style buffer is built, and refuse the fastpath when it no longer matches
this.dpr. The rebuild is always correct, anda dpr change is rare enough that paying for one is fine. As a side effect this
also clears the pre-existing whole-trace staleness for streaming charts.
Two latent traps (neither reachable today)
t.kind === "scatter"branch.A line has no per-point buffers today, so the path is correct — but by
coincidence, not by construction. Now any non-scatter mark carrying one is
refused.
tier_updatereallocates the x/y/base data stores viabufferDatawithoutclearing
_fcCapBytes. Unreachable becausedecimate_viewskips exactly thetraces the fast path accepts (
tier === "direct"), but if that ever drifts astale capacity means a tail
bufferSubDatapast the end of the store —silently dropped by WebGL. Zeroed at each realloc.
Smoke hardening
scripts/append_stream_smoke.pywas passing partly for the wrong reasons:fed back 20,200-row payloads. Shipping fewer rows than are resident fails
the
len >= old_lenprefix check, so all six ticks took the rebuild path —it was measuring rebuild-path coalescing, not fast-path coalescing. Now
streams six fresh monotonic payloads (21,200 → 22,200). Raised by CodeRabbit
on Streaming appends: tail-only GPU uploads, coalesced tier refines, throttled reopen re-sync #188.
DECIMATED_PXwas 2048 against a ~620 px plot. The at-home skippredicate
decimation_px >= plot.wheld with ~3x of slack, so a unit erroror a dpr-scaled comparison on either side would still have passed. Tightened
to 640, just above the real plot width, making the assertion load-bearing.
stroke_widthchannel appends once at a fixed dpr (asserting the fast pathis taken), then moves
devicePixelRatiothrough_resizeand appendsagain (asserting a rebuild).
Tidy-up
lod.LOG_FAMILY_SCALES+pins_offset_to_zero()replace the duplicated("log", "symlog")test in_payload.shipandgeometry_offset. If thosedrifted,
shipwould start pinning a sticky midpoint on an axis whose shadertransform requires the zero origin — silent, and wrong exactly where a log
scale earns its keep. Pinned by a test.
Verification
the new test.
ruff check,ruff format --check,node js/build.mjs(tsc + bundle): clean.append_stream_smoke.py: passes, with the previous run's numbers unchanged(4800 B steady tail, 0 reallocs, home 0/5, zoomed 2/6, 0 pixel mismatches).
the client rebuilt it reports
dprChangeRebuilds: 0and fails with theintended message, then passes once restored. It is a regression test, not a
test that merely agrees with the current code.
Summary by CodeRabbit
Bug Fixes
Documentation