perf(react): grouped streaming stops paying a full-set height-index pass per commit - #529
Merged
Conversation
…cessor per commit Every grouped streaming commit re-ran resolveEffectiveColumns — the engine's snapshot.query getter mints a fresh defensive copy per read, so the memo feeding it can never hold — and the inline `value: () => ""` on the synthetic group column handed controller.setColumns a fresh accessor identity each time. The deep-equality check failed on that one field, and every applyTransaction under grouping paid the columns-reset path: a synchronous FULL-SET clearEstimates walk + balanced rebuild (a full cooperative re-ingest before #522 — ~40% of a traced S5 group-updates streaming window's second half). Diagnosed off two CDP traces and pinned end to end: 20 streaming commits, zero columns resets, zero replacements, and the streamed aggregate visibly lands. The row-select synth keeps its inline closure for now: hoisting it surfaces a pre-existing react-hooks/refs finding at the surfaceGrid useMemo that the inline closure keeps bailed out — noted at the site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
blove
enabled auto-merge (squash)
August 30, 2026 03:16
Contributor
Vercel preview readyPreview: https://pretable-e4flte5nu-cacheplane.vercel.app Updated automatically by the |
blove
added a commit
that referenced
this pull request
Aug 30, 2026
…commit (#533) * perf(react): the row-select column stops re-minting its accessor per commit The sibling of #529: every streaming commit re-runs resolveEffectiveColumns (the engine's snapshot.query getter mints a fresh defensive copy per read, so the memo feeding it can never hold), and the synthetic row-select column's inline `value: () => ""` handed controller.setColumns a fresh accessor identity each time. The field-scoped equality check failed on that one field, and every applyTransaction on a rowSelectionColumn grid paid the columns-reset path: a synchronous full-set clearEstimates walk (a full cooperative re-ingest before #522). The accessor now shares #529's hoisted SYNTHETIC_EMPTY_VALUE constant — `value` was the only per-call-minted field among the eight setColumns compares. The lint entanglement #529 deferred is resolved for real, not suppressed: hoisting the accessor un-bailed the react-hooks compiler on this path and surfaced the pre-existing react-hooks/refs finding at the surfaceGrid useMemo — Object.assign received an object literal whose closures capture editOperationTokenRef/pendingQueryRef, and the analysis must assume an opaque function may call them during render. The facade now uses prototype delegation with direct property stores, which keeps the deferred-ref-access provable; lint is clean with no suppressions. Pinned red-first by row-select-streaming-layout-cost.test.tsx: 20 streaming commits cost 20 columns resets before the fix, zero after, and the streamed value visibly lands. Mutation-proved (reverting the hoist turns the pin red). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review: mapped mutable-partial types the facade; fallback assert becomes a delta The surfaceGrid facade's intermediate is no longer Record<string, unknown>: a mapped mutable-partial of PretableSurfaceGrid makes a typo'd key or a drifted override signature fail to compile — the typo/signature guard the old Object.assign shape never had. The final cast drops its unknown hop. And the new pin's columnsResetFallbackCount assertion becomes a delta like its four siblings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
The diagnosis (this was a diagnosis-first investigation)
Two CDP traces of S5
group-updates(20k rows, 1000 upd/s) had shown the react layout layer's per-commit row-sequence re-ingest at ~40% of the streaming window's second half —#stepIngestbalanced-sequence frames and row-model group-index reads among the top frames. #522 fixed the grouping-APPLY replacement waste and deliberately left this steady-state seam alone. This branch answers what the steady-state cost actually is.Ingest proportionality answer: per grouped streaming commit, the row-layout controller was doing full-set work where zero extra work is achievable — and the trigger was columns, not rows:
CompiledQuery.get query()mints a fresh defensive copy on every read (packages/row-model/src/compiled-query.ts:1756→snapshotQuery). Deliberate and test-pinned: an exposed Date operand a consumer mutates must not poison later reads (compiled-query.test.ts"detaches Date operands…"). SoobservedQueryinusePretableModelInternalchanges identity on every controller publish.columnsmemo (packages/react/src/pretable-model.ts:835), soresolveEffectiveColumns(packages/react/src/pretable-surface.tsx) re-runs per commit — and minted the synthetic group column with a freshvalue: () => ""closure each time.controller.setColumns's deep-equality check (packages/renderer-dom/src/row-layout-controller.ts:2277) failed on[__pretable_group__].valuealone — instrumented directly: 30 streaming commits → 30 diffs, every one only that field.RowHeightIndex.clearEstimates()(packages/layout-core/src/row-height-index.ts:1625) — a synchronous full-set in-order walk + balanced rebuild, O(rows) per commit. Before perf(renderer-dom): grouping apply pays one height-index replacement, not three #522 the same trigger started a full cooperative replacement per commit — which is exactly the#stepIngestsignature the original traces caught.The journal path itself is healthy: with a bare controller (no react layer), 60 grouped streaming commits produced zero replacements and zero fallbacks — the per-commit ingest is batch-proportional. The waste was entirely this columns seam.
The fix
Hoist the synthetic group column's
valueaccessor to a module constant (SYNTHETIC_EMPTY_VALUE) so its identity is stable across renders. One field, and the deep-equality check now short-circuits every steady-state commit.Not touched, and documented at the site: the row-select synth keeps its inline closure — hoisting it un-bails the react-hooks compiler analysis and surfaces a pre-existing
react-hooks/refsfinding at thesurfaceGriduseMemo (the indexed facade carries ref-reading closures). A grid withrowSelectionColumnenabled still pays the per-commit columns reset; follow-up filed.Numbers
Pre-fix (traced, S5
group-updatestarget, streaming window 3050ms):clearEstimateswas the top react-layout frame in the window: 18.9ms self, ~4.2% of non-idle time, called once per commit viasetColumns(caller chain verified through the trace's node parents:clearEstimates ← setColumns ← usePretableModelInternal's layout effect).columnsResetPathCount +20.Post-fix (same recipe, same machine):
clearEstimatesabsent from the streaming window's top frames; window idle 85.4% → 86.8%.jsdom pin: 20 streaming commits →
columnsResetPathCount +0,replacementStartCount +0, all fallback counters 0, and the streamed "Bank" group sum visibly lands in the DOM (the cheap path did not buy its zeros by dropping commits).Bench matrix (5 repeats, S5 target, 1000 upd/s;
updatesis the like-for-like control, same machine, same evening):group-updatesupdates(control)No regression; the control's spread brackets both sides, so the p95 stays ~10.0ms (this metric sits on the frame cadence, which is why the win shows up in the trace's busy-time share rather than p95 — the fix is headroom, exactly as scoped).
Scale note:
clearEstimatesis O(rows) per commit, so pre-fix the seam grew linearly toward the frame budget with row count (100k = 5× the walk, plus the GC pressure of rebuilding a 100k-node balanced tree per commit). Post-fix the steady-state layout cost is journal-path only, i.e. batch-proportional.Tests
packages/react/src/__tests__/grouped-streaming-layout-cost.test.tsx— mutation-verified (reverting the one-line fix turns it red withcolumnsResetPathCount 20 ≠ 0).🤖 Generated with Claude Code