From 59dac9d7a6a2f0b66003cf717d71b5587265423f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sun, 1 Apr 2018 19:10:37 +0100 Subject: [PATCH] Fix DEV performance regression by avoiding Object.assign on Fibers (#12510) * Fix DEV performance regression by avoiding Object.assign on Fibers * Reduce allocations in hot path by reusing the stash Since performUnitOfWork() is not reentrant, it should be safe to reuse the same stash every time instead of creating a new object. --- packages/react-reconciler/src/ReactFiber.js | 44 +++++++++++++++++++ .../src/ReactFiberScheduler.js | 12 +++-- ...actIncrementalErrorReplay-test.internal.js | 43 ++++++++++++++++++ 3 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.internal.js diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 6c88c0aa18b02..1ffa168b11a30 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -471,3 +471,47 @@ export function createFiberFromPortal( }; return fiber; } + +// Used for stashing WIP properties to replay failed work in DEV. +export function assignFiberPropertiesInDEV( + target: Fiber | null, + source: Fiber, +): Fiber { + if (target === null) { + // This Fiber's initial properties will always be overwritten. + // We only use a Fiber to ensure the same hidden class so DEV isn't slow. + target = createFiber(IndeterminateComponent, null, null, NoContext); + } + + // This is intentionally written as a list of all properties. + // We tried to use Object.assign() instead but this is called in + // the hottest path, and Object.assign() was too slow: + // https://github.com/facebook/react/issues/12502 + // This code is DEV-only so size is not a concern. + + target.tag = source.tag; + target.key = source.key; + target.type = source.type; + target.stateNode = source.stateNode; + target.return = source.return; + target.child = source.child; + target.sibling = source.sibling; + target.index = source.index; + target.ref = source.ref; + target.pendingProps = source.pendingProps; + target.memoizedProps = source.memoizedProps; + target.updateQueue = source.updateQueue; + target.memoizedState = source.memoizedState; + target.mode = source.mode; + target.effectTag = source.effectTag; + target.nextEffect = source.nextEffect; + target.firstEffect = source.firstEffect; + target.lastEffect = source.lastEffect; + target.expirationTime = source.expirationTime; + target.alternate = source.alternate; + target._debugID = source._debugID; + target._debugSource = source._debugSource; + target._debugOwner = source._debugOwner; + target._debugIsCurrentlyTiming = source._debugIsCurrentlyTiming; + return target; +} diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 2737e2a7e008f..3ec6af40bcdc8 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -76,7 +76,7 @@ import { startCommitLifeCyclesTimer, stopCommitLifeCyclesTimer, } from './ReactDebugFiberPerf'; -import {createWorkInProgress} from './ReactFiber'; +import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber'; import {onCommitRoot} from './ReactFiberDevToolsHook'; import { NoWork, @@ -267,7 +267,10 @@ export default function( stashedWorkInProgressProperties = null; replayUnitOfWork = (failedUnitOfWork: Fiber, isAsync: boolean) => { // Retore the original state of the work-in-progress - Object.assign(failedUnitOfWork, stashedWorkInProgressProperties); + assignFiberPropertiesInDEV( + failedUnitOfWork, + stashedWorkInProgressProperties, + ); switch (failedUnitOfWork.tag) { case HostRoot: popHostContainer(failedUnitOfWork); @@ -846,7 +849,10 @@ export default function( } if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { - stashedWorkInProgressProperties = Object.assign({}, workInProgress); + stashedWorkInProgressProperties = assignFiberPropertiesInDEV( + stashedWorkInProgressProperties, + workInProgress, + ); } let next = beginWork(current, workInProgress, nextRenderExpirationTime); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.internal.js new file mode 100644 index 0000000000000..6c97c46b82187 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.internal.js @@ -0,0 +1,43 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @jest-environment node + */ + +'use strict'; + +describe('ReactIncrementalErrorReplay-test', () => { + it('copies all keys when stashing potentially failing work', () => { + // Note: this test is fragile and relies on internals. + // We almost always try to avoid such tests, but here the cost of + // the list getting out of sync (and causing subtle bugs in rare cases) + // is higher than the cost of maintaing the test. + const { + // Any Fiber factory function will do. + createHostRootFiber, + // This is the method we're going to test. + // If this is no longer used, you can delete this test file. + assignFiberPropertiesInDEV, + } = require('../ReactFiber'); + + // Get a real fiber. + const realFiber = createHostRootFiber(false); + const stash = assignFiberPropertiesInDEV(null, realFiber); + + // Verify we get all the same fields. + expect(realFiber).toEqual(stash); + + // Mutate the original. + for (let key in realFiber) { + realFiber[key] = key + '_' + Math.random(); + } + expect(realFiber).not.toEqual(stash); + + // Verify we can still "revert" to the stashed properties. + expect(assignFiberPropertiesInDEV(realFiber, stash)).toBe(realFiber); + expect(realFiber).toEqual(stash); + }); +});