@samitouri / QOS-React / commits / 9fca812139

Fix NoWork bug

Thanks @acdlite. Add comment about future unit test coverage. This was actually hiding the fact that we are only able to reuse existing work if it was marked as completely finished which it won't be if we reuse its pending priority. However, we should be able to bail out even if there is work remaining in a subtree.

Sebastian Markbage committed Jul 18, 2016 at 18:11 UTC 9fca81213967210ea3f74daab7a0f64a454bd3b6

2 files changed +27 -8

src/renderers/shared/fiber/ReactFiberBeginWork.js

+16 -6

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

169 // it until the next tick.

170 workInProgress.child = current.child;

171 reuseChildren(workInProgress, workInProgress.child);

172 - if (workInProgress.pendingWorkPriority <= priorityLevel) {

172 + if (workInProgress.pendingWorkPriority !== NoWork && workInProgress.pendingWorkPriority <= priorityLevel) {

173 // TODO: This passes the current node and reads the priority level and

174 // pending props from that. We want it to read our priority level and

175 // pending props from the work in progress. Needs restructuring.

@@ -184,15 +184,25 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {

184 }

185

186 if (!workInProgress.hasWorkInProgress &&

187 - workInProgress.pendingProps === workInProgress.memoizedProps &&

188 - workInProgress.pendingWorkPriority === NoWork) {

187 + workInProgress.pendingProps === workInProgress.memoizedProps) {

188 // If we started this work before, and finished it, or if we're in a

189 // ping-pong update scenario, this version could already be what we're

190 // looking for. In that case, we should be able to just bail out.

191 + const priorityLevel = workInProgress.pendingWorkPriority;

192 workInProgress.pendingProps = null;

193 - // TODO: We should be able to bail out if there is remaining work at a lower

194 - // priority too. However, I don't know if that is safe or even better since

195 - // the other tree could've potentially finished that work.

193 + workInProgress.pendingWorkPriority = NoWork;

194 + if (workInProgress.child) {

195 + // If we bail out but still has work with the current priority in this

196 + // subtree, we need to go find it right now. If we don't, we won't flush

197 + // it until the next tick.

198 + reuseChildren(workInProgress, workInProgress.child);

199 + if (workInProgress.pendingWorkPriority !== NoWork && workInProgress.pendingWorkPriority <= priorityLevel) {

200 + // TODO: This passes the current node and reads the priority level and

201 + // pending props from that. We want it to read our priority level and

202 + // pending props from the work in progress. Needs restructuring.

203 + return findNextUnitOfWorkAtPriority(workInProgress, priorityLevel);

204 + }

205 + }

206 return null;

207 }

208

src/renderers/shared/fiber/ReactFiberPendingWork.js

+11 -2

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

22 } = require('ReactPriorityLevel');

24 function cloneSiblings(current : Fiber, workInProgress : Fiber, returnFiber : Fiber) {

25 + workInProgress.return = returnFiber;

26 while (current.sibling) {

27 current = current.sibling;

28 workInProgress = workInProgress.sibling = cloneFiber(

@@ -49,6 +50,7 @@ exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLev

50 workInProgress.pendingProps = current.pendingProps;

51 return workInProgress;

52 }

53 +

54 // If we have a child let's see if any of our children has work to do.

55 // Only bother doing this at all if the current priority level matches

56 // because it is the highest priority for the whole subtree.

@@ -62,8 +64,15 @@ exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLev

64 throw new Error('Should have wip now');

65 }

66 workInProgress.pendingWorkPriority = current.pendingWorkPriority;

65 - workInProgress.child = cloneFiber(currentChild, NoWork);

66 - workInProgress.child.return = workInProgress;

67 + // TODO: The below priority used to be set to NoWork which would've

68 + // dropped work. This is currently unobservable but will become

69 + // observable when the first sibling has lower priority work remaining

70 + // than the next sibling. At that point we should add tests that catches

71 + // this.

72 + workInProgress.child = cloneFiber(

73 + currentChild,

74 + currentChild.pendingWorkPriority

75 + );

76 cloneSiblings(currentChild, workInProgress.child, workInProgress);

77 current = current.child;

78 continue;