Skip to content

Commit

Permalink
Implement requestPaint in the actual scheduler (facebook#31784)
Browse files Browse the repository at this point in the history
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in facebook#31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬
  • Loading branch information
sebmarkbage authored Dec 14, 2024
1 parent c32780e commit c80b336
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 3 deletions.
4 changes: 2 additions & 2 deletions packages/scheduler/src/__tests__/Scheduler-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe('SchedulerBrowser', () => {
it('task with continuation', () => {
scheduleCallback(NormalPriority, () => {
runtime.log('Task');
// Request paint so that we yield at the end of the frame interval
// Request paint so that we yield immediately
requestPaint();
while (!Scheduler.unstable_shouldYield()) {
runtime.advanceTime(1);
Expand All @@ -199,7 +199,7 @@ describe('SchedulerBrowser', () => {
runtime.assertLog([
'Message Event',
'Task',
gate(flags => (flags.www ? 'Yield at 10ms' : 'Yield at 5ms')),
'Yield at 0ms',
'Post Message',
]);

Expand Down
11 changes: 10 additions & 1 deletion packages/scheduler/src/forks/Scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ var isPerformingWork = false;
var isHostCallbackScheduled = false;
var isHostTimeoutScheduled = false;

var needsPaint = false;

// Capture local references to native APIs, in case a polyfill overrides them.
const localSetTimeout = typeof setTimeout === 'function' ? setTimeout : null;
const localClearTimeout =
Expand Down Expand Up @@ -456,6 +458,10 @@ let frameInterval = frameYieldMs;
let startTime = -1;

function shouldYieldToHost(): boolean {
if (needsPaint) {
// Yield now.
return true;
}
const timeElapsed = getCurrentTime() - startTime;
if (timeElapsed < frameInterval) {
// The main thread has only been blocked for a really short amount of time;
Expand All @@ -466,7 +472,9 @@ function shouldYieldToHost(): boolean {
return true;
}

function requestPaint() {}
function requestPaint() {
needsPaint = true;
}

function forceFrameRate(fps: number) {
if (fps < 0 || fps > 125) {
Expand All @@ -486,6 +494,7 @@ function forceFrameRate(fps: number) {
}

const performWorkUntilDeadline = () => {
needsPaint = false;
if (isMessageLoopRunning) {
const currentTime = getCurrentTime();
// Keep track of the start time so we can measure how long the main thread
Expand Down

0 comments on commit c80b336

Please sign in to comment.