Skip to content

Commit

Permalink
Fix unmounting performance regression in __DEV__ (#7463)
Browse files Browse the repository at this point in the history 
* Comment previous occurrences of this issue

* Fix DEV performance regression in V8

* Extract try/catch into a separate function when calling hooks
  • Loading branch information
@gaearon
gaearon authored Aug 10, 2016
1 parent 178cb7d commit afa27bc
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 72 deletions.
76 changes: 48 additions & 28 deletions src/isomorphic/hooks/ReactComponentTreeHook.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,33 @@ var ReactCurrentOwner = require('ReactCurrentOwner');
var invariant = require('invariant');
var warning = require('warning');

var tree = {};
var itemByKey = {};
var unmountedIDs = {};
var rootIDs = {};

function updateTree(id, update) {
if (!tree[id]) {
tree[id] = {
// Use non-numeric keys to prevent V8 performance issues:
// https://github.com/facebook/react/pull/7232
function getKeyFromID(id) {
return '.' + id;
}
function getIDFromKey(key) {
return parseInt(key.substr(1), 10);
}

function get(id) {
var key = getKeyFromID(id);
return itemByKey[key];
}

function remove(id) {
var key = getKeyFromID(id);
delete itemByKey[key];
}

function update(id, updater) {
var key = getKeyFromID(id);
if (!itemByKey[key]) {
itemByKey[key] = {
element: null,
parentID: null,
ownerID: null,
Expand All @@ -33,14 +53,14 @@ function updateTree(id, update) {
updateCount: 0,
};
}
update(tree[id]);
updater(itemByKey[key]);
}

function purgeDeep(id) {
var item = tree[id];
var item = get(id);
if (item) {
var {childIDs} = item;
delete tree[id];
remove(id);
childIDs.forEach(purgeDeep);
}
}
Expand Down Expand Up @@ -75,15 +95,15 @@ function describeID(id) {

var ReactComponentTreeHook = {
onSetDisplayName(id, displayName) {
updateTree(id, item => item.displayName = displayName);
update(id, item => item.displayName = displayName);
},

onSetChildren(id, nextChildIDs) {
updateTree(id, item => {
update(id, item => {
item.childIDs = nextChildIDs;

nextChildIDs.forEach(nextChildID => {
var nextChild = tree[nextChildID];
var nextChild = get(nextChildID);
invariant(
nextChild,
'Expected hook events to fire for the child ' +
Expand Down Expand Up @@ -123,39 +143,39 @@ var ReactComponentTreeHook = {
},

onSetOwner(id, ownerID) {
updateTree(id, item => item.ownerID = ownerID);
update(id, item => item.ownerID = ownerID);
},

onSetParent(id, parentID) {
updateTree(id, item => item.parentID = parentID);
update(id, item => item.parentID = parentID);
},

onSetText(id, text) {
updateTree(id, item => item.text = text);
update(id, item => item.text = text);
},

onBeforeMountComponent(id, element) {
updateTree(id, item => item.element = element);
update(id, item => item.element = element);
},

onBeforeUpdateComponent(id, element) {
updateTree(id, item => item.element = element);
update(id, item => item.element = element);
},

onMountComponent(id) {
updateTree(id, item => item.isMounted = true);
update(id, item => item.isMounted = true);
},

onMountRootComponent(id) {
rootIDs[id] = true;
},

onUpdateComponent(id) {
updateTree(id, item => item.updateCount++);
update(id, item => item.updateCount++);
},

onUnmountComponent(id) {
updateTree(id, item => item.isMounted = false);
update(id, item => item.isMounted = false);
unmountedIDs[id] = true;
delete rootIDs[id];
},
Expand All @@ -173,7 +193,7 @@ var ReactComponentTreeHook = {
},

isMounted(id) {
var item = tree[id];
var item = get(id);
return item ? item.isMounted : false;
},

Expand Down Expand Up @@ -209,44 +229,44 @@ var ReactComponentTreeHook = {
},

getChildIDs(id) {
var item = tree[id];
var item = get(id);
return item ? item.childIDs : [];
},

getDisplayName(id) {
var item = tree[id];
var item = get(id);
return item ? item.displayName : 'Unknown';
},

getElement(id) {
var item = tree[id];
var item = get(id);
return item ? item.element : null;
},

getOwnerID(id) {
var item = tree[id];
var item = get(id);
return item ? item.ownerID : null;
},

getParentID(id) {
var item = tree[id];
var item = get(id);
return item ? item.parentID : null;
},

getSource(id) {
var item = tree[id];
var item = get(id);
var element = item ? item.element : null;
var source = element != null ? element._source : null;
return source;
},

getText(id) {
var item = tree[id];
var item = get(id);
return item ? item.text : null;
},

getUpdateCount(id) {
var item = tree[id];
var item = get(id);
return item ? item.updateCount : 0;
},

Expand All @@ -255,7 +275,7 @@ var ReactComponentTreeHook = {
},

getRegisteredIDs() {
return Object.keys(tree);
return Object.keys(itemByKey).map(getIDFromKey);
},
};

Expand Down
2 changes: 2 additions & 0 deletions src/renderers/dom/client/eventPlugins/SimpleEventPlugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,8 @@ var ON_CLICK_KEY = keyOf({onClick: null});
var onClickListeners = {};

function getDictionaryKey(inst) {
// Prevents V8 performance issue:
// https://github.com/facebook/react/pull/7232
return '.' + inst._rootNodeID;
}

Expand Down
48 changes: 27 additions & 21 deletions src/renderers/dom/shared/ReactDOMDebugTool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,37 +17,43 @@ var ReactDebugTool = require('ReactDebugTool');

var warning = require('warning');

var eventHandlers = [];
var handlerDoesThrowForEvent = {};
var hooks = [];
var didHookThrowForEvent = {};

function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) {
eventHandlers.forEach(function(handler) {
try {
if (handler[handlerFunctionName]) {
handler[handlerFunctionName](arg1, arg2, arg3, arg4, arg5);
}
} catch (e) {
warning(
handlerDoesThrowForEvent[handlerFunctionName],
'exception thrown by hook while handling %s: %s',
handlerFunctionName,
e + '\n' + e.stack
);
handlerDoesThrowForEvent[handlerFunctionName] = true;
function callHook(event, fn, context, arg1, arg2, arg3, arg4, arg5) {
try {
fn.call(context, arg1, arg2, arg3, arg4, arg5);
} catch (e) {
warning(
didHookThrowForEvent[event],
'Exception thrown by hook while handling %s: %s',
event,
e + '\n' + e.stack
);
didHookThrowForEvent[event] = true;
}
}

function emitEvent(event, arg1, arg2, arg3, arg4, arg5) {
for (var i = 0; i < hooks.length; i++) {
var hook = hooks[i];
var fn = hook[event];
if (fn) {
callHook(event, fn, hook, arg1, arg2, arg3, arg4, arg5);
}
});
}
}

var ReactDOMDebugTool = {
addHook(hook) {
ReactDebugTool.addHook(hook);
eventHandlers.push(hook);
hooks.push(hook);
},
removeHook(hook) {
ReactDebugTool.removeHook(hook);
for (var i = 0; i < eventHandlers.length; i++) {
if (eventHandlers[i] === hook) {
eventHandlers.splice(i, 1);
for (var i = 0; i < hooks.length; i++) {
if (hooks[i] === hook) {
hooks.splice(i, 1);
i--;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/__tests__/ReactDOMDebugTool-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('ReactDOMDebugTool', function() {
ReactDOMDebugTool.onTestEvent();
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toContain(
'exception thrown by hook while handling ' +
'Exception thrown by hook while handling ' +
'onTestEvent: Error: Hi.'
);

Expand Down
48 changes: 27 additions & 21 deletions src/renderers/shared/ReactDebugTool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,31 @@ var ExecutionEnvironment = require('ExecutionEnvironment');
var performanceNow = require('performanceNow');
var warning = require('warning');

var eventHandlers = [];
var handlerDoesThrowForEvent = {};
var hooks = [];
var didHookThrowForEvent = {};

function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) {
eventHandlers.forEach(function(handler) {
try {
if (handler[handlerFunctionName]) {
handler[handlerFunctionName](arg1, arg2, arg3, arg4, arg5);
}
} catch (e) {
warning(
handlerDoesThrowForEvent[handlerFunctionName],
'exception thrown by hook while handling %s: %s',
handlerFunctionName,
e + '\n' + e.stack
);
handlerDoesThrowForEvent[handlerFunctionName] = true;
function callHook(event, fn, context, arg1, arg2, arg3, arg4, arg5) {
try {
fn.call(context, arg1, arg2, arg3, arg4, arg5);
} catch (e) {
warning(
didHookThrowForEvent[event],
'Exception thrown by hook while handling %s: %s',
event,
e + '\n' + e.stack
);
didHookThrowForEvent[event] = true;
}
}

function emitEvent(event, arg1, arg2, arg3, arg4, arg5) {
for (var i = 0; i < hooks.length; i++) {
var hook = hooks[i];
var fn = hook[event];
if (fn) {
callHook(event, fn, hook, arg1, arg2, arg3, arg4, arg5);
}
});
}
}

var isProfiling = false;
Expand Down Expand Up @@ -185,12 +191,12 @@ function resumeCurrentLifeCycleTimer() {

var ReactDebugTool = {
addHook(hook) {
eventHandlers.push(hook);
hooks.push(hook);
},
removeHook(hook) {
for (var i = 0; i < eventHandlers.length; i++) {
if (eventHandlers[i] === hook) {
eventHandlers.splice(i, 1);
for (var i = 0; i < hooks.length; i++) {
if (hooks[i] === hook) {
hooks.splice(i, 1);
i--;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/__tests__/ReactDebugTool-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('ReactDebugTool', function() {
ReactDebugTool.onTestEvent();
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toContain(
'exception thrown by hook while handling ' +
'Exception thrown by hook while handling ' +
'onTestEvent: Error: Hi.'
);

Expand Down
2 changes: 2 additions & 0 deletions src/renderers/shared/stack/event/EventPluginHub.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ var executeDispatchesAndReleaseTopLevel = function(e) {
};

var getDictionaryKey = function(inst) {
// Prevents V8 performance issue:
// https://github.com/facebook/react/pull/7232
return '.' + inst._rootNodeID;
};

Expand Down

0 comments on commit afa27bc

Please sign in to comment.