perf(render): rewrite VirtualDomVertical windowing on a Fenwick coordinate model - #4937
Closed
lukecotter wants to merge 6 commits into
Closed
perf(render): rewrite VirtualDomVertical windowing on a Fenwick coordinate model#4937lukecotter wants to merge 6 commits into
lukecotter wants to merge 6 commits into
Conversation
Chrome's scroll anchoring adjusts scrollTop when the virtual renderer inserts rows above the viewport, double-compensating against the renderer's own paddingTop/scrollTop management and causing drift on scroll-up. Set overflow-anchor:none on .tabulator-tableholder, the standard approach for JS-managed virtual scrollers.
e2e guard that .tabulator-tableholder computes overflow-anchor:none, so the scroll-anchoring fix can't silently regress out of the built CSS.
…nate model INCOMPLETE — do not merge. Replaces the incremental padding/tracker model with: - dual Fenwick trees for O(log n) cumHeight / O(log^2 n) findRowAt - padding recomputed from the index each render (never incrementally clamped) - diff-based windowing (detach leavers / attach enterers only) - row-count overscan, batched attach phases, durable data-keyed height cache - previous implementation retained as VirtualDomVerticalLegacy behind the renderVerticalLegacy option Status: jsdom suite green (45/469); 17/21 e2e green; rendered window down to ~12 rows vs 40 (the churn target). Outstanding: variable-height scroll-up content shift (scroll-jump canaries report 328-781px) and frozen-rows layout. Measurement guard now matches the legacy one (!heightInitialized || !getHeight()) — without it rows measured while detached stayed permanently unmeasured and the coordinate space never left the estimate.
Three defects found by tracing the render path frame by frame: 1. Insertion routing. _diffRender advanced renderedRange BEFORE calling _attachRanges, but _attachRanges decides prepend-vs-append by comparing the incoming range against the range still rendered. With it already advanced the prepend test could never be true, so rows extending the window upward were appended BELOW it and the DOM fell out of index order. This was the dominant bug: scroll-up jump 604px -> 36px. 2. Measurement guard. Rows were gated on !heightInitialized alone, but a row first measured while detached caches a zero outerHeight and is still flagged initialized, so it stayed permanently unmeasured (measuredCount stuck at 18 of 2000) and the coordinate space never left the estimate. Now matches the legacy guard: !heightInitialized || !getHeight(). 3. Height fed to the index was read before setCellHeight() normalized the cell heights, so the index disagreed with the DOM. Re-read after normalization. Also merges the overflow-anchor fix, without which the browser's own scroll anchoring fought the padding writes and pinned scrollTop at the bottom. 500k bench vs stock (custom renderer target in brackets): steady uniform churn 8982 -> 5361 [1794] steady variable churn 2715 -> 1487 [466] fling uniform churn 14205 -> 6474 [0] fling uniform p95 4.10 -> 2.50 [0.10] initial render 98 -> 100 [98] Remaining: scroll-jump canaries report 36px/57px against a 20px tolerance, and frozen-rows tracks a row across a 500px scroll that no longer stays in the smaller window (test encodes the old 3-viewport buffer; rows and the frozen row verified correct after the scroll).
…fset _findRowAt binary-searched over _cumHeight — about 19 probes, each walking about 19 tree levels, twice per scroll frame. A linear combination of two Fenwick trees over the same index space is itself a valid Fenwick tree, so the combined node value can be formed per node and the tree descended directly in O(log n). Measured at 500k rows: _cumHeight calls per scroll tick 45.9 -> 12, time inside _findRowAt 0.031 -> 0.006 ms. The descent accumulates nodes in ascending index order whereas prefixSum walks descending, so the two sums diverge in the last bits of a float at or near a row boundary — measured at 2.6% of 3.69M boundary-heavy probes. _findRowAt reconciles against _cumHeight, so the answer is identical to the binary search it replaces. The new spec keeps that binary search as a reference oracle, over 6 height distributions and 17 sizes straddling powers of two.
With renderHorizontal:"virtual", a row that leaves the vertical window is detached but not deinitialized, so it keeps `initialized === true` and the cell set it held when it left. VirtualDomHorizontal's addColRight/addColLeft update row.modules.vdomHoz only for the rows visible at that moment, so a row re-entering the window after a horizontal scroll carried a stale column window: wrong columns, misaligned with the header. Row.initialize's else-branch is the resync hook — it falls through to rerenderRowCells, whose leftCol/rightCol guard rebuilds the cells only when the column window really moved. The diff path called initialize only for rows it had never initialized, so that branch was never reached. The legacy renderer called it unconditionally on its incremental scroll paths. Deliberately no second argument: VirtualDomHorizontal.rerenderRowCells reads it as `force`, and a forced rebuild of every re-attached row on every scroll tick is the cost the diff path exists to avoid. Row.create is a guarded no-op and the base rerenderRowCells is empty, so with the default horizontal renderer this costs nothing: at 500k rows steady-scroll churn stays 5361, nodes 1789, p95 0.9 ms.
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.
Draft: the numbers below are from a headless harness, and a manual real-browser pass is
still outstanding. Opening it now so the approach can be discussed before that.
What this does
Rewrites
VirtualDomVertical's windowing on a Fenwick (binary indexed) coordinate modelwith diff-based reconciliation: a scroll detaches only the rows that actually leave the
window and attaches only those that enter, instead of walking rows off one end and
rebuilding.
The previous implementation is kept verbatim as
VirtualDomVerticalLegacy.jsbehinda temporary
renderVerticalLegacyoption, so this is revertible per-table at runtime.That option and that file are meant to come out after a short beta, not to stay.
Numbers
500k rows, headless Chromium, K=5, medians.
customis a downstream fork of thisrenderer that motivated the work — shown as the target, not as something to merge.
Live-tree mutation churn falls 40% at steady scroll speed and 55% on a fling, and fling
p95 halves on both height profiles.
The steady p95 deltas of 0.1–0.2 ms are inside the harness's resolution and should
not be read as a regression. Relative standard deviation on that metric runs 9–29% at
sub-millisecond frame times. An earlier "1.8 ms steady p95 regression" from this same
harness did not reproduce under a same-session interleaved A/B and was withdrawn.
Re-render and sort:
rerenderInPlacenodesFilterrerenderInPlacenodesClearrerenderInPlacenodesReplacererenderInPlacemsReplacesortChurnnodesDesc / nodesAscsortChurnmsDesc / msAscrerenderTreenodesCollapse / nodesExpandrerenderTreemsCollapse / msExpandnodesReplace160 → 31 andnodesClear80 → 25 come from a durable height cache:rerenderRowsreuses cached row heights instead of remeasuring the window, soreplaceDatawith the same shape stops being a full rebuild. On tree toggles this isthe fastest of the three.
nodesis row attach/detach counted synchronously on.tabulator-table, so unlikeMutationObserverchurn it is comparable across renderers that build in differentplaces.
One figure that moves the wrong way
At 1px scroll steps, render batches go 24 → 48. The diff path renders on every
scroll event where the old window check renders on every other one. Total row work is
identical at 48 nodes and live churn is lower at 144 vs 240, so each render is cheaper —
the extra batches are the diff path catching a one-row change that the coarser check
misses. Stating it rather than hiding it behind the churn win.
A correctness point from the same scenario: a scroll that stays inside the overscan does
0 nodes / 0 churn / 0 batches on both master and this PR, and 2/2/2 on the downstream
fork.
Height-index lookup
_findRowAtbinary-searched over cumulative prefix sums — roughly 19 probes times 19tree levels, twice per scroll tick. It is now a single descent over the two combined
Fenwick trees, valid because a linear combination of two Fenwick trees over one index
space is itself a Fenwick tree.
Gated by 104 differential tests in
test/unit/core/rendering/VirtualDomVerticalIndex.spec.js, keeping the old binarysearch as the reference oracle over 6 height distributions and 17 sizes straddling
powers of two. The descent sums nodes in the opposite order to the prefix sum, so in the
last bits of a float it can land one row either side of a boundary — measured at 2.6% of
3.69M boundary-heavy probes.
_findRowAtreconciles against the prefix sum for thatreason; those loops are load-bearing, not defensive padding.
Open items, disclosed
A scrollbar drag scrolls on the compositor thread, so a small overscan lets the
compositor out-run main-thread rendering and briefly expose blank padding. Measured
mid-list headroom is 403px above / 1411px below on master and 211/197 here; shrinking
it further trades that blank band for churn.
tolerance. They previously read 36px and 57px against a 20px tolerance on this branch.
scroll-jumpcases computed the jump asmoved − scrolled, wherescrolledis the holder's own
scrollTopdelta. A virtual renderer revises its totalscrollHeightas it measures real row heights, andscrollTopis read from the top ofthat changing document, so it shifts by the height correction while nothing on screen
moves. In every sample the reported jump equalled the
scrollHeightchange exactly,while the tracked row moved the full requested distance on every step. The fix compares
screen movement against the requested distance and lives in fix(render): disable browser scroll anchoring on the table holder #4935, because with an
honest metric the old measurement also fails on current master, at 124px.
frozen-rowsused the last rendered row as a sanity check and required it to still bein the DOM after a 500px scroll, which asserts a buffer size rather than frozen-row
behaviour. It now checks that the rendered window moved on. After the scroll this
branch renders 20 rows and the frozen row is still pinned.
Both tests still fail on the code they were written to catch:
scroll-jumpat 280pxbefore the Tabulator vertical scroll skipping / jumping #3654 fix,
frozen-rowsat its layout assertion before the Freezing a row no longer works; #4871 fix. The20px and 5px tolerances are untouched. The suite is 22/22 e2e and 576/576 unit here.
turns a synchronous path asynchronous and so changes a behavioural contract.
overflow-anchor: noneon the table holder is load-bearing here; that fix is includedas the first two commits and also proposed on its own.
Depends on
The
overflow-anchorfix, included here as the first two commits. Merge that PR first;this one then reduces to four commits.