Skip to content

[Fiber] Simplify coroutines by making yields stateless #8840

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

Merged
merged 5 commits into from
Jan 26, 2017
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
2 changes: 2 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,9 @@ src/renderers/shared/__tests__/ReactPerf-test.js

src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js
* should render a coroutine
* should update a coroutine
* should unmount a composite in a coroutine
* should handle deep updates in coroutine

src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* should render a simple component
Expand Down
3 changes: 1 addition & 2 deletions src/isomorphic/classic/element/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,7 @@ var ReactElementValidator = {
createElement: function(type, props, children) {
var validType =
typeof type === 'string' ||
typeof type === 'function' ||
type !== null && typeof type === 'object' && typeof type.tag === 'number';
typeof type === 'function';
// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
if (!validType) {
Expand Down
62 changes: 20 additions & 42 deletions src/renderers/shared/fiber/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ var {
} = require('ReactPortal');

var ReactFiber = require('ReactFiber');
var ReactReifiedYield = require('ReactReifiedYield');
var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect');
var ReactTypeOfWork = require('ReactTypeOfWork');

Expand All @@ -53,11 +52,6 @@ const {
createFiberFromPortal,
} = ReactFiber;

const {
createReifiedYield,
createUpdatedReifiedYield,
} = ReactReifiedYield;

const isArray = Array.isArray;

const {
Expand Down Expand Up @@ -316,21 +310,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
yieldNode : ReactYield,
priority : PriorityLevel
) : Fiber {
// TODO: Should this also compare continuation to determine whether to reuse?
if (current == null || current.tag !== YieldComponent) {
// Insert
const reifiedYield = createReifiedYield(yieldNode);
const created = createFiberFromYield(yieldNode, priority);
created.type = reifiedYield;
created.type = yieldNode.value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this break hidden classes if it's a number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but only in V8. Others use NaN tagging. It's likely already unlike that you'd use it this way. We can warn if it becomes an issue.

created.return = returnFiber;
return created;
} else {
// Move based on index
const existing = useFiber(current, priority);
existing.type = createUpdatedReifiedYield(
current.type,
yieldNode
);
existing.type = yieldNode.value;
existing.return = returnFiber;
return existing;
}
Expand Down Expand Up @@ -411,9 +400,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}

case REACT_YIELD_TYPE: {
const reifiedYield = createReifiedYield(newChild);
const created = createFiberFromYield(newChild, priority);
created.type = reifiedYield;
created.type = newChild.value;
created.return = returnFiber;
return created;
}
Expand Down Expand Up @@ -474,7 +462,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}

case REACT_YIELD_TYPE: {
if (newChild.key === key) {
// Yields doesn't have keys. If the previous node is implicitly keyed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: don't have keys

// we can continue to replace it without aborting even if it is not a
// yield.
if (key === null) {
return updateYield(returnFiber, oldFiber, newChild, priority);
} else {
return null;
Expand Down Expand Up @@ -527,9 +518,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}

case REACT_YIELD_TYPE: {
const matchedFiber = existingChildren.get(
newChild.key === null ? newIdx : newChild.key
) || null;
// Yields doesn't have keys, so we neither have to check the old nor
// new node for the key. If both are yields, they match.
const matchedFiber = existingChildren.get(newIdx) || null;
return updateYield(returnFiber, matchedFiber, newChild, priority);
}

Expand Down Expand Up @@ -561,7 +552,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
switch (child.$$typeof) {
case REACT_ELEMENT_TYPE:
case REACT_COROUTINE_TYPE:
case REACT_YIELD_TYPE:
case REACT_PORTAL_TYPE:
const key = child.key;
if (typeof key !== 'string') {
Expand Down Expand Up @@ -1019,34 +1009,22 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
yieldNode : ReactYield,
priority : PriorityLevel
) : Fiber {
const key = yieldNode.key;
// There's no need to check for keys on yields since they're stateless.
let child = currentFirstChild;
while (child) {
// TODO: If key === null and child.key === null, then this only applies to
// the first item in the list.
if (child.key === key) {
if (child.tag === YieldComponent) {
deleteRemainingChildren(returnFiber, child.sibling);
const existing = useFiber(child, priority);
existing.type = createUpdatedReifiedYield(
child.type,
yieldNode
);
existing.return = returnFiber;
return existing;
} else {
deleteRemainingChildren(returnFiber, child);
break;
}
if (child) {
if (child.tag === YieldComponent) {
deleteRemainingChildren(returnFiber, child.sibling);
const existing = useFiber(child, priority);
existing.type = yieldNode.value;
existing.return = returnFiber;
return existing;
} else {
deleteChild(returnFiber, child);
deleteRemainingChildren(returnFiber, child);
}
child = child.sibling;
}

const reifiedYield = createReifiedYield(yieldNode);
const created = createFiberFromYield(yieldNode, priority);
created.type = reifiedYield;
created.type = yieldNode.value;
created.return = returnFiber;
return created;
}
Expand Down
3 changes: 1 addition & 2 deletions src/renderers/shared/fiber/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,7 @@ exports.createFiberFromCoroutine = function(coroutine : ReactCoroutine, priority
};

exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : PriorityLevel) : Fiber {
const fiber = createFiber(YieldComponent, yieldNode.key);
fiber.pendingProps = {};
const fiber = createFiber(YieldComponent, null);
return fiber;
};

Expand Down
47 changes: 44 additions & 3 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -516,13 +516,54 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
}
}
} else if (nextCoroutine === null || workInProgress.memoizedProps === nextCoroutine) {
return bailoutOnAlreadyFinishedWork(current, workInProgress);
nextCoroutine = workInProgress.memoizedProps;
// TODO: When bailing out, we might need to return the stateNode instead
// of the child. To check it for work.
// return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

const nextChildren = nextCoroutine.children;
const priorityLevel = workInProgress.pendingWorkPriority;

// The following is a fork of reconcileChildrenAtPriority but using
// stateNode to store the child.

// At this point any memoization is no longer valid since we'll have changed
// the children.
workInProgress.memoizedProps = null;
if (!current) {
workInProgress.stateNode = mountChildFibersInPlace(
workInProgress,
workInProgress.stateNode,
nextChildren,
priorityLevel
);
} else if (current.child === workInProgress.child) {
clearDeletions(workInProgress);

workInProgress.stateNode = reconcileChildFibers(
workInProgress,
workInProgress.stateNode,
nextChildren,
priorityLevel
);

transferDeletions(workInProgress);
} else {
workInProgress.stateNode = reconcileChildFibersInPlace(
workInProgress,
workInProgress.stateNode,
nextChildren,
priorityLevel
);

transferDeletions(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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: probably need to change workInProgress.child to workInProgress.stateNode in the comment (or drop it)

return workInProgress.child;
return workInProgress.stateNode;
}

function updatePortalComponent(current, workInProgress) {
Expand Down
4 changes: 0 additions & 4 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
while (node.tag !== HostComponent && node.tag !== HostText) {
// If it is not host node and, we might have a host node inside it.
// Try to search down until we find one.
// TODO: For coroutines, this will have to search the stateNode.
if (node.effectTag & Placement) {
// If we don't have a child, try the siblings instead.
continue siblings;
Expand Down Expand Up @@ -198,7 +197,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// down its children. Instead, we'll get insertions from each child in
// the portal directly.
} else if (node.child) {
// TODO: Coroutines need to visit the stateNode.
node.child.return = node;
node = node.child;
continue;
Expand Down Expand Up @@ -229,7 +227,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// Visit children because they may contain more composite or host nodes.
// Skip portals because commitUnmount() currently visits them recursively.
if (node.child && node.tag !== HostPortal) {
// TODO: Coroutines need to visit the stateNode.
node.child.return = node;
node = node.child;
continue;
Expand Down Expand Up @@ -273,7 +270,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
commitUnmount(node);
// Visit children because we may find more host components below.
if (node.child) {
// TODO: Coroutines need to visit the stateNode.
node.child.return = node;
node = node.child;
continue;
Expand Down
34 changes: 22 additions & 12 deletions src/renderers/shared/fiber/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import type { Fiber } from 'ReactFiber';
import type { HostContext } from 'ReactFiberHostContext';
import type { FiberRoot } from 'ReactFiberRoot';
import type { HostConfig } from 'ReactFiberReconciler';
import type { ReifiedYield } from 'ReactReifiedYield';

var { reconcileChildFibers } = require('ReactChildFiber');
var {
Expand Down Expand Up @@ -66,29 +65,40 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
popHostContainer,
} = hostContext;

function markChildAsProgressed(current, workInProgress, priorityLevel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move to child fiber since it's duplicated between phases?

// We now have clones. Let's store them as the currently progressed work.
workInProgress.progressedChild = workInProgress.child;
workInProgress.progressedPriority = priorityLevel;
if (current) {
// We also store it on the current. When the alternate swaps in we can
// continue from this point.
current.progressedChild = workInProgress.progressedChild;
current.progressedPriority = workInProgress.progressedPriority;
}
}

function markUpdate(workInProgress : Fiber) {
// Tag the fiber with an update effect. This turns a Placement into
// an UpdateAndPlacement.
workInProgress.effectTag |= Update;
}

function appendAllYields(yields : Array<ReifiedYield>, workInProgress : Fiber) {
let node = workInProgress.child;
function appendAllYields(yields : Array<mixed>, workInProgress : Fiber) {
let node = workInProgress.stateNode;
if (node) {
node.return = workInProgress;
}
while (node) {
if (node.tag === HostComponent || node.tag === HostText ||
node.tag === HostPortal) {
throw new Error('A coroutine cannot have host component children.');
} else if (node.tag === YieldComponent) {
yields.push(node.type);
} else if (node.child) {
// TODO: Coroutines need to visit the stateNode.
node.child.return = node;
node = node.child;
continue;
}
if (node === workInProgress) {
return;
}
while (!node.sibling) {
if (!node.return || node.return === workInProgress) {
return;
Expand Down Expand Up @@ -117,22 +127,23 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

// Build up the yields.
// TODO: Compare this to a generator or opaque helpers like Children.
var yields : Array<ReifiedYield> = [];
var yields : Array<mixed> = [];
appendAllYields(yields, workInProgress);
var fn = coroutine.handler;
var props = coroutine.props;
var nextChildren = fn(props, yields);

var currentFirstChild = current ? current.stateNode : null;
var currentFirstChild = current ? current.child : null;
// Inherit the priority of the returnFiber.
const priority = workInProgress.pendingWorkPriority;
workInProgress.stateNode = reconcileChildFibers(
workInProgress.child = reconcileChildFibers(
workInProgress,
currentFirstChild,
nextChildren,
priority
);
return workInProgress.stateNode;
markChildAsProgressed(current, workInProgress, priority);
return workInProgress.child;
}

function appendAllChildren(parent : I, workInProgress : Fiber) {
Expand All @@ -147,7 +158,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// down its children. Instead, we'll get insertions from each child in
// the portal directly.
} else if (node.child) {
// TODO: Coroutines need to visit the stateNode.
node = node.child;
continue;
}
Expand Down
2 changes: 2 additions & 0 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,8 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
newPriority = getPendingPriority(queue);
}

// TODO: Coroutines need to visit stateNode

// progressedChild is going to be the child set with the highest priority.
// Either it is the same as child, or it just bailed out because it choose
// not to do the work.
Expand Down
1 change: 0 additions & 1 deletion src/renderers/shared/fiber/ReactFiberTreeReflection.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ exports.findCurrentHostFiber = function(parent : Fiber) : Fiber | null {
return node;
} else if (node.child) {
// TODO: If we hit a Portal, we're supposed to skip it.
// TODO: Coroutines need to visit the stateNode.
node.child.return = node;
node = node.child;
continue;
Expand Down
Loading