[scheduler] Pass didTimeout argument to callbacks (#14931)
As I prepare to refactor the Fiber scheduler, I've noticed some quirks in our implementation. This PR addressed one of them. --- There's no reason for a timed out Scheduler callback to check `shouldYield`, because the value will always be false until the work has completed. The `didTimeout` argument provides this information to the callback so it can avoid the redundant checks. React's existing check for whether a callback has timed out didn't make any sense, but happened to work anyway. I don't think the wrongness of the old implementation is observable via React APIs but it's incoherent regardless.
Andrew Clark committed
Feb 22, 2019 at 16:39 UTC
920b0bbb3c308490757accb9b8fc524d19505394
4 files changed
+52
-24
packages/react-noop-renderer/src/createReactNoop.js
+18
-12
@@ -540,31 +540,37 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
540
let unitsRemaining;
541
542
function shouldYield() {
543
+ // Check if we already yielded
544
+ if (didYield || yieldedValues !== null) {
545
+ return true;
546
+ }
547
+
548
+ // If there are no remaining units of work, and we haven't timed out, then
549
+ // we should yield.
550
if (
544
- scheduledCallbackTimeout === -1 ||
545
- elapsedTimeInMs > scheduledCallbackTimeout
551
+ unitsRemaining-- <= 0 &&
552
+ (scheduledCallbackTimeout === -1 ||
553
+ elapsedTimeInMs < scheduledCallbackTimeout)
554
) {
547
- return false;
548
- } else {
549
- if (didYield || yieldedValues !== null) {
550
- return true;
551
- }
552
- if (unitsRemaining-- > 0) {
553
- return false;
554
- }
555
didYield = true;
556
return true;
557
}
558
+
559
+ // Otherwise, keep working.
560
+ return false;
561
}
562
563
function* flushUnitsOfWork(n: number): Generator<Array<mixed>, void, void> {
561
- unitsRemaining = n + 1;
564
+ unitsRemaining = n;
565
didYield = false;
566
try {
567
while (!didYield && scheduledCallback !== null) {
568
let cb = scheduledCallback;
569
scheduledCallback = null;
567
- cb();
570
+ const didTimeout =
571
+ scheduledCallbackTimeout !== -1 &&
572
+ scheduledCallbackTimeout < elapsedTimeInMs;
573
+ cb(didTimeout);
574
if (yieldedValues !== null) {
575
const values = yieldedValues;
576
yieldedValues = null;
packages/react-reconciler/src/ReactFiberScheduler.js
+2
-2
@@ -2219,9 +2219,9 @@ function shouldYieldToRenderer() {
2219
return false;
2220
}
2221
2222
-function performAsyncWork() {
2222
+function performAsyncWork(didTimeout) {
2223
try {
2224
- if (!shouldYieldToRenderer()) {
2224
+ if (didTimeout) {
2225
// The callback timed out. That means at least one update has expired.
2226
// Iterate through the root schedule. If they contain expired work, set
2227
// the next render expiration time to the current time. This has the effect
packages/scheduler/src/Scheduler.js
+1
-1
@@ -94,7 +94,7 @@ function flushFirstCallback() {
94
currentExpirationTime = expirationTime;
95
var continuationCallback;
96
try {
97
- continuationCallback = callback();
97
+ continuationCallback = callback(currentDidTimeout);
98
} finally {
99
currentPriorityLevel = previousPriorityLevel;
100
currentExpirationTime = previousExpirationTime;
packages/scheduler/src/__tests__/Scheduler-test.js
+31
-9
@@ -246,26 +246,48 @@ describe('Scheduler', () => {
246
});
247
248
it('expires work', () => {
249
- scheduleCallback(() => doWork('A', 100));
249
+ scheduleCallback(didTimeout =>
250
+ doWork(`A (did timeout: ${didTimeout})`, 100),
251
+ );
252
runWithPriority(UserBlockingPriority, () => {
251
- scheduleCallback(() => doWork('B', 100));
253
+ scheduleCallback(didTimeout =>
254
+ doWork(`B (did timeout: ${didTimeout})`, 100),
255
+ );
256
});
253
- scheduleCallback(() => doWork('C', 100));
257
runWithPriority(UserBlockingPriority, () => {
255
- scheduleCallback(() => doWork('D', 100));
258
+ scheduleCallback(didTimeout =>
259
+ doWork(`C (did timeout: ${didTimeout})`, 100),
260
+ );
261
});
262
263
// Advance time, but not by enough to expire any work
264
advanceTime(249);
265
expect(clearYieldedValues()).toEqual([]);
266
262
- // Advance by just a bit more to expire the high pri callbacks
267
+ // Schedule a few more callbacks
268
+ scheduleCallback(didTimeout =>
269
+ doWork(`D (did timeout: ${didTimeout})`, 100),
270
+ );
271
+ scheduleCallback(didTimeout =>
272
+ doWork(`E (did timeout: ${didTimeout})`, 100),
273
+ );
274
+
275
+ // Advance by just a bit more to expire the user blocking callbacks
276
advanceTime(1);
264
- expect(clearYieldedValues()).toEqual(['B', 'D']);
277
+ expect(clearYieldedValues()).toEqual([
278
+ 'B (did timeout: true)',
279
+ 'C (did timeout: true)',
280
+ ]);
281
266
- // Expire the rest
267
- advanceTime(10000);
268
- expect(clearYieldedValues()).toEqual(['A', 'C']);
282
+ // Expire A
283
+ advanceTime(4600);
284
+ expect(clearYieldedValues()).toEqual(['A (did timeout: true)']);
285
+
286
+ // Flush the rest without expiring
287
+ expect(flushWork()).toEqual([
288
+ 'D (did timeout: false)',
289
+ 'E (did timeout: false)',
290
+ ]);
291
});
292
293
it('has a default expiration of ~5 seconds', () => {