@samitouri / QOS-React / commits / 0ba8434a33

[Fiber] Remove output field (#8406)

* Remove output field The idea was originally that each fiber has a return value. In practice most of what we're modelling here are void functions and we track side effects instead of return values. We do use this for coroutines to short cut access to terminal yields. However, since this can be nested fragments we end up searching the tree anyway. We also have to manage this in transferOutput so it ends up being as expensive. Maybe we can save some traversal for updates when coroutine branches bail out but meh. * Unmount child from the first phase of a coroutine

Sebastian Markbåge committed Nov 24, 2016 at 10:03 UTC 0ba8434a33a23c12e3ad0e8a0ccfce72d4c4ddd4

8 files changed +116 -63

scripts/fiber/tests-passing.txt

@@ -1031,6 +1031,7 @@ src/renderers/shared/__tests__/ReactPerf-test.js

1031

1032 src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js

1033 * should render a coroutine

1034 +* should unmount a composite in a coroutine

1035

1036 src/renderers/shared/fiber/__tests__/ReactIncremental-test.js

1037 * should render a simple component

src/renderers/shared/fiber/ReactChildFiber.js

+7 -7

@@ -294,14 +294,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

294 // Insert

295 const reifiedYield = createReifiedYield(yieldNode);

296 const created = createFiberFromYield(yieldNode, priority);

297 - created.output = reifiedYield;

297 + created.type = reifiedYield;

298 created.return = returnFiber;

299 return created;

300 } else {

301 // Move based on index

302 const existing = useFiber(current, priority);

303 - existing.output = createUpdatedReifiedYield(

304 - current.output,

303 + existing.type = createUpdatedReifiedYield(

304 + current.type,

305 yieldNode

306 );

307 existing.return = returnFiber;

@@ -386,7 +386,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

386 case REACT_YIELD_TYPE: {

387 const reifiedYield = createReifiedYield(newChild);

388 const created = createFiberFromYield(newChild, priority);

389 - created.output = reifiedYield;

389 + created.type = reifiedYield;

390 created.return = returnFiber;

391 return created;

392 }

@@ -790,8 +790,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

790 if (child.tag === YieldComponent) {

791 deleteRemainingChildren(returnFiber, child.sibling);

792 const existing = useFiber(child, priority);

793 - existing.output = createUpdatedReifiedYield(

794 - child.output,

793 + existing.type = createUpdatedReifiedYield(

794 + child.type,

795 yieldNode

796 );

797 existing.return = returnFiber;

@@ -808,7 +808,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

808

809 const reifiedYield = createReifiedYield(yieldNode);

810 const created = createFiberFromYield(yieldNode, priority);

811 - created.output = reifiedYield;

811 + created.type = reifiedYield;

812 created.return = returnFiber;

813 return created;

814 }

src/renderers/shared/fiber/ReactFiber.js

-5

@@ -97,9 +97,6 @@ export type Fiber = {

97 memoizedState: any,

98 // Linked list of callbacks to call after updates are committed.

99 callbackList: ?UpdateQueue,

100 - // Output is the return value of this fiber, or a linked list of return values

101 - // if this returns multiple values. Such as a fragment.

102 - output: any, // This type will be more specific once we overload the tag.

100

101 // Effect

102 effectTag: TypeOfSideEffect,

@@ -190,7 +187,6 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber {

187 updateQueue: null,

188 memoizedState: null,

189 callbackList: null,

193 - output: null,

190

191 effectTag: NoEffect,

192 nextEffect: null,

@@ -267,7 +263,6 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi

263

264 alt.memoizedProps = fiber.memoizedProps;

265 alt.memoizedState = fiber.memoizedState;

270 - alt.output = fiber.output;

266

267 return alt;

268 };

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -21,6 +21,7 @@ var {

21 HostContainer,

22 HostComponent,

23 HostText,

24 + CoroutineComponent,

25 Portal,

26 } = ReactTypeOfWork;

27 var { callCallbacks } = require('ReactFiberUpdateQueue');

@@ -278,6 +279,10 @@ module.exports = function<T, P, I, TI, C>(

279 detachRef(current);

280 return;

281 }

282 + case CoroutineComponent: {

283 + commitNestedUnmounts(current.stateNode);

284 + return;

285 + }

286 }

287 }

288

src/renderers/shared/fiber/ReactFiberCompleteWork.js

+30 -39

@@ -62,28 +62,31 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

62 workInProgress.effectTag |= Callback;

63 }

65 - function transferOutput(child : ?Fiber, returnFiber : Fiber) {

66 - // If we have a single result, we just pass that through as the output to

67 - // avoid unnecessary traversal. When we have multiple output, we just pass

68 - // the linked list of fibers that has the individual output values.

69 - returnFiber.output = (child && !child.sibling) ? child.output : child;

70 - returnFiber.memoizedProps = returnFiber.pendingProps;

71 - }

72 -

73 - function recursivelyFillYields(yields, output : ?Fiber | ?ReifiedYield) {

74 - if (!output) {

75 - // Ignore nulls etc.

76 - } else if (output.tag !== undefined) { // TODO: Fix this fragile duck test.

77 - // Detect if this is a fiber, if so it is a fragment result.

78 - // $FlowFixMe: Refinement issue.

79 - var item = (output : Fiber);

80 - do {

81 - recursivelyFillYields(yields, item.output);

82 - item = item.sibling;

83 - } while (item);

84 - } else {

85 - // $FlowFixMe: Refinement issue. If it is not a Fiber or null, it is a yield

86 - yields.push(output);

65 + function appendAllYields(yields : Array<ReifiedYield>, workInProgress : Fiber) {

66 + let node = workInProgress.child;

67 + while (node) {

68 + if (node.tag === HostComponent || node.tag === HostText ||

69 + node.tag === Portal) {

70 + throw new Error('A coroutine cannot have host component children.');

71 + } else if (node.tag === YieldComponent) {

72 + yields.push(node.type);

73 + } else if (node.child) {

74 + // TODO: Coroutines need to visit the stateNode.

75 + node.child.return = node;

76 + node = node.child;

77 + continue;

78 + }

79 + if (node === workInProgress) {

80 + return;

81 + }

82 + while (!node.sibling) {

83 + if (!node.return || node.return === workInProgress) {

84 + return;

85 + }

86 + node = node.return;

87 + }

88 + node.sibling.return = node.return;

89 + node = node.sibling;

90 }

91 }

@@ -105,11 +108,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

108 // Build up the yields.

109 // TODO: Compare this to a generator or opaque helpers like Children.

110 var yields : Array<ReifiedYield> = [];

108 - var child = workInProgress.child;

109 - while (child) {

110 - recursivelyFillYields(yields, child.output);

111 - child = child.sibling;

112 - }

111 + appendAllYields(yields, workInProgress);

112 var fn = coroutine.handler;

113 var props = coroutine.props;

114 var nextChildren = fn(props, yields);

@@ -158,10 +157,9 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

157 function completeWork(current : ?Fiber, workInProgress : Fiber) : ?Fiber {

158 switch (workInProgress.tag) {

159 case FunctionalComponent:

161 - transferOutput(workInProgress.child, workInProgress);

160 + workInProgress.memoizedProps = workInProgress.pendingProps;

161 return null;

162 case ClassComponent:

164 - transferOutput(workInProgress.child, workInProgress);

163 // We are leaving this subtree, so pop context if any.

164 if (isContextProvider(workInProgress)) {

165 popContextProvider();

@@ -191,7 +189,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

189 }

190 return null;

191 case HostContainer: {

194 - transferOutput(workInProgress.child, workInProgress);

192 + workInProgress.memoizedProps = workInProgress.pendingProps;

193 popContextProvider();

194 const fiberRoot = (workInProgress.stateNode : FiberRoot);

195 if (fiberRoot.pendingContext) {

@@ -221,8 +219,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

219 // This returns true if there was something to update.

220 markUpdate(workInProgress);

221 }

224 - // TODO: Is this actually ever going to change? Why set it every time?

225 - workInProgress.output = instance;

222 } else {

223 if (!newProps) {

224 if (workInProgress.stateNode === null) {

@@ -242,9 +238,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

238 appendAllChildren(instance, workInProgress);

239 finalizeInitialChildren(instance, workInProgress.type, newProps);

240

245 - // TODO: This seems like unnecessary duplication.

241 workInProgress.stateNode = instance;

247 - workInProgress.output = instance;

242 if (workInProgress.ref) {

243 // If there is a ref on a host node we need to schedule a callback

244 markUpdate(workInProgress);

@@ -268,16 +262,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

262 }

263 }

264 const textInstance = createTextInstance(newText, workInProgress);

271 - // TODO: This seems like unnecessary duplication.

265 workInProgress.stateNode = textInstance;

273 - workInProgress.output = textInstance;

266 }

267 workInProgress.memoizedProps = newText;

268 return null;

269 case CoroutineComponent:

270 return moveCoroutineToHandlerPhase(current, workInProgress);

271 case CoroutineHandlerPhase:

280 - transferOutput(workInProgress.stateNode, workInProgress);

272 + workInProgress.memoizedProps = workInProgress.pendingProps;

273 // Reset the tag to now be a first phase coroutine.

274 workInProgress.tag = CoroutineComponent;

275 return null;

@@ -285,12 +277,11 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

277 // Does nothing.

278 return null;

279 case Fragment:

288 - transferOutput(workInProgress.child, workInProgress);

280 + workInProgress.memoizedProps = workInProgress.pendingProps;

281 return null;

282 case Portal:

283 // TODO: Only mark this as an update if we have any pending callbacks.

284 markUpdate(workInProgress);

293 - workInProgress.output = null;

285 workInProgress.memoizedProps = workInProgress.pendingProps;

286 return null;

287

src/renderers/shared/fiber/ReactFiberReconciler.js

-5

@@ -14,7 +14,6 @@

15 import type { Fiber } from 'ReactFiber';

16 import type { FiberRoot } from 'ReactFiberRoot';

17 -import type { TypeOfWork } from 'ReactTypeOfWork';

17 import type { PriorityLevel } from 'ReactPriorityLevel';

19 var {

@@ -39,10 +38,6 @@ export type Deadline = {

38 timeRemaining : () => number

39 };

42 -type HostChildNode<I> = { tag: TypeOfWork, output: HostChildren<I>, sibling: any };

43 -

44 -export type HostChildren<I> = null | void | I | HostChildNode<I>;

45 -

41 type OpaqueNode = Fiber;

43 export type HostConfig<T, P, I, TI, C> = {

src/renderers/shared/fiber/ReactFiberScheduler.js

+7 -4

@@ -286,6 +286,12 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

286 workInProgress.pendingProps = null;

287 workInProgress.updateQueue = null;

288

289 + if (next) {

290 + // If completing this work spawned new work, do that next. We'll come

291 + // back here again.

292 + return next;

293 + }

294 +

295 const returnFiber = workInProgress.return;

296

297 if (returnFiber) {

@@ -318,10 +324,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

324 }

325 }

326

321 - if (next) {

322 - // If completing this work spawned new work, do that next.

323 - return next;

324 - } else if (workInProgress.sibling) {

327 + if (workInProgress.sibling) {

328 // If there is more work to do in this returnFiber, do that next.

329 return workInProgress.sibling;

330 } else if (returnFiber) {

src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js

+66 -3

@@ -24,10 +24,8 @@ describe('ReactCoroutine', () => {

24 });

26 it('should render a coroutine', () => {

27 -

27 var ops = [];

30 -

29 function Continuation({ isSame }) {

30 ops.push(['Continuation', isSame]);

31 return <span>{isSame ? 'foo==bar' : 'foo!=bar'}</span>;

@@ -84,7 +82,72 @@ describe('ReactCoroutine', () => {

82 ['Continuation', true],

83 ['Continuation', false],

84 ]);

87 -

85 });

87 + it('should unmount a composite in a coroutine', () => {

88 + var ops = [];

89 +

90 + class Continuation extends React.Component {

91 + render() {

92 + ops.push('Continuation');

93 + return <div />;

94 + }

95 + componentWillUnmount() {

96 + ops.push('Unmount Continuation');

97 + }

98 + }

99 +

100 + class Child extends React.Component {

101 + render() {

102 + ops.push('Child');

103 + return ReactCoroutine.createYield({}, Continuation, null);

104 + }

105 + componentWillUnmount() {

106 + ops.push('Unmount Child');

107 + }

108 + }

109 +

110 + function HandleYields(props, yields) {

111 + ops.push('HandleYields');

112 + return yields.map(y => <y.continuation />);

113 + }

114 +

115 + class Parent extends React.Component {

116 + render() {

117 + ops.push('Parent');

118 + return ReactCoroutine.createCoroutine(

119 + this.props.children,

120 + HandleYields,

121 + this.props

122 + );

123 + }

124 + componentWillUnmount() {

125 + ops.push('Unmount Parent');

126 + }

127 + }

128 +

129 + ReactNoop.render(<Parent><Child /></Parent>);

130 + ReactNoop.flush();

131 +

132 + expect(ops).toEqual([

133 + 'Parent',

134 + 'Child',

135 + 'HandleYields',

136 + 'Continuation',

137 + ]);

138 +

139 + ops = [];

140 +

141 + ReactNoop.render(<div />);

142 + ReactNoop.flush();

143 +

144 + expect(ops).toEqual([

145 + 'Unmount Parent',

146 + // TODO: This should happen in the order Child, Continuation which it

147 + // will once we swap stateNode and child positions of these.

148 + 'Unmount Continuation',

149 + 'Unmount Child',

150 + ]);

151 +

152 + });

153 });