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] Priority levels, continuations, and wrapped callbacks #13720

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
7 participants
@acdlite
Member

acdlite commented Sep 25, 2018

All of these features are based on features of React's internal scheduler. The eventual goal is to lift as much as possible out of the React internals into the Scheduler package.

Includes some renaming of existing methods.

  • scheduleWork is now scheduleCallback
  • cancelScheduledWork is now cancelCallback

Priority levels

Adds the ability to schedule callbacks at different priority levels. The current levels are (final names TBD):

  • Immediate priority. Fires at the end of the outermost currently executing (similar to a microtask).
  • Interactive priority. Fires within a few hundred milliseconds. This should only be used to provide quick feedback to the user as a result of an interaction.
  • Normal priority. This is the default. Fires within several seconds.
  • "Maybe" priority. Only fires if there's nothing else to do. Used for prerendering or warming a cache.

The priority is changed using runWithPriority:

runWithPriority(InteractivePriority, () => {
  scheduleCallback(callback);
});

Continuations

Adds the ability for a callback to yield without losing its place in the queue, by returning a continuation. The continuation will have the same expiration as the callback that yielded.

Wrapped callbacks

Adds the ability to wrap a callback so that, when it is called, it receives the priority of the current execution context.

@n8schloss

@acdlite acdlite requested review from bvaughn and gaearon Sep 25, 2018

@acdlite

This comment has been minimized.

Member

acdlite commented Sep 25, 2018

Planning to open a Scheduler RFC later this week

<head>
<meta charset="utf-8">
<title>Scheduler Test Page</title>

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Uh I guess Prettier ran on this file :D

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Yeah, because of the renamed schedule method 😄

@@ -17,7 +17,8 @@ describe('Scheduling UMD bundle', () => {
});
function filterPrivateKeys(name) {
return !name.startsWith('_');
// TODO: Figure out how to forward priority levels.

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Probably should inline them? Also should use Symbols, with a fallback to magic numbers.

@sizebot

This comment has been minimized.

sizebot commented Sep 25, 2018

React: size: 🔺+10.9%, gzip: 🔺+8.3%

Details of bundled changes.

Comparing: 970a34b...a92dc96

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +7.7% +5.6% 83.75 KB 90.2 KB 22.69 KB 23.96 KB UMD_DEV
react.production.min.js 🔺+10.9% 🔺+8.3% 10.16 KB 11.27 KB 4.13 KB 4.48 KB UMD_PROD
react.profiling.min.js +9.0% +6.8% 12.31 KB 13.43 KB 4.67 KB 4.99 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 +48.2% +30.5% 13.86 KB 20.55 KB 4.25 KB 5.55 KB NODE_DEV
scheduler.production.min.js 🔺+41.0% 🔺+26.8% 3.18 KB 4.49 KB 1.39 KB 1.77 KB NODE_PROD
Scheduler-dev.js +48.2% +30.5% 14.04 KB 20.81 KB 4.28 KB 5.59 KB FB_WWW_DEV
Scheduler-prod.js 🔺+55.9% 🔺+33.2% 8.13 KB 12.68 KB 2.06 KB 2.74 KB FB_WWW_PROD

Generated by 🚫 dangerJS

// Math.pow(2, 30) - 1
// 0b111111111111111111111111111111
var maxSigned31BitInt = 1073741823;

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

Why not just import maxSigned31BitInt.js

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Because eventually this will live in a separate repo

firstCallbackNode.expirationTime < currentExpirationTime
) {
return 0;
}

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

I'm a little confused here, it seems we can just do

const now = hasNativePerformanceNow ? performance.now() : Date.now();
// or for tree-shaking? we can use if (hasNativePerformanceNow) { now = performance.now()} else { now = Date.now() }

timeRemaining = function() { ... } // we just need do this once rather than twice currently

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Whether a native lib like performance.now is available depends on a fixed thing, e.g. the browser+version. So assigning the function up front avoids us having to do a conditional check inside of a very "hot" function (one that's called lots of times). The resulting code size will be slightly larger but it's worth the runtime performance gains.

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 26, 2018

Contributor

@bvaughn I don't understand. The code I show above don't "do a conditional check inside of a very "hot" function", it just do the check once when the index.js which be bundled first run. And it reduces duplicate code. So it's a win-win thing

This comment has been minimized.

@bvaughn

bvaughn Sep 26, 2018

Contributor

The timeRemaining function is called many times, and so it's performance sensitive. Each time it's called, it needs to read the current time (now) so setting this value once would not work.

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 26, 2018

Contributor

Sorry for the typo, I mean:
const now = hasNativePerformanceNow ? performance.now : Date.now, so later we can just use now() to read the current time. @bvaughn

next.previous = previous;
var lastCallbackNode = firstCallbackNode.previous;
firstCallbackNode = lastCallbackNode.next = next;
next.previous = lastCallbackNode;

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

nice rename ~

var node = firstCallbackNode;
do {
if (node.expirationTime >= expirationTime) {
// This callback is equal or lower priority than the new one.

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

Maybe "This callback priority is equal or lower than the new one" is better just IMO

previous: null,
};
// Insert the new callback into the list, sorted by its timeout.

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

timeout -> expirationTime?

This comment has been minimized.

@NE-SmallTown

NE-SmallTown Sep 25, 2018

Contributor

And seems it doesn't do sort, it just do a find & insert operation

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 25, 2018

So for now this just adds things? How much do you anticipate being able to remove (to balance out the size increase)?

@bvaughn bvaughn self-assigned this Sep 25, 2018

@acdlite

This comment has been minimized.

Member

acdlite commented Sep 25, 2018

@gaearon This PR adds roughly 200 lines of code to the Scheduler package. I expect this will be offset when we reimplement React's root scheduling and expiration time system on top of the new Scheduler primitives.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 25, 2018

Right. Now that I re-read it, it only increased UMD and 10% is actually pretty little compared to overall size of React UMD.

@bvaughn

This looks good 👍

<head>
<meta charset="utf-8">
<title>Scheduler Test Page</title>

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Yeah, because of the renamed schedule method 😄

// TODO: Use symbols?
var ImmediatePriority = 1;
var InteractivePriority = 2;
var NormalPriority = 3;

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

I don't feel strongly about this, but I prefer DefaultPriority

var IMMEDIATE_PRIORITY_TIMEOUT = -1;
// Eventually times out
var INTERACTIVE_PRIORITY_TIMEOUT = 250;
var DEFAULT_PRIORITY_TIMEOUT = 5000;

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Either we should rename NormalPriority -> DefaultPriority or we should rename this to NORMAL_PRIORITY_TIMEOUT (but I prefer the former)

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

D'oh. Good catch. I keep going back and forth on which one I prefer, but usually in conversation I end up saying "normal" so that's what I went with here. These aren't final though.

firstCallbackNode.expirationTime < currentExpirationTime
) {
return 0;
}

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Whether a native lib like performance.now is available depends on a fixed thing, e.g. the browser+version. So assigning the function up front avoids us having to do a conditional check inside of a very "hot" function (one that's called lots of times). The resulting code size will be slightly larger but it's worth the runtime performance gains.

} else {
var nextAfterContinuation = null;
var node = firstCallbackNode;
do {

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

So this loop is handling the case where a higher priority callback is scheduled while we're executing and a continuation is returned– so we want to drop the continuation in where the previous callback was, without it preempting the higher priority work?

I think this is not obvious from the scope of this function and we should add an inline comment.

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Yeah I'll add a comment. It's mostly just a fork of scheduleWork but it inserts the continuation before the first callback with equal expiration instead of after the last callback with equal expiration time.

} finally {
currentPriorityLevel = previousPriorityLevel;
currentEventStartTime = previousEventStartTime;
flushImmediateWork();

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Is it intentional that we still flush immediate in the event of an error?

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Yeah it's like try/finally. I'll add a test.

} finally {
currentPriorityLevel = previousPriorityLevel;
currentEventStartTime = previousEventStartTime;
flushImmediateWork();

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

(Same question about flushing after an error)

var nextAfterContinuation = null;
var node = firstCallbackNode;
do {
if (node.expirationTime >= expirationTime) {

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Might be worth a comment here that we check ">=" (instead of ">" like in unstable_scheduleCallback) intentionally, because we want the continuation to be the first callback with this priority. (It's probably not that subtle but still may be worth mentioning explicitly...)

'B',
'Schedule high pri',
// Even though there's time left in the frame, the low pri callback
// should yield to the high pri callback

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

Nice! Glad to see this explicitly tested

// Now advance by just a bit more
it('wrapped callbacks inherit the current even when nested', () => {

This comment has been minimized.

@bvaughn

bvaughn Sep 25, 2018

Contributor

nit "wrapped callbacks inherit the current priority" ?

if (
tasks.length > 0 &&
!deadline.didTimeout &&
deadline.timeRemaining() <= 0

This comment has been minimized.

@plievone

plievone Sep 25, 2018

Contributor

Drive-by-comment: This seems to be the suggested scheduler usage code, so could you clarify these terms more? 1) Perhaps deadline.didTimeout is rather didExpire (as you have called these elsewhere), but is it a deadline that expires or is there a way to express this without negation. 2) Also what happens if one forgets to check expiration, I would guess the scheduler calls us immediately again so it works, just slower, or is the problem more drastic. 3) Also timeRemaining is about the current frame (or how do you call it) which might confuse the user as it reads now, just after the didTimeout.

This comment has been minimized.

@acdlite

acdlite Sep 25, 2018

Member

Thanks for the feedback. Note that the actual names used here are not final yet.

Perhaps deadline.didTimeout is rather didExpire (as you have called these elsewhere), but is it a deadline that expires or is there a way to express this without negation

The deadline naming is inherited from requestIdleCallback. You're right that the naming is confusing because it's about the frame deadline, which is different from the expiration. We haven't figured out the best terms to use yet but we'll take all this into consideration before we reach stable.

Also what happens if one forgets to check expiration, I would guess the scheduler calls us immediately again so it works, just slower, or is the problem more drastic.

If you forget to check if the work is expired, but you do check timeRemaining(), then it will still work because timeRemaining() will be 0 (though I guess I'm missing a test for this). Maybe this implies that we should unify the two APIs into one (these are also inherited from requestIdleCallback). I think it's likely we'll replace both with a single shouldYield() method. But I do think it's useful to provide a estimate for how much time before the next frame. But that could be a separate API.

[scheduler] Priority levels, continuations, and wrapped callbacks
All of these features are based on features of React's internal
scheduler. The eventual goal is to lift as much as possible out of the
React internals into the Scheduler package.

Includes some renaming of existing methods.

- `scheduleWork` is now `scheduleCallback`
- `cancelScheduledWork` is now `cancelCallback`


Priority levels
---------------

Adds the ability to schedule callbacks at different priority levels.
The current levels are (final names TBD):

- Immediate priority. Fires at the end of the outermost currently
executing (similar to a microtask).
- Interactive priority. Fires within a few hundred milliseconds. This
should only be used to provide quick feedback to the user as a result
of an interaction.
- Normal priority. This is the default. Fires within several seconds.
- "Maybe" priority. Only fires if there's nothing else to do. Used for
prerendering or warming a cache.

The priority is changed using `runWithPriority`:

```js
runWithPriority(InteractivePriority, () => {
  scheduleCallback(callback);
});
```


Continuations
-------------

Adds the ability for a callback to yield without losing its place
in the queue, by returning a continuation. The continuation will have
the same expiration as the callback that yielded.


Wrapped callbacks
-----------------

Adds the ability to wrap a callback so that, when it is called, it
receives the priority of the current execution context.

@acdlite acdlite merged commit f305d2a into facebook:master Sep 25, 2018

1 check was pending

ci/circleci CircleCI is running your tests
Details

acdlite added a commit to plievone/react that referenced this pull request Oct 5, 2018

[scheduler] Priority levels, continuations, and wrapped callbacks (fa…
…cebook#13720)

All of these features are based on features of React's internal
scheduler. The eventual goal is to lift as much as possible out of the
React internals into the Scheduler package.

Includes some renaming of existing methods.

- `scheduleWork` is now `scheduleCallback`
- `cancelScheduledWork` is now `cancelCallback`


Priority levels
---------------

Adds the ability to schedule callbacks at different priority levels.
The current levels are (final names TBD):

- Immediate priority. Fires at the end of the outermost currently
executing (similar to a microtask).
- Interactive priority. Fires within a few hundred milliseconds. This
should only be used to provide quick feedback to the user as a result
of an interaction.
- Normal priority. This is the default. Fires within several seconds.
- "Maybe" priority. Only fires if there's nothing else to do. Used for
prerendering or warming a cache.

The priority is changed using `runWithPriority`:

```js
runWithPriority(InteractivePriority, () => {
  scheduleCallback(callback);
});
```


Continuations
-------------

Adds the ability for a callback to yield without losing its place
in the queue, by returning a continuation. The continuation will have
the same expiration as the callback that yielded.


Wrapped callbacks
-----------------

Adds the ability to wrap a callback so that, when it is called, it
receives the priority of the current execution context.

@gaearon gaearon referenced this pull request Oct 23, 2018

Merged

Add 16.6.0 changelog #13927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment