[Datagrid] Possibly Memory Leak

[alexmarmon]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

When navigating to and from a page that mounts Datagrid with a large dataset, the total JS heap size grows in size continuously.

Expected behavior 🤔

Spikes of the JS heap size are expected during render/mount, but once the Datagrid is unmounted, allocated memory should be garbage collected.

Steps to reproduce 🕹

Link to live example: https://stackblitz.com/edit/datagrid-pro-memory-issue

Steps:

1. In Stackblitz, click "Open in New Tab" top right.
2. Open Chrome DevTools for the demo and navigate to the Memory tab.
3. In the demo itself, click the "Demo" link top left.
4. Observe a spike in JS heap size during render, then it should normalize within 5-10s.
5. Click the "Home" link top left, then click back into "Demo" again.
6. Observe another spike in JS heap size, then notice the normalized value is higher than in step 4.
7. Repeat steps 3 - 6.

Example screenshots:
Load #1 - https://drive.google.com/file/d/1Wa-GEeVBZz-aYYakCfC6oQ6oWbosKwkX/view?usp=sharing
Load #2 - https://drive.google.com/file/d/1BFfDUGHt2scTmVHnqaQ_e-SeXq0JL4v9/view?usp=sharing
Load #3 - https://drive.google.com/file/d/17yu7vklO_-klD0So5Wm2OHViP5idXQ9r/view?usp=sharing
Load #4 - https://drive.google.com/file/d/1DusjWBpLSniFhCVDlQxkw_l6xfgVWTeL/view?usp=sharing

Comparison Steps:

1. In the demo you'll notice another commented out data table.
2. Uncomment that able and comment out DataGrid
3. Repeat steps 1-7 above.
4. Notice the JS heap size does not increase

Example Alt screenshots:
Load #1 - https://drive.google.com/file/d/1Y7E_O46EIzyJhS3mn16yYcwNb6dNhADO/view?usp=sharing
Load #2 - https://drive.google.com/file/d/1dqMSTig0q1iFoR4JWI2FM_LreaBI35xn/view?usp=sharing
Load #3 - https://drive.google.com/file/d/1X4hsI72htZFWmLM0nuLEhpMK9bHhOpwY/view?usp=sharing
Load #4 - https://drive.google.com/file/d/12iX3RTzqd7eEyZASqqIRztgEfTK52oyq/view?usp=sharing

Context 🔦

We have an unfortunately large dataset being loaded into the browser for our larger customers. Refactoring the entire stack to not expect all data to be available at all times is a long term goal but fixing this leak will be a massive win in the short term. In some cases we've observed our customer's heap size in excess of 1gb.

Your environment 🌎

Chrome
Please see Stackblitz for versions being used, should be latest in all cases.

Order ID 💳 (optional)

37236

[cherniavskii]

Hey @alexmarmon
Thanks for reporting this!

I can confirm there's a memory leak.
I've opened #6579 with a fix 🚀

[alexmarmon]

@cherniavskii Thank you for taking a look, really appreciate you jumping in so quickly! 🙏 How long does it typically take for a fix to be merged and deployed?

[mindbergh]

@alexmarmon what would be the workaround before the fix is released? Is there a stable version you can recommend using before the fix is released?

[cherniavskii]

@alexmarmon the fix should be included in this week's release on Thursday

[alexmarmon]

@cherniavskii Amazing. thank you!

[cherniavskii]

@alexmarmon Thanks for reporting the issue!

[zroug]

I have tested version 6.0.2 and I believe there is still a bug with the recent bugfix.

The problem is that unstable_resetCreateSelectorCache is not called for every instanceId.

This may be related to React Strict Mode.

From the React documentation:

Strict Mode enables the following development-only behaviors:

• Your components will re-render an extra time to find bugs caused by impure rendering.
• Your components will re-run Effects an extra time to find bugs caused by missing Effect cleanup.
• Your components will be checked for usage of deprecated APIs.

It's possible that React is reusing the Ref object, and by the time the cleanup hook gets called, apiRef.current has already changed.

[m4theushw]

@zroug unstable_resetCreateSelectorCache is called inside an effect. If this effect never runs—which may happen with Strict Mode—the cache is not cleaned. We relay on effects because it's a way to know when a component is unmounted.

[zroug]

Sure, but when the cache is not cleaned for all cache keys, that means the memory leak still exists.

[cherniavskii]

@zroug
Indeed, there is a memory leak in development, here's a pure React demo that demonstrates what is happening: https://codesandbox.io/s/priceless-dream-3riivj?file=/src/App.js
You can see that the ref with id 0 is discarded - only the ref with id 1 is visible during mount/unmount.

This statement explains it well:

In Strict Mode, React will call your component function twice in order to help you find accidental impurities. This is development-only behavior and does not affect production. Each ref object will be created twice, but one of the versions will be discarded. If your component function is pure (as it should be), this should not affect the behavior.
Source: https://react.dev/reference/react/useRef#useref

Here's a production deployment of the demo I've linked above: https://csb-3riivj-hpx7jnjbt-cherniavskii.vercel.app/
In the logs, you can see id 0 printed during initialization and mount, so there won't be any memory leaks in production.

Do you experience any specific issues with this approach in development?

[zroug]

Thank you for providing the example, it does show the problem very well. While it may not be a significant concern if this happens only during development, it could be beneficial to document this behavior. This is because memory leaks like this (especially if they happen in foreign code) can be challenging to debug and may arise during testing in development mode.

Alternatively, addressing the issue permanently could prevent confusion and would avoid this improper usage of React APIs. Currently, Record<CacheKey, _> is utilized as a cache. However, WeakMap<CacheKey, _> could serve as an almost drop-in replacement that automatically handles cleanup. The only caveat is that CacheKey must be a type that can be garbage collected. This can be resolved by using apiRef.current instead of apiRef.current.instanceId as the cache key. I guess that the equality of the identity of apiRef.current is equavilent to the equality of apiRef.current.instanceId. Explicit cleanup of the cache can then be completely removed.

[cherniavskii]

memory leaks like this (especially if they happen in foreign code) can be challenging to debug and may arise during testing in development mode.

Thanks for the context, I find this very convincing!

I looked into it and I think we can indeed leverage WeakMap.