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

Use ES6 Map in ReactComponentTreeHook if available #7491

Merged
merged 3 commits into from Aug 13, 2016
Merged

Use ES6 Map in ReactComponentTreeHook if available #7491

merged 3 commits into from Aug 13, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 13, 2016

This makes DEV unmounting consistently faster in Chrome, Firefox, and Safari.
It’s because purgeUnmountedComponent becomes less intensive and doesn’t use deletes.

Browsers that don’t have Map or Array.from will fall back to using objects.
We may want to try the same pattern for other global maps that we write/read often.

Chrome (before)

mount: 2678
update: 480
update: 463
update: 443
unmount: 594

mount: 2674
update: 506
update: 456
update: 453
unmount: 600

mount: 2833
update: 464
update: 445
update: 447
unmount: 648

Chrome (after)

mount: 2457
update: 470
update: 436
update: 438
unmount: 518

mount: 2542
update: 447
update: 444
update: 438
unmount: 570

mount: 2616
update: 454
update: 454
update: 448
unmount: 552

Firefox (before)

mount: 3942
update: 293
update: 276
update: 309
unmount: 714

mount: 3965
update: 291
update: 250
update: 320
unmount: 689

mount: 3953
update: 311
update: 252
update: 319
unmount: 687

Firefox (after):

mount: 3777
update: 316
update: 200
update: 172
unmount: 507

mount: 3700
update: 357
update: 189
update: 164
unmount: 490

mount: 4482
update: 282
update: 193
update: 175
unmount: 521

Safari (before)

mount: 4050
update: 337
update: 328
update: 308
unmount: 947

mount: 3864
update: 383
update: 309
update: 312
unmount: 899

mount: 3931
update: 434
update: 395
update: 334
unmount: 814

Safari (after)


mount: 4221
update: 388
update: 298
update: 393
unmount: 732

mount: 3765
update: 358
update: 300
update: 284
unmount: 723

mount: 3634
update: 355
update: 328
update: 304
unmount: 658

@gaearon gaearon added this to the 15-next milestone Aug 13, 2016
gaearon
gaearon reviewed Aug 13, 2016
View reviewed changes
src/isomorphic/hooks/ReactComponentTreeHook.js
@@ -292,12 +308,21 @@ var ReactComponentTreeHook = {
},

getRootIDs() {
return Object.keys(rootIDs);
var registeredIDs = ReactComponentTreeHook.getRegisteredIDs();
return registeredIDs.filter(ReactComponentTreeHook.isRootID);
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 a rare operation (I currently only use it in tree hook tests) so it’s fine to be slow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename it to getRootIDsSlow?

@syranide
Copy link
Contributor

syranide commented Aug 13, 2016 •
edited

If perf is important then you should probably make sure that the Map implementation is native as well, polyfills are considerably slower as you might imagine.

@aweary
Copy link
Contributor

aweary commented Aug 13, 2016

There's been a long standing TODO in ReactInstanceMap to use a Map. Assuming that TODO is still valid, If we're doing this canUseMap check is there a way we can break that out so it can be used there as well?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2016

I thought we could use it in one place, see how it goes, and gradually extract a helper if it seems worth it.

ReactInstanceMap works in a different way though: its keys are objects so it has to put a field on them.

This is another reason I didn't want to abstract prematurely. Different use cases across the codebase. Better to abstract after we actually see repeating patterns.

@sophiebits
Copy link
Collaborator

lgtm. If the logic gets any more complicated let's make our own IntMap class with the same signature as Map so all the code doesn't need to branch.

@ghost ghost added the CLA Signed label Aug 13, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2016

Addressed feedback:

  • Now detecting native Map and only using that (checked by forcing this polyfill to be used, mounting used to take 30 seconds before I added isNative check 😄 )
  • Made getRootIDs() fast again so I don’t have to admit defeat

@gaearon gaearon merged commit db452bd into facebook:master Aug 13, 2016
@gaearon gaearon deleted the use-map-in-hook branch August 13, 2016 20:43
sophiebits
sophiebits reviewed Aug 14, 2016
View reviewed changes
src/isomorphic/hooks/ReactComponentTreeHook.js
@@ -184,9 +232,14 @@ var ReactComponentTreeHook = {
// got a chance to mount, but it still gets an unmounting event during
// the error boundary cleanup.
item.isMounted = false;
if (item.parentID === 0) {
var indexInRootIDs = rootIDs.indexOf(id);
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 do something that's not O(n^2) to unmount n roots? If getRootIDs is only used in tests than the slow implementation you had would be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will do on Monday

@gaearon gaearon mentioned this pull request Aug 14, 2016
Merged
@zpao zpao modified the milestones: 15.3.1, 15-next Aug 15, 2016
zpao pushed a commit that referenced this pull request Aug 15, 2016
@gaearon @zpao
Use ES6 Map in ReactComponentTreeHook if available (#7491)
b19f202 
* Use ES6 Map in ReactComponentTreeHook if available

* Make getRootIDs fast again

* Only use native Map

(cherry picked from commit db452bd)
@sanniassin sanniassin mentioned this pull request Aug 28, 2016
Closed
@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

@sophiebits sophiebits

Labels
CLA Signed
Projects
None yet
Milestone
15.3.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gaearon @syranide @aweary @sophiebits @zpao