Fix unmounting performance regression in __DEV__ (#7463)

* 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)
facebook · Aug 12, 2016 · 5dce7c2 · 5dce7c2
1 parent 382f151
commit 5dce7c2
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 72 deletions.
diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js
@@ -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,
@@ -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);
   }
 }
@@ -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 ' +
@@ -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];
   },
@@ -173,7 +193,7 @@ var ReactComponentTreeHook = {
   },
 
   isMounted(id) {
-    var item = tree[id];
+    var item = get(id);
     return item ? item.isMounted : false;
   },
 
@@ -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;
   },
 
@@ -255,7 +275,7 @@ var ReactComponentTreeHook = {
   },
 
   getRegisteredIDs() {
-    return Object.keys(tree);
+    return Object.keys(itemByKey).map(getIDFromKey);
   },
 };
 

diff --git a/src/renderers/dom/client/eventPlugins/SimpleEventPlugin.js b/src/renderers/dom/client/eventPlugins/SimpleEventPlugin.js
@@ -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;
 }
 

diff --git a/src/renderers/dom/shared/ReactDOMDebugTool.js b/src/renderers/dom/shared/ReactDOMDebugTool.js
@@ -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--;
       }
     }

diff --git a/src/renderers/dom/shared/__tests__/ReactDOMDebugTool-test.js b/src/renderers/dom/shared/__tests__/ReactDOMDebugTool-test.js
@@ -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.'
     );
 

diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js
@@ -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;
@@ -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--;
       }
     }

diff --git a/src/renderers/shared/__tests__/ReactDebugTool-test.js b/src/renderers/shared/__tests__/ReactDebugTool-test.js
@@ -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.'
     );
 

diff --git a/src/renderers/shared/stack/event/EventPluginHub.js b/src/renderers/shared/stack/event/EventPluginHub.js
@@ -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;
 };