[DataGridPremium] Support sort-dependent aggregation and improve performance #18348
Conversation
|
Deploy preview: https://deploy-preview-18348--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+10.5KB(+0.08%) - Total Gzip Change: 🔺+3.64KB(+0.09%) Show details for 100 more bundles (22 more not shown)@mui/x-data-grid-premium parsed: 🔺+3.25KB(+0.57%) gzip: 🔺+1.07KB(+0.66%) |
|
Now also exposed |
|
Enabling pivot seems to trigger 5 instances of "filteredRowsSet" (each of them retriggers aggregations). And the same for "sortedRowsSet for whatever reason @cherniavskii Adding Just press the toggle button and check console: |
|
I tried deduplicating some more expensive calls, but probably broke some tests (not sure if for functionality, but at least it will complain about One thing I still don't understand though – when stopping pivoting, columns get rehydrated 6 times: |
|
Did a bunch of optimisations in aggregation calculations. For a a large pivot table (10k rows, pivoted by date (value) and commodity in the dataset, summing "total in usd") – computation time went down from 6000ms+ to 375ms. A big culprit was nested function calls to access row values, which we can skip for pivoting + doing another full recursion over all rows/columns. isNumber custom function wrapper also added quite significant overhead for summing. Can be still polished a bit I guess. |
|
From 375ms -> 260ms in my test case now that I stoped storing undefined values. I guess it's fair to assume that undefined has no meaning in aggreations, nulls might (although even that is slightly questionable, and could be optionally toggled). And there's tons of undefined values in pivoting at least. |
|
Fixed the last issue as well with cascading |
|
Generalized the aggregation now for any depth. For traversal, we only fetch direct leaf values, otherwise merge the precomputed values on group level, instead of going through the nested tree over and over again. |
Otherwise, columns are not regenerated and groupingColDef is basically ignored
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| @@ -65,6 +69,8 @@ export const useGridPipeProcessing = (apiRef: RefObject<GridPrivateApiCommon>) = | |||
| isRunning.current = false; | |||
| }, []); | |||
|
|
|||
| const runAppliersDeferred = useRunOncePerLoop(runAppliers, false, getGroupScopeKey); | |||
There was a problem hiding this comment.
cc @romgrk I used the same approach with useRunOncePerLoop to only run appliers once after all pipe processors change. Before that, appliers were called after each pipe processor change, which in some cases hurts performance.
I modified useRunOncePerLoop to support scopes, so that a single hook can be used to schedule multiple appliers independently.
Any objections?
This change naturally inflicted a lot of updates to tests (mostly await mictotasks() at the end of the test to avoid React act warnings).
I think it's fine, and I'll mention this in the changelog and in the docs.
There was a problem hiding this comment.
Is there a case in which this could introduce flickering or other unwanted behaviour – e.g. some changes get flushed to DOM based on updated state, but pipe processing takes place after that (e.g. state changes -> DOM updates based on the change state -> columns get rehydrated only on the next render). Essentially the equivalent of setting state in useEffect vs. useLayoutEffect.
In the case of aggregation it's fine (especially with batched processing now), but broadly applying it to all pipe processors might make be a bit more questionable / tricky.
Alternative approach for this case would be to mark a group as pending for updates, and add a hook as the last entry in useDatagridPremiumComponent that clears the queue / triggers runAppliers.
Edit: could potentially explain the next comment regarding increased call count (just a guess at this point, haven't dug into it)
Edit 2: also, since formerly runAppliers was called effectively inline in the component body, any state updates from pipe processing will prevent effects from being run. But in the case of the new implementation with useRunOncePerLoop, all effects are run before the appliers, which could (in theory) negate performance gains from deduplicating calls to runAppliers. Check: https://codesandbox.io/p/sandbox/ptnhjf
There was a problem hiding this comment.
Is there a case in which this could introduce flickering or other unwanted behaviour – e.g. some changes get flushed to DOM based on updated state, but pipe processing takes place after that (e.g. state changes -> DOM updates based on the change state -> columns get rehydrated only on the next render). Essentially the equivalent of setting state in useEffect vs. useLayoutEffect.
I haven't noticed anything like that.
Alternative approach for this case would be to mark a group as pending for updates, and add a hook as the last entry in useDatagridPremiumComponent that clears the queue / triggers runAppliers.
That's a good idea, I'll give it a try 👍🏻
But in the case of the new implementation with useRunOncePerLoop, all effects are run before the appliers, which could (in theory) negate performance gains from deduplicating calls to runAppliers. Check: codesandbox.io/p/sandbox/ptnhjf
Hmm, interesting. What would be a potential scenario where performance gains will be negated? In the demo you shared, useRunOncePerLoop results in less rerenders and less effect calls:
| useRunOncePerLoop | inline |
|---|---|
| // + 2x when sortedRowsSet is fired | ||
| const expectedCallCount = reactMajor >= 19 ? 4 : 8; | ||
| const expectedCallCount = reactMajor >= 19 ? 6 : 12; |
There was a problem hiding this comment.
I'm not exactly sure why this needed to be updated...
run appliers after all pipe processors have updated, instead of after each processor updated
cf6d47b to
2003195
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Thanks for all the heavy lifting @cherniavskii for making this PR happen – and sorry for all the rework for tests this brought 🙏❤️ |
|
@lauri865 Thank you for making it possible, we appreciate it! |
Fixes #18342 (the first part of it)
Adds a new
applySorting?: booleanproperty toGridAggregationFunctionThe only thing that probably needs additional polish is this:
These can in some instances (on mount) be triggered sequentially if I remember correctly (correct me if I'm wrong), so might be some room for optimisation.
Changes:
pivotModeChangecontrol state, becausepivotModeis not strictly controlled, and in an uncontrolled mode, it delays updates by 1 renderapplyAggregation(currently it was called many times in a row)applySortingin aggregation functionsfieldtogetCellValuein aggregation functions to make custom logic possible, especially in the context of pivoting where fields are automatically generatedsetRowswhileisLoadinginternal state is still true. Common pattern would be to set rows withinuseLayoutEffectwhenisLoadingis changing (to avoid flickering), and that can't be caught by the current logic since an upperleveluseLayoutEffectwill always run before the update of the internal state. Deferred initializingnonPivotDataRefto the first row update (works with all ofsetRows,updateRowsandprops.rows) instead of listening to the isLoading change of internal state.Performance updates; before:
before-agg.mp4
After:
smooth-operator.mp4