[schedule] Support multiple callbacks in scheduler #12746

flarnie · 2018-05-04T23:13:30Z

NOTE: We still need a way to test the timing out of callbacks, probably with some type of injection we we could create a 'noopSchedule' similar to 'noopRenderer'.

I'm going to go back and implement that as a 2/n PR, but wanted review of this because it's the direction I would ideally take after that.

This PR would be followed by a PR to add error handling.

Support multiple callbacks in `ReactScheduler`

**what is the change?:**
We keep a queue of callbacks instead of just one at a time, and call
them in order first by their timeoutTime and then by the order which
they were scheduled in.

**why make this change?:**
We plan on using this module to coordinate JS outside of React, so we
will need to schedule more than one callback at a time.

**test plan:**
Added a boatload of shiny new tests. :)

Plus ran all the old ones.

**what is the change?:** see title **why make this change?:** Once we support multiple callbacks you will need to use the id to specify which callback you mean. **test plan:** Added a test, ran all tests, lint, etc.

**what is the change?:** We keep a queue of callbacks instead of just one at a time, and call them in order first by their timeoutTime and then by the order which they were scheduled in. **why make this change?:** We plan on using this module to coordinate JS outside of React, so we will need to schedule more than one callback at a time. **test plan:** Added a boatload of shiny new tests. :) Plus ran all the old ones. NOTE: The tests do not yet cover the vital logic of callbacks timing out, and later commits will add the missing test coverage.

**what is the change?:** Tracks the current soonest timeOut time for all scheduled callbacks. **why make this change?:** We were checking every scheduled callback to see if it timed out on every tick. It's more efficient to skip that O(n) check if we know that none have timed out. **test plan:** Ran existing tests. Will write new tests to cover timeout behavior in more detail soon.

**what is the change?:** See title **why make this change?:** We don't have error handling in place yet, so should maintain the old behavior until that is in place. But want to get this far to continue making incremental changes. **test plan:** Updated and ran tests.

**what is the change?:** see title **why make this change?:** We haven't added error handling yet, so should not expose this feature. **test plan:** Ran all tests, temporarily split out the tests for multiple callbacks into separate file. Will recombine once we remove the flag.

pull-bot · 2018-05-04T23:23:09Z

ReactDOM: size: 🔺+0.4%, gzip: 🔺+0.4%

Details of bundled changes.

Comparing: 25dda90...5b1c090

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.5%	+0.6%	611.89 KB	614.9 KB	141.4 KB	142.2 KB	UMD_DEV
react-dom.production.min.js	🔺+0.4%	🔺+0.4%	100.07 KB	100.43 KB	31.84 KB	31.98 KB	UMD_PROD
react-dom.development.js	+0.5%	+0.6%	596.26 KB	599.28 KB	137.23 KB	138.05 KB	NODE_DEV
react-dom.production.min.js	🔺+0.4%	🔺+0.4%	98.51 KB	98.87 KB	31.07 KB	31.21 KB	NODE_PROD
ReactDOM-dev.js	+0.5%	+0.6%	620.7 KB	623.75 KB	140 KB	140.84 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.5%	🔺+0.6%	284.23 KB	285.74 KB	51.94 KB	52.25 KB	FB_WWW_PROD
ReactDOMServer-dev.js	0.0%	-0.0%	94.11 KB	94.11 KB	24.03 KB	24.03 KB	FB_WWW_DEV

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.7%	+0.9%	414.79 KB	417.84 KB	90.31 KB	91.16 KB	UMD_DEV
react-art.production.min.js	🔺+0.4%	🔺+0.5%	90.39 KB	90.76 KB	27.49 KB	27.63 KB	UMD_PROD
react-art.development.js	+0.9%	+1.2%	340.63 KB	343.67 KB	71.63 KB	72.49 KB	NODE_DEV
react-art.production.min.js	🔺+0.7%	🔺+0.9%	54.92 KB	55.28 KB	16.78 KB	16.92 KB	NODE_PROD
ReactART-dev.js	+0.9%	+1.2%	348.89 KB	351.97 KB	71.22 KB	72.08 KB	FB_WWW_DEV
ReactART-prod.js	🔺+0.9%	🔺+1.1%	166.97 KB	168.5 KB	27.44 KB	27.74 KB	FB_WWW_PROD

react-scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-scheduler.development.js	+29.7%	+24.4%	10.2 KB	13.23 KB	3.6 KB	4.48 KB	UMD_DEV
react-scheduler.production.min.js	🔺+22.2%	🔺+20.0%	1.58 KB	1.93 KB	860 B	1.01 KB	UMD_PROD
react-scheduler.development.js	+30.3%	+24.9%	10.01 KB	13.04 KB	3.55 KB	4.43 KB	NODE_DEV
react-scheduler.production.min.js	🔺+21.9%	🔺+20.7%	1.66 KB	2.02 KB	871 B	1.03 KB	NODE_PROD

Generated by 🚫 dangerJS

acdlite · 2018-05-09T20:10:01Z

packages/react-scheduler/src/ReactScheduler.js

+  // unregistered by removing the id from this object.
+  // Then we skip calling any callback which is not registered.
+  // This means cancelling is an O(1) time complexity instead of O(n).
+  const registeredCallbackIds: {[number]: boolean} = {};


Sebastian said in our last discussion to not use a Map, mainly to avoid dependencies on polyfills.

-> We don't use 'Map' because this library is intended for use outside of React, don't want to add dependency on a polyfill if we can avoid it.

in the previous PR in the stack.

That was in reference to using a linked list entry instead, not if we're going to use a Map algorithm.

