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

Fixes memory leak in the React editor #113

Closed
wants to merge 1 commit into from
Closed

Conversation

ncordon
Copy link
Collaborator

@ncordon ncordon commented Oct 13, 2023

This was reproducible when making the web repl grow gradually. For example by copy pasting the same example query back to back:

MATCH (n:Person)
WHERE n.name = "Steve" 
RETURN n 
LIMIT 12;

The memory consumption in Firefox jumped from 25 MiB to 400 MiB, I have even seen GiB if you keep on writing queries.

After the change:

Why

I don't have a clear idea yet why this was happening, but I suspect we are holding the old string in the onChange, so we are just preventing the GC from cleaning old values of the editor?

@ncordon ncordon requested a review from OskarDamkjaer October 13, 2023 17:53
@ncordon ncordon assigned ncordon and OskarDamkjaer and unassigned ncordon Oct 13, 2023
@OskarDamkjaer
Copy link
Collaborator

If you only remove the ...props does that fix in by itself? or are both changes needed

@ncordon
Copy link
Collaborator Author

ncordon commented Oct 17, 2023

After discussing we think we'll have to solve this in a different way. Removing the setState from the onChange appears to prevent the tree visualization, as we don't update the variable that holds the new query when we modify the editor text.

@OskarDamkjaer has also proved you can have memory leaks by just pressing and unpressing the dark mode button several times. The leaks are likely coming from the react-codemirror library, so we'll try replacing that part

@ncordon ncordon closed this Oct 17, 2023
@ncordon ncordon deleted the fixes-memory-leak branch October 25, 2023 14:15
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.

2 participants