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 GC memory leak #4510

Merged
merged 3 commits into from May 17, 2023
Merged

Fix GC memory leak #4510

merged 3 commits into from May 17, 2023

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented May 17, 2023

Turns out we introduced a memory leak on the GC module when we launched doubly linked lists.

The root cause is that now order is important

  1. We have to start with ElementNodes first, if a child is removed from the Map there's no way to traverse (next) children later (unless we keep a separate copy).
  2. The attached assumption on children node in $garbageCollectDetachedDeepChildNodes was not correct. It will never be true because the parent is not attached in the first place.

Fixes #4509

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2023
@vercel
Copy link

vercel bot commented May 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2023 5:29pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2023 5:29pm

@github-actions
Copy link

github-actions bot commented May 17, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 27.21 KB (+0.03% 🔺) 545 ms (+0.03% 🔺) 254 ms (+34.91% 🔺) 798 ms
packages/lexical-rich-text/dist/LexicalRichText.js 38.16 KB (+0.01% 🔺) 764 ms (+0.01% 🔺) 417 ms (+31.02% 🔺) 1.2 s
packages/lexical-plain-text/dist/LexicalPlainText.js 38.13 KB (+0.01% 🔺) 763 ms (+0.01% 🔺) 424 ms (-16.06% 🔽) 1.2 s

@@ -47,6 +47,7 @@ function $garbageCollectDetachedDeepChildNodes(
let child = node.getFirstChild();

while (child !== null) {
const nextChild = child.getNextSibling();
const childKey = child.__key;
if (child !== undefined && child.__parent === parentKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to PR changes, but wondering why would we have undefined check here, it would throw one line above when accessing __key, and we have null check in while loop

@zurfyx zurfyx merged commit 7ed7e89 into main May 17, 2023
42 of 45 checks passed
@zurfyx zurfyx deleted the memory-gc branch May 17, 2023 17:57
@zurfyx zurfyx mentioned this pull request May 17, 2023
vuamitom pushed a commit to bitbriks/lexical that referenced this pull request May 19, 2023
This was referenced May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does $nodesOfType access a stale state?
3 participants