Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[scheduler] Eagerly schedule rAF at beginning of frame #13785

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 5, 2018

Eagerly schedule the next animation callback at the beginning of the frame. If the scheduler queue is not empty at the end of the frame, it will continue flushing inside that callback. If the queue is empty, then it will exit immediately. Posting the callback at the start of the frame ensures it's fired within the earliest possible frame. If we waited until the end of the frame to post the callback, we risk the browser skipping a frame and not firing the callback until the frame after that.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2018

Should we add a more aggressive manual testing fixture?

@sophiebits
Copy link
Collaborator

I don't understand what this changes.

@acdlite
Copy link
Collaborator Author

acdlite commented Oct 6, 2018

It's to defend against the browser pushing our animation callback into the frame after the next one, by scheduling it as early as possible. Eagerly, even before we know whether we need another frame to do work in or not.

Can't think of a good fixture to simulate getting pushed into the next frame because it's so timing dependent.

@acdlite
Copy link
Collaborator Author

acdlite commented Oct 6, 2018

I suppose

function animate() {
  sleep(12); // or some number that will push us into next frame if scheduled too late
  requestAnimationFrame(animate);
}
requestAnimationFrame(animate);

and then mounting a bunch of slow components and measuring the overall time might work. I'll try it.

@sizebot
Copy link

sizebot commented Oct 6, 2018

React: size: 🔺+0.3%, gzip: 🔺+0.4%

Details of bundled changes.

Comparing: e2e7cb9...f92bbaa

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +1.0% +1.0% 91.64 KB 92.6 KB 24.25 KB 24.5 KB UMD_DEV
react.production.min.js 🔺+0.3% 🔺+0.4% 11.65 KB 11.69 KB 4.59 KB 4.61 KB UMD_PROD
react.profiling.min.js +0.3% +0.2% 13.8 KB 13.84 KB 5.1 KB 5.12 KB UMD_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD
scheduler.development.js +4.5% +4.2% 21.35 KB 22.31 KB 5.75 KB 6 KB NODE_DEV
scheduler.production.min.js 🔺+0.8% 🔺+1.1% 4.76 KB 4.8 KB 1.85 KB 1.87 KB NODE_PROD
Scheduler-dev.js +4.4% +4.1% 21.62 KB 22.58 KB 5.8 KB 6.04 KB FB_WWW_DEV
Scheduler-prod.js 🔺+2.5% 🔺+1.7% 13.3 KB 13.64 KB 2.87 KB 2.92 KB FB_WWW_PROD

Generated by 🚫 dangerJS

let startOfLatestFrame = Date.now();
let currentTime = Date.now();
let startOfLatestFrame = 0;
let currentTime = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make debugging easier. These tests use overridden Date.now() so the start time is unimportant.

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Oct 6, 2018

I suppose

function animate() {
  sleep(12); // or some number that will push us into next frame if scheduled too late
  requestAnimationFrame(animate);
}
requestAnimationFrame(animate);

and then mounting a bunch of slow components and measuring the overall time might work. I'll try it.

I'm a little confused here to use sleep, nested rAF will be scheduled in the next frame, this is not related to whether we call something like sleep(i.e. even there is no sleep the nested rAF will be scheduled in the next frame)

If I'm wrong, please let me know, thanks!

requestAnimationFrameWithTimeout(animationTick);
} else {
// No pending work. Exit.
isAnimationFrameScheduled = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can now unify these checks so we can get rid of this flag and just use scheduledCallback as the flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this would work but now I don't think it does. There are three callbacks: the one scheduled with requestAnimationFrame (represented by isAnimationFrameScheduled), the message event handler (represented by isIdleScheduled), and the one that actually contains the work (scheduledCallback). Need all three concepts. For example, as added by this PR, sometimes an animation frame is scheduled even if there's no actual work scheduled. So I think all three are needed.

I'll rename these a bit so it's less confusing.

Eagerly schedule the next animation callback at the beginning of the
frame. If the scheduler queue is not empty at the end of the frame, it
will continue flushing inside that callback. If the queue *is* empty,
then it will exit immediately. Posting the callback at the start of the
frame ensures it's fired within the earliest possible frame. If we
waited until the end of the frame to post the callback, we risk the
browser skipping a frame and not firing the callback until the frame
after that.
@acdlite acdlite merged commit b2cea90 into facebook:master Oct 9, 2018
linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018
* [scheduler] Eagerly schedule rAF at beginning of frame

Eagerly schedule the next animation callback at the beginning of the
frame. If the scheduler queue is not empty at the end of the frame, it
will continue flushing inside that callback. If the queue *is* empty,
then it will exit immediately. Posting the callback at the start of the
frame ensures it's fired within the earliest possible frame. If we
waited until the end of the frame to post the callback, we risk the
browser skipping a frame and not firing the callback until the frame
after that.

* Re-name scheduledCallback -> scheduledHostCallback
@gaearon gaearon mentioned this pull request Oct 23, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* [scheduler] Eagerly schedule rAF at beginning of frame

Eagerly schedule the next animation callback at the beginning of the
frame. If the scheduler queue is not empty at the end of the frame, it
will continue flushing inside that callback. If the queue *is* empty,
then it will exit immediately. Posting the callback at the start of the
frame ensures it's fired within the earliest possible frame. If we
waited until the end of the frame to post the callback, we risk the
browser skipping a frame and not firing the callback until the frame
after that.

* Re-name scheduledCallback -> scheduledHostCallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants