[Scheduler] Use rIC to post first callback #16166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Scheduler uses `requestAnimationFrame` to post tasks to the browser. If this happens at the beginning of a frame, the callback might not fire until the subsequent frame, even if the main thread is idle. Our theory was that this wouldn't be an issue in practice, because once the first rAF fires, we schedule the next rAF as early as possible in that frame. Combined with our heuristic for computing frame deadlines, we shouldn't get any idle time in between frames — only before the *first* frame. This reasoning holds true when you have a small number of large tasks, such as the ones posted by React. The idle time before the task starts is negligible relative to the lifetime of the entire task. However, it does not hold if you have many small tasks posted over a span of time, perhaps spawned by a flurry of IO events. In this case, instead of single, contiguous rAF loop preceded by an idle frame, you get many rAF loops preceded by many idle frames. Our new theory is that this is contributing to poor main thread utilization during page loads. To try to reclaim as much idle time as possible, this PR adds two experimental flags. The first one adds a `requestIdleCallback` call to start the rAF loop, which will fire before rAF if there's idle time left in the frame. (We still call rAF in case there isn't. We start working in whichever event fires first.) The second flag tries a similar strategy using `setTimeout(fn, 0)`. If the timer fires before rAF, we'll assume that the main thread is idle. If either `requestIdleCallback` or `setTimeout` fires before rAF, we'll immediately peform some work. Since we don't have a real frame time that we can use to compute the frame deadline, we'll do an entire frame length of work. This will probably push us past the vsync, but this is fine because we can catch up during the next frame, by which point a real rAF will have fired and the loop can proceed the same way it does today. Test plan: Try this on Facebook to see if it improves load times
|
React: size: -0.9%, gzip: 0.0% Details of bundled changes.Comparing: 2c4d61e...08633f9 react
scheduler
Generated by 🚫 dangerJS |
acdlite
force-pushed
the
reclaim-idle-time
branch
8 times, most recently
from
7584310 to
173b0ac
Compare
acdlite
changed the title
bvaughn
reviewed
Jul 22, 2019
| fpsLocked = true; | ||
| } else { | ||
| // reset the framerate | ||
| activeFrameTime = 33; | ||
| frameLength = 33; |
There was a problem hiding this comment.
Should you be resetting to 33.33?
| scheduledHostCallback = null; | ||
| } | ||
| } catch (error) { | ||
| // If a scheduler task throws, exit the current browser task so the | ||
| // error can be observed, and post a new task as soon as possible | ||
| // so we can continue where we left off. | ||
| isMessageEventScheduled = true; | ||
| port.postMessage(undefined); | ||
| port.postMessage(null); |
There was a problem hiding this comment.
Is undefined -> null an important change?
| }; | ||
|
|
||
| requestHostCallback = function(callback) { | ||
| if (scheduledHostCallback === null) { | ||
| if (!isRAFLoopRunning) { | ||
| isRAFLoopRunning = false; |
There was a problem hiding this comment.
if (!isRAFLoopRunning) {
isRAFLoopRunning = false;These lines look fishy. Did you mean to set isRAFLoopRunning = true?
There was a problem hiding this comment.
Good catch, I fixed this locally but didn't commit it
acdlite
force-pushed
the
reclaim-idle-time
branch
from
173b0ac to
ac1abe4
Compare
acdlite
force-pushed
the
reclaim-idle-time
branch
from
ac1abe4 to
08633f9
Compare
bvaughn
approved these changes
Jul 22, 2019
| } | ||
| // Yielding to the browser will give it a chance to paint, so we can | ||
| // reset this. | ||
| needsPaint = false; |
There was a problem hiding this comment.
Is this still valid in the event that performWorkUntilDeadline gets called by timeout or rIC?
There was a problem hiding this comment.
Not in that case but it is relevant in the case where forceFrameRate sets the frame length to an artificially large size. We should track the chosen frame length and the hardware frame length separately. Planning to do that in a follow up.
| } | ||
| frameDeadline = getCurrentTime() + frameLength; | ||
| performWorkUntilDeadline(); | ||
| }, 0); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| cancelHostCallback = function() { |
There was a problem hiding this comment.
Worth checking/clearing the timeout/rIC here?
There was a problem hiding this comment.
performWorkUntilDeadline will exit immediately when scheduledHostCallback is null so it's fine. I wanted to keep the callback/timeout IDs local.
There was a problem hiding this comment.
Yeah, I know it's a no-op in that case.
Seems reasonable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Scheduler uses
requestAnimationFrameto post tasks to the browser. If this happens at the beginning of a frame, the callback might not fire until the subsequent frame, even if the main thread is idle.Our theory was that this wouldn't be an issue in practice, because once the first rAF fires, we schedule the next rAF as early as possible in that frame. Combined with our heuristic for computing frame deadlines, we shouldn't get any idle time in between frames — only before the first frame.
This reasoning holds true when you have a small number of large tasks, such as the ones posted by React. The idle time before the task starts is negligible relative to the lifetime of the entire task.
However, it does not hold if you have many small tasks posted over a span of time, perhaps spawned by a flurry of IO events. In this case, instead of single, contiguous rAF loop preceded by an idle frame, you get many rAF loops preceded by many idle frames. Our new theory is that this is contributing to poor main thread utilization during page loads.
To try to reclaim as much idle time as possible, this PR adds two experimental flags. The first one adds a
requestIdleCallbackcall to start the rAF loop, which will fire before rAF if there's idle time left in the frame. (We still call rAF in case there isn't. We start working in whichever event fires first.)The second flag tries a similar strategy using
setTimeout(fn, 0). If the timer fires before rAF, we'll assume that the main thread is idle.If either
requestIdleCallbackorsetTimeoutfires before rAF, we'll immediately perform some work. Since we don't have a real frame time that we can use to compute the frame deadline, we might miss the next vsync, but this is fine because we can catch up during the next frame, by which point a real rAF will have fired and the loop can proceed the same way it does today.Test plan: Try this on Facebook to see if it improves load times.