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.

Sign up for GitHub

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

Jump to bottom

Fix unmounting performance regression in __DEV__ #7463

Merged
merged 3 commits into from
Aug 10, 2016
Merged

Fix unmounting performance regression in __DEV__ #7463

merged 3 commits into from
Aug 10, 2016
+108 −72

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 10, 2016 •
edited
Loading

This is the same issue as was previously fixed by #7232, but in __DEV__.
Test plan:

  1. Grab https://github.com/gaearon/js-framework-benchmark/tree/master/react-v15.3.0
  2. npm run build-dev
  3. Mount 10K nodes, then “clear”

Before this change: ~6 seconds.
After this change: ~2 seconds.

This seems only relevant for Chrome.

@gaearon gaearon changed the title Fix unmounting performance regression in __DEV__ (WIP) Fix unmounting performance regression in __DEV__ Aug 10, 2016
@ghost ghost added the CLA Signed label Aug 10, 2016
@gaearon gaearon changed the title (WIP) Fix unmounting performance regression in __DEV__ Fix unmounting performance regression in __DEV__ Aug 10, 2016
@ghost ghost added the CLA Signed label Aug 10, 2016
@vjeux
Copy link
Contributor

vjeux commented Aug 10, 2016

Quickly skimmed over it, nothing looks crazy. Stamping to unblock

@gaearon gaearon merged commit afa27bc into facebook:master Aug 10, 2016
@gaearon gaearon deleted the perf-regressions branch August 10, 2016 19:15
@gaearon gaearon added this to the 15-next milestone Aug 10, 2016
@sophiebits
Copy link
Collaborator

Was the try/catch difference noticeable?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 11, 2016

Not very much but it got emitEvent down from 18% to 14% in my testing.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 11, 2016

I'm curious if just having a single handler in hooks and passing the name as an argument could be faster to avoid dynamic checks and lookups.

@sophiebits
Copy link
Collaborator

That's actually a very big difference! How much do those numbers vary from run to run?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 11, 2016

To be honest I don’t remember about this particular commit (I’ve been testing many different ways, and some were more flaky than others). I can re-test it again in isolation later if you’d like.

@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
@gaearon @zpao
Fix unmounting performance regression in __DEV__ (#7463)
5dce7c2 
* Comment previous occurrences of this issue

* Fix DEV performance regression in V8

* Extract try/catch into a separate function when calling hooks

(cherry picked from commit afa27bc)
@gaearon gaearon mentioned this pull request Aug 13, 2016
Merged
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2016

@zpao This needs an additional fix in #7490

gaearon added a commit that referenced this pull request Aug 13, 2016
@gaearon
Fix ReactComponentTreeHook.getRegisteredIDs() to work with inlined co…
a738864 
…ntent (#7490)

I broke this in #7463: parseInt() cuts off #text at the end.
Changing to just use negative numbers instead.
zpao pushed a commit that referenced this pull request Aug 15, 2016
@gaearon @zpao
Fix ReactComponentTreeHook.getRegisteredIDs() to work with inlined co…
ce24114 
…ntent (#7490)

I broke this in #7463: parseInt() cuts off #text at the end.
Changing to just use negative numbers instead.
(cherry picked from commit a738864)
@renovate renovate bot mentioned this pull request Feb 2, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
15.3.1
Development

Successfully merging this pull request may close these issues.
4 participants
@gaearon @vjeux @sophiebits @zpao