Move priority reset to the end and search pending work in work tree
The priority level gets reset at the wrong time because I rely on mutating it at the wrong point. This moves it to the end of completed work as a second pass over the children to see what the highest priority is. This is inefficient for large sets but we can try to find a smarter way to do this later. E.g. passing it down the stack. This bug fix revealed another bug that I had flagged before that we're finding work to do in the "current" tree instead of the working tree. For trees that were paused, e.g. childInProgress trees, this won't work since don't have a current tree to search. Therefore I fixed findNextUnitOfWorkAtPriority to use workInProgress instead of current.
Sebastian Markbage committed
Aug 8, 2016 at 19:22 UTC
9e52130fb7cfd584993f3745a860a6cf7e037300
6 files changed
+117
-91
src/renderers/shared/fiber/ReactChildFiber.js
+2
@@ -63,6 +63,7 @@ function ChildReconciler(shouldClone) {
63
// Will fix reconciliation properly later.
64
const clone = shouldClone ? cloneFiber(existingChild, priority) : existingChild;
65
if (!shouldClone) {
66
+ // TODO: This might be lowering the priority of nested unfinished work.
67
clone.pendingWorkPriority = priority;
68
}
69
clone.pendingProps = element.props;
@@ -134,6 +135,7 @@ function ChildReconciler(shouldClone) {
135
// Get the clone of the existing fiber.
136
const clone = shouldClone ? cloneFiber(existingChild, priority) : existingChild;
137
if (!shouldClone) {
138
+ // TODO: This might be lowering the priority of nested unfinished work.
139
clone.pendingWorkPriority = priority;
140
}
141
clone.pendingProps = element.props;
src/renderers/shared/fiber/ReactFiber.js
+6
@@ -168,6 +168,9 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
168
alt.pendingProps = fiber.pendingProps;
169
alt.pendingWorkPriority = priorityLevel;
170
171
+ alt.memoizedProps = fiber.memoizedProps;
172
+ alt.output = fiber.output;
173
+
174
// Whenever we clone, we do so to get a new work in progress.
175
// This ensures that we've reset these in the new tree.
176
alt.nextEffect = null;
@@ -189,6 +192,9 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
192
alt.pendingProps = fiber.pendingProps;
193
alt.pendingWorkPriority = priorityLevel;
194
195
+ alt.memoizedProps = fiber.memoizedProps;
196
+ alt.output = fiber.output;
197
+
198
alt.alternate = fiber;
199
fiber.alternate = alt;
200
return alt;
src/renderers/shared/fiber/ReactFiberBeginWork.js
+10
-24
@@ -40,6 +40,8 @@ var { findNextUnitOfWorkAtPriority } = require('ReactFiberPendingWork');
40
module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
41
42
function reconcileChildren(current, workInProgress, nextChildren) {
43
+ // TODO: Children needs to be able to reconcile in place if we are
44
+ // overriding progressed work.
45
const priority = workInProgress.pendingWorkPriority;
46
reconcileChildrenAtPriority(current, workInProgress, nextChildren, priority);
47
}
@@ -76,7 +78,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
78
var props = workInProgress.pendingProps;
79
var nextChildren = fn(props);
80
reconcileChildren(current, workInProgress, nextChildren);
79
- workInProgress.pendingWorkPriority = NoWork;
81
}
82
83
function updateClassComponent(current : ?Fiber, workInProgress : Fiber) {
@@ -106,7 +107,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
107
instance.props = props;
108
var nextChildren = instance.render();
109
reconcileChildren(current, workInProgress, nextChildren);
109
- workInProgress.pendingWorkPriority = NoWork;
110
return workInProgress.childInProgress;
111
}
112
@@ -125,11 +125,9 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
125
// becomes part of the render tree, even though it never completed. Its
126
// `output` property is unpredictable because of it.
127
reconcileChildrenAtPriority(current, workInProgress, nextChildren, OffscreenPriority);
128
- workInProgress.pendingWorkPriority = OffscreenPriority;
128
return null;
129
} else {
130
reconcileChildren(current, workInProgress, nextChildren);
132
- workInProgress.pendingWorkPriority = NoWork;
131
return workInProgress.childInProgress;
132
}
133
}
@@ -153,7 +151,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
151
}
152
}
153
reconcileChildren(current, workInProgress, value);
156
- workInProgress.pendingWorkPriority = NoWork;
154
}
155
156
function updateCoroutineComponent(current, workInProgress) {
@@ -162,10 +159,11 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
159
throw new Error('Should be resolved by now');
160
}
161
reconcileChildren(current, workInProgress, coroutine.children);
165
- workInProgress.pendingWorkPriority = NoWork;
162
}
163
164
function reuseChildren(returnFiber : Fiber, firstChild : Fiber) {
165
+ // TODO on the TODO: Is this not necessary anymore because I moved the
166
+ // priority reset?
167
// TODO: None of this should be necessary if structured better.
168
// The returnFiber pointer only needs to be updated when we walk into this child
169
// which we don't do right now. If the pending work priority indicated only
@@ -210,7 +208,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
208
workInProgress.output = current.output;
209
const priorityLevel = workInProgress.pendingWorkPriority;
210
workInProgress.pendingProps = null;
213
- workInProgress.pendingWorkPriority = NoWork;
211
workInProgress.stateNode = current.stateNode;
212
workInProgress.childInProgress = current.childInProgress;
213
if (current.child) {
@@ -220,10 +217,10 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
217
workInProgress.child = current.child;
218
reuseChildren(workInProgress, workInProgress.child);
219
if (workInProgress.pendingWorkPriority !== NoWork && workInProgress.pendingWorkPriority <= priorityLevel) {
223
- // TODO: This passes the current node and reads the priority level and
224
- // pending props from that. We want it to read our priority level and
225
- // pending props from the work in progress. Needs restructuring.
226
- return findNextUnitOfWorkAtPriority(current, priorityLevel);
220
+ return findNextUnitOfWorkAtPriority(
221
+ workInProgress,
222
+ workInProgress.pendingWorkPriority
223
+ );
224
} else {
225
return null;
226
}
@@ -239,22 +236,13 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
236
// looking for. In that case, we should be able to just bail out.
237
const priorityLevel = workInProgress.pendingWorkPriority;
238
workInProgress.pendingProps = null;
242
- workInProgress.pendingWorkPriority = NoWork;
239
240
workInProgress.firstEffect = null;
241
workInProgress.nextEffect = null;
242
workInProgress.lastEffect = null;
243
248
- if (workInProgress.child) {
249
- // On the way up here, we reset the child node to be the current one by
250
- // cloning. However, it is really the original child that represents the
251
- // already completed work. Therefore we have to reuse the alternate.
252
- // But if we don't have a current, this was not cloned. This is super weird.
253
- const child = !current ? workInProgress.child : workInProgress.child.alternate;
254
- if (!child) {
255
- throw new Error('We must have a current child to be able to use this.');
256
- }
257
- workInProgress.child = child;
244
+ const child = workInProgress.child;
245
+ if (child) {
246
// Ensure that the effects of reused work are preserved.
247
reuseChildrenEffects(workInProgress, child);
248
// If we bail out but still has work with the current priority in this
@@ -299,7 +287,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
287
reconcileChildren(current, workInProgress, workInProgress.pendingProps);
288
// A yield component is just a placeholder, we can just run through the
289
// next one immediately.
302
- workInProgress.pendingWorkPriority = NoWork;
290
if (workInProgress.childInProgress) {
291
return beginWork(
292
workInProgress.childInProgress.alternate,
@@ -327,7 +314,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
314
case YieldComponent:
315
// A yield component is just a placeholder, we can just run through the
316
// next one immediately.
330
- workInProgress.pendingWorkPriority = NoWork;
317
if (workInProgress.sibling) {
318
return beginWork(
319
workInProgress.sibling.alternate,
src/renderers/shared/fiber/ReactFiberPendingWork.js
+73
-57
@@ -34,20 +34,50 @@ function cloneSiblings(current : Fiber, workInProgress : Fiber, returnFiber : Fi
34
workInProgress.sibling = null;
35
}
36
37
-exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLevel : PriorityLevel) : ?Fiber {
38
- let current = currentRoot;
39
- while (current) {
40
- if (current.pendingWorkPriority !== NoWork &&
41
- current.pendingWorkPriority <= priorityLevel) {
37
+function cloneChildrenIfNeeded(workInProgress : Fiber) {
38
+ const current = workInProgress.alternate;
39
+ if (!current || workInProgress.child !== current.child) {
40
+ // If there is no alternate, then we don't need to clone the children.
41
+ // If the children of the alternate fiber is a different set, then we don't
42
+ // need to clone. We need to reset the return fiber though since we'll
43
+ // traverse down into them.
44
+ // TODO: I don't think it is actually possible for them to be anything but
45
+ // equal at this point because this fiber was just cloned. Can we skip this
46
+ // check? Similar question about the return fiber.
47
+ let child = workInProgress.child;
48
+ while (child) {
49
+ child.return = workInProgress;
50
+ child = child.sibling;
51
+ }
52
+ return;
53
+ }
54
+ // TODO: This used to reset the pending priority. Not sure if that is needed.
55
+ // workInProgress.pendingWorkPriority = current.pendingWorkPriority;
56
+
57
+ // TODO: The below priority used to be set to NoWork which would've
58
+ // dropped work. This is currently unobservable but will become
59
+ // observable when the first sibling has lower priority work remaining
60
+ // than the next sibling. At that point we should add tests that catches
61
+ // this.
62
+
63
+ const currentChild = current.child;
64
+ if (!currentChild) {
65
+ return;
66
+ }
67
+ workInProgress.child = cloneFiber(
68
+ currentChild,
69
+ currentChild.pendingWorkPriority
70
+ );
71
+ cloneSiblings(currentChild, workInProgress.child, workInProgress);
72
+}
73
+
74
+exports.findNextUnitOfWorkAtPriority = function(workRoot : Fiber, priorityLevel : PriorityLevel) : ?Fiber {
75
+ let workInProgress = workRoot;
76
+ while (workInProgress) {
77
+ if (workInProgress.pendingWorkPriority !== NoWork &&
78
+ workInProgress.pendingWorkPriority <= priorityLevel) {
79
// This node has work to do that fits our priority level criteria.
43
- if (current.pendingProps !== null) {
44
- // We found some work to do. We need to return the "work in progress"
45
- // of this node which will be the alternate.
46
- const workInProgress = current.alternate;
47
- if (!workInProgress) {
48
- throw new Error('Should have wip now');
49
- }
50
- workInProgress.pendingProps = current.pendingProps;
80
+ if (workInProgress.pendingProps !== null) {
81
return workInProgress;
82
}
83
@@ -56,71 +86,57 @@ exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLev
86
// because it is the highest priority for the whole subtree.
87
// TODO: Coroutines can have work in their stateNode which is another
88
// type of child that needs to be searched for work.
59
- if (current.childInProgress) {
60
- let workInProgress = current.childInProgress;
61
- while (workInProgress) {
62
- workInProgress.return = current.alternate;
63
- workInProgress = workInProgress.sibling;
89
+ if (workInProgress.childInProgress) {
90
+ let child = workInProgress.childInProgress;
91
+ while (child) {
92
+ child.return = workInProgress;
93
+ child = child.sibling;
94
}
65
- workInProgress = current.childInProgress;
66
- while (workInProgress) {
67
- // Don't bother drilling further down this tree if there is no child.
68
- if (workInProgress.pendingWorkPriority !== NoWork &&
69
- workInProgress.pendingWorkPriority <= priorityLevel &&
70
- workInProgress.pendingProps !== null) {
71
- return workInProgress;
95
+ child = workInProgress.childInProgress;
96
+ while (child) {
97
+ // Don't bother drilling further down this tree if there is no child
98
+ // with more content.
99
+ // TODO: Shouldn't this still drill down even though the first
100
+ // shallow level doesn't have anything pending on it.
101
+ if (child.pendingWorkPriority !== NoWork &&
102
+ child.pendingWorkPriority <= priorityLevel &&
103
+ child.pendingProps !== null) {
104
+ return child;
105
}
73
- workInProgress = workInProgress.sibling;
106
+ child = child.sibling;
107
}
75
- } else if (current.child) {
76
- let currentChild = current.child;
77
- currentChild.return = current;
78
- // Ensure we have a work in progress copy to backtrack through.
79
- let workInProgress = current.alternate;
80
- if (!workInProgress) {
81
- throw new Error('Should have wip now');
82
- }
83
- workInProgress.pendingWorkPriority = current.pendingWorkPriority;
84
- // TODO: The below priority used to be set to NoWork which would've
85
- // dropped work. This is currently unobservable but will become
86
- // observable when the first sibling has lower priority work remaining
87
- // than the next sibling. At that point we should add tests that catches
88
- // this.
89
- workInProgress.child = cloneFiber(
90
- currentChild,
91
- currentChild.pendingWorkPriority
92
- );
93
- cloneSiblings(currentChild, workInProgress.child, workInProgress);
94
- current = currentChild;
108
+ } else if (workInProgress.child) {
109
+ cloneChildrenIfNeeded(workInProgress);
110
+ workInProgress = workInProgress.child;
111
continue;
112
}
113
// If we match the priority but has no child and no work to do,
114
// then we can safely reset the flag.
99
- current.pendingWorkPriority = NoWork;
115
+ workInProgress.pendingWorkPriority = NoWork;
116
}
101
- if (current === currentRoot) {
102
- if (current.pendingWorkPriority <= priorityLevel) {
117
+ if (workInProgress === workRoot) {
118
+ if (workInProgress.pendingWorkPriority <= priorityLevel) {
119
// If this subtree had work left to do, we would have returned it by
120
// now. This could happen if a child with pending work gets cleaned up
121
// but we don't clear the flag then. It is safe to reset it now.
106
- current.pendingWorkPriority = NoWork;
122
+ workInProgress.pendingWorkPriority = NoWork;
123
}
124
return null;
125
}
110
- while (!current.sibling) {
111
- current = current.return;
112
- if (!current) {
126
+ while (!workInProgress.sibling) {
127
+ workInProgress = workInProgress.return;
128
+ if (!workInProgress || workInProgress === workRoot) {
129
return null;
130
}
115
- if (current.pendingWorkPriority <= priorityLevel) {
131
+ if (workInProgress.pendingWorkPriority <= priorityLevel) {
132
// If this subtree had work left to do, we would have returned it by
133
// now. This could happen if a child with pending work gets cleaned up
134
// but we don't clear the flag then. It is safe to reset it now.
119
- current.pendingWorkPriority = NoWork;
135
+ workInProgress.pendingWorkPriority = NoWork;
136
}
137
}
122
- current.sibling.return = current.return;
123
- current = current.sibling;
138
+ workInProgress.sibling.return = workInProgress.return;
139
+ workInProgress = workInProgress.sibling;
140
}
141
return null;
142
};
src/renderers/shared/fiber/ReactFiberReconciler.js
+2
@@ -70,6 +70,8 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) : Reconci
70
const root = createFiberRoot(containerInfo);
71
const container = root.current;
72
// TODO: Use pending work/state instead of props.
73
+ // TODO: This should not override the pendingWorkPriority if there is
74
+ // higher priority work in the subtree.
75
container.pendingProps = element;
76
container.pendingWorkPriority = LowPriority;
77
src/renderers/shared/fiber/ReactFiberScheduler.js
+24
-10
@@ -66,22 +66,25 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
66
// roots for high priority work before moving on to lower priorities.
67
let root = nextScheduledRoot;
68
while (root) {
69
- cloneFiber(root.current, root.current.pendingWorkPriority);
69
+ let rootInProgress = cloneFiber(
70
+ root.current,
71
+ root.current.pendingWorkPriority
72
+ );
73
// Find the highest possible priority work to do.
74
// This loop is unrolled just to satisfy Flow's enum constraint.
75
// We could make arbitrary many idle priority levels but having
76
// too many just means flushing changes too often.
74
- let work = findNextUnitOfWorkAtPriority(root.current, HighPriority);
77
+ let work = findNextUnitOfWorkAtPriority(rootInProgress, HighPriority);
78
if (work) {
79
nextPriorityLevel = HighPriority;
80
return work;
81
}
79
- work = findNextUnitOfWorkAtPriority(root.current, LowPriority);
82
+ work = findNextUnitOfWorkAtPriority(rootInProgress, LowPriority);
83
if (work) {
84
nextPriorityLevel = LowPriority;
85
return work;
86
}
84
- work = findNextUnitOfWorkAtPriority(root.current, OffscreenPriority);
87
+ work = findNextUnitOfWorkAtPriority(rootInProgress, OffscreenPriority);
88
if (work) {
89
nextPriorityLevel = OffscreenPriority;
90
return work;
@@ -114,6 +117,21 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
117
}
118
}
119
120
+ function resetWorkPriority(workInProgress : Fiber) {
121
+ let newPriority = NoWork;
122
+ let child = workInProgress.childInProgress || workInProgress.child;
123
+ while (child) {
124
+ // Ensure that remaining work priority bubbles up.
125
+ if (child.pendingWorkPriority !== NoWork &&
126
+ (newPriority === NoWork ||
127
+ newPriority > child.pendingWorkPriority)) {
128
+ newPriority = child.pendingWorkPriority;
129
+ }
130
+ child = child.sibling;
131
+ }
132
+ workInProgress.pendingWorkPriority = newPriority;
133
+ }
134
+
135
function completeUnitOfWork(workInProgress : Fiber) : ?Fiber {
136
while (true) {
137
// The current, flushed, state of this fiber is the alternate.
@@ -123,6 +141,8 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
141
const current = workInProgress.alternate;
142
const next = completeWork(current, workInProgress);
143
144
+ resetWorkPriority(workInProgress);
145
+
146
// The work is now done. We don't need this anymore. This flags
147
// to the system not to redo any work here.
148
workInProgress.pendingProps = null;
@@ -130,12 +150,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
150
const returnFiber = workInProgress.return;
151
152
if (returnFiber) {
133
- // Ensure that remaining work priority bubbles up.
134
- if (workInProgress.pendingWorkPriority !== NoWork &&
135
- (returnFiber.pendingWorkPriority === NoWork ||
136
- returnFiber.pendingWorkPriority > workInProgress.pendingWorkPriority)) {
137
- returnFiber.pendingWorkPriority = workInProgress.pendingWorkPriority;
138
- }
153
// Ensure that the first and last effect of the parent corresponds
154
// to the children's first and last effect. This probably relies on
155
// children completing in order.