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

Remove unnecessary indirection and events from the hooks #7464

Merged
merged 3 commits into from
Aug 10, 2016
Merged

Remove unnecessary indirection and events from the hooks #7464

merged 3 commits into from
Aug 10, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 10, 2016

Removing some additional DEV regressions.

This does two things:

  • Removes unnecessary indirection with nested function in the tree hook
  • Replaces onSetOwner, onSetDisplayName and onSetText with one event

We can do this now easily because we track elements.

This gives me 1.5x improvements in both mounting and unmounting time in DEV.
I tested with https://github.com/gaearon/js-framework-benchmark/tree/master/react-v15.3.0.

@ghost ghost added the CLA Signed label Aug 10, 2016
@gaearon gaearon added this to the 15-next milestone Aug 10, 2016
@gaearon gaearon changed the title Remove unnecessary indirection from the tree hook Remove unnecessary indirection and events from the hooks Aug 10, 2016
@@ -384,6 +384,7 @@ var NoopInternalComponent = function(element) {
this._renderedOutput = element;
this._currentElement = element;
this._debugID = nextDebugID++;
ReactInstrumentation.debugTool.onInstantiateComponent(this._debugID, element);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might also resolve some issues we saw regarding missing elements in stacks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which were those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm thinking #7240 (comment) and further reports. But not sure if that was related or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(y)

Less events is better.
onSetDisplayName, onSetOwner, and onSetText only existed because we didn't initially track elements.
@ghost ghost added the CLA Signed label Aug 10, 2016
onSetText(id, text) {
update(id, item => item.text = text);
onInstantiateComponent(id, element) {
create(id, element);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now the only place where we create entries. All others have been changed to vanilla get.

@sophiebits
Copy link
Collaborator

This is excellent. I love the create/get separation.

@ghost ghost added the CLA Signed label Aug 10, 2016
@gaearon gaearon merged commit 1f31357 into facebook:master Aug 10, 2016
@gaearon gaearon deleted the hook-optimizations branch August 11, 2016 13:56
@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 unnecessary indirection from the tree hook

* Replace onSetDisplayName, onSetOwner, onSetText with one event

Less events is better.
onSetDisplayName, onSetOwner, and onSetText only existed because we didn't initially track elements.

* Remove unused variables

(cherry picked from commit 1f31357)
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