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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b08d45
[scheduler] Eagerly schedule rAF at beginning of frame
acdlite Oct 5, 2018
f92bbaa
Re-name scheduledCallback -> scheduledHostCallback
acdlite Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 38 additions & 19 deletions packages/scheduler/src/Scheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,13 +534,13 @@ if (typeof window !== 'undefined' && window._schedMock) {
}
}

var scheduledCallback = null;
var isIdleScheduled = false;
var scheduledHostCallback = null;
var isMessageEventScheduled = false;
var timeoutTime = -1;

var isAnimationFrameScheduled = false;

var isPerformingIdleWork = false;
var isFlushingHostCallback = false;

var frameDeadline = 0;
// We start out assuming that we run at 30fps but then the heuristic tracking
Expand All @@ -564,15 +564,20 @@ if (typeof window !== 'undefined' && window._schedMock) {
return;
}

isIdleScheduled = false;
isMessageEventScheduled = false;

var prevScheduledCallback = scheduledHostCallback;
var prevTimeoutTime = timeoutTime;
scheduledHostCallback = null;
timeoutTime = -1;

var currentTime = getCurrentTime();

var didTimeout = false;
if (frameDeadline - currentTime <= 0) {
// There's no time left in this idle period. Check if the callback has
// a timeout and whether it's been exceeded.
if (timeoutTime !== -1 && timeoutTime <= currentTime) {
if (prevTimeoutTime !== -1 && prevTimeoutTime <= currentTime) {
// Exceeded the timeout. Invoke the callback even though there's no
// time left.
didTimeout = true;
Expand All @@ -584,19 +589,18 @@ if (typeof window !== 'undefined' && window._schedMock) {
requestAnimationFrameWithTimeout(animationTick);
}
// Exit without invoking the callback.
scheduledHostCallback = prevScheduledCallback;
timeoutTime = prevTimeoutTime;
return;
}
}

timeoutTime = -1;
var callback = scheduledCallback;
scheduledCallback = null;
if (callback !== null) {
isPerformingIdleWork = true;
if (prevScheduledCallback !== null) {
isFlushingHostCallback = true;
try {
callback(didTimeout);
prevScheduledCallback(didTimeout);
} finally {
isPerformingIdleWork = false;
isFlushingHostCallback = false;
}
}
};
Expand All @@ -605,7 +609,22 @@ if (typeof window !== 'undefined' && window._schedMock) {
window.addEventListener('message', idleTick, false);

var animationTick = function(rafTime) {
isAnimationFrameScheduled = false;
if (scheduledHostCallback !== null) {
// 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.
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.

return;
}

var nextFrameTime = rafTime - frameDeadline + activeFrameTime;
if (
nextFrameTime < activeFrameTime &&
Expand All @@ -629,16 +648,16 @@ if (typeof window !== 'undefined' && window._schedMock) {
previousFrameTime = nextFrameTime;
}
frameDeadline = rafTime + activeFrameTime;
if (!isIdleScheduled) {
isIdleScheduled = true;
if (!isMessageEventScheduled) {
isMessageEventScheduled = true;
window.postMessage(messageKey, '*');
}
};

requestHostCallback = function(callback, absoluteTimeout) {
scheduledCallback = callback;
scheduledHostCallback = callback;
timeoutTime = absoluteTimeout;
if (isPerformingIdleWork || absoluteTimeout < 0) {
if (isFlushingHostCallback || absoluteTimeout < 0) {
// Don't wait for the next frame. Continue working ASAP, in a new event.
window.postMessage(messageKey, '*');
} else if (!isAnimationFrameScheduled) {
Expand All @@ -652,8 +671,8 @@ if (typeof window !== 'undefined' && window._schedMock) {
};

cancelHostCallback = function() {
scheduledCallback = null;
isIdleScheduled = false;
scheduledHostCallback = null;
isMessageEventScheduled = false;
timeoutTime = -1;
};
}
Expand Down
26 changes: 23 additions & 3 deletions packages/scheduler/src/__tests__/SchedulerDOM-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,18 @@ describe('SchedulerDOM', () => {
function runRAFCallbacks() {
startOfLatestFrame += frameSize;
currentTime = startOfLatestFrame;
rAFCallbacks.forEach(cb => cb());
const cbs = rAFCallbacks;
rAFCallbacks = [];
cbs.forEach(cb => cb());
}
function advanceOneFrame(config: FrameTimeoutConfigType = {}) {
runRAFCallbacks();
runPostMessageCallbacks(config);
}

let frameSize = 33;
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.


beforeEach(() => {
// TODO pull this into helper method, reduce repetition.
Expand Down Expand Up @@ -109,6 +110,25 @@ describe('SchedulerDOM', () => {
expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number');
});

it('inserts its rAF callback as early into the queue as possible', () => {
const {unstable_scheduleCallback: scheduleCallback} = Scheduler;
const log = [];
const useRAFCallback = () => {
log.push('userRAFCallback');
};
scheduleCallback(() => {
// Call rAF while idle work is being flushed.
requestAnimationFrame(useRAFCallback);
});
advanceOneFrame({timeLeftInFrame: 1});
// There should be two callbacks: the one scheduled by Scheduler at the
// beginning of the frame, and the one scheduled later during that frame.
expect(rAFCallbacks.length).toBe(2);
// The user callback should be the second callback.
rAFCallbacks[1]();
expect(log).toEqual(['userRAFCallback']);
});

describe('with multiple callbacks', () => {
it('accepts multiple callbacks and calls within frame when not blocked', () => {
const {unstable_scheduleCallback: scheduleCallback} = Scheduler;
Expand Down