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 memory leak in ReactChildrenMutationWarningHook for SSR #7410

Merged
merged 2 commits into from
Aug 10, 2016

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Aug 3, 2016

Fixes #7406. Thanks @franjohn21 for providing such a detailed investigation!

Reviewers: @gaearon @jimfb

Keyan Zhang added 2 commits August 2, 2016 14:38
and get element from `ReactComponentTreeHook` instead of keeping an internal store
@keyz keyz changed the title Fix memory leak in ReactChildrenMutationWarningHook Fix memory leak in ReactChildrenMutationWarningHook for SSR Aug 3, 2016
@ghost ghost added the CLA Signed label Aug 3, 2016
@lynn
Copy link

lynn commented Aug 3, 2016

Awesome job! Will there be a patch release for this fix?

);
});

it('should warn when children are mutated', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: not very clear why there are two tests, and how they are different (right not the name only differences is starting with "should")

@gaearon
Copy link
Collaborator

gaearon commented Aug 3, 2016

This will probably go in 15.3.1.

@franjohn21 Can you build from master and verify this fixes it?

@gaearon
Copy link
Collaborator

gaearon commented Aug 3, 2016

This looks right but I'd appreciate if you could verify this indeed fixes the leak.

@franjohn21
Copy link

@keyanzhang @gaearon I can confirm I am seeing the memory leak removed with this change. Thanks for the quick turnaround time!

@ghost ghost added the CLA Signed label Aug 3, 2016
@gaearon gaearon added this to the 15-next milestone Aug 4, 2016
@ghost ghost added the CLA Signed label Aug 4, 2016
@keyz
Copy link
Contributor Author

keyz commented Aug 5, 2016

Note to self: #7424 and #7426

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2016

I’m going to get this in because it’s better than status quo and I also need it for DEV perf regression I’m tackling right now.

@gaearon gaearon merged commit 5514ea3 into facebook:master Aug 10, 2016
},
onComponentHasMounted(debugID) {
handleElement(debugID, elements[debugID]);
delete elements[debugID];
Copy link
Collaborator

@gaearon gaearon Aug 10, 2016

Choose a reason for hiding this comment

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

For future reference: delete is super slow on V8 with many items when using numeric keys. Even toString() is not enough—you have to prepend them with something like .

gaearon added a commit that referenced this pull request Aug 11, 2016
* Remove onBeforeMountComponent hook event

It is unnecessary.
We now pass the element as part of onInstantiateComponent, and it can't change before mounting.

* Remove onComponentHasMounted hook event

It is unused after #7410.

* Replace on(Begin|End)ReconcilerTimer hook events

We already have onBeforeUpdateComponent.
Let's just have on(Before?)(Mount|Update|Unmount)Component and stick with them.

This removes double event dispatches in some hot spots.

* Remove onComponentHasUpdated hook

The tests still pass so presumably it was not necessary.

* Add missing __DEV__ to TestUtils code

* Replace on(InstantiateComponent|SetParent) with onBeforeMountComponent

This lets us further consolidate hooks.
The parent ID is now passed as an argument to onBeforeMountComponent() with the element.

* Remove onMountRootComponent hook event

It is unnecessary now that we pass the parent ID to onBeforeMountComponent.

* Use parentDebugID = 0 both for roots and production

This removes some awkward branching.
@zpao zpao modified the milestones: 15.3.1, 15-next Aug 12, 2016
zpao pushed a commit that referenced this pull request Aug 12, 2016
* corrected ReactChildrenMutationWarningHook's name

* changed `onComponentHasMounted` to `onMountComponent`

and get element from `ReactComponentTreeHook` instead of keeping an internal store

(cherry picked from commit 5514ea3)
zpao pushed a commit that referenced this pull request Aug 12, 2016
* Remove onBeforeMountComponent hook event

It is unnecessary.
We now pass the element as part of onInstantiateComponent, and it can't change before mounting.

* Remove onComponentHasMounted hook event

It is unused after #7410.

* Replace on(Begin|End)ReconcilerTimer hook events

We already have onBeforeUpdateComponent.
Let's just have on(Before?)(Mount|Update|Unmount)Component and stick with them.

This removes double event dispatches in some hot spots.

* Remove onComponentHasUpdated hook

The tests still pass so presumably it was not necessary.

* Add missing __DEV__ to TestUtils code

* Replace on(InstantiateComponent|SetParent) with onBeforeMountComponent

This lets us further consolidate hooks.
The parent ID is now passed as an argument to onBeforeMountComponent() with the element.

* Remove onMountRootComponent hook event

It is unnecessary now that we pass the parent ID to onBeforeMountComponent.

* Use parentDebugID = 0 both for roots and production

This removes some awkward branching.

(cherry picked from commit 0e976e1)
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.

6 participants