Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DEV performance regression by avoiding Object.assign on Fibers #12510

Merged
merged 2 commits into from
Apr 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if there was a way to validate that all the enumerable keys from source were indeed copied to target, but where would you put it? __DEV__? :trollface:

Any way to write a test to see if nothing is missing, or not all fibers have the expected properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the goal is to be super fast I think the tradeoff of writing an “internal” test for this (and doing no validation in DEV) is the right one.

}
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in Retore word. Probably you mean Restore.

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(
stashedWorkInProgressProperties,
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] = 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);
Copy link

@halftrue halftrue Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a test by comparing assignFiberPropertiesInDEV and Object.assgin's results could be better.

});
});