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] Fix memory leaks in development #8301

Merged
merged 2 commits into from Apr 5, 2023

Conversation

cherniavskii
Copy link
Member

WeakMap only accepts object as a key, this is why the signature of the instanceId must be changed to an object
@cherniavskii cherniavskii added performance component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience labels Mar 20, 2023
@mui-bot
Copy link

mui-bot commented Mar 20, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8301--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 642.7 1,217.7 647.7 820.98 220.72
Sort 100k rows ms 648.2 1,267.7 648.2 976.78 220.087
Select 100k rows ms 232.9 476.6 236.8 307.7 96.86
Deselect 100k rows ms 154.4 313.9 192.5 214.38 60.445

Generated by 🚫 dangerJS against 3a0cba1

@zroug
Copy link

zroug commented Mar 21, 2023

Great to see this so quickly!

Not a strong opinion, but it might make sense to use an opaque new Object() (without a numeric id property) as the instance id. I'm not familiar with the codebase, but if the numeric id isn't needed elsewhere, that avoids confusion about whether structural or identity equality is being used.

@cherniavskii
Copy link
Member Author

Not a strong opinion, but it might make sense to use an opaque new Object() (without a numeric id property) as the instance id.

You're right, the only reason to keep the id is for testing/debugging to make it easier to distinguish different API objects.

@cherniavskii cherniavskii marked this pull request as ready for review March 29, 2023 15:39
@cherniavskii cherniavskii merged commit f282cc3 into mui:master Apr 5, 2023
20 checks passed
@cherniavskii cherniavskii deleted the fix-memory-leaks-in-dev-mode branch April 5, 2023 17:22
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! dx Related to developers' experience performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Datagrid] Possibly Memory Leak
4 participants