Provide solution to long standing memleak #4853
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
|
Size Change: +103 B (+0.22%) Total Size: 47.6 kB
ℹ️ View Unchanged
|
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
3 times, most recently
from
062d750 to
700495a
Compare
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
2 times, most recently
from
2de3d28 to
2c711b4
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
JoviDeCroock
commented
Aug 5, 2025
|
|
||
| function removeOriginal(vnode, detachedParent, originalParent) { | ||
| if (vnode && originalParent) { | ||
| if (typeof vnode.type == 'string') { |
Member
Author
There was a problem hiding this comment.
For DOM nodes we need to revalidate the props so that we re-attach event-listeners which are deleted by unmount as we do dom._listeners = undefined
JoviDeCroock
commented
Aug 5, 2025
compat/src/suspense.js
Outdated
| suspendedVNode._component._parentDom, | ||
| suspendedVNode._component._originalParentDom | ||
| ); | ||
| if (!c._vnode._children) { |
Member
Author
There was a problem hiding this comment.
It's now possible in non hydration suspense that we've cleared _children
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
8 times, most recently
from
63b4b37 to
2f5a9b8
Compare
rschristian
approved these changes
Aug 5, 2025
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
5 times, most recently
from
c34bd59 to
2a72320
Compare
JoviDeCroock
commented
Aug 6, 2025
| } | ||
|
|
||
| vnode._component = vnode._parent = vnode._dom = UNDEFINED; | ||
| vnode._dom = vnode._component = vnode._parent = NULL; |
Member
Author
There was a problem hiding this comment.
I stopped removing the reference to children because it has perf issues in the micro benchmarks, going to verify this further
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
3 times, most recently
from
228462c to
ee2dc7d
Compare
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
6 times, most recently
from
337944a to
d17b911
Compare
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
2 times, most recently
from
dafa9e0 to
05d8984
Compare
JoviDeCroock
force-pushed
the
solve-possible-memory-leak
branch
from
05d8984 to
1aaaa3d
Compare
|
Great job Jovi, thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We can't safely remove
_listenersas that would mean we are risking suspense-hydration breaking 😅 basically during suspense we store the vnodes which means that old and new vnode will always be equal, if we want to remove_listenersas well we'll have to probably stop storing our virtual DOM tree when it's a mount and store the previous variant when it's an update. This would however mean that our mutative approach is a bad thought.EDIT: I solved the
_listenersissueResolves #3668