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

Fix unmounting performance regression in V8 #7232

Merged
merged 1 commit into from
Jul 9, 2016
Merged

Fix unmounting performance regression in V8 #7232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/renderers/dom/client/eventPlugins/SimpleEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,10 @@ for (var type in topLevelEventsToDispatchConfig) {
var ON_CLICK_KEY = keyOf({onClick: null});
var onClickListeners = {};

function getDictionaryKey(inst) {
return '.' + inst._rootNodeID;
}

Copy link

@vitkarpov vitkarpov Jul 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@gaearon I have a couple of questions about this solution, could you please explain this.

  • I'm not sure about why this changes don't break the current behaviour. In the previous code the key was just inst._rootNodeId and not it's '.' + inst._rootNodeId. How this could be the same thing as there're no other changes?
  • Why does it improve performance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't break existing behavior because there is no observable difference as to what we use for keys. We just need to store some stuff on an object for each ID.

It improves perf because of some weird way V8 treats numeric keys. When all keys are numeric (like IDs are), for some reason delete becomes slower. Maybe it is similar to how sparse arrays are deoptimized. However if keys are not numeric (which we force by adding a dot before the number) it appears to create hash tables for those objects which is exactly what we want.

In general relying on something like this is very brittle. It is just VM implementation detail and can change any day. Still, if we can easily fix it without affecting performance in other engines, I think it's worth doing like in this case.

Copy link

Choose a reason for hiding this comment

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

Why not just toString?

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 still a "numeric" key as considered by V8. (I tried that and it doesn't help.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't forget, I am as puzzled that this works as you are. 😄 I don't have good explanations, I'm just saying I tried other ways and they didn't help.

Copy link

Choose a reason for hiding this comment

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

That's what you get with a dynamically typed language 😄

Copy link

Choose a reason for hiding this comment

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

Guys, seems pretty reasonable for me now. @gaearon Thanks!

Also it seems that getDictionaryKey should have a good explanation, comment above. V8's being developed so this could be irrelevant eventually.

var SimpleEventPlugin = {

eventTypes: eventTypes,
Expand Down Expand Up @@ -620,10 +624,10 @@ var SimpleEventPlugin = {
// fire. The workaround for this bug involves attaching an empty click
// listener on the target node.
if (registrationName === ON_CLICK_KEY) {
var id = inst._rootNodeID;
var key = getDictionaryKey(inst);
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
if (!onClickListeners[id]) {
onClickListeners[id] = EventListener.listen(
if (!onClickListeners[key]) {
onClickListeners[key] = EventListener.listen(
node,
'click',
emptyFunction
Expand All @@ -634,9 +638,9 @@ var SimpleEventPlugin = {

willDeleteListener: function(inst, registrationName) {
if (registrationName === ON_CLICK_KEY) {
var id = inst._rootNodeID;
onClickListeners[id].remove();
delete onClickListeners[id];
var key = getDictionaryKey(inst);
onClickListeners[key].remove();
delete onClickListeners[key];
}
},

Expand Down
22 changes: 15 additions & 7 deletions src/renderers/shared/stack/event/EventPluginHub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ var executeDispatchesAndReleaseTopLevel = function(e) {
return executeDispatchesAndRelease(e, false);
};

var getDictionaryKey = function(inst) {
return '.' + inst._rootNodeID;
};

/**
* This is a unified interface for event plugins to be installed and configured.
*
Expand Down Expand Up @@ -96,7 +100,7 @@ var EventPluginHub = {
},

/**
* Stores `listener` at `listenerBank[registrationName][id]`. Is idempotent.
* Stores `listener` at `listenerBank[registrationName][key]`. Is idempotent.
*
* @param {object} inst The instance, which is the source of events.
* @param {string} registrationName Name of listener (e.g. `onClick`).
Expand All @@ -109,9 +113,10 @@ var EventPluginHub = {
registrationName, typeof listener
);

var key = getDictionaryKey(inst);
var bankForRegistrationName =
listenerBank[registrationName] || (listenerBank[registrationName] = {});
bankForRegistrationName[inst._rootNodeID] = listener;
bankForRegistrationName[key] = listener;

var PluginModule =
EventPluginRegistry.registrationNameModules[registrationName];
Expand All @@ -127,7 +132,8 @@ var EventPluginHub = {
*/
getListener: function(inst, registrationName) {
var bankForRegistrationName = listenerBank[registrationName];
return bankForRegistrationName && bankForRegistrationName[inst._rootNodeID];
var key = getDictionaryKey(inst);
return bankForRegistrationName && bankForRegistrationName[key];
},

/**
Expand All @@ -146,7 +152,8 @@ var EventPluginHub = {
var bankForRegistrationName = listenerBank[registrationName];
// TODO: This should never be null -- when is it?
if (bankForRegistrationName) {
delete bankForRegistrationName[inst._rootNodeID];
var key = getDictionaryKey(inst);
delete bankForRegistrationName[key];
}
},

Expand All @@ -156,12 +163,13 @@ var EventPluginHub = {
* @param {object} inst The instance, which is the source of events.
*/
deleteAllListeners: function(inst) {
var key = getDictionaryKey(inst);
for (var registrationName in listenerBank) {
if (!listenerBank.hasOwnProperty(registrationName)) {
continue;
}
if (!listenerBank[registrationName][inst._rootNodeID]) {

if (!listenerBank[registrationName][key]) {
continue;
}

Expand All @@ -171,7 +179,7 @@ var EventPluginHub = {
PluginModule.willDeleteListener(inst, registrationName);
}

delete listenerBank[registrationName][inst._rootNodeID];
delete listenerBank[registrationName][key];
}
},

Expand Down