-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Performance: small optimizations #12346
Conversation
Deploy preview: https://deploy-preview-12346--material-ui-x.netlify.app/ |
{...slotProps?.cell} | ||
pinnedOffset={pinnedOffset} | ||
pinnedPosition={pinnedPosition} | ||
sectionIndex={indexInSection} | ||
sectionLength={sectionLength} | ||
gridHasScrollX={dimensions.hasScrollX} | ||
dimensions={dimensions} | ||
{...slotProps?.cell} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructuring in the middle creates 3 objects (_extends({ a: 1}, slotProps.cell, { b: 2 })
).
|
||
const cellStyle: React.CSSProperties = | ||
height !== dimensions.rowHeight | ||
? { | ||
'--width': `${width}px`, | ||
'--height': typeof height === 'number' ? `${height}px` : height, | ||
...styleProp, | ||
} | ||
: { | ||
'--width': `${width}px`, | ||
...styleProp, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid setting --height
unless required. .setAttribute
is one of our highest costs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need --height
here? It seems to be always set to rowHeight
:
height={rowHeight} |
interface GridPipeGroupCache { | ||
processors: Map<string, GridPipeProcessor<any> | null>; | ||
type Cache = { | ||
[G in GridPipeProcessorGroup]?: GroupCache; | ||
}; | ||
|
||
type GroupCache = { | ||
processors: Map<string, GridPipeProcessor<any>>; | ||
processorsAsArray: GridPipeProcessor<any>[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor along with the optimization, the signal-to-noise ratio is high with the long names & prefixes everywhere. This is internal, no need to add the grid prefix.
const preProcessors = Array.from(processorsCache.current[group]!.processors.values()); | ||
return preProcessors.reduce((acc, preProcessor) => { | ||
if (!preProcessor) { | ||
return acc; | ||
} | ||
|
||
return preProcessor(acc, context); | ||
}, value); | ||
const processors = cache.current[group]!.processorsAsArray; | ||
let result = value; | ||
for (let i = 0; i < processors.length; i += 1) { | ||
result = processors[i](result, context); | ||
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is expensive. Map -> Iterator object -> Array.from() -> array.reduce
is costly when this is run 2x for each cell.
const value = apiRef.current.getCellValue(id, field); | ||
const row = apiRef.current.getRow(id); | ||
const rowNode = apiRef.current.getRowNode(id); | ||
|
||
if (!row || !rowNode) { | ||
throw new MissingRowIdError(`No row with id #${id} found`); | ||
} | ||
|
||
const rawValue = row[colDef.field]; | ||
const value = colDef.valueGetter | ||
? colDef.valueGetter(rawValue as never, row, colDef, apiRef) | ||
: rawValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.getCellValue
fetches row
and rowNode
again but we already have them here, and the pattern rowsById[rowId]
is expensive when there is a large number of entries.
const { cache } = cacheContainer; | ||
const cacheArgsInit = cache.get(cacheKey); | ||
const cacheArgs = cacheArgsInit ?? new Map(); | ||
const cacheFn = cacheArgs?.get(args); | ||
|
||
if (cache.get(cacheKey) && cache.get(cacheKey)?.get(args)) { | ||
if (cacheArgs && cacheFn) { | ||
// We pass the cache key because the called selector might have as | ||
// dependency another selector created with this `createSelector`. | ||
return cache.get(cacheKey)?.get(args)(state, cacheKey); | ||
return cacheFn(state, cacheKey); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing map.get
4 times but there are only 2 levels of depth. This is called very often.
|
||
const cellStyle: React.CSSProperties = | ||
height !== dimensions.rowHeight | ||
? { | ||
'--width': `${width}px`, | ||
'--height': typeof height === 'number' ? `${height}px` : height, | ||
...styleProp, | ||
} | ||
: { | ||
'--width': `${width}px`, | ||
...styleProp, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need --height
here? It seems to be always set to rowHeight
:
height={rowHeight} |
I've addressed the comments. The failing test is a flaky datepicker test. Approved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Various small optimizations.
Result: small 2-3% improvement
Benchmark: scroll the grid by a constant offset, record the total CPU time