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

Consolidate hook events #7472

Merged
merged 8 commits into from
Aug 11, 2016
Merged

Consolidate hook events #7472

merged 8 commits into from
Aug 11, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 11, 2016

Our DEV performance suffers because we have too many hook events right now.
I want to cut down on their number and be stricter in the future about adding more events.

I removed:

  • onInstantiateComponent
  • onSetParent
  • onComponentHasMounted
  • onComponentHasUpdated
  • onBeginReconcilerTimer
  • onEndReconcilerTimer

Instead, I added onBeforeUnmountComponent, and moved parentID into onBeforeMountComponent.

As a result we now have a very simple (and small) hook event footprint for reconciliation:

  • onBeforeMountComponent and onMountComponent
  • onBeforeUpdateComponent and onUpdateComponent
  • onBeforeUnmountComponent and onUnmountComponent

Basically a pair of events for every operation we support.

This will reqiure changes to RN and WTF integration.

It is unnecessary.
We now pass the element as part of onInstantiateComponent, and it can't change before mounting.
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.
@ghost ghost added the CLA Signed label Aug 11, 2016
The tests still pass so presumably it was not necessary.
@gaearon gaearon changed the title (WIP) Consolidate hook events Consolidate hook events Aug 11, 2016
@gaearon gaearon changed the title Consolidate hook events (WIP) Consolidate hook events Aug 11, 2016
@sophiebits
Copy link
Collaborator

Does this change ReactPerf output at all?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 11, 2016

No, this shouldn’t affect it.
I’ll need to make some changes to internal trace and to RN before landing this though.

@ghost ghost added the CLA Signed label Aug 11, 2016
This lets us further consolidate hooks.
The parent ID is now passed as an argument to onBeforeMountComponent() with the element.
);
return nextChildren;
}
}
nextChildren = flattenChildren(nextNestedChildrenElements);
nextChildren = flattenChildren(nextNestedChildrenElements, selfDebugID);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting that we weren’t passing it before (this code path could still run in __DEV__).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, when is this._currentElement not set? Not even sure why we have that check.

@gaearon gaearon changed the title (WIP) Consolidate hook events Consolidate hook events Aug 11, 2016
@ghost ghost added the CLA Signed label Aug 11, 2016
@@ -109,12 +109,17 @@ function mountComponentIntoNode(
console.time(markerName);
}

var parentDebugID;
if (__DEV__) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we drop these __DEV__ blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea.

It is unnecessary now that we pass the parent ID to onBeforeMountComponent.
@ghost ghost added the CLA Signed label Aug 11, 2016
This removes some awkward branching.
@gaearon gaearon added this to the 15-next milestone Aug 11, 2016
@gaearon gaearon merged commit 0e976e1 into facebook:master Aug 11, 2016
@gaearon gaearon deleted the less-hooks branch August 11, 2016 18:35
@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
* 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)
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2016

@zpao Note that this requires a Systrace change in RN I haven’t submitted yet.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2016

ghost pushed a commit to facebook/react-native that referenced this pull request Aug 18, 2016
Summary:
This brings RN up to date with facebook/react#7472 (scheduled for React 15.3.1).
If you use React 15.3.1 with RN without this patch, Systrace output will lack the reconciler events.

(Since it’s a niche feature it didn’t seem to me that full backward compat is necessary so I just removed old hooks. This won’t cause crashes—it’s just you won’t get events in Systrace if you use new React but old RN.)
Closes #9383

Differential Revision: D3738463

Pulled By: spicyj

fbshipit-source-id: 791cdbc5558666a101fa403f4e7852f700038fc9
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.

3 participants