perf(datagrid): mount only the columns near the viewport on a wide result - #2254
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
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)
NSTableViewvirtualises 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.
ColumnWindowResolverowns 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.isHiddencould 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+Fwould 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:
tableView.tableColumnsin the app already resolves a data index throughdataColumnIndex(from:)or casts toSortableHeaderCellfirst. The spacers own identifiers outsidedataColumnPrefix, socaptureColumnLayout, the value-filter indicators, column selection and the sort indicators all skip them with no change at those call sites.Also fixed here:
detachFromTableViewwasO(columns^2), 250,000 identity comparisons at 500 columns.A hidden column lost its place in the column order
captureColumnLayoutbuilds 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.applyGeometryin memory andFileColumnLayoutPersister.saveon 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.mergedColumnOrdersplices 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 whatFileColumnLayoutPersisterTests.saveOverwritesExistingEntryalready pinned.applyGeometrydeliberately preserveshiddenColumnsfor 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;JsonRowConverterhad no idea about it and printed the token as a string value. It renders asnullnow, and the sentinel has one named home shared with the grid's reader instead of a thirteenth string literal. Deliberately app-side rather than onPluginCellValuein PluginKit, so there is no ABI surface and no plugin re-release.worktree.sh
A fresh worktree could not build
AllPlugins:Native/DamengBridge/libis gitignored and was not among the paths the script links, soDamengDriverfailed on a missinglibdameng_bridge.athat reads like a broken change.Verification
Run through
verify.shin an isolated worktree rebased ondfbdca780.generatePASS,buildPASStestPASS, 148 of 148: ColumnWindowResolverTests, DataGridColumnPoolTests, ColumnOrderMergeTests, JsonRowConverterTests, ColumnLayoutStateTests, FileColumnLayoutPersisterTests, ColumnLayoutSyncTests, ResultsJsonViewTests, DataGridPerformanceTests, DataGridUpdateSnapshotTestsswiftlint --strictclean over every touched fileNew 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
TableProUITestscase, 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.