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] Store Tasks on a Min Binary Heap #16245

Merged
merged 2 commits into from Aug 8, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 29, 2019

Switches Scheduler's priority queue implementation (for both tasks and timers) to an array-based min binary heap.

This replaces the naive linked-list implementation that was left over from the queue we once used to schedule React roots. A list was arguably fine when it was only used for roots, since the total number of roots is usually small, and is only 1 in the common case of a single-page app.

Since Scheduler is now used for many types of JavaScript tasks (e.g. including timers), the total number of tasks can be much larger.

Heaps are the standard way to implement priority queues. Insertion is O(1) in the average case (append to the end) and O(log n) in the worst. Deletion is O(log n). Peek is O(1).

@sizebot
Copy link

sizebot commented Jul 29, 2019

React: size: -1.4%, gzip: 0.0%

Details of bundled changes.

Comparing: 95767ac...f2318c1

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js -2.3% -1.1% 117.26 KB 114.55 KB 29.39 KB 29.07 KB UMD_DEV
react.production.min.js -1.4% 0.0% 13.03 KB 12.84 KB 5.08 KB 5.08 KB UMD_PROD
react.profiling.min.js -1.3% +0.3% 15.23 KB 15.03 KB 5.61 KB 5.63 KB UMD_PROFILING
react.production.min.js 0.0% -0.0% 6.68 KB 6.68 KB 2.75 KB 2.75 KB NODE_PROD
React-dev.js 0.0% -0.0% 70.14 KB 70.14 KB 17.98 KB 17.98 KB FB_WWW_DEV
React-prod.js 0.0% 0.0% 17.39 KB 17.39 KB 4.54 KB 4.54 KB FB_WWW_PROD
React-profiling.js 0.0% 0.0% 17.39 KB 17.39 KB 4.54 KB 4.54 KB FB_WWW_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
SchedulerMock-dev.js -13.1% -6.4% 21.36 KB 18.55 KB 4.66 KB 4.36 KB FB_WWW_DEV
SchedulerMock-prod.js -5.3% 🔺+1.2% 13.94 KB 13.2 KB 2.93 KB 2.96 KB FB_WWW_PROD
scheduler.development.js -9.1% -3.8% 29.96 KB 27.24 KB 7.35 KB 7.07 KB NODE_DEV
scheduler.production.min.js -3.7% 🔺+1.5% 5.68 KB 5.48 KB 2.18 KB 2.21 KB NODE_PROD
Scheduler-dev.js -9.1% -3.9% 30.79 KB 27.98 KB 7.48 KB 7.19 KB FB_WWW_DEV
Scheduler-prod.js -4.5% 🔺+0.1% 17.92 KB 17.12 KB 3.7 KB 3.71 KB FB_WWW_PROD
scheduler-tracing.production.min.js 0.0% 🔺+0.3% 728 B 728 B 382 B 383 B NODE_PROD
scheduler-unstable_mock.development.js -13.0% -5.9% 20.8 KB 18.09 KB 4.56 KB 4.29 KB UMD_DEV
scheduler-tracing.profiling.min.js 0.0% -0.2% 3.26 KB 3.26 KB 1001 B 999 B NODE_PROFILING
scheduler-unstable_mock.production.min.js -4.1% 🔺+2.2% 5.08 KB 4.88 KB 2 KB 2.04 KB UMD_PROD
scheduler-unstable_mock.development.js -13.2% -5.9% 20.61 KB 17.9 KB 4.5 KB 4.24 KB NODE_DEV
scheduler-unstable_mock.production.min.js -4.1% 🔺+1.8% 5.07 KB 4.86 KB 1.94 KB 1.98 KB NODE_PROD

Generated by 🚫 dangerJS

@@ -0,0 +1,92 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean we've given up on having a single easy to reason about file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Personally I don’t think it’s worth it but I can move this into the same file if you want me to

To maximize adoption, I think polyfilling the native API is the better approach.

@acdlite acdlite force-pushed the scheduler-min-heap branch 2 times, most recently from bd83da5 to 0e2c76a Compare July 30, 2019 04:43
if (typeof continuationCallback === 'function') {
var expirationTime = task.expirationTime;
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 whole branch is left over from before the "return a continuation" API existed. Instead of popping the task from the queue, checking if there's a continuation, and then rescheduling it, we can peek at the task and only remove it once it completes. If there's a continuation, then the task remains in its current position.


function siftUp(heap, node, index) {
while (index > 0) {
const parentIndex = Math.floor((index + 1) / 2) - 1;
Copy link
Collaborator

@sophiebits sophiebits Aug 3, 2019

Choose a reason for hiding this comment

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

Math.floor((index - 1) / 2) is equivalent and simpler?

const first = heap[0];
if (first !== undefined) {
const last = heap.pop();
if (last !== undefined && last !== first) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

last === undefined never happens, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think maybe this was to satisfy Flow? Will check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind, forgot Flow array access is intentionally unsafe for exactly this reason :D


function siftDown(heap, node, index) {
const length = heap.length;
while (index < length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is never false, right? Should it just be while (true)?

task.next = task.previous = null;
// Null out the callback to indicate the task has been canceled. (Can't remove
// from the queue because you can't remove arbitrary nodes from an array based
// heap, only the first one.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you sift up/down as appropriate?

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 don’t think so because I don’t know the index of the task.

@acdlite
Copy link
Collaborator Author

acdlite commented Aug 3, 2019

Will wait to merge this until after 16.9 is released

Switches Scheduler's priority queue implementation (for both tasks and
timers) to an array-based min binary heap.

This replaces the naive linked-list implementation that was left over
from the queue we once used to schedule React roots. A list was arguably
fine when it was only used for roots, since the total number of roots is
usually small, and is only 1 in the common case of a single-page app.

Since Scheduler is now used for many types of JavaScript tasks (e.g.
including timers), the total number of tasks can be much larger.

Binary heaps are the standard way to implement priority queues.
Insertion is O(1) in the average case (append to the end) and O(log n)
in the worst. Deletion is O(log n). Peek is O(1).
@acdlite acdlite merged commit d77c623 into facebook:master Aug 8, 2019
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.

None yet

5 participants