Skip to content

perf(datagrid): mount only the columns near the viewport on a wide result - #2254

Merged
datlechin merged 1 commit into
mainfrom
fix/editor-state-json-mode-and-persistence
Aug 19, 2026
Merged

perf(datagrid): mount only the columns near the viewport on a wide result#2254
datlechin merged 1 commit into
mainfrom
fix/editor-state-json-mode-and-persistence

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #1219.

Three fixes in the data grid, plus a script line. Two came from investigating #2234; #1219 is the reported one.

Scrolling a wide result (#1219)

NSTableView virtualises rows but never columns. A prepared row builds one cell view for every column that is not hidden, whatever the horizontal viewport actually shows, so a 500-column result holds roughly 18,000 live cell views and lays all of them out on every scroll frame. Neither #1726 nor #1742 touched this: both are per-row costs, and the cost here is per-column.

Only the columns near the viewport are mounted now. The rest are hidden, which is measurably close to free, and a spacer column at each end carries their width so the document keeps its full extent and the horizontal scroller still spans the whole result.

ColumnWindowResolver owns the geometry and is pure, because that is the part worth testing: which columns the viewport intersects, how much overscan to keep, and when the window is allowed to slide. The window holds while the viewport stays inside its margin, then jumps by a whole overscan. Sliding per column puts the re-window tile inside the scroll frame and trades a steady cost for an intermittent stall.

isHidden could not carry this on its own. It already meant "the user hid this column", and windowing would have overloaded it to also mean "not near the viewport". Six call sites read it and would have quietly changed meaning: Copy Row, Copy with Headers and Copy as JSON would have put only the mounted columns on the clipboard and silently dropped the rest of a 500-column row; Cmd+F would have reported no matches for a value in column 300 while scrolled to column 5; Shift-Tab and Left-arrow would have landed on the edge of the window instead of the real last column; Size All Columns to Fit would have fitted the window and persisted a half-fitted layout. presentsColumn(_:) is now the predicate those sites ask, and it answers "the result shows this column" independently of what is mounted.

Three more things made this safer than it looks:

  • Every loop over tableView.tableColumns in the app already resolves a data index through dataColumnIndex(from:) or casts to SortableHeaderCell first. The spacers own identifiers outside dataColumnPrefix, so captureColumnLayout, the value-filter indicators, column selection and the sort indicators all skip them with no change at those call sites.
  • Columns the user hid are tracked separately from columns the window unmounted, so a window slide can never bring a hidden column back.
  • Only columns in the current result are windowed. The pool never shrinks, so the surplus slots from a previously wider result stay attached and hidden, and mounting one would resurrect a column that is not in this result.

Also fixed here: detachFromTableView was O(columns^2), 250,000 identity comparisons at 500 columns.

A hidden column lost its place in the column order

captureColumnLayout builds the order from the columns the query returned, and hiding a column takes it out of that query. Both sinks then wrote that partial order over the stored one: ColumnLayoutState.applyGeometry in memory and FileColumnLayoutPersister.save on disk. So hiding two columns and dragging a third erased the hidden columns' positions for good, and Show All appended them at the far right of a grid the user had arranged.

mergedColumnOrder splices a capture into the stored order, keeping names the capture never saw. It only splices when the capture is a narrowing of the same column set; a capture naming a column the stored order never had describes a different result and replaces it outright, which is what FileColumnLayoutPersisterTests.saveOverwritesExistingEntry already pinned. applyGeometry deliberately preserves hiddenColumns for the same reason, so this follows a precedent that was already there.

JSON printed an internal marker as data

A pending inserted row carries __DEFAULT__ for the columns the server fills in. The grid draws that as a placeholder; JsonRowConverter had no idea about it and printed the token as a string value. It renders as null now, and the sentinel has one named home shared with the grid's reader instead of a thirteenth string literal. Deliberately app-side rather than on PluginCellValue in PluginKit, so there is no ABI surface and no plugin re-release.

worktree.sh

A fresh worktree could not build AllPlugins: Native/DamengBridge/lib is gitignored and was not among the paths the script links, so DamengDriver failed on a missing libdameng_bridge.a that reads like a broken change.

Verification

Run through verify.sh in an isolated worktree rebased on dfbdca780.

  • generate PASS, build PASS
  • test PASS, 148 of 148: ColumnWindowResolverTests, DataGridColumnPoolTests, ColumnOrderMergeTests, JsonRowConverterTests, ColumnLayoutStateTests, FileColumnLayoutPersisterTests, ColumnLayoutSyncTests, ResultsJsonViewTests, DataGridPerformanceTests, DataGridUpdateSnapshotTests
  • swiftlint --strict clean over every touched file

New tests pin the parts that would break quietly: the document width is preserved at every scroll extreme and for 1, 50 and 500 columns; the window holds through a small scroll and moves through a large one; the slide margin stays below the overscan; a user-hidden column survives a window slide; a table with no laid-out viewport mounts everything rather than hiding it all; the spacers are removed on teardown; and a hidden column keeps its position through a capture.

On the numbers, to be precise about what is and is not measured here. The investigation measured the problem and prototyped the fix: 500 visible columns gave 7.24ms p50 and 37.8ms mean per vertical scroll frame with 18,000 live cell views, against 0.479ms p50 at 50 columns, and a windowing prototype brought that to 0.479ms p50, 1.293ms mean and 1,152 subviews. Those are the prototype's numbers, not this implementation's. I have no 500-column ClickHouse table to measure this branch against, and horizontal scrolling is not deterministic enough for a TableProUITests case, so what is verified here is the geometry and the pool behaviour, not a frame time. Worth a look on a real wide table before release.

The reporter last confirmed the lag on 0.39.1 and has not said which version they retested on, so their confirmation on a current build is still worth having either way.

@mintlify

mintlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 19, 2026, 4:21 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 752019a into main Aug 19, 2026
9 checks passed
@datlechin
datlechin deleted the fix/editor-state-json-mode-and-persistence branch August 19, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI Lag While Scrolling Large Result Sets from ClickHouse

1 participant