Treat Suspend and Resume as exiting and entering work loop

Their definitions used to be more fuzzy. For example, Suspend didn't always fire on exit, and sometimes fired when we did _not_ exit (such as at task enqueue). I chatted to Boone, and he's saying treating Suspend and Resume as strictly exiting and entering the loop is fine for their use case.
facebook · Aug 22, 2019 · 1f5f7df · 1f5f7df
1 parent 8482f5d
commit 1f5f7df
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 44 deletions.
diff --git a/packages/scheduler/src/Scheduler.js b/packages/scheduler/src/Scheduler.js
@@ -13,9 +13,8 @@ import {
   enableProfiling,
 } from './SchedulerFeatureFlags';
 import {
-  requestHostCallback as requestHostCallbackWithoutProfiling,
+  requestHostCallback,
   requestHostTimeout,
-  cancelHostCallback,
   cancelHostTimeout,
   shouldYieldToHost,
   getCurrentTime,
@@ -79,17 +78,6 @@ var isPerformingWork = false;
 var isHostCallbackScheduled = false;
 var isHostTimeoutScheduled = false;
 
-function requestHostCallbackWithProfiling(cb, time) {
-  if (enableProfiling) {
-    markSchedulerSuspended(time);
-    requestHostCallbackWithoutProfiling(cb);
-  }
-}
-
-const requestHostCallback = enableProfiling
-  ? requestHostCallbackWithProfiling
-  : requestHostCallbackWithoutProfiling;
-
 function advanceTimers(currentTime) {
   // Check for tasks that are no longer delayed and add them to the queue.
   let timer = peek(timerQueue);
@@ -121,7 +109,7 @@ function handleTimeout(currentTime) {
   if (!isHostCallbackScheduled) {
     if (peek(taskQueue) !== null) {
       isHostCallbackScheduled = true;
-      requestHostCallback(flushWork, currentTime);
+      requestHostCallback(flushWork);
     } else {
       const firstTimer = peek(timerQueue);
       if (firstTimer !== null) {
@@ -132,7 +120,7 @@ function handleTimeout(currentTime) {
 }
 
 function flushWork(hasTimeRemaining, initialTime) {
-  if (isHostCallbackScheduled) {
+  if (enableProfiling) {
     markSchedulerUnsuspended(initialTime);
   }
 
@@ -190,8 +178,6 @@ function flushWork(hasTimeRemaining, initialTime) {
     }
     // Return whether there's additional work
     if (currentTask !== null) {
-      markSchedulerSuspended(currentTime);
-      isHostCallbackScheduled = true;
       return true;
     } else {
       let firstTimer = peek(timerQueue);
@@ -213,6 +199,10 @@ function flushWork(hasTimeRemaining, initialTime) {
     currentTask = null;
     currentPriorityLevel = previousPriorityLevel;
     isPerformingWork = false;
+    if (enableProfiling) {
+      const currentTime = getCurrentTime();
+      markSchedulerSuspended(currentTime);
+    }
   }
 }
 
@@ -378,7 +368,7 @@ function unstable_scheduleCallback(priorityLevel, callback, options) {
     // wait until the next time we yield.
     if (!isHostCallbackScheduled && !isPerformingWork) {
       isHostCallbackScheduled = true;
-      requestHostCallback(flushWork, currentTime);
+      requestHostCallback(flushWork);
     }
   }
 
@@ -393,12 +383,7 @@ function unstable_continueExecution() {
   isSchedulerPaused = false;
   if (!isHostCallbackScheduled && !isPerformingWork) {
     isHostCallbackScheduled = true;
-    if (enableProfiling) {
-      const currentTime = getCurrentTime();
-      requestHostCallbackWithProfiling(flushWork, currentTime);
-    } else {
-      requestHostCallback(flushWork);
-    }
+    requestHostCallback(flushWork);
   }
 }
 
@@ -407,10 +392,12 @@ function unstable_getFirstCallbackNode() {
 }
 
 function unstable_cancelCallback(task) {
-  if (enableProfiling && task.isQueued) {
-    const currentTime = getCurrentTime();
-    markTaskCanceled(task, currentTime);
-    task.isQueued = false;
+  if (enableProfiling) {
+    if (task.isQueued) {
+      const currentTime = getCurrentTime();
+      markTaskCanceled(task, currentTime);
+      task.isQueued = false;
+    }
   }
 
   // Null out the callback to indicate the task has been canceled. (Can't

diff --git a/packages/scheduler/src/__tests__/SchedulerProfiling-test.js b/packages/scheduler/src/__tests__/SchedulerProfiling-test.js
@@ -217,10 +217,10 @@ describe('Scheduler', () => {
 
     const mainThreadLabelColumn = '!!! Main thread              ';
     let mainThreadTimelineColumn = '';
-    let isMainThreadBusy = false;
+    let isMainThreadBusy = true;
     for (const time of mainThreadRuns) {
       const index = time / msPerChar;
-      mainThreadTimelineColumn += (isMainThreadBusy ? '█' : ' ').repeat(
+      mainThreadTimelineColumn += (isMainThreadBusy ? '█' : '░').repeat(
         index - mainThreadTimelineColumn.length,
       );
       isMainThreadBusy = !isMainThreadBusy;
@@ -325,7 +325,7 @@ describe('Scheduler', () => {
 
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │          ██
+!!! Main thread              │██░░░░░░░░██░░░░░░░░░░░░
 Task 2 [User-blocking]       │        ░░░░██████
 Task 1 [Normal]              │  ████████░░░░░░░░██████
 `,
@@ -353,15 +353,11 @@ Task 1 [Normal]              │  ████████░░░░░░░
 
     cancelCallback(task);
 
-    // Advance more time. This should not affect the size of the main
-    // thread row, since the Scheduler queue is empty.
     Scheduler.unstable_advanceTime(1000);
     expect(Scheduler).toFlushWithoutYielding();
-
-    // The main thread row should end when the callback is cancelled.
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │      ██
+!!! Main thread              │░░░░░░██████████████████████
 Task 1 [Normal]              │██████░░🡐 canceled
 `,
     );
@@ -378,15 +374,11 @@ Task 1 [Normal]              │██████░░🡐 canceled
     expect(Scheduler).toFlushAndThrow('Oops');
     Scheduler.unstable_advanceTime(100);
 
-    // Advance more time. This should not affect the size of the main
-    // thread row, since the Scheduler queue is empty.
     Scheduler.unstable_advanceTime(1000);
     expect(Scheduler).toFlushWithoutYielding();
-
-    // The main thread row should end when the callback is cancelled.
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │
+!!! Main thread              │░░░░░░██████████████████████
 Task 1 [Normal]              │██████🡐 errored
 `,
     );
@@ -431,7 +423,7 @@ Task 1 [Normal]              │██████🡐 errored
     // The main thread row should end when the callback is cancelled.
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │      ██
+!!! Main thread              │░░░░░░██████████████████████
 Task 1 [Normal]              │██████░░🡐 canceled
 Task 2 [Normal]              │░░░░░░░░🡐 canceled
 `,
@@ -449,7 +441,7 @@ Task 2 [Normal]              │░░░░░░░░🡐 canceled
     cancelCallback(task);
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │
+!!! Main thread              │░░░░░░░░░░░░░░░░░░░░
 Task 1 [Normal]              │████████████████████
 `,
     );
@@ -482,7 +474,7 @@ Task 1 [Normal]              │████████████████
     expect(Scheduler).toFlushAndYield(['A']);
     expect(stopProfilingAndPrintFlamegraph()).toEqual(
       `
-!!! Main thread              │████████████
+!!! Main thread              │████████████░░░░░░░░░░░░░░░░░░░░
 Task 1 [Normal]              │░░░░░░░░░░░░████████████████████
 Task 2 [Normal]              │    ░░░░░░░░🡐 canceled
 `,