Skip to content
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

[data grid] Performance degradation with v6.x and tree data #8581

Closed
2 tasks done
cipherCOM opened this issue Apr 11, 2023 · 11 comments · Fixed by #8990 or #9682
Closed
2 tasks done

[data grid] Performance degradation with v6.x and tree data #8581

cipherCOM opened this issue Apr 11, 2023 · 11 comments · Fixed by #8990 or #9682
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Tree data Related to the data grid Tree data feature performance plan: Premium Impact at least one Premium user regression A bug, but worse

Comments

@cipherCOM
Copy link

cipherCOM commented Apr 11, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/sad-night-kj9npb?file=/demo.tsx:975-1024
Necessary setup for it to occur: many rows, tree data

Steps:

  1. Just load sandbox and measure time to render

Current behavior 😯

After updating to 6.x (here 6.1.0) it takes much longer to pass first render. With 5.x the longest method was sortRowTree with e.g. 600ms execution with the given data set. With 6.x and the new row structure we see the new method rowsStateInitializer and deeper createRowTree which now costs around 10x, so with the given data set roughly 6s.

Expected behavior 🤔

Same performance as with 5.x

Context 🔦

I kept the sample as near as possible to our data without actually including the concrete data. Namely we're trying to use grouping + aggregation to display ~18200 rows of data with ~25 columns of which most are included in the aggregations so the values also show in the grouping rows. The problem is that performance degrades significantly when aggregations are activated (see #8579). So we switched over to tree data + custom aggregation directly within the data. But when trying to update to 6.x we experience a major performance degradation which hinders us from updating to the newest major version.

We're planning to buy the premium version but had problems with groupings + aggregations so we did a workaround with tree data instead.

Your environment 🌎

See codesandbox.io sample, but here also my personal env

`npx @mui/envinfo` ``` System: OS: macOS 13.2.1 Binaries: Node: 18.14.2 - ~/.asdf/installs/nodejs/18.14.2/bin/node Yarn: 1.22.19 - /opt/homebrew/bin/yarn npm: 9.5.0 - ~/.asdf/plugins/nodejs/shims/npm Browsers: Chrome: 112.0.5615.49 Edge: 112.0.1722.39 Firefox: Not Found Safari: 16.3 npmPackages: @emotion/react: 11.10.6 => 11.10.6 @emotion/styled: 11.10.6 => 11.10.6 @mui/base: 5.0.0-alpha.118 @mui/core-downloads-tracker: 5.11.9 @mui/icons-material: 5.11.9 => 5.11.9 @mui/material: 5.11.10 => 5.11.10 @mui/private-theming: 5.11.9 @mui/styled-engine: 5.11.9 @mui/system: 5.11.9 @mui/types: 7.2.3 @mui/utils: 5.11.13 @mui/x-data-grid: 6.1.0 @mui/x-data-grid-premium: 6.1.0 => 6.1.0 @mui/x-data-grid-pro: 6.1.0 @mui/x-license-pro: 6.0.4 @types/react: 18.0.20 => 18.0.20 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 typescript: 4.8.3 => 4.8.3 ```

Order ID or Support key 💳 (optional)

68355

@cipherCOM cipherCOM added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 11, 2023
@cipherCOM cipherCOM changed the title Performance degradation with v6.x and tree data [data grid] Performance degradation with v6.x and tree data Apr 11, 2023
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Apr 12, 2023
@cherniavskii
Copy link
Member

Thanks, @cipherCOM
I confirm - this is a regression.

v6: ~12s
https://codesandbox.io/s/dank-glitter-xchz7d?file=/demo.tsx

Screen.Recording.2023-04-14.at.20.16.16.mov

v5: ~3s
https://codesandbox.io/s/gifted-bartik-etu1xx?file=/demo.tsx

Screen.Recording.2023-04-14.at.20.18.09.mov

@cherniavskii cherniavskii added performance regression A bug, but worse feature: Tree data Related to the data grid Tree data feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 14, 2023
@MBilalShafi MBilalShafi self-assigned this May 1, 2023
@cipherCOM
Copy link
Author

cipherCOM commented Jun 19, 2023

Hi MUI-X Team,

thanks for the quick solution and the corresponding version. We tested it on Friday but sadly have to report, that we're still seeing performance regressions with v6.7.0. Could you please take a look into this again? I will attach call trees from the first render. Hopefully this helps to understand what we're experiencing. If anything else is missing just mention it.

v5.x
image

v6.7.0
image

`npx @mui/envinfo` ``` System: OS: macOS 13.4 Binaries: Node: 18.14.2 - ~/.asdf/installs/nodejs/18.14.2/bin/node Yarn: 1.22.19 - /opt/homebrew/bin/yarn npm: 9.5.0 - ~/.asdf/plugins/nodejs/shims/npm Browsers: Chrome: 114.0.5735.133 Edge: Not Found Firefox: Not Found Safari: 16.5 npmPackages: @emotion/react: 11.11.1 => 11.11.1 @emotion/styled: 11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.4 @mui/core-downloads-tracker: 5.13.4 @mui/icons-material: 5.11.16 => 5.11.16 @mui/material: 5.13.5 => 5.13.5 @mui/private-theming: 5.13.1 @mui/styled-engine: 5.13.2 @mui/system: 5.13.5 @mui/types: 7.2.4 @mui/utils: 5.13.1 @mui/x-data-grid: 6.7.0 => 6.7.0 @mui/x-data-grid-premium: 6.7.0 => 6.7.0 @mui/x-data-grid-pro: 6.7.0 @mui/x-date-pickers: 6.7.0 @mui/x-date-pickers-pro: 6.7.0 => 6.7.0 @mui/x-license-pro: 6.6.0 @types/react: 18.2.12 => 18.2.12 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 typescript: 4.9.5 => 4.9.5 ```

Also I got the details for our license and here is the key for reference

Order ID or Support key 💳 (optional)

68355

@cherniavskii
Copy link
Member

Hey @cipherdom
I cannot reproduce this issue using v6.7.0 in this sandbox https://codesandbox.io/s/silly-panini-62m3zz?file=/demo.tsx
Can you provide a minimal reproduction example?

@cherniavskii cherniavskii reopened this Jun 19, 2023
@cherniavskii cherniavskii added status: waiting for author Issue with insufficient information plan: Premium Impact at least one Premium user labels Jun 19, 2023
@cipherCOM
Copy link
Author

cipherCOM commented Jun 20, 2023

Finally found the cause. Our rows array first contain the leaf nodes which results in auto generated parents and then the real parents which causes the already created auto gen parents to be updated/removed.

Here is an example: https://codesandbox.io/s/infallible-hertz-7lwkzs
Source:

if (existingNodeIdWithPartialPath == null) {

Is this something you can/want to optimize on your end or is this something only the user of MUI can improve / should watch out for? Talking about the order of the given rows

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Jun 20, 2023
@cipherCOM
Copy link
Author

Could you reproduce this issue on your end and was the example of any help?

@romgrk
Copy link
Contributor

romgrk commented Jul 5, 2023

This is very likely solved by #9200 where we removed the quadratic runtime cost for grouping.

@cipherCOM Can you confirm if you're still experiencing the issue with the latest version?

@romgrk romgrk added the status: waiting for author Issue with insufficient information label Jul 5, 2023
@cipherCOM
Copy link
Author

cipherCOM commented Jul 5, 2023

The mentioned PR #9200 is already included in v6.7.0 as far as I can tell and the given sample (https://codesandbox.io/s/infallible-hertz-7lwkzs) already uses this version.

I also tried the current v6.9.1 and it still shows the same performance degradation when updating from v5.x.

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Jul 5, 2023
@romgrk
Copy link
Contributor

romgrk commented Jul 5, 2023

Quick profiling:

  • whatever we are doing to init the data, we are doing it twice (see below)
  • we are building the tree by using immutable data structures (as is customary in react). we need to use a mutable structure for that use-case to get proper performance

image

@romgrk romgrk assigned romgrk and unassigned MBilalShafi Jul 5, 2023
@till1993
Copy link

Hi @romgrk,

Do you have any updates on this issue?
Is there anything else you need? Please feel free to reach out to us if you have any further questions.

We are eagerly awaiting a fix as this performance issue is blocking our product release. We need to migrate to mui 6.x before going live.

Do you have any timeline or estimation for the progress?

Best regards
Till (part of cipherCOM's development team.)

@cherniavskii
Copy link
Member

Hey @till1993
We are looking into it, I've opened a draft PR that improves performance to match v5: #9682

@till1993
Copy link

till1993 commented Jul 14, 2023

Hey @cherniavskii,

thank you very much for the quick reponse and the PR. Looking good so far.

Best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Tree data Related to the data grid Tree data feature performance plan: Premium Impact at least one Premium user regression A bug, but worse
Projects
None yet
6 participants