[DataGridPro] Improve treeData and rowGrouping performance

[MBilalShafi]

Fixes #8581

The regression was introduced due to a function createRowTree introduced in the new internal structure for v6.

Initial benchmarks:

For the same data (size: 40000), (i.e the one used in the linked codesandbox)

• buildRowTree (v5) took 176ms which is approximately 0.004ms per row
• createRowTree (v6) took 3700ms which is approximately 0.09ms per row (almost 20x slow)

Reason:
Looking deeper into the main function being used in createRowTree (i.e. insertNodeInTree) there are not many complex computations apart from some references being updated.

Doing a comparison yielded that most of the time is being consumed in destructuring and updating references of variables childrenFromPath and children, so avoiding that and keeping the same reference improved the performance.

Updated Benchmarks (to render 40000 rows with treeData):

Macbook Pro M1, Chrome browser:

v5 version: ~2s
v6 version (HEAD): ~10s
v6 version (this PR): ~1.8s

Macbook Pro M1, Firefox browser:

v5 version: ~8.5s
v6 version (HEAD): ~13s
v6 version (this PR): ~8.9s

Side note: Chromium v8 seems to handle reference-based assignments way faster than Firefox, the performance on Chrome seems improved from the v5 version.

---

Fixes #8999

On my test (MacOS, Chrome v113), the time to render the grid (for 100,000 rows) reduced from 22s to 0.8s

Before: https://codesandbox.io/s/aggregation-performance-regression-zx0rrd?file=/src/App.tsx
After: https://codesandbox.io/s/aggregation-performance-regression-fixed-x7k6gm?file=/src/App.tsx

[mui-bot]

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8990--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	628.1	999.5	730.9	794.36	133.16
Sort 100k rows	ms	619.2	1,049.8	814.3	840.42	139.366
Select 100k rows	ms	182.3	338.1	221.2	250.4	58.907
Deselect 100k rows	ms	129.4	294.6	144.9	172.48	62.346

Generated by 🚫 dangerJS against dc029a1

[flaviendelangle]

Glad that this fix solves the performance problem that well !

I am a little worried about the idea of mutating the children property (childrenFromPath less so, it's never used outside of the tree-building process).

Because now if you have a useMemo in a component that relies on rowNode.children, it won't re-run on tree updates.

My proposal was to do one spread per parent instead of one spread per children.
Not sure how to do that cleanly in the code.
We could pass a prevTree object that equals the tree before the current batch and only spread if prevTree[parentNode.id].children === tree[parentNode.id].children.

[flaviendelangle]

Could even be prevTree[parentNode.id] === tree[parentNode.id], if the parent has the same ref as on the prev tree, then it has never been touched during this batch of update and we need to recreate it to avoid mutation.
Otherwise we mutate because it's just locale mutation.

[MBilalShafi]

Thanks @flaviendelangle for the suggestions, I tried to do a comparison with the previousTree

Because now if you have a useMemo in a component that relies on rowNode.children, it won't re-run on tree updates.

If you have any example in mind (or on docs) using which I can validate that this use case works as expected now, please share. I didn't quite get that in what circumstances a user would need to memoize rowNode.children.
Thanks

[flaviendelangle]

I tried to do a comparison with the previousTree

Is it working or is it causing some issue ?

I didn't quite get that in what circumstances a user would need to memoize rowNode.children.

When providing a custom renderCell, the user gets props containing rowNode which is the object where the children key we are mutating is.

If you look at the demo TreeDataCustomGroupingColumn, imagine that we add some code like

const lastHired = React.useMemo(() => {
  return rowNode.children.reduce((acc, el) => {
    if (!acc || acc.recruitmentDate.getTime() < el.recruitmentDate.getTIme()) {
      return el
    }

    return acc
  }, null)  
}, [rowNode.children])

And then we render this lastHired
Then I think your PR would be a breaking change, since children no longer changes its reference.

I honestly don't think a lot of people are using rowNode.children
We never do it in the doc as far as I'm aware
But having a prop that's mutates feels like a bad pattern that will lead to nasty bugs imho

[MBilalShafi]

Is it working or is it causing some issue ?

Apparently it seems to be working, I will try with the example you mentioned. 👍

[cherniavskii]

Passing previousTree everywhere doesn't look great, but I don't really see a better option 🤨

[flaviendelangle]

An alternative is to create a dictionnary that contains all the updated nodes during a batch.
But same problem, we end up having an additional param all over the place.

[m4theushw]

Does this one fix #8999?

[MBilalShafi]

@flaviendelangle I confirmed, the mentioned use-case seems to work fine: https://codesandbox.io/s/keen-spence-hqg9q4?file=/demo.tsx (note the new date correctly being picked of children)

Does this one fix #8999?

It does, on my test (MacOS, Chrome v113), the time to render the grid (for 100,000 rows) reduced from 22s to 0.8s

Before: https://codesandbox.io/s/aggregation-performance-regression-zx0rrd?file=/src/App.tsx
After: https://codesandbox.io/s/aggregation-performance-regression-fixed-x7k6gm?file=/src/App.tsx

I think, after this change, MUI Grid will be the fastest to render this much aggregation rows among the mentioned competitors on #8999 🎉

[flaviendelangle]

For the title, it improved the row grouping and the tree data
It does not improve the aggregation, strictly speaking, just the aggregation is often used with the tree data / row grouping.