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 severe perf problems in component tree devtool #6770

Merged
merged 1 commit into from
May 14, 2016

Conversation

sophiebits
Copy link
Collaborator

One of the ReactMultiChildText tests renders 2145 roots (and even more components) and unmounts none of them. Now we don't loop through them all a bunch of times so the test takes 20 seconds instead of 60.

We should clean up instantiateReactComponent somehow so that the onSetDisplayName call isn't produced for the TopLevelWrapper, which should allow us to just store an array of unmountedIDs instead of a hash map so we at least don't have double maps. This change mirrors the old logic though.

Reviewers: @gaearon, @sebmarkbage

One of the ReactMultiChildText tests renders 2145 roots (and even more components) and unmounts none of them. Now we don't loop through them all a bunch of times so the test takes 20 seconds instead of 60.

We should clean up instantiateReactComponent somehow so that the onSetDisplayName call isn't produced for the TopLevelWrapper, which should allow us to just store an array of unmountedIDs instead of a hash map so we at least don't have double maps. This change mirrors the old logic though.

Reviewers: @gaearon, @sebmarkbage
@gaearon
Copy link
Collaborator

gaearon commented May 14, 2016

Lgtm. I will be more careful with perf implications of my code on the existing tests in the future!
Ideally we should just set some secret field on both TopLevelWrappers we have, but let’s do this later.

@sophiebits
Copy link
Collaborator Author

Might not be slow just for tests but maybe all dev code! I did a profile and some of the devtools stuff showed up so it might be good to dig in and see what we can optimize. One idea I have is to make each devtool have a consistent shape. That is, in addDevtool you could do

devtools.push({
  onA: devtools.onA,
  onB: devtools.onB,
  onC: devtools.onC,
})

for every possible event. I think JS VMs will generally be happier about that.

@sophiebits sophiebits merged commit 3cc733a into facebook:master May 14, 2016
@gaearon
Copy link
Collaborator

gaearon commented May 14, 2016

Yeah. It’s a bit hard with ad-hoc profiling and no agreed way of tracking perf regressions.

@zpao zpao modified the milestones: 15.1.0, 15.y.0 May 20, 2016
@zpao zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
One of the ReactMultiChildText tests renders 2145 roots (and even more components) and unmounts none of them. Now we don't loop through them all a bunch of times so the test takes 20 seconds instead of 60.

We should clean up instantiateReactComponent somehow so that the onSetDisplayName call isn't produced for the TopLevelWrapper, which should allow us to just store an array of unmountedIDs instead of a hash map so we at least don't have double maps. This change mirrors the old logic though.

Reviewers: @gaearon, @sebmarkbage
(cherry picked from commit 3cc733a)
zpao pushed a commit that referenced this pull request Jun 14, 2016
One of the ReactMultiChildText tests renders 2145 roots (and even more components) and unmounts none of them. Now we don't loop through them all a bunch of times so the test takes 20 seconds instead of 60.

We should clean up instantiateReactComponent somehow so that the onSetDisplayName call isn't produced for the TopLevelWrapper, which should allow us to just store an array of unmountedIDs instead of a hash map so we at least don't have double maps. This change mirrors the old logic though.

Reviewers: @gaearon, @sebmarkbage
(cherry picked from commit 3cc733a)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
gaearon added a commit that referenced this pull request Jul 5, 2016
* Add failing tests for #7187 and #7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in #6770 and caused #7187 and #7190.

* Move logic for choosing whether to use debugID outside instantiate
zpao pushed a commit that referenced this pull request Jul 8, 2016
* Add failing tests for #7187 and #7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in #6770 and caused #7187 and #7190.

* Move logic for choosing whether to use debugID outside instantiate

(cherry picked from commit d2ff462)
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
* Add failing tests for facebook#7187 and facebook#7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in facebook#6770 and caused facebook#7187 and facebook#7190.

* Move logic for choosing whether to use debugID outside instantiate
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