DataGrid - Performance is slow if a large number of summaries is used (T1316562)#32921
DataGrid - Performance is slow if a large number of summaries is used (T1316562)#32921dmirgaev wants to merge 5 commits intoDevExpress:26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
Improves DataGrid group summary performance by avoiding repeated columnsController.columnOption(...) lookups when many summary items are configured (T1316562), replacing them with a precomputed column lookup Map.
Changes:
- Added a cached column lookup map built once per group-items processing cycle and reused during summary cell calculation.
- Introduced internal summary types (
SummaryItem,ColumnMap) and a small map lookup helper. - Added Jest unit + integration coverage for the new utilities and the performance optimization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/data_grid/summary/utils.ts | Adds getColumnFromMap helper for map-based column lookup. |
| packages/devextreme/js/__internal/grids/data_grid/summary/types.ts | Adds internal SummaryItem and ColumnMap typings for the optimization. |
| packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts | Builds and reuses a column lookup map to reduce repeated columnOption calls during group summary rendering. |
| packages/devextreme/js/__internal/grids/data_grid/summary/tests/utils.test.ts | Adds unit tests for getSummaryCellIndex and getColumnFromMap. |
| packages/devextreme/js/__internal/grids/data_grid/summary/tests/m_summary.integration.test.ts | Adds an integration test asserting fewer columnOption calls on refresh with many group summaries. |
...ges/devextreme/js/__internal/grids/data_grid/summary/__tests__/m_summary.integration.test.ts
Show resolved
Hide resolved
| const columnMap: ColumnMap = new Map(); | ||
| const allColumns = [ | ||
| ...this._columnsController.getColumns(), | ||
| ...(this._columnsController._commandColumns ?? []), |
There was a problem hiding this comment.
Even though the _commandColumns property has a public modifier, it should actually be private. Perhaps we should implement a public getCommandColumns method at the columnsController level? We could also add a public getAllColumns method to the columnController
| } | ||
|
|
||
| private _calculateSummaryCells( | ||
| summaryItems: SummaryItem[], |
There was a problem hiding this comment.
Minor comment: There are too many arguments. Can we group some of them into an object?
| if (!options.summaryGroupItems) { | ||
| options.summaryGroupItems = that.option('summary.groupItems') || []; | ||
| } | ||
| options.summaryGroupItems ??= that.option('summary.groupItems') || []; |
There was a problem hiding this comment.
looks like no reason to use alias for this here, same in line 524
tip: if use const { summary: { groupItems } } = this.option() it will not lose types when options typed, while usage with argument loses
No description provided.