Don't block deep low pri updates when bailing out
First I fix a bug where host components didn't properly bail out although this was unobservable. When we bail out, we need to ensure that we preserve the highest remaining priority work that is left to do for that subtree. This still isn't properly handling the case when that work has the *same* priority as the current one. That work will be flushed the *next* tick instead of the current pass. I can't create a test for that yet since I need setState to get to that state.
Sebastian Markbage committed
Jun 19, 2016 at 00:02 UTC
5971411e873ac777adb6e3c458720ba1499f9dce
4 files changed
+103
-6
src/renderers/shared/fiber/ReactFiberBeginWork.js
+29
-4
@@ -114,6 +114,26 @@ function updateCoroutineComponent(current, workInProgress) {
114
workInProgress.pendingWorkPriority = NoWork;
115
}
116
117
+function reuseChildren(newParent : Fiber, firstChild : Fiber) {
118
+ // TODO: None of this should be necessary if structured better.
119
+ // The parent pointer only needs to be updated when we walk into this child
120
+ // which we don't do right now. If the pending work priority indicated only
121
+ // if a child has work rather than if the node has work, then we would know
122
+ // by a single lookup on workInProgress rather than having to go through
123
+ // each child.
124
+ let child = firstChild;
125
+ do {
126
+ // Update the parent of the child to the newest fiber.
127
+ child.parent = newParent;
128
+ // Retain the priority if there's any work left to do in the children.
129
+ if (child.pendingWorkPriority !== NoWork &&
130
+ (newParent.pendingWorkPriority === NoWork ||
131
+ newParent.pendingWorkPriority > child.pendingWorkPriority)) {
132
+ newParent.pendingWorkPriority = child.pendingWorkPriority;
133
+ }
134
+ } while (child = child.sibling);
135
+}
136
+
137
function beginWork(current : ?Fiber, workInProgress : Fiber) : ?Fiber {
138
// The current, flushed, state of this fiber is the alternate.
139
// Ideally nothing should rely on this, but relying on it here
@@ -123,18 +143,23 @@ function beginWork(current : ?Fiber, workInProgress : Fiber) : ?Fiber {
143
// The most likely scenario is that the previous copy of the tree contains
144
// the same props as the new one. In that case, we can just copy the output
145
// and children from that node.
146
+ workInProgress.memoizedProps = workInProgress.pendingProps;
147
workInProgress.output = current.output;
127
- workInProgress.child = current.child;
128
- if (workInProgress.child) {
129
- workInProgress.child.parent = workInProgress;
148
+ workInProgress.pendingWorkPriority = NoWork;
149
+ if (current.child) {
150
+ workInProgress.child = current.child;
151
+ reuseChildren(workInProgress, workInProgress.child);
152
+ } else {
153
+ workInProgress.child = null;
154
}
155
workInProgress.stateNode = current.stateNode;
132
- workInProgress.pendingWorkPriority = NoWork;
156
return null;
157
}
158
if (workInProgress.pendingProps === workInProgress.memoizedProps) {
159
// In a ping-pong scenario, this version could actually contain the
160
// old props. In that case, we can just bail out.
161
+ // TODO: I don't yet quite understand what should happen on the
162
+ // reused node here.
163
return null;
164
}
165
switch (workInProgress.tag) {
src/renderers/shared/fiber/ReactFiberCompleteWork.js
+1
@@ -107,6 +107,7 @@ exports.completeWork = function(current : ?Fiber, workInProgress : Fiber) : ?Fib
107
case HostContainer:
108
return null;
109
case HostComponent:
110
+ transferOutput(workInProgress.child, workInProgress);
111
console.log('/host component', workInProgress.type);
112
return null;
113
case CoroutineComponent:
src/renderers/shared/fiber/ReactFiberScheduler.js
+5
@@ -157,6 +157,10 @@ module.exports = function<T, P, I>(config : HostConfig<T, P, I>) {
157
const current = workInProgress.alternate;
158
const next = completeWork(current, workInProgress);
159
160
+ // TODO: If workInProgress returns null but still has work with the
161
+ // current priority in its subtree, we need to go find it right now.
162
+ // If we don't, we won't flush it until the next tick.
163
+
164
// The work is now done. We don't need this anymore. This flags
165
// to the system not to redo any work here.
166
workInProgress.pendingProps = null;
@@ -192,6 +196,7 @@ module.exports = function<T, P, I>(config : HostConfig<T, P, I>) {
196
// "next" scheduled work since we've already scanned passed. That
197
// also ensures that work scheduled during reconciliation gets deferred.
198
// const hasMoreWork = workInProgress.pendingWorkPriority !== NoWork;
199
+ console.log('----- COMPLETED with remaining work:', workInProgress.pendingWorkPriority);
200
const nextWork = findNextUnitOfWork();
201
// if (!nextWork && hasMoreWork) {
202
// TODO: This can happen when some deep work completes and we don't
src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+68
-2
@@ -109,6 +109,8 @@ describe('ReactIncremental', function() {
109
ReactNoop.render(<Foo text="bar" />);
110
ReactNoop.flush();
111
112
+ // TODO: Test bail out of host components. This is currently unobservable.
113
+
114
// Since this is an update, it should bail out and reuse the work from
115
// Header and Content.
116
expect(ops).toEqual(['Foo', 'Content']);
@@ -182,9 +184,9 @@ describe('ReactIncremental', function() {
184
return (
185
<div>
186
<Bar>{props.text}</Bar>
185
- <content hidden={true}>
187
+ <section hidden={true}>
188
<Middle>{props.text}</Middle>
187
- </content>
189
+ </section>
190
<Bar>{props.text}</Bar>
191
<footer hidden={true}>
192
<Middle>Footer</Middle>
@@ -217,4 +219,68 @@ describe('ReactIncremental', function() {
219
220
});
221
222
+ it('can resume work in a subtree even when a parent bails out', function() {
223
+
224
+ var ops = [];
225
+
226
+ function Bar(props) {
227
+ ops.push('Bar');
228
+ return <div>{props.children}</div>;
229
+ }
230
+
231
+ function Tester() {
232
+ // This component is just here to ensure that the bail out is
233
+ // in fact in effect in the expected place for this test.
234
+ ops.push('Tester');
235
+ return <div />;
236
+ }
237
+
238
+ function Middle(props) {
239
+ ops.push('Middle');
240
+ return <span>{props.children}</span>;
241
+ }
242
+
243
+ var middleContent = (
244
+ <aaa>
245
+ <Tester />
246
+ <bbb hidden={true}>
247
+ <ccc>
248
+ <Middle>Hi</Middle>
249
+ </ccc>
250
+ </bbb>
251
+ </aaa>
252
+ );
253
+
254
+ function Foo(props) {
255
+ ops.push('Foo');
256
+ return (
257
+ <div>
258
+ <Bar>{props.text}</Bar>
259
+ {middleContent}
260
+ <Bar>{props.text}</Bar>
261
+ </div>
262
+ );
263
+ }
264
+
265
+ // Init
266
+ ReactNoop.render(<Foo text="foo" />);
267
+ ReactNoop.flushLowPri(52);
268
+
269
+ expect(ops).toEqual(['Foo', 'Bar', 'Tester', 'Bar']);
270
+
271
+ ops = [];
272
+
273
+ // We're now rendering an update that will bail out on updating middle.
274
+ ReactNoop.render(<Foo text="bar" />);
275
+ ReactNoop.flushLowPri(45);
276
+
277
+ expect(ops).toEqual(['Foo', 'Bar', 'Bar']);
278
+
279
+ ops = [];
280
+
281
+ // Flush the rest to make sure that the bailout didn't block this work.
282
+ ReactNoop.flush();
283
+ expect(ops).toEqual(['Middle']);
284
+
285
+ });
286
});