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

Clear fiber field from host instance on unmount #18690

Closed
wants to merge 1 commit into from

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Apr 21, 2020

Given we're still seeing memory leaks internally and that we already traverse each host component when we detach its fiber – let's also clear the fiber field from the host instance. I did some quick checks and didn't notice any performance runtime implications, in fact I noticed that overall memory consumed was lower from this change (testing internally)!

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 21, 2020
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f2940a8:

Sandbox Source
compassionate-elbakyan-0ju0c Configuration

@sizebot
Copy link

sizebot commented Apr 21, 2020

Warnings
⚠️ Could not find build artifacts for base commit: ff431b7

Size changes (stable)

Generated by 🚫 dangerJS against f2940a8

@sizebot
Copy link

sizebot commented Apr 21, 2020

Warnings
⚠️ Could not find build artifacts for base commit: ff431b7

Size changes (experimental)

Generated by 🚫 dangerJS against f2940a8

@gaearon
Copy link
Collaborator

gaearon commented May 4, 2020

How do you feel about only doing this for node with refs? Like Seb suggested in #16087:

What we would do here is special case DOM nodes with refs on them, and always detach their back pointer to the React Fiber, if it was ever fully mounted. We currently traverse these trees anyway when they get deleted. We want to stop doing this for most things but for nodes with a ref it seems minor to special case since they typically need to be invoked with null anyway.

This might be sufficient to fix most of the problem without doing it for every single node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants