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

[react-core] Clear more properties in detachFiber #16807

Merged
merged 1 commit into from
Sep 17, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Sep 17, 2019

This PR aims at tackling some potentially retained memory after a fiber gets detached. To do this, we clear down more fields on the fiber to be null in detachFiber.

@gaearon
Copy link
Collaborator

gaearon commented Sep 17, 2019

I think @sebmarkbage's point in #16115 and later #16087 is that we shouldn't play whack-a-mole with trying to clear as much as we can since it has a cost, and often masks the actual underlying issues. For each field clearing which "helps" we need to find a minimal reproducing case that demonstrates a leak.

@sizebot
Copy link

sizebot commented Sep 17, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 21fa5d2

@sebmarkbage
Copy link
Collaborator

In particular, the question would be, this fiber itself has been deleted. So if it’s still retain, what is holding onto to it? However in this case we actually have the very answer to that question in the comment above in the code. It’s an architectural quirks that leaves this node alive for a bit longer so we clear it.

I believe we actually do have a way to clear the right parent now so we might be able to do that instead.

@@ -903,13 +904,14 @@ function detachFiber(current: Fiber) {
current.memoizedState = null;
current.updateQueue = null;
current.dependencies = null;
const alternate = current.alternate;
current.sibling = null;
current.alternate = null;
Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 17, 2019

Choose a reason for hiding this comment

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

I believe this shouldn’t matter to set which I guess is why the code was structured that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed but it reduces the amount of code quite a bit by re-using the function (which should be hot). I can go back to the previous way if you feel it's better.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 17, 2019

@gaearon I'd also not play whack-a-mole, but as per Sebastian's above comment, the reason we have to do this to begin with is because of the architectural quirks. If we have a better solution, then I'm all ears. :)

I believe we actually do have a way to clear the right parent now so we might be able to do that instead.

@sebmarkbage What would that be? That sounds like a better approach.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 17, 2019

When we add deleted fibers to the effect list. We know their parent’s work in progress. If we set the .return at that point then we’ll know which parent was the work in progress. Then we could do current.return.alternate.child = null here.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 17, 2019

@sebmarkbage Makes sense. Shall we close this PR in favor of that approach? We can roll with this change until we land that change as a follow up.

@sebmarkbage
Copy link
Collaborator

Yea let’s land this first in case that other approach doesn’t work.

@trueadm trueadm merged commit 2f1e8c5 into facebook:master Sep 17, 2019
@trueadm trueadm deleted the clear-sibling branch September 17, 2019 15:30
gaearon pushed a commit to gaearon/react that referenced this pull request Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants