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.

Sign up for GitHub

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

Jump to bottom

[Fiber] Move memoization to begin phase #8655

Merged
merged 3 commits into from
Jan 13, 2017
Merged

[Fiber] Move memoization to begin phase #8655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ed304f
Move memoization to begin phase
acdlite Dec 30, 2016
d17b9a1
Confirm that a shallow bailout does not drop work in the child
acdlite Jan 8, 2017
54ebcbc
Add test for state merging when sCU is false
sophiebits Jan 12, 2017
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
3 changes: 0 additions & 3 deletions scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
* does not do a deep comparison

src/addons/__tests__/ReactFragment-test.js
* should throw if a plain object is used as a child
* should throw if a plain object even if it is in an owner
Expand Down
4 changes: 4 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ scripts/error-codes/__tests__/invertObject-test.js

src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
* provides a default shouldComponentUpdate implementation
* does not do a deep comparison

src/addons/__tests__/ReactFragment-test.js
* warns for numeric keys on objects as children
Expand Down Expand Up @@ -1142,7 +1143,9 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* can resume work in a subtree even when a parent bails out
* can resume work in a bailed subtree within one pass
* can reuse work done after being preempted
* can reuse work that began but did not complete, after being preempted
* can reuse work if shouldComponentUpdate is false, after being preempted
* memoizes work even if shouldComponentUpdate returns false
* can update in the middle of a tree using setState
* can queue multiple state updates
* can use updater form of setState
Expand Down Expand Up @@ -1407,6 +1410,7 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js
* should call componentDidUpdate of children first
* should batch unmounts
* should update state when called from child cWRP
* should merge state when sCU returns false

src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js
* should not produce child DOM nodes for null and false
Expand Down
101 changes: 69 additions & 32 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ var {
OffscreenPriority,
} = require('ReactPriorityLevel');
var {
Update,
Placement,
ContentReset,
Err,
Expand Down Expand Up @@ -91,7 +90,12 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
mountClassInstance,
resumeMountClassInstance,
updateClassInstance,
} = ReactFiberClassComponent(scheduleUpdate, getPriorityContext);
} = ReactFiberClassComponent(
scheduleUpdate,
getPriorityContext,
memoizeProps,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an abstraction worthy the pain of passing around? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha I almost did it so I could log when I was happening :D And at one point it was a bit more involved.

I agree this could be restructured in a better way but when I'm unsure I prefer to just find something that works and passes the tests and then clean it up later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*mostly, not almost

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example if I rewrote all the stuff in ReactFiberClassComponent, and then you told me the overall approach was wrong, I'd have wasted a bunch of time. And it would be harder to review. Easier to rewrite after overall approach is figured out.

memoizeState,
);

function markChildAsProgressed(current, workInProgress, priorityLevel) {
// We now have clones. Let's store them as the currently progressed work.
Expand Down Expand Up @@ -176,12 +180,13 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
if (nextChildren === null) {
nextChildren = current && current.memoizedProps;
nextChildren = workInProgress.memoizedProps;
}
} else if (nextChildren === null || workInProgress.memoizedProps === nextChildren) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextChildren);
return workInProgress.child;
}

Expand All @@ -202,16 +207,20 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
if (nextProps === null) {
nextProps = current && current.memoizedProps;
nextProps = memoizedProps;
}
} else {
if (nextProps == null || memoizedProps === nextProps) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
// TODO: Disable this before release, since it is not part of the public API
// I use this for testing to compare the relative overhead of classes.
if (typeof fn.shouldComponentUpdate === 'function' &&
!fn.shouldComponentUpdate(memoizedProps, nextProps)) {
// Memoize props even if shouldComponentUpdate returns false
memoizeProps(workInProgress, nextProps);
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
} else if (nextProps === null || memoizedProps === nextProps || (
// TODO: Disable this before release, since it is not part of the public API
// I use this for testing to compare the relative overhead of classes.
memoizedProps !== null &&
typeof fn.shouldComponentUpdate === 'function' &&
!fn.shouldComponentUpdate(memoizedProps, nextProps)
)) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

var unmaskedContext = getUnmaskedContext(workInProgress);
Expand All @@ -226,6 +235,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
nextChildren = fn(nextProps, context);
}
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
}

Expand Down Expand Up @@ -258,31 +268,23 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
shouldUpdate : boolean,
hasContext : boolean,
) {
// Schedule side-effects

// Refs should update even if shouldComponentUpdate returns false
markRef(current, workInProgress);

if (shouldUpdate) {
workInProgress.effectTag |= Update;
} else {
// If an update was already in progress, we should schedule an Update
// effect even though we're bailing out, so that cWU/cDU are called.
if (current) {
const instance = current.stateNode;
if (instance.props !== current.memoizedProps ||
instance.state !== current.memoizedState) {
workInProgress.effectTag |= Update;
}
}
if (!shouldUpdate) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

// Rerender
const instance = workInProgress.stateNode;

// Rerender
ReactCurrentOwner.current = workInProgress;
const nextChildren = instance.render();
reconcileChildren(current, workInProgress, nextChildren);
// Memoize props and state using the values we just used to render.
// TODO: Restructure so we never read values from the instance.
memoizeState(workInProgress, instance.state);
memoizeProps(workInProgress, instance.props);

// The context might have changed so we need to recalculate it.
if (hasContext) {
Expand Down Expand Up @@ -321,7 +323,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
}
const element = state.element;
reconcileChildren(current, workInProgress, element);
workInProgress.memoizedState = state;
memoizeState(workInProgress, state);
return workInProgress.child;
}
// If there is no update queue, that's a bailout because the root has no props.
Expand All @@ -338,7 +340,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
if (nextProps === null) {
nextProps = prevProps;
nextProps = memoizedProps;
if (!nextProps) {
throw new Error('We should always have pending or current props.');
}
Expand Down Expand Up @@ -403,6 +405,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(

// Reconcile the children and stash them for later work.
reconcileChildrenAtPriority(current, workInProgress, nextChildren, OffscreenPriority);
memoizeProps(workInProgress, nextProps);
workInProgress.child = current ? current.child : null;

if (!current) {
Expand All @@ -422,10 +425,22 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
return null;
} else {
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
}
}

function updateHostText(current, workInProgress) {
let nextProps = workInProgress.pendingProps;
if (nextProps === null) {
nextProps = workInProgress.memoizedProps;
}
memoizeProps(workInProgress, nextProps);
// Nothing to do here. This is terminal. We'll do the completion step
// immediately after.
return null;
}

function mountIndeterminateComponent(current, workInProgress, priorityLevel) {
if (current) {
throw new Error('An indeterminate component should never have mounted.');
Expand Down Expand Up @@ -484,6 +499,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
}
}
reconcileChildren(current, workInProgress, value);
memoizeProps(workInProgress, props);
return workInProgress.child;
}
}
Expand All @@ -503,6 +519,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
reconcileChildren(current, workInProgress, nextCoroutine.children);
memoizeProps(workInProgress, nextCoroutine);
// This doesn't take arbitrary time so we could synchronously just begin
// eagerly do the work of workInProgress.child as an optimization.
return workInProgress.child;
Expand Down Expand Up @@ -537,9 +554,11 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
nextChildren,
priorityLevel
);
memoizeProps(workInProgress, nextChildren);
markChildAsProgressed(current, workInProgress, priorityLevel);
} else {
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextChildren);
}
return workInProgress.child;
}
Expand Down Expand Up @@ -606,6 +625,18 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
return null;
}

function memoizeProps(workInProgress : Fiber, nextProps : any) {
workInProgress.memoizedProps = nextProps;
// Reset the pending props
workInProgress.pendingProps = null;
}

function memoizeState(workInProgress : Fiber, nextState : any) {
workInProgress.memoizedState = nextState;
// Don't reset the updateQueue, in case there are pending updates. Resetting
// is handled by beginUpdateQueue.
}

function beginWork(current : ?Fiber, workInProgress : Fiber, priorityLevel : PriorityLevel) : ?Fiber {
if (workInProgress.pendingWorkPriority === NoWork ||
workInProgress.pendingWorkPriority > priorityLevel) {
Expand Down Expand Up @@ -639,9 +670,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
case HostComponent:
return updateHostComponent(current, workInProgress);
case HostText:
// Nothing to do here. This is terminal. We'll do the completion step
// immediately after.
return null;
return updateHostText(current, workInProgress);
case CoroutineHandlerPhase:
// This is a restart. Reset the tag to the initial phase.
workInProgress.tag = CoroutineComponent;
Expand Down Expand Up @@ -683,6 +712,14 @@ module.exports = function<T, P, I, TI, PI, C, CX>(
// Unmount the current children as if the component rendered null
const nextChildren = null;
reconcileChildren(current, workInProgress, nextChildren);

if (workInProgress.tag === ClassComponent) {
const instance = workInProgress.stateNode;
workInProgress.memoizedProps = instance.props;
workInProgress.memoizedState = instance.state;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Wouldn't this already have been memoized if a child threw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put the memoization after reconciling the children because it seemed to make the most sense conceptually. Perhaps I could move it earlier before any lifecycles. The tricky part about that is that some things need to read from the previous memoized values. So you'd have to pass those around and it gets a bit hard to follow. But there's likely a better way to structure all this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to get to the point where we never read from the instance at all. This is one of two (I believe) places where we still do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't understand why this needs to be done in beginFailedWork specifically.

If anything, this should set the memoized values to null since this component is now in an invalid state. The children output doesn't correspond to the input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree but that breaks an assumption we make in a few places that we always have either pending or memoized props. I figured that was outside the scope of this PR but I'll fix it here if it helps this land. Shouldn't be too difficult.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That wasn't the case before because we would always memoize in completeWork, even if it failed in beginWork.

Copy link
Collaborator Author

@acdlite acdlite Jan 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Haha if I remove those invariants everything seems to still work. I guess I thought it'd be a bit more complicated than that. I'll go ahead and push that :)

Edit: Never mind, this actually does break some things

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm although what should get passed to shouldComponentUpdate the next time if we set memoizedProps to null? An empty object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess in that case we shouldn't call shouldComponentUpdate at all since it's as if it's mounting for the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that something is broken here with this error handling logic but I understand now that you're only preserving existing behavior by moving it in here. I think that's fine for the first iteration.

workInProgress.pendingProps = null;
}

return workInProgress.child;
}

Expand Down
Loading