fix: performance improvements related to cell order#6389
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit bc0f71a
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR optimizes table-core hot paths: row cells are now cached per-row via a WeakMap to avoid reconstruction, column index lookups use a new memoized ChangesPerformance optimizations
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant row_getAllCells
participant CellsCache
participant constructCell
Caller->>row_getAllCells: getAllCells()
row_getAllCells->>CellsCache: lookup column in _cellsCache
alt cell cached
CellsCache-->>row_getAllCells: return cached Cell
else cell missing
row_getAllCells->>constructCell: constructCell(column)
constructCell-->>row_getAllCells: new Cell
row_getAllCells->>CellsCache: cache.set(column, cell)
end
row_getAllCells-->>Caller: cells array
sequenceDiagram
participant Column
participant column_getIndex
participant table_getColumnIndexes
participant Table
Column->>column_getIndex: getIndex(position)
column_getIndex->>table_getColumnIndexes: callMemoOrStaticFn(table)
table_getColumnIndexes->>Table: read column order/pinning/visibility/grouping
table_getColumnIndexes-->>column_getIndex: ColumnIndexes map
column_getIndex-->>Column: indexes[region][column.id] ?? -1
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/table-core/src/features/column-ordering/columnOrderingFeature.utils.ts (1)
40-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider building all four index maps in a single pass.
table_getColumnIndexescallstable_getPinnedVisibleLeafColumnsfour times (once per region) even though this memo is invalidated on every column-order/pinning/visibility change — exactly the hot path this PR targets (e.g. drag reordering). Per the column-pinning pipeline, the "all" list is already produced as[...left, ...center, ...right], so a single pass overtable_getPinnedVisibleLeafColumns(table)combined with each column's pinned state could populateall,left,center, andrightsimultaneously, cutting the work from 4 scans to 1.♻️ Sketch of a single-pass builder
export function table_getColumnIndexes<...>(table: Table_Internal<TFeatures, TData>): ColumnIndexes { - const buildIndexes = (columns) => { ... } - return { - all: buildIndexes(table_getPinnedVisibleLeafColumns(table)), - center: buildIndexes(table_getPinnedVisibleLeafColumns(table, 'center')), - left: buildIndexes(table_getPinnedVisibleLeafColumns(table, 'left')), - right: buildIndexes(table_getPinnedVisibleLeafColumns(table, 'right')), - } + const all = makeObjectMap<number>() + const center = makeObjectMap<number>() + const left = makeObjectMap<number>() + const right = makeObjectMap<number>() + const columns = table_getPinnedVisibleLeafColumns(table) + let leftIdx = 0, centerIdx = 0, rightIdx = 0 + for (let i = 0; i < columns.length; i++) { + const column = columns[i]! + all[column.id] = i + const pinned = column.getIsPinned() + if (pinned === 'left') left[column.id] = leftIdx++ + else if (pinned === 'right') right[column.id] = rightIdx++ + else center[column.id] = centerIdx++ + } + return { all, center, left, right } }This depends on the exact ordering contract of
table_getPinnedVisibleLeafColumns(table)without a position argument (whether it truly returns left/center/right concatenated, matching each region's own filtered order) — worth confirming before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/table-core/src/features/column-ordering/columnOrderingFeature.utils.ts` around lines 40 - 63, table_getColumnIndexes is doing four separate scans by calling table_getPinnedVisibleLeafColumns for all/center/left/right, which is wasteful on this hot path. Refactor table_getColumnIndexes to build the all, center, left, and right index maps in one pass over the unfiltered pinned visible leaf columns, using each column’s pinned region to assign into the correct maps. Keep the existing ordering contract aligned with table_getPinnedVisibleLeafColumns and preserve the current ColumnIndexes shape.packages/table-core/src/features/column-pinning/columnPinningFeature.utils.ts (1)
142-157: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
Setfor O(1) membership checks.
left.includes()/right.includes()inside the leaf-column loop is still O(leafColumns × pinCount) in the worst case (e.g. large grouped columns with many pinned entries). Since this PR's theme is eliminating linear scans (cf. the memoizedtable_getColumnIndexesused forcolumn_getIndexelsewhere in the stack), convertingleft/righttoSetbefore the loops would reduce membership checks to O(1) each.♻️ Optional optimization using Set
const { left, right } = column.table.atoms.columnPinning?.get() ?? getDefaultColumnPinningState() + const leftSet = new Set(left) + const rightSet = new Set(right) + for (let i = 0; i < leafColumns.length; i++) { - if (left.includes(leafColumns[i]!.id)) { + if (leftSet.has(leafColumns[i]!.id)) { return 'left' } } for (let i = 0; i < leafColumns.length; i++) { - if (right.includes(leafColumns[i]!.id)) { + if (rightSet.has(leafColumns[i]!.id)) { return 'right' } }For the common case of a single leaf column this is a wash, but it helps grouped columns with many leaves and/or large pin lists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/table-core/src/features/column-pinning/columnPinningFeature.utils.ts` around lines 142 - 157, The pin-detection logic in column pinning still does linear membership checks against left and right for every leaf column. Update the getPinnedPosition logic in columnPinningFeature.utils.ts to convert the pin arrays from columnPinning state into Set instances before iterating over column.getLeafColumns(), then use constant-time membership checks when determining whether a column is pinned left or right.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/table-core/src/features/column-ordering/columnOrderingFeature.utils.ts`:
- Around line 40-63: table_getColumnIndexes is doing four separate scans by
calling table_getPinnedVisibleLeafColumns for all/center/left/right, which is
wasteful on this hot path. Refactor table_getColumnIndexes to build the all,
center, left, and right index maps in one pass over the unfiltered pinned
visible leaf columns, using each column’s pinned region to assign into the
correct maps. Keep the existing ordering contract aligned with
table_getPinnedVisibleLeafColumns and preserve the current ColumnIndexes shape.
In
`@packages/table-core/src/features/column-pinning/columnPinningFeature.utils.ts`:
- Around line 142-157: The pin-detection logic in column pinning still does
linear membership checks against left and right for every leaf column. Update
the getPinnedPosition logic in columnPinningFeature.utils.ts to convert the pin
arrays from columnPinning state into Set instances before iterating over
column.getLeafColumns(), then use constant-time membership checks when
determining whether a column is pinned left or right.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 103e437d-9e26-4aaa-8e0b-fe91b6d6f470
📒 Files selected for processing (13)
packages/table-core/src/core/rows/coreRowsFeature.types.tspackages/table-core/src/core/rows/coreRowsFeature.utils.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.types.tspackages/table-core/src/features/column-ordering/columnOrderingFeature.utils.tspackages/table-core/src/features/column-pinning/columnPinningFeature.utils.tspackages/table-core/src/utils.tspackages/table-core/tests/unit/core/rows/coreRowsFeature.utils.test.tspackages/table-core/tests/unit/features/column-ordering/columnOrderingFeature.utils.test.tspackages/table-core/tests/unit/features/column-pinning/columnPinningFeature.utils.test.tsperf-done.mdperf-skipped.mdperf-todo.md
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.
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Performance Improvements
Bug Fixes