Make ReactPerf.start() work during reconciliation (#7208)

* Add failing test demonstrating a ReactPerf warning * Make the failing ReactPerf test more precise * Make ReactPerf.start() work during reconciliation * Reorder lifecycle methods for greater clarity * Fix memory leak * Error boundaries should not break ReactPerf * Put onBeginFlush/onEndFlush into transaction wrappers This looks cleaner even though it is not strictly necessary. We still call them manually for unmounting because it doesn't have a transaction.
facebook · Jul 7, 2016 · 1a0e3a3 · 1a0e3a3
1 parent 21ce271
commit 1a0e3a3
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 38 deletions.
diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js
@@ -315,10 +315,6 @@ var ReactMount = {
     shouldReuseMarkup,
     context
   ) {
-    if (__DEV__) {
-      ReactInstrumentation.debugTool.onBeginFlush();
-    }
-
     // Various parts of our code (such as ReactCompositeComponent's
     // _renderValidatedComponent) assume that calls to render aren't nested;
     // verify that that's the case.
@@ -364,7 +360,6 @@ var ReactMount = {
       ReactInstrumentation.debugTool.onMountRootComponent(
         componentInstance._renderedComponent._debugID
       );
-      ReactInstrumentation.debugTool.onEndFlush();
     }
 
     return componentInstance;

diff --git a/src/renderers/dom/client/ReactReconcileTransaction.js b/src/renderers/dom/client/ReactReconcileTransaction.js
@@ -15,6 +15,7 @@ var CallbackQueue = require('CallbackQueue');
 var PooledClass = require('PooledClass');
 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
 var ReactInputSelection = require('ReactInputSelection');
+var ReactInstrumentation = require('ReactInstrumentation');
 var Transaction = require('Transaction');
 var ReactUpdateQueue = require('ReactUpdateQueue');
 
@@ -91,6 +92,13 @@ var TRANSACTION_WRAPPERS = [
   ON_DOM_READY_QUEUEING,
 ];
 
+if (__DEV__) {
+  TRANSACTION_WRAPPERS.push({
+    initialize: ReactInstrumentation.debugTool.onBeginFlush,
+    close: ReactInstrumentation.debugTool.onEndFlush,
+  });
+}
+
 /**
  * Currently:
  * - The order that these are listed in the transaction is critical:

diff --git a/src/renderers/dom/server/ReactServerRendering.js b/src/renderers/dom/server/ReactServerRendering.js
@@ -37,9 +37,6 @@ function renderToStringImpl(element, makeStaticMarkup) {
     transaction = ReactServerRenderingTransaction.getPooled(makeStaticMarkup);
 
     return transaction.perform(function() {
-      if (__DEV__) {
-        ReactInstrumentation.debugTool.onBeginFlush();
-      }
       var componentInstance = instantiateReactComponent(element, true);
       var markup = ReactReconciler.mountComponent(
         componentInstance,
@@ -52,7 +49,6 @@ function renderToStringImpl(element, makeStaticMarkup) {
         ReactInstrumentation.debugTool.onUnmountComponent(
           componentInstance._debugID
         );
-        ReactInstrumentation.debugTool.onEndFlush();
       }
       if (!makeStaticMarkup) {
         markup = ReactMarkupChecksum.addChecksumToMarkup(markup);

diff --git a/src/renderers/dom/server/ReactServerRenderingTransaction.js b/src/renderers/dom/server/ReactServerRenderingTransaction.js
@@ -13,6 +13,7 @@
 
 var PooledClass = require('PooledClass');
 var Transaction = require('Transaction');
+var ReactInstrumentation = require('ReactInstrumentation');
 var ReactServerUpdateQueue = require('ReactServerUpdateQueue');
 
 
@@ -23,6 +24,13 @@ var ReactServerUpdateQueue = require('ReactServerUpdateQueue');
  */
 var TRANSACTION_WRAPPERS = [];
 
+if (__DEV__) {
+  TRANSACTION_WRAPPERS.push({
+    initialize: ReactInstrumentation.debugTool.onBeginFlush,
+    close: ReactInstrumentation.debugTool.onEndFlush,
+  });
+}
+
 var noopCallbackQueue = {
   enqueue: function() {},
 };

diff --git a/src/renderers/native/ReactNativeMount.js b/src/renderers/native/ReactNativeMount.js
@@ -138,10 +138,6 @@ var ReactNativeMount = {
     var instance = instantiateReactComponent(nextWrappedElement, false);
     ReactNativeMount._instancesByContainerID[containerTag] = instance;
 
-    if (__DEV__) {
-      ReactInstrumentation.debugTool.onBeginFlush();
-    }
-
     // The initial render is synchronous but any updates that happen during
     // rendering, in componentWillMount or componentDidMount, will be batched
     // according to the current batching strategy.
@@ -156,7 +152,6 @@ var ReactNativeMount = {
       ReactInstrumentation.debugTool.onMountRootComponent(
         instance._renderedComponent._debugID
       );
-      ReactInstrumentation.debugTool.onEndFlush();
     }
     var component = instance.getPublicInstance();
     if (callback) {

diff --git a/src/renderers/native/ReactNativeReconcileTransaction.js b/src/renderers/native/ReactNativeReconcileTransaction.js
@@ -14,6 +14,7 @@
 var CallbackQueue = require('CallbackQueue');
 var PooledClass = require('PooledClass');
 var Transaction = require('Transaction');
+var ReactInstrumentation = require('ReactInstrumentation');
 var ReactUpdateQueue = require('ReactUpdateQueue');
 
 /**
@@ -43,6 +44,13 @@ var ON_DOM_READY_QUEUEING = {
  */
 var TRANSACTION_WRAPPERS = [ON_DOM_READY_QUEUEING];
 
+if (__DEV__) {
+  TRANSACTION_WRAPPERS.push({
+    initialize: ReactInstrumentation.debugTool.onBeginFlush,
+    close: ReactInstrumentation.debugTool.onEndFlush,
+  });
+}
+
 /**
  * Currently:
  * - The order that these are listed in the transaction is critical:

diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js
@@ -79,7 +79,7 @@ function resetMeasurements() {
   var previousMeasurements = currentFlushMeasurements || [];
   var previousOperations = ReactHostOperationHistoryDevtool.getHistory();
 
-  if (!isProfiling || currentFlushNesting === 0) {
+  if (currentFlushNesting === 0) {
     currentFlushStartTime = null;
     currentFlushMeasurements = null;
     clearHistory();
@@ -106,7 +106,7 @@ function checkDebugID(debugID) {
 }
 
 function beginLifeCycleTimer(debugID, timerType) {
-  if (!isProfiling || currentFlushNesting === 0) {
+  if (currentFlushNesting === 0) {
     return;
   }
   warning(
@@ -125,7 +125,7 @@ function beginLifeCycleTimer(debugID, timerType) {
 }
 
 function endLifeCycleTimer(debugID, timerType) {
-  if (!isProfiling || currentFlushNesting === 0) {
+  if (currentFlushNesting === 0) {
     return;
   }
   warning(
@@ -137,11 +137,13 @@ function endLifeCycleTimer(debugID, timerType) {
     currentTimerType || 'no',
     (debugID === currentTimerDebugID) ? 'the same' : 'another'
   );
-  currentFlushMeasurements.push({
-    timerType,
-    instanceID: debugID,
-    duration: performanceNow() - currentTimerStartTime - currentTimerNestedFlushDuration,
-  });
+  if (isProfiling) {
+    currentFlushMeasurements.push({
+      timerType,
+      instanceID: debugID,
+      duration: performanceNow() - currentTimerStartTime - currentTimerNestedFlushDuration,
+    });
+  }
   currentTimerStartTime = null;
   currentTimerNestedFlushDuration = null;
   currentTimerDebugID = null;
@@ -194,6 +196,7 @@ var ReactDebugTool = {
     isProfiling = true;
     flushHistory.length = 0;
     resetMeasurements();
+    ReactDebugTool.addDevtool(ReactHostOperationHistoryDevtool);
   },
   endProfiling() {
     if (!isProfiling) {
@@ -202,6 +205,7 @@ var ReactDebugTool = {
 
     isProfiling = false;
     resetMeasurements();
+    ReactDebugTool.removeDevtool(ReactHostOperationHistoryDevtool);
   },
   getFlushHistory() {
     return flushHistory;
@@ -236,6 +240,12 @@ var ReactDebugTool = {
     checkDebugID(debugID);
     emitEvent('onEndReconcilerTimer', debugID, timerType);
   },
+  onError(debugID) {
+    if (currentTimerDebugID != null) {
+      endLifeCycleTimer(currentTimerDebugID, currentTimerType);
+    }
+    emitEvent('onError', debugID);
+  },
   onBeginProcessingChildContext() {
     emitEvent('onBeginProcessingChildContext');
   },
@@ -309,7 +319,6 @@ var ReactDebugTool = {
 
 ReactDebugTool.addDevtool(ReactInvalidSetStateWarningDevTool);
 ReactDebugTool.addDevtool(ReactComponentTreeDevtool);
-ReactDebugTool.addDevtool(ReactHostOperationHistoryDevtool);
 ReactDebugTool.addDevtool(ReactChildrenMutationWarningDevtool);
 var url = (ExecutionEnvironment.canUseDOM && window.location.href) || '';
 if ((/[?&]react_perf\b/).test(url)) {

diff --git a/src/renderers/shared/__tests__/ReactPerf-test.js b/src/renderers/shared/__tests__/ReactPerf-test.js
@@ -467,5 +467,49 @@ describe('ReactPerf', function() {
     expect(console.error.calls.count()).toBe(1);
 
     __DEV__ = true;
-  })
+  });
+
+  it('should work when measurement starts during reconciliation', () => {
+    // https://github.com/facebook/react/issues/6949#issuecomment-230371009
+    var Measurer = React.createClass({
+      componentWillMount() {
+        ReactPerf.start();
+      },
+      componentDidMount() {
+        ReactPerf.stop();
+      },
+      componentWillUpdate() {
+        ReactPerf.start();
+      },
+      componentDidUpdate() {
+        ReactPerf.stop();
+      },
+      render() {
+        // Force reconciliation despite constant element
+        return React.cloneElement(this.props.children);
+      },
+    });
+
+    var container = document.createElement('div');
+    ReactDOM.render(<Measurer><App /></Measurer>, container);
+    expect(ReactPerf.getWasted()).toEqual([]);
+
+    ReactDOM.render(<Measurer><App /></Measurer>, container);
+    expect(ReactPerf.getWasted()).toEqual([{
+      key: 'Measurer',
+      instanceCount: 1,
+      inclusiveRenderDuration: 4,
+      renderCount: 1,
+    }, {
+      key: 'App',
+      instanceCount: 1,
+      inclusiveRenderDuration: 3,
+      renderCount: 1,
+    }, {
+      key: 'App > Box',
+      instanceCount: 2,
+      inclusiveRenderDuration: 2,
+      renderCount: 2,
+    }]);
+  });
 });
diff --git a/src/renderers/shared/devtools/__tests__/ReactHostOperationHistoryDevtool-test.js b/src/renderers/shared/devtools/__tests__/ReactHostOperationHistoryDevtool-test.js
@@ -13,6 +13,7 @@
 
 describe('ReactHostOperationHistoryDevtool', () => {
   var React;
+  var ReactPerf;
   var ReactDOM;
   var ReactDOMComponentTree;
   var ReactDOMFeatureFlags;
@@ -22,12 +23,19 @@ describe('ReactHostOperationHistoryDevtool', () => {
     jest.resetModuleRegistry();
 
     React = require('React');
+    ReactPerf = require('ReactPerf');
     ReactDOM = require('ReactDOM');
     ReactDOMComponentTree = require('ReactDOMComponentTree');
     ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
     ReactHostOperationHistoryDevtool = require('ReactHostOperationHistoryDevtool');
+
+    ReactPerf.start();
   });
 
+  afterEach(() => {
+    ReactPerf.stop();
+  })
+
   function assertHistoryMatches(expectedHistory) {
     var actualHistory = ReactHostOperationHistoryDevtool.getHistory();
     expect(actualHistory).toEqual(expectedHistory);

diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js
@@ -467,6 +467,11 @@ var ReactCompositeComponentMixin = {
     try {
       markup = this.performInitialMount(renderedElement, hostParent, hostContainerInfo, transaction, context);
     } catch (e) {
+      if (__DEV__) {
+        if (this._debugID !== 0) {
+          ReactInstrumentation.debugTool.onError();
+        }
+      }
       // Roll back to checkpoint, handle error (which may add items to the transaction), and take a new checkpoint
       transaction.rollback(checkpoint);
       this._instance.unstable_handleError(e);

diff --git a/src/renderers/shared/stack/reconciler/ReactUpdates.js b/src/renderers/shared/stack/reconciler/ReactUpdates.js
@@ -14,7 +14,6 @@
 var CallbackQueue = require('CallbackQueue');
 var PooledClass = require('PooledClass');
 var ReactFeatureFlags = require('ReactFeatureFlags');
-var ReactInstrumentation = require('ReactInstrumentation');
 var ReactReconciler = require('ReactReconciler');
 var Transaction = require('Transaction');
 
@@ -193,10 +192,6 @@ function runBatchedUpdates(transaction) {
 }
 
 var flushBatchedUpdates = function() {
-  if (__DEV__) {
-    ReactInstrumentation.debugTool.onBeginFlush();
-  }
-
   // ReactUpdatesFlushTransaction's wrappers will clear the dirtyComponents
   // array and perform any updates enqueued by mount-ready handlers (i.e.,
   // componentDidUpdate) but we need to check here too in order to catch
@@ -216,10 +211,6 @@ var flushBatchedUpdates = function() {
       CallbackQueue.release(queue);
     }
   }
-
-  if (__DEV__) {
-    ReactInstrumentation.debugTool.onEndFlush();
-  }
 };
 
 /**

diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js
@@ -122,10 +122,6 @@ var ReactHostMount = {
 
     var instance = instantiateReactComponent(nextWrappedElement, false);
 
-    if (__DEV__) {
-      ReactInstrumentation.debugTool.onBeginFlush();
-    }
-
     // The initial render is synchronous but any updates that happen during
     // rendering, in componentWillMount or componentDidMount, will be batched
     // according to the current batching strategy.
@@ -139,7 +135,6 @@ var ReactHostMount = {
       ReactInstrumentation.debugTool.onMountRootComponent(
         instance._renderedComponent._debugID
       );
-      ReactInstrumentation.debugTool.onEndFlush();
     }
     return new ReactTestInstance(instance);
   },