Don't check for a pendingCommit on every iteration
Only check if we're in a deferred work batch and performUnitOfWork returns null.
Andrew Clark committed
Dec 6, 2016 at 19:19 UTC
dcc17a966e55e137a38e463954acd8b2a01abff3
3 files changed
+100
-97
src/renderers/shared/fiber/ReactFiberScheduler.js
+96
-84
@@ -82,8 +82,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
82
// Keeps track of whether we're currently in a work loop. Used to batch
83
// nested updates.
84
let isPerformingWork : boolean = false;
85
- // This lets us know if we should defer a commit until the next frame.
86
- let isPerformingDeferredWork : boolean = false;
85
86
// The next work in progress fiber that we're currently working on.
87
let nextUnitOfWork : ?Fiber = null;
@@ -148,10 +146,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
146
// If there's no work on it, it will get unscheduled too.
147
nextScheduledRoot = next;
148
}
151
- if (pendingCommit) {
152
- nextPriorityLevel = pendingCommit.pendingWorkPriority;
153
- return null;
154
- }
149
+
150
let root = nextScheduledRoot;
151
let highestPriorityRoot = null;
152
let highestPriorityLevel = NoWork;
@@ -178,7 +173,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
173
}
174
175
// Returns true if the commit phase finishes without any errors.
181
- function commitAllWork(finishedWork : Fiber): boolean {
176
+ function commitAllWork(finishedWork : Fiber) {
177
// We keep track of this so that captureError can collect any boundaries
178
// that capture an error during the commit phase. The reason these aren't
179
// local to this function is because errors that occur during cWU are
@@ -325,7 +320,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
320
}
321
continue pass2;
322
}
328
- priorityContext = previousPriorityContext;
323
break;
324
}
325
@@ -333,19 +327,18 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
327
328
// If we caught any errors during this commit, schedule their boundaries
329
// to update.
336
- let noErrors;
330
if (commitPhaseBoundaries) {
338
- noErrors = false;
331
commitPhaseBoundaries.forEach(scheduleUpdate);
332
commitPhaseBoundaries = null;
341
- } else {
342
- noErrors = true;
333
}
334
335
priorityContext = previousPriorityContext;
346
- nextUnitOfWork = null;
336
348
- return noErrors;
337
+ // Clear any errors that may have occurred during this commit
338
+ // TODO: Possibly recursive. Can we move this into performWork? Not sure how
339
+ // because we need to clear errors after every commit, and one of the call
340
+ // sites of commitAllWork is completeUnitOfWork.
341
+ errorLoop();
342
}
343
344
function resetWorkPriority(workInProgress : Fiber) {
@@ -429,8 +422,16 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
422
workInProgress = returnFiber;
423
continue;
424
} else {
432
- // If we've reached the root, commit during the next unit of work.
433
- pendingCommit = workInProgress;
425
+ // We've reached the root. Unless we're current performing deferred
426
+ // work, we should commit the completed work immediately. If we are
427
+ // performing deferred work, returning null indicates to the caller
428
+ // that we just completed the root so they can handle that case correctly.
429
+ if (nextPriorityLevel < HighPriority) {
430
+ // Otherwise, we should commit immediately.
431
+ commitAllWork(workInProgress);
432
+ } else {
433
+ pendingCommit = workInProgress;
434
+ }
435
return null;
436
}
437
}
@@ -515,84 +516,71 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
516
}
517
518
function errorLoop() {
519
+ if (!nextUnitOfWork) {
520
+ nextUnitOfWork = findNextUnitOfWork();
521
+ }
522
// Keep performing work until there are no more errors
519
- while (capturedErrors && capturedErrors.size) {
520
- if (!nextUnitOfWork && !pendingCommit) {
521
- nextUnitOfWork = findNextUnitOfWork();
522
- }
523
- if (pendingCommit) {
524
- commitAllWork(pendingCommit);
525
- } else if (nextUnitOfWork) {
526
- if (hasCapturedError(nextUnitOfWork)) {
527
- // Use a forked version of performUnitOfWork
528
- nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
529
- } else {
530
- nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
531
- }
523
+ while (capturedErrors && capturedErrors.size &&
524
+ nextUnitOfWork &&
525
+ nextPriorityLevel !== NoWork &&
526
+ nextPriorityLevel <= TaskPriority) {
527
+ if (hasCapturedError(nextUnitOfWork)) {
528
+ // Use a forked version of performUnitOfWork
529
+ nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
530
} else {
533
- return;
531
+ nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
532
+ }
533
+ if (!nextUnitOfWork) {
534
+ nextUnitOfWork = findNextUnitOfWork();
535
}
536
}
537
}
538
539
function workLoop(priorityLevel) {
539
- while (true) {
540
- if (!nextUnitOfWork && !pendingCommit) {
540
+ if (!nextUnitOfWork) {
541
+ nextUnitOfWork = findNextUnitOfWork();
542
+ }
543
+ while (nextUnitOfWork &&
544
+ nextPriorityLevel !== NoWork &&
545
+ nextPriorityLevel <= priorityLevel) {
546
+ nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
547
+ if (!nextUnitOfWork) {
548
nextUnitOfWork = findNextUnitOfWork();
549
}
543
-
544
- // If we're performing Task work and we're inside a deferred work batch,
545
- // then we shouldn't commit any work yet. Defer it to the next frame.
546
- // TODO: Is there a better way check if the pending commit should be
547
- // deferred? It's not as simple as retaining work priority because
548
- // the root may receive a lower priority update before the completed
549
- // work is flushed, causing the new update to be up-prioritized.
550
- if (
551
- pendingCommit &&
552
- !(priorityLevel === TaskPriority && isPerformingDeferredWork)
553
- ) {
554
- const noError = commitAllWork(pendingCommit);
555
- if (!noError) {
556
- // There was an error during the commit phase. Clear them before
557
- // continuing.
558
- errorLoop();
559
- continue;
560
- }
561
- } else if (nextUnitOfWork && nextPriorityLevel !== NoWork && nextPriorityLevel <= priorityLevel) {
562
- nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
563
- } else {
564
- return;
565
- }
550
}
551
}
552
569
- function deferredWorkLoop(deadline) {
553
+ // Returns true if we exit because the deadline has expired
554
+ function deferredWorkLoop(deadline) : boolean {
555
+ let deadlineHasExpired = false;
556
if (!nextUnitOfWork) {
557
nextUnitOfWork = findNextUnitOfWork();
558
}
573
-
574
- while (true) {
559
+ while (nextUnitOfWork && !deadlineHasExpired) {
560
if (deadline.timeRemaining() > timeHeuristicForUnitOfWork) {
576
- if (!nextUnitOfWork && !pendingCommit) {
577
- nextUnitOfWork = findNextUnitOfWork();
578
- }
579
- if (pendingCommit) {
580
- const noError = commitAllWork(pendingCommit);
581
- if (!noError) {
582
- // There was an error during the commit phase. Clear them before
583
- // continuing.
584
- errorLoop();
585
- continue;
561
+ nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
562
+ // The order of the checks here is important: we should only check if
563
+ // there's a pending commit if performUnitOfWork returns null.
564
+ // Alternatively, to avoid checking for the pending commit, we could
565
+ // get it from the nextScheduledRoot. However, that requires additional
566
+ // checks to satisfy Flow. Since the logical operator short-circuits,
567
+ // this should be fine.
568
+ if (!nextUnitOfWork && pendingCommit) {
569
+ // performUnitOfWork returned null and we have a pendingCommit. If we
570
+ // have time, we should commit the work now.
571
+ if (deadline.timeRemaining() > timeHeuristicForUnitOfWork) {
572
+ commitAllWork(pendingCommit);
573
+ nextUnitOfWork = findNextUnitOfWork();
574
+ } else {
575
+ deadlineHasExpired = true;
576
}
587
- } else if (nextUnitOfWork) {
588
- nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
589
- } else {
590
- return;
577
+ // Otherwise the root will committed in the next frame.
578
}
579
} else {
593
- return;
580
+ deadlineHasExpired = true;
581
}
582
}
583
+ return deadlineHasExpired;
584
}
585
586
function performWork(priorityLevel : PriorityLevel, deadline : null | Deadline) {
@@ -600,7 +588,8 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
588
throw new Error('performWork was called recursively.');
589
}
590
isPerformingWork = true;
603
- isPerformingDeferredWork = priorityLevel >= HighPriority;
591
+ const isPerformingDeferredWork = Boolean(deadline);
592
+ let deadlineHasExpired = false;
593
594
// Perform work until either there's no more work at this priority level, or
595
// (in the case of deferred work) we've run out of time.
@@ -612,19 +601,35 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
601
// work. The loop we're in now runs infrequently: to flush task work at
602
// the end of a frame, or to restart after an error.
603
while (priorityLevel !== NoWork) {
615
- // If there are errors, clear them first.
616
- if (capturedErrors && capturedErrors.size) {
617
- errorLoop();
604
+ // Before starting any work, check to see if there are any pending
605
+ // commits from the previous frame. An exception if we're flushing
606
+ // Task work in a deferred batch and the pending commit does not
607
+ // have Task priority.
608
+ if (pendingCommit) {
609
+ const isFlushingTaskWorkInDeferredBatch =
610
+ priorityLevel === TaskPriority &&
611
+ isPerformingDeferredWork &&
612
+ pendingCommit.pendingWorkPriority !== TaskPriority;
613
+ if (!isFlushingTaskWorkInDeferredBatch) {
614
+ commitAllWork(pendingCommit);
615
+ }
616
}
617
620
- if (priorityLevel >= HighPriority) {
621
- if (!deadline) {
622
- throw new Error('Cannot perform deferred work without a deadline.');
623
- }
618
+ // Clear any errors.
619
+ errorLoop();
620
+
621
+ if (deadline) {
622
// The deferred work loop will run until there's no time left in
625
- // the current frame
626
- deferredWorkLoop(deadline);
623
+ // the current frame.
624
+ if (!deadlineHasExpired) {
625
+ deadlineHasExpired = deferredWorkLoop(deadline);
626
+ }
627
} else {
628
+ if (priorityLevel >= HighPriority) {
629
+ throw new Error(
630
+ 'Deferred work should only be performed using deferredWorkLoop'
631
+ );
632
+ }
633
// The non-deferred work loop will run until there's no more work
634
// at the given priority level
635
workLoop(priorityLevel);
@@ -633,6 +638,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
638
// Stop performing work
639
priorityLevel = NoWork;
640
641
+ // If we additional work, and we're in a deferred batch, check to see
642
+ // if the deadline has expired. If not, we may have more time to
643
+ // do work.
644
+ if (nextPriorityLevel !== NoWork && isPerformingDeferredWork && !deadlineHasExpired) {
645
+ priorityLevel = nextPriorityLevel;
646
+ continue;
647
+ }
648
+
649
// There might still be work left. Depending on the priority, we should
650
// either perform it now or schedule a callback to perform it later.
651
switch (nextPriorityLevel) {
@@ -691,7 +704,6 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
704
705
// We're done performing work. Time to clean up.
706
isPerformingWork = false;
694
- isPerformingDeferredWork = false;
707
capturedErrors = null;
708
failedBoundaries = null;
709
src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+1
-10
@@ -625,16 +625,7 @@ describe('ReactIncremental', () => {
625
// we should be able to reuse the reconciliation work that we already did
626
// without restarting.
627
ReactNoop.flush();
628
- // TODO: Content never fully completed its render so can't completely bail
629
- // out on the entire subtree. However, we could do a shallow bail out and
630
- // not rerender Content, but keep going down the incomplete tree.
631
- // Normally shouldComponentUpdate->false is not enough to determine that we
632
- // can safely reuse the old props, but I think in this case it would be ok,
633
- // since it is a resume of already started work.
634
- // Because of the above we can not reuse the work of Bar because the
635
- // rerender of Content will generate a new element which will mean we don't
636
- // auto-bail out from Bar.
637
- expect(ops).toEqual(['Bar', 'Middle']);
628
+ expect(ops).toEqual(['Middle']);
629
630
});
631
src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
+3
-3
@@ -419,11 +419,11 @@ describe('ReactIncrementalScheduling', () => {
419
});
420
// We're flushing deferred work
421
// Still, roots with animation work are handled first
422
- ReactNoop.flushDeferredPri(15 + 5);
422
+ ReactNoop.flushDeferredPri(15);
423
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
424
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
425
expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]);
426
- ReactNoop.flushDeferredPri(15 + 5);
426
+ ReactNoop.flushDeferredPri(15);
427
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
428
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
429
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
@@ -434,7 +434,7 @@ describe('ReactIncrementalScheduling', () => {
434
ReactNoop.renderToRootWithID(<span prop="b:3" />, 'b');
435
});
436
// Animation is still handled first
437
- ReactNoop.flushDeferredPri(15 + 5);
437
+ ReactNoop.flushDeferredPri(15);
438
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
439
expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]);
440
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);