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

[DataGrid] Filtering performance: compile filter applier with eval #9635

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jul 10, 2023

This PR applies point 5 of #9120.

@romgrk romgrk added component: data grid This is the name of the generic UI component, not the React module! performance labels Jul 10, 2023
@mui-bot
Copy link

mui-bot commented Jul 10, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9635--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 315.3 445.7 364.7 370.24 51.36
Sort 100k rows ms 526.3 1,085.5 840.9 870.16 190.918
Select 100k rows ms 171.5 340 209.8 226.7 59.566
Deselect 100k rows ms 179.1 277.5 213.1 222.36 39.779

Generated by 🚫 dangerJS against f21730f

@@ -234,20 +236,60 @@ export const buildAggregatedFilterItemsApplier = (
return null;
}

return (row, shouldApplyFilter) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share a benchmark compared to the master branch?
Curious to learn what is the benefit in this PR on its own 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering 100,000 rows on my device:

Case Result
Without 70.2 +- 3.8
With 62.6 +- 4.7

The improvement is consistenly around 11% for a realistic number of rows, but with a small improvement as N grows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the benchmarks, seems to improve the filtering performance further. 🎉 I think we can go ahead with it and see how it works.
I couldn't see any major risks or downsides of using eval other than code being slightly less manageable (for modifying, one may have to copy paste the commented version, change it and convert to the the stringified one again)

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! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants