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

fix: reduce memory leak by remove memo function #5514

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KonghaYao
Copy link

I recently used table-core to complete a virtual table. I rendered it with a dataset of 1000*1000, and found that the memory usage of table-core was unreasonably high. Since no one had fixed this issue(#5352), I attempted to debug it myself. Upon investigation, I discovered that memo was causing memory leaks in many places. This prevented JavaScript from reclaiming a massive number of rows and cells. Once I removed them, memory could be reclaimed again.

iShot_2024-04-27_10.14.43.mp4

@KevinVandy
Copy link
Member

removing memo in these places actually has a lot of performance implications here. If there is a memory problem, we should try to address that, but not at the expense of losing all of the render performance optimizations.

Also, this PR changes a lot of the code style for no reason, as far as I can tell. Why function() with this? Was that part of the memory leak fix, or just personal preference?

@KonghaYao
Copy link
Author

I'm very sorry. At first, I thought that creating a loop reference to itself during object creation would cause a memory leak. After removing the memo, I forgot to handle it. Now I have tested it and found that the reason for the memory leak was the use of memo. The new commit has removed the use of function and style.

@KonghaYao
Copy link
Author

Deleting the memo did indeed cause a high memory usage issue. I will investigate the reason for the memory leak caused by the memo.

@KonghaYao
Copy link
Author

In the example /examples/react/virtualized-columns, horizontal scrolling does not cause any memory issues, but vertical scrolling leads to memory leaks. The function getVisibleCells is called, which accesses several functions that I modified. However, the specific location and trigger of the memory leak have not yet been identified.

@KonghaYao
Copy link
Author

image
This is not a caching issue caused by memo. I deleted all references within the cell instance on the original branch and did not find any obvious memory leaks. However, when I restored cell.column, the memory leak was very severe. It seems that the strong memory leak was caused by the circular reference between cell and column.

@Stax124
Copy link

Stax124 commented Jun 24, 2024

I can verify that this bug is present. We have a table solution written in Vue and swapping pages or refreshing data in any way leads to insane memory leaks - sometimes even causes page crashes as the heap cannot take it anymore.

@attaboy
Copy link

attaboy commented Jul 26, 2024

This problem (or a similar one) seems to be affecting our product as well. We've tried reducing a specific table down to just a single column with a simple string value in each row's cell, but with 200+ rows, we run into major memory problems that are making the page unusable by our customers — on some computers, the page crashes entirely.

@KevinVandy
Copy link
Member

In the next version of TanStack Table, it's going to be easy to opt out of memoization for any of the APIs. I'm not sure how we can address it in the current version right now though.

@curvecatchdev
Copy link

We are also having the same issue, and if we garbage collect it once the table is rendered - it runs fine for a while. @KevinVandy in which version do you plan to release a flag to opt out of memoization btw? Can;t find it in the docs meaning it's not here yet, right?

@KevinVandy
Copy link
Member

By next version, I mean v9 which still has a lot of development work left

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants