From fe17821c19fb55cf20852091e82088cfea81320e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 3 Feb 2020 13:46:13 -0800 Subject: [PATCH 1/3] [Mock Scheduler] Mimic browser's advanceTime The mock Scheduler that we use in our tests has its own fake timer implementation. The `unstable_advanceTime` method advances the timeline. Currently, a call to `unstable_advanceTime` will also flush any pending expired work. But that's not how it works in the browser: when a timer fires, the corresponding task is added to the Scheduler queue. However, we will still wait until the next message event before flushing it. This commit changes `unstable_advanceTime` to more closely resemble the browser behavior, by removing the automatic flushing of expired work. ```js // Before this commit Scheduler.unstable_advanceTime(ms); // Equivalent behavior after this commit Scheduler.unstable_advanceTime(ms); Scheduler.unstable_flushExpired(); ``` The general principle is to prefer separate APIs for scheduling tasks and flushing them. This change does not affect any public APIs. `unstable_advanceTime` is only used by our own test suite. It is not used by `act`. However, we may need to update tests in www, like Relay's. --- .../__tests__/ChangeEventPlugin-test.internal.js | 1 + .../ReactIncrementalUpdates-test.internal.js | 4 ++-- .../ReactSuspenseWithNoopRenderer-test.internal.js | 4 ++-- packages/scheduler/src/__tests__/Scheduler-test.js | 10 +++++----- .../scheduler/src/forks/SchedulerHostConfig.mock.js | 11 ++++------- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js index c39febb976e..8357dbc633c 100644 --- a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js @@ -779,6 +779,7 @@ describe('ChangeEventPlugin', () => { // 3s should be enough to expire the updates Scheduler.unstable_advanceTime(3000); + expect(Scheduler).toFlushExpired([]); expect(container.textContent).toEqual('hovered'); }); }, diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index bcb9f7189c0..231dcde677e 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -540,7 +540,7 @@ describe('ReactIncrementalUpdates', () => { // All the updates should render and commit in a single batch. Scheduler.unstable_advanceTime(10000); - expect(Scheduler).toHaveYielded(['Render: goodbye']); + expect(Scheduler).toFlushExpired(['Render: goodbye']); // Passive effect expect(Scheduler).toFlushAndYield(['Commit: goodbye']); }); @@ -645,7 +645,7 @@ describe('ReactIncrementalUpdates', () => { // All the updates should render and commit in a single batch. Scheduler.unstable_advanceTime(10000); - expect(Scheduler).toHaveYielded([ + expect(Scheduler).toFlushExpired([ 'Render: goodbye', 'Commit: goodbye', 'Render: goodbye', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 361d8c14670..cd378d4bd94 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -848,7 +848,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); Scheduler.unstable_advanceTime(10000); - expect(Scheduler).toHaveYielded([ + expect(Scheduler).toFlushExpired([ 'Suspend! [A]', 'Suspend! [B]', 'Loading...', @@ -987,7 +987,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { Scheduler.unstable_advanceTime(10000); jest.advanceTimersByTime(10000); - expect(Scheduler).toHaveYielded([ + expect(Scheduler).toFlushExpired([ 'Suspend! [goodbye]', 'Loading...', 'Commit: goodbye', diff --git a/packages/scheduler/src/__tests__/Scheduler-test.js b/packages/scheduler/src/__tests__/Scheduler-test.js index 881d6d9ffe1..51de6f7b558 100644 --- a/packages/scheduler/src/__tests__/Scheduler-test.js +++ b/packages/scheduler/src/__tests__/Scheduler-test.js @@ -117,14 +117,14 @@ describe('Scheduler', () => { // Advance by just a bit more to expire the user blocking callbacks Scheduler.unstable_advanceTime(1); - expect(Scheduler).toHaveYielded([ + expect(Scheduler).toFlushExpired([ 'B (did timeout: true)', 'C (did timeout: true)', ]); // Expire A Scheduler.unstable_advanceTime(4600); - expect(Scheduler).toHaveYielded(['A (did timeout: true)']); + expect(Scheduler).toFlushExpired(['A (did timeout: true)']); // Flush the rest without expiring expect(Scheduler).toFlushAndYield([ @@ -140,7 +140,7 @@ describe('Scheduler', () => { expect(Scheduler).toHaveYielded([]); Scheduler.unstable_advanceTime(1); - expect(Scheduler).toHaveYielded(['A']); + expect(Scheduler).toFlushExpired(['A']); }); it('continues working on same task after yielding', () => { @@ -217,7 +217,7 @@ describe('Scheduler', () => { // Advance time by just a bit more. This should expire all the remaining work. Scheduler.unstable_advanceTime(1); - expect(Scheduler).toHaveYielded(['C', 'D']); + expect(Scheduler).toFlushExpired(['C', 'D']); }); it('continuations are interrupted by higher priority work', () => { @@ -705,7 +705,7 @@ describe('Scheduler', () => { // Now it expires Scheduler.unstable_advanceTime(1); - expect(Scheduler).toHaveYielded(['A']); + expect(Scheduler).toFlushExpired(['A']); }); it('cancels a delayed task', () => { diff --git a/packages/scheduler/src/forks/SchedulerHostConfig.mock.js b/packages/scheduler/src/forks/SchedulerHostConfig.mock.js index 36570923359..cef9f1b2e10 100644 --- a/packages/scheduler/src/forks/SchedulerHostConfig.mock.js +++ b/packages/scheduler/src/forks/SchedulerHostConfig.mock.js @@ -201,13 +201,10 @@ export function unstable_yieldValue(value: mixed): void { export function unstable_advanceTime(ms: number) { currentTime += ms; - if (!isFlushing) { - if (scheduledTimeout !== null && timeoutTime <= currentTime) { - scheduledTimeout(currentTime); - timeoutTime = -1; - scheduledTimeout = null; - } - unstable_flushExpired(); + if (scheduledTimeout !== null && timeoutTime <= currentTime) { + scheduledTimeout(currentTime); + timeoutTime = -1; + scheduledTimeout = null; } } From e81f35c388be43e7cbb4a510809d3fbb7ce49bb5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 3 Feb 2020 15:33:26 -0800 Subject: [PATCH 2/3] shouldYield: Always return false if task expired The current behavior of shouldYield is unspecified except in cases that happen to be relied on by the React work loop's implementation. However, some of the unspecified cases are important and the consequences of assuming the wrong behavior are really bad. For example, it's natural to assume that `shouldYield` returns false when called from within an expired task. But the current behavior usually does that, unless there's a higher priority task or a main thread task (i.e. at the end of the 5ms message loop interval). This fixes `shouldYield` so that it always returns false inside an expired task, regardless of the other tasks in the queue. --- packages/scheduler/src/Scheduler.js | 50 ++++++- .../scheduler/src/__tests__/Scheduler-test.js | 129 ++++++++++++++++++ 2 files changed, 172 insertions(+), 7 deletions(-) diff --git a/packages/scheduler/src/Scheduler.js b/packages/scheduler/src/Scheduler.js index bc5d061c562..595d02e13d6 100644 --- a/packages/scheduler/src/Scheduler.js +++ b/packages/scheduler/src/Scheduler.js @@ -396,17 +396,53 @@ function unstable_getCurrentPriorityLevel() { } function unstable_shouldYield() { + // Note: In general, we should bias toward returning `false` instead of + // `true`, since in the extreme cases, the consequences of yielding too much + // (indefinite starvation) are worse than the consequences of blocking the + // main thread (temporary unresponsiveness). + + if (currentTask === null) { + // There's no currently running task. + // TODO: Is this desirable behavior? Maybe we should pretend unscheduled + // work has default priority. + return false; + } + + // Check if the current task expired. const currentTime = getCurrentTime(); + const currentExpiration = currentTask.expirationTime; + if (currentExpiration <= currentTime) { + // Never yield from an expired task, even if there is a pending main thread + // task or a higher priority task in the queue. + return false; + } + + // Check if there's a pending main thread task. + if (shouldYieldToHost()) { + return true; + } + + // Check a higher priority task was scheduled. But first, check if any timers + // have elapsed since we started working on this task. If so, this transfers + // them to the task queue. advanceTimers(currentTime); + + // Now compare the priority of the current task to the highest priority task. const firstTask = peek(taskQueue); return ( - (firstTask !== currentTask && - currentTask !== null && - firstTask !== null && - firstTask.callback !== null && - firstTask.startTime <= currentTime && - firstTask.expirationTime < currentTask.expirationTime) || - shouldYieldToHost() + firstTask !== null && + firstTask.expirationTime < currentExpiration && + // TODO: This checks if the highest-priority callback was canceled. But + // there could be another high priority callback between the current task + // and the head of the queue. This is an edge case, but we can solve by + // removing canceled tasks from the head of the queue in `cancelTask`. + // Either way, it's probably not a big deal because you only hit this case + // if you schedule a high pri task inside another task, and then cancel it + // before the next `shouldYield`. + firstTask.callback !== null && + // Defensive coding to make absolutely sure we don't yield to the currently + // running task, which could cause an infinite loop. + firstTask !== currentTask ); } diff --git a/packages/scheduler/src/__tests__/Scheduler-test.js b/packages/scheduler/src/__tests__/Scheduler-test.js index 51de6f7b558..efef24c4fef 100644 --- a/packages/scheduler/src/__tests__/Scheduler-test.js +++ b/packages/scheduler/src/__tests__/Scheduler-test.js @@ -191,6 +191,135 @@ describe('Scheduler', () => { expect(Scheduler).toFlushAndYield(['C2', 'C3', 'D', 'E']); }); + it("shouldYield returns false if task has expired, even if there's a pending higher priority task", () => { + let step = 0; + function workLoop() { + while (step < 4) { + step += 1; + Scheduler.unstable_yieldValue('Step ' + step); + if (step === 2) { + // This schedules a high pri task + scheduleCallback(Scheduler.unstable_ImmediatePriority, () => { + Scheduler.unstable_yieldValue('High-pri task'); + }); + // This expires the current task + Scheduler.unstable_yieldValue('Expire!'); + Scheduler.unstable_advanceTime(10000); + } + if (shouldYield()) { + // We should never hit this path, because should have already expired. + Scheduler.unstable_yieldValue('Yield!'); + return workLoop; + } + } + } + + scheduleCallback(NormalPriority, workLoop); + expect(Scheduler).toFlushAndYield([ + 'Step 1', + 'Step 2', + 'Expire!', + 'Step 3', + 'Step 4', + 'High-pri task', + ]); + }); + + it( + "shouldYield returns false if task has expired, even if there's a " + + 'pending main thread task (e.g. requestPaint)', + () => { + let step = 0; + function workLoop() { + while (step < 4) { + step += 1; + Scheduler.unstable_yieldValue('Step ' + step); + if (step === 2) { + // This asks Scheduler to yield to the main thread + Scheduler.unstable_requestPaint(); + // This expires the current task + Scheduler.unstable_yieldValue('Expire!'); + Scheduler.unstable_advanceTime(10000); + } + if (shouldYield()) { + // We should never hit this path, because should have already expired. + Scheduler.unstable_yieldValue('Yield!'); + return workLoop; + } + } + } + + scheduleCallback(NormalPriority, workLoop); + expect(Scheduler).toFlushUntilNextPaint([ + 'Step 1', + 'Step 2', + 'Expire!', + 'Step 3', + 'Step 4', + ]); + }, + ); + + it( + 'shouldYield returns false if there are higher priority tasks that were ' + + 'already canceled.', + () => { + let step = 0; + function workLoop() { + while (step < 4) { + step += 1; + Scheduler.unstable_yieldValue('Step ' + step); + if (step === 2) { + // This asks Scheduler to yield to the main thread + const task1 = scheduleCallback( + Scheduler.unstable_ImmediatePriority, + () => { + Scheduler.unstable_yieldValue('High-pri task 1'); + }, + ); + const task2 = scheduleCallback( + Scheduler.unstable_ImmediatePriority, + () => { + Scheduler.unstable_yieldValue('High-pri task 2'); + }, + ); + // TODO: What if task1 is canceled but not task2? + cancelCallback(task1); + cancelCallback(task2); + } + if (shouldYield()) { + // We should never hit this path, because should have already expired. + Scheduler.unstable_yieldValue('Yield!'); + return workLoop; + } + } + } + + scheduleCallback(NormalPriority, workLoop); + expect(Scheduler).toFlushAndYield([ + 'Step 1', + 'Step 2', + 'Step 3', + 'Step 4', + ]); + }, + ); + + it('shouldYield returns false if there are no pending tasks', () => { + expect(Scheduler.unstable_shouldYield()).toBe(false); + }); + + // TODO: Is this desirable behavior? Maybe we should pretend unscheduled + // work has default priority. + it( + 'shouldYield returns false if called outside of a Scheduler task, even ' + + 'if there are pending tasks', + () => { + scheduleCallback(ImmediatePriority, () => {}); + expect(Scheduler.unstable_shouldYield()).toBe(false); + }, + ); + it('continuation callbacks inherit the expiration of the previous callback', () => { const tasks = [ ['A', 125], From 776b4e46133ee8c99dfec5e6a52323162e917dac Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 3 Feb 2020 16:37:51 -0800 Subject: [PATCH 3/3] Optimize `shouldYield` by comparing to head Since the queue is already sorted by priority, it's faster and less error-prone for `shouldYield` to check if the current task is at the head of the list. The only caveat is that a higher priority task may have been canceled, but is still part of the queue. This is because we don't always remove canceled tasks from the queue immediately, because deletion of an arbitrary node from a binary heap is O(log n). We address this by popping high priority canceled tasks from the queue upon the next call to `shouldYield`. Since this is an edge case, and since it prevents us from yielding unnecessarily, the shifted cost is worth it. It's not extra work because we would have done this anyway the next time we enter the Scheduler work loop. Another argument in favor is that it's better to do this work during a yieldy, default priority task than at the beginning of a blocking, high priority task. --- packages/scheduler/src/Scheduler.js | 36 +++++++------- .../scheduler/src/__tests__/Scheduler-test.js | 47 ++++++++++++++++++- 2 files changed, 66 insertions(+), 17 deletions(-) diff --git a/packages/scheduler/src/Scheduler.js b/packages/scheduler/src/Scheduler.js index 595d02e13d6..02aec304559 100644 --- a/packages/scheduler/src/Scheduler.js +++ b/packages/scheduler/src/Scheduler.js @@ -428,22 +428,26 @@ function unstable_shouldYield() { advanceTimers(currentTime); // Now compare the priority of the current task to the highest priority task. - const firstTask = peek(taskQueue); - return ( - firstTask !== null && - firstTask.expirationTime < currentExpiration && - // TODO: This checks if the highest-priority callback was canceled. But - // there could be another high priority callback between the current task - // and the head of the queue. This is an edge case, but we can solve by - // removing canceled tasks from the head of the queue in `cancelTask`. - // Either way, it's probably not a big deal because you only hit this case - // if you schedule a high pri task inside another task, and then cancel it - // before the next `shouldYield`. - firstTask.callback !== null && - // Defensive coding to make absolutely sure we don't yield to the currently - // running task, which could cause an infinite loop. - firstTask !== currentTask - ); + // This equivalent to checking if the current task is the head of the queue. + let firstTask = peek(taskQueue); + while (firstTask !== currentTask && firstTask !== null) { + if (firstTask.callback !== null) { + // There's a higher priority task. Yield. + return true; + } else { + // There's a higher priority task, but it was canceled. This happens + // because tasks are not always removed from the queue immediately when + // they are canceled, since removal of an arbitrary node from a binary + // heap is O(log n). Instead, we null out the `callback` field and remove + // it the next time we traverse the queue. + // + // Pop the canceled task from the queue and proceed to the next task. + pop(taskQueue); + firstTask = peek(taskQueue); + } + } + // There's no higher priority task. + return false; } const unstable_requestPaint = requestPaint; diff --git a/packages/scheduler/src/__tests__/Scheduler-test.js b/packages/scheduler/src/__tests__/Scheduler-test.js index efef24c4fef..3a44a23ba7b 100644 --- a/packages/scheduler/src/__tests__/Scheduler-test.js +++ b/packages/scheduler/src/__tests__/Scheduler-test.js @@ -283,7 +283,6 @@ describe('Scheduler', () => { Scheduler.unstable_yieldValue('High-pri task 2'); }, ); - // TODO: What if task1 is canceled but not task2? cancelCallback(task1); cancelCallback(task2); } @@ -305,6 +304,52 @@ describe('Scheduler', () => { }, ); + it( + 'shouldYield returns true if there are higher priority tasks, some of ' + + 'which were canceled, but not all of them', + () => { + // Tests an implementation-specific edge case. Canceled are not always + // removed from the queue when they are canceled, since removal of an + // arbitrary node from a binary heap is O(log n). + let step = 0; + function workLoop() { + while (step < 4) { + step += 1; + Scheduler.unstable_yieldValue('Step ' + step); + if (step === 2) { + // This asks Scheduler to yield to the main thread + const task1 = scheduleCallback( + Scheduler.unstable_ImmediatePriority, + () => { + Scheduler.unstable_yieldValue('High-pri task 1'); + }, + ); + scheduleCallback(Scheduler.unstable_ImmediatePriority, () => { + Scheduler.unstable_yieldValue('High-pri task 2'); + }); + // Cancel the first task, but not the second one. + cancelCallback(task1); + } + if (shouldYield()) { + // We should never hit this path, because should have already expired. + Scheduler.unstable_yieldValue('Yield!'); + return workLoop; + } + } + } + + scheduleCallback(NormalPriority, workLoop); + expect(Scheduler).toFlushUntilNextPaint([ + 'Step 1', + 'Step 2', + 'Yield!', + 'High-pri task 2', + 'Step 3', + 'Step 4', + ]); + }, + ); + it('shouldYield returns false if there are no pending tasks', () => { expect(Scheduler.unstable_shouldYield()).toBe(false); });