We still could use a linked list. Is the potential size of the map a concern?

The linked list would return the link to user space. Not a number.

Removing the link is:

link.next.prev = link.prev; link.prev.next = link.next;

Removing from a Map is O(n) worst case. The O(1) big O notation is misleading for maps. Accessing a few pointers is always much faster than scanning a tree no matter how clever your search is.

The linked list can add more GC work in theory but they'll mostly be young generation.

Yeah I think we should use a linked list instead of ids for cancellation, but it's fine to fix that in a follow-up.

I like the efficiency. Would be curious to know more about the efficiency of deletion in a Map or object. It sounds like the slowest way is to search in an array and delete with splice, then the next fastest is deleting from a Map, and then even faster is using a linked list.

It's possible that other users of this API are relying on the fact that the 'id' returned is typed as a number, but seems unlikely. I had initially avoided changing the type because it introduces inconsistency in our API for how the 'cancel' method works in our renderer in general.

Note that in V8, objects with numeric keys are slow. So if we don't use a Map here (or something else) then it might be a good idea to prefix keys, e.g. id_${id}.

Thanks - that is useful! Will keep it in mind if we end up keeping the map/array approach.

I have a PR that moves this to a linked list1, but before landing that needed to open a PR to increase test coverage.2

I need to bookmark this.

Even today's scheduler uses minHeap instead of linked list, I think this discussion in the past is super valuable: it well represents how React core team thinks of "optimization" and "performance", not just using a {} or [] to cache value.

I will probably use this as an argument when I heard "it makes no sense to interview frontend with data structure and algorithm"..well in this context it does make sense :)

acdlite · 2018-05-09T20:13:52Z

packages/react-scheduler/src/ReactScheduler.js

+        const timeoutTime = currentCallbackConfig.timeoutTime;
+        if (timeoutTime !== -1 && timeoutTime <= currentTime) {
+          // it has timed out!
+          foundTimedOutCallback = true;


What is this for? Don't we have to check the entire list regardless?

I don't understand why this is a nested loop. What is the purpose of the outer loop?

The nesting is because a callback in the list might time out while we are in the process of calling other callbacks.

Example:

We have callbacks [A, B, C] and B has timed out.

We call B. It takes a while to run, and while it is running A times out.

We finish our loop by checking C, but we don't know that A has also now timed out!

So that's why we keep looping until we verify that nothing has timed out.

Discussing in person, we agreed that it could just get pushed into the next tick if something times out while one cb is running.

acdlite · 2018-05-09T20:15:41Z

packages/shared/ReactFeatureFlags.js

 export const warnAboutDeprecatedLifecycles = false;

+// Controls enabling of experimental multi-callback support in 'schedule'
+export const scheduleModuleSupportsMultipleCallbacks = false;


Why is this a feature flag? I don't think there's any valid use of the schedule module unless it supports multiple callbacks. Except for its use by React, but in that case it's only used for async mode, which is already behind its own feature flag. I don't think we need this one.

Was being cautious, but if we can skip the feature flag that is simpler.

See comments on facebook#12743

**what is the change?:** We used to re-run any callbacks which time out whilst other callbacks are running, but now we will only check once for timed out callbacks then then run them. **why make this change?:** To simplify the code and the behavior of this module. **test plan:** Ran all existing tests.

**what is the change?:** see title **why make this change?:** Because only React is using this, and it sounds like async. rendering won't hit any different behavior due to these changes. **test plan:** Existing tests pass, and this allowed us to recombine all tests to run in both 'test' and 'test-build' modes.

flarnie · 2018-05-09T21:50:11Z

@acdlite this is ready for re-review, I'm going to start working on top of it and will do non-critical improvements in the follow-up PR.

acdlite

I think this is good enough to land so we can start trying it out in www, and address the remaining stuff in follow up PRs.

flarnie added 9 commits May 4, 2018 10:37

Support using id to cancel scheduled callback

1d0c68d

**what is the change?:** see title **why make this change?:** Once we support multiple callbacks you will need to use the id to specify which callback you mean. **test plan:** Added a test, ran all tests, lint, etc.

ran prettier

e32075c

fix lint

5faa555

Use object for storing callback info in scheduler

be7254b

Wrap initial test in a describe block

499483a

flarnie requested review from acdlite and sebmarkbage May 4, 2018 23:13

facebook-github-bot added the CLA Signed label May 4, 2018

acdlite requested changes May 9, 2018

View reviewed changes

flarnie mentioned this pull request May 9, 2018

[schedule 1/n] Support cancelling scheduled callback with an id #12743

Closed

flarnie added 6 commits May 9, 2018 13:40

Fix nits from code review

740509c

See comments on facebook#12743

update checklist in comments

b8eca99

remove outdated file

b739a92

fix typo

5b1c090

flarnie changed the title ~~[schedule 3/n] Support multiple callbacks in scheduler~~ [schedule] Support multiple callbacks in scheduler May 9, 2018

acdlite approved these changes May 9, 2018

View reviewed changes

flarnie merged commit a9abd27 into facebook:master May 9, 2018

[schedule] Support multiple callbacks in scheduler #12746

[schedule] Support multiple callbacks in scheduler #12746

Uh oh!

Conversation

flarnie commented May 4, 2018

Uh oh!

pull-bot commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

react-dom

react-art

react-scheduler

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkjpotato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flarnie commented May 9, 2018

Uh oh!

acdlite left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pull-bot commented May 4, 2018 •

edited

Loading

sebmarkbage May 9, 2018 •

edited

Loading

hkjpotato Nov 18, 2021 •

edited

Loading