Skip to content

Commit

Permalink
Fix DEV performance regression by avoiding Object.assign on Fibers
Browse files Browse the repository at this point in the history
  • Loading branch information
gaearon committed Apr 1, 2018
1 parent 7d31311 commit 0f15512
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 3 deletions.
44 changes: 44 additions & 0 deletions packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
12 changes: 9 additions & 3 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ import {
startCommitLifeCyclesTimer,
stopCommitLifeCyclesTimer,
} from './ReactDebugFiberPerf';
import {createWorkInProgress} from './ReactFiber';
import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber';
import {onCommitRoot} from './ReactFiberDevToolsHook';
import {
NoWork,
Expand Down Expand Up @@ -260,7 +260,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
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);
Expand Down Expand Up @@ -787,7 +790,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
}

if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
stashedWorkInProgressProperties = Object.assign({}, workInProgress);
stashedWorkInProgressProperties = assignFiberPropertiesInDEV(
null,
workInProgress,
);
}
let next = beginWork(current, workInProgress, nextRenderExpirationTime);

Expand Down
Original file line number Diff line number Diff line change
@@ -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] = 42;
}
expect(realFiber).not.toEqual(stash);

// Verify we can still "revert" to the stashed properties.
expect(assignFiberPropertiesInDEV(realFiber, stash)).toBe(realFiber);
expect(realFiber).toEqual(stash);
});
});

0 comments on commit 0f15512

Please sign in to comment.