[DataGrid] Fix row id bug #10051
[DataGrid] Fix row id bug #10051
Conversation
romgrk
added
the
component: data grid
Netlify deploy previewNetlify deploy preview: https://deploy-preview-10051--material-ui-x.netlify.app/ Updated pagesNo updates. These are the results for the performance tests:
|
| @@ -109,6 +109,19 @@ export const useGridRows = ( | |||
| [apiRef], | |||
| ); | |||
|
|
|||
| const getRowId = React.useCallback<GridRowApi['getRowId']>( | |||
There was a problem hiding this comment.
I was wondering about the performance of this.
And if like AG Grid, it can make sense to have an intermediary data structure for caching https://www.ag-grid.com/javascript-data-grid/client-side-row-stages/#state-2-all-rows
There was a problem hiding this comment.
Which kind of performance hit are you worried about?
And I'm not sure how that structure applies here.
There was a problem hiding this comment.
The apiRef.current indirection might prevent optimization, but I think if we destructure the API method into a const before usage like const getRowId = apiRef.current.getRowId it becomes very very easy for the JITs to optimize it away with inlining, and the if (getRowIdProp) branch is also optimized away in most cases due to getRowIdProp being a const undefined.
But we can keep that for cases where it matters. In the case for this PR it doesn't matter enough.
There was a problem hiding this comment.
Oh actually it's for filtering, so it might matter. I'll benchmark at some point and decide from there.
There was a problem hiding this comment.
Which kind of performance hit are you worried about?
I was wondering of the time it take to access row.id vs. calling getRowId(row) when we do it a lot of time, e.g. sorting 100,000 rows client side, but also filtering, grouping, pivoting
There was a problem hiding this comment.
It would be nice to avoid, but let's start with correctness and we can benchmark performance again soon, I'm completing one more perf change and I'll be done with the work from #9120. Anyway I think we've gained enough performance lately to afford one more fcall in there :)
Fixes #9915 (comment)
Add a
api.getRowIdmethod and use it in the aggregation code. We should eventually migrate all ourrow.idaccesses to that function, it ensures we use thegetRowIdprop and theGRID_ID_AUTOGENERATEDif applicable.