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] Recomputation of all aggregations with each interaction #8579

Closed
2 tasks done
cipherCOM opened this issue Apr 11, 2023 · 3 comments · Fixed by #8671
Closed
2 tasks done

[data grid] Recomputation of all aggregations with each interaction #8579

cipherCOM opened this issue Apr 11, 2023 · 3 comments · Fixed by #8671
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Aggregation Related to the data grid Aggregation feature feature: Row grouping Related to the data grid Row grouping feature performance

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/friendly-gwen-w9ftyn?file=/demo.tsx
Necessary setup for it to occur: many rows, many aggregated columns, groupings

Steps:

  1. Open first group
  2. Open first subgroup of first group
  3. Experiencing bad performance during each interaction

Current behavior 😯

Every aggregated value for each group and its subgroups gets recalculated. Even subgroups that aren't even visible currently. See console for output

Expected behavior 🤔

Opening and closing shouldn't call aggregation functions at all

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. First I thought that it is just to much but during closer inspection it revealed that MUI-X just recalculates every aggregation when e.g. opening or closing a grouping. This is why I chose to label it as a bug, because the lost performance just stems from unnecessary calculations. This happens for both 5.x and 6.x of MUI-X.

With tree data + custom aggregations this performance impact does not occur. We just added the aggregated data directly into the rows as columns to display. We're planning to buy the premium version but had to switch over to pro and using tree data + custom aggregations due to this problem during prototype development.

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: 5.17.26 @mui/x-data-grid-premium: 5.17.26 => 5.17.26 @mui/x-data-grid-pro: 5.17.26 @mui/x-license-pro: 5.17.12 @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)

We're planing to buy the pro/premium version

@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 Recomputation of all aggregations with each interaction [data grid] Recomputation of all aggregations with each interaction 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 cherniavskii added performance feature: Row grouping Related to the data grid Row grouping feature feature: Aggregation Related to the data grid Aggregation feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 14, 2023
@cherniavskii
Copy link
Member

Thanks @cipherCOM for opening the issue!
We will investigate this.

@cherniavskii
Copy link
Member

cherniavskii commented Apr 18, 2023

The issue comes from this line:

useGridApiEventHandler(apiRef, 'rowExpansionChange', apiRef.current.unstable_applyFilters);

It triggers the filtering pipeline, which in turn triggers the aggregation pipeline:

useGridApiEventHandler(apiRef, 'filteredRowsSet', applyAggregation);

I think the only reason to apply filters on rowExpansionChange is to update state.filter.visibleRowsLookup.
At this point, I don't think visibleRowsLookup should be part of the filtering state.

visibleRowsLookup is derived from state.rows.tree and state.filter.filteredRowsLookup, so it might make sense to move state.filter.filteredRowsLookup to state.visibleRowsLookup. This would allow skipping filtering and aggregation pipelines when the row is expanded/collapsed.

@cherniavskii
Copy link
Member

This issue is noticeable in https://deploy-preview-36598--material-ui.netlify.app/x/ after the data grid demo was updated in mui/material-ui#37001

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: Aggregation Related to the data grid Aggregation feature feature: Row grouping Related to the data grid Row grouping feature performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants