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

Add support for multiple callbacks in ReactScheduler #12682

Closed
wants to merge 14 commits into from

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented Apr 24, 2018

what is the change?:
We want to support calling ReactScheduler multiple times with different
callbacks, even if the initial callback hasn't been called yet.

There are two possible ways ReactScheduler can handle multiple
callbacks, and in one case we know that callbackA depends on callbackB
having been called already. For example;
callbackA -> updates SelectionState in a textArea
callbackB -> processes the deletion of the text currently selected.

We want to ensure that callbackA happens before callbackB. For now we
will flush callbackB as soon as callbackA is added.

In the next commit we'll split this into two methods, which support two
different behaviors here. We will support the usual behavior, which
would defer both callbackA and callbackB.

One issue with this is that we now create a new object to pass to the
callback for every use of the scheduler, while before we reused the same
object and mutated the 'didExpire' before passing it to each new
callback. With multiple callbacks, I think this leads to a risk of
mutating the object which is being used by multiple callbacks.

why make this change?:
We want to use this scheduling helper to coordinate between React and
non-React scripts.

test plan:
Added and ran a unit test.

@pull-bot
Copy link

pull-bot commented Apr 24, 2018

ReactDOM: size: 🔺+0.7%, gzip: 🔺+0.8%

Details of bundled changes.

Comparing: 25dda90...5ba093b

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.7% +0.6% 611.89 KB 616.08 KB 141.4 KB 142.28 KB UMD_DEV
react-dom.production.min.js 🔺+0.7% 🔺+0.8% 100.07 KB 100.8 KB 31.84 KB 32.1 KB UMD_PROD
react-dom.development.js +0.7% +0.7% 596.26 KB 600.46 KB 137.23 KB 138.14 KB NODE_DEV
react-dom.production.min.js 🔺+0.7% 🔺+0.8% 98.51 KB 99.24 KB 31.07 KB 31.31 KB NODE_PROD
ReactDOM-dev.js +0.7% +0.7% 620.7 KB 624.97 KB 140 KB 140.92 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.9% 🔺+0.9% 284.23 KB 286.76 KB 51.94 KB 52.4 KB FB_WWW_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +1.0% +1.0% 414.79 KB 419.01 KB 90.31 KB 91.24 KB UMD_DEV
react-art.production.min.js 🔺+0.8% 🔺+1.0% 90.39 KB 91.13 KB 27.49 KB 27.77 KB UMD_PROD
react-art.development.js +1.2% +1.3% 340.63 KB 344.85 KB 71.63 KB 72.56 KB NODE_DEV
react-art.production.min.js 🔺+1.3% 🔺+1.5% 54.92 KB 55.65 KB 16.78 KB 17.03 KB NODE_PROD
ReactART-dev.js +1.2% +1.3% 348.89 KB 353.18 KB 71.22 KB 72.16 KB FB_WWW_DEV
ReactART-prod.js 🔺+1.5% 🔺+1.6% 166.97 KB 169.52 KB 27.44 KB 27.87 KB FB_WWW_PROD

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +41.3% +27.8% 10.2 KB 14.42 KB 3.6 KB 4.6 KB UMD_DEV
react-scheduler.production.min.js 🔺+45.1% 🔺+33.5% 1.58 KB 2.29 KB 860 B 1.12 KB UMD_PROD
react-scheduler.development.js +42.1% +28.3% 10.01 KB 14.23 KB 3.55 KB 4.55 KB NODE_DEV
react-scheduler.production.min.js 🔺+44.1% 🔺+33.5% 1.66 KB 2.39 KB 871 B 1.14 KB NODE_PROD

Generated by 🚫 dangerJS

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

See last comment

const remaining = frameDeadline - now();
return remaining > 0 ? remaining : 0;
},
const buildFrameDeadlineObject = function(didTimeout: boolean) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per our discussion, let's go back to having a single, pooled deadline object

// define a helper for this because they should usually happen together
const clearTimeoutTimeAndScheduledCallback = function() {
timeoutTime = -1;
scheduledRICCallback = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name of this function and its implementation are identical. I think that's a smell. Seems like the point of the helper is so you don't accidentally reset the callback without also resetting timeoutTime. So maybe can we give it a more semantic name, like resetScheduledCallback?

rIC(callbackB);
expect(callbackA.mock.calls.length).toBe(1);
expect(callbackB.mock.calls.length).toBe(0);
// after a delay, calls the latest callback passed
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should assert that the callbacks were called in the correct order

// serial updates, such that each update relies on the previous ones
// having been called before it runs.
const didTimeout = (timeoutTime !== -1) && (timeoutTime <= now());
scheduledRICCallback(buildFrameDeadlineObject(didTimeout));
Copy link
Collaborator

@acdlite acdlite Apr 25, 2018

Choose a reason for hiding this comment

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

It's common for callbacks to reschedule themselves if time runs out in the frame and they have remaining work. (The exception is callbacks that ignore the deadline object entirely.) I don't think this implementation accounts for that case.

Example:

function callbackB() {}

function callbackA(deadline) {
  while (hasWork()) {
    if (!deadline.didTimeout && deadline.timeRemaining() < 1) {
      // Ran out of time. Reschedule self so we can continue later.
      rIC(callbackA);
    }
    doUnitOfWork();
  }
}

// Schedule a callback
rIC(callbackA);
// With your PR, this will early flush callback A...
// If callback A runs out of time, it will reschedule itself, all before we get a chance to schedule callback B.
rIC(callbackB);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

I will write some tests around this and take it into account.

let isIdleScheduled = false;
let timeoutTime = -1;
let isCurrentlyRunningCallback = false;
// We may need to keep a queue of pending callbacks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need to keep a queue... -> We keep a queue...

if (!isAnimationFrameScheduled) {
// If rAF didn't already schedule one, we need to schedule a frame.
// TODO: If this rAF doesn't materialize because the browser throttles, we
// might want to still have setTimeout trigger rIC as a backup to ensure
// that we keep performing work.
isAnimationFrameScheduled = true;
requestAnimationFrame(animationTick);
return requestAnimationFrame(animationTick);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this method is supposed to return a number, and it seemed to me it should return the request id from the rAF call. But I can remove that change if it needs further discussion.

Copy link
Contributor Author

@flarnie flarnie Apr 27, 2018

Choose a reason for hiding this comment

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

Realized this doesn't quite make sense, because sometimes the initial rAF call just schedules a new rAF call, so even if you got the id of the first call, and called 'cancelAnimationFrame' you might not cancel the overall chain of rAF calls.

I'll just always return 0 as a compromise, but really the return value doesn't mean much and I'd rather drop the return value.

It depends on how the 'cIC' method will work once we support serial and deferred callbacks. If it takes the callback id, then returning a number from 'rIC' makes sense.

Returning an id which could be used to cancel the callback might be useful, but it depends how we implement cIC for deferred priority callbacks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The number is meant to be used as an id to cancel callbacks. This still matters for this PR, since it is possible to have multiple callbacks scheduled at a time (pendingCallbacks). When I talked to @sebmarkbage about this a few weeks ago, he suggested that instead of using an id — which requires us to store to map of ids to callbacks — we could store the pending callbacks as a doubly-linked list, and the return value would be the item in the list, which the caller treats as opaque. Then you could remove the callback from the list without needing a map. If you want to do that in a follow up, a map is fine for now, but either way we should account for cancellation in this PR.

@flarnie
Copy link
Contributor Author

flarnie commented Apr 26, 2018

I'm not happy with the increased bitsize, this will only continue to get bigger as we add support for non-serial callbacks though.

@gaearon any suggestions of things I can remove to offset this change?

expect(callbackB.mock.calls.length).toBe(1);
expect(callbackLog.length).toBe(2);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you assert on the entire array, like this...

expect(callbackLog).toEqual(['A', 'B']);

then it guarantees that the callbacks are only called once.

It's also easier to read, IMO. I have a harder time trying to follow what's happening when I have to recreate the list of operations in my head :D

frameDeadlineObject.didTimeout =
pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now();
isCurrentlyRunningCallback = true;
pendingCallback(frameDeadlineObject);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This callback could throw, which would leave us in a broken state.

if (options != null && typeof options.timeout === 'number') {
timeoutTime = now() + options.timeout;
} else {
timeoutTime = -1;
}
// If we have previousCallback, call it. This may trigger recursion.
if (
previousCallback &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

These checks are wasteful, since we already checked if scheduleCallback is null above. This block should be moved into the earlier one:

  • Check if there's an already scheduled callback
    • If so, flush it
  • Schedule the incoming callback

// the callback recursively called rIC and new callbacks are pending
const {
pendingCallback,
pendingCallbackTimeout,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also avoid using destructuring, except for module-style functions. Can't remember the full rationale, ask @sebmarkbage. I think the main reason is because it obscures how many reads there are?

callback(frameDeadlineObject);
isCurrentlyRunningCallback = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This callback could throw, in which case isCurrentlyRunningCallback won't be reset.

if (!isAnimationFrameScheduled) {
// If rAF didn't already schedule one, we need to schedule a frame.
// TODO: If this rAF doesn't materialize because the browser throttles, we
// might want to still have setTimeout trigger rIC as a backup to ensure
// that we keep performing work.
isAnimationFrameScheduled = true;
requestAnimationFrame(animationTick);
return requestAnimationFrame(animationTick);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The number is meant to be used as an id to cancel callbacks. This still matters for this PR, since it is possible to have multiple callbacks scheduled at a time (pendingCallbacks). When I talked to @sebmarkbage about this a few weeks ago, he suggested that instead of using an id — which requires us to store to map of ids to callbacks — we could store the pending callbacks as a doubly-linked list, and the return value would be the item in the list, which the caller treats as opaque. Then you could remove the callback from the list without needing a map. If you want to do that in a follow up, a map is fine for now, but either way we should account for cancellation in this PR.

});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs tests for:

  • Errors
  • Timeouts

flarnie added 11 commits May 2, 2018 09:55
**what is the change?:**
We want to support calling ReactScheduler multiple times with different
callbacks, even if the initial callback hasn't been called yet.

There are two possible ways ReactScheduler can handle multiple
callbacks, and in one case we know that callbackA depends on callbackB
having been called already. For example;
callbackA -> updates SelectionState in a textArea
callbackB -> processes the deletion of the text currently selected.

We want to ensure that callbackA happens before callbackB. For now we
will flush callbackB as soon as callbackA is added.

In the next commit we'll split this into two methods, which support two
different behaviors here. We will support the usual behavior, which
would defer both callbackA and callbackB.

One issue with this is that we now create a new object to pass to the
callback for every use of the scheduler, while before we reused the same
object and mutated the 'didExpire' before passing it to each new
callback. With multiple callbacks, I think this leads to a risk of
mutating the object which is being used by multiple callbacks.

**why make this change?:**
We want to use this scheduling helper to coordinate between React and
non-React scripts.

**test plan:**
Added and ran a unit test.
**what is the change?:**
We added support for serial scheduled callbacks to schedule more
callbacks, and maintained the order of first-in first-called.

**why make this change?:**
This is sort of a corner case, but it's totally possible and we do
something similar in React.

We wouldn't do this with serial callbacks, but with deferred callbacks
we do a pattern like this:
```
   +                                             +
   |   +--------------------+ +----------------+ | +--------------------------------+ +-------------------------+
   |   |                    | |                | | |                                | |                         |
   |   | main thread blocked| |callback A runs | | | main thread blocked again      | |callback A runs again, finishes
   |   +--------------------+ +-----+----------+ | +--------------------------------+ ++------------------------+
   v                                ^            v                                     ^
schedule +------------------------->+            no time left!+----------------------->+
callbackA                                        reschedule   |
                                                 callbackA    +
                                                 to do more
                                                 work later.

```

**test plan:**
Wrote some fun new tests and ran them~
Also ran existing React unit tests. As of this PR they are still
directly using this module.
**what is the change?:**
We may call a sequence of callbacks in our `schedule` helper, and this
catches any errors and re-throws them later, so we don't get
interrupted or left in an invalid state.

**why make this change?:**
If you have callbacks A, B, and C scheduled, and B throws, we still want
C to get called.

I would prefer if we could find a way to throw and continue syncronously
but for now this approach, of deferring the error, will work.

**test plan:**
Added a unit test.
**what is the change?:**
see title

**why make this change?:**
We are about to add functionality to cIC, where we take an id.
This is in preparation for when we support 'deferred' as well as
'serial' callbacks.

**test plan:**
Run the test.
**what is the change?:**
Some various clean-up changes to pave the way for adding an 'id' for
each callback which can be used to cancel the callback.

**why make this change?:**
To make the next diff easier.

**test plan:**
Ran the tests.
**what is the change?:**
See title.

**why make this change?:**
When we support multiple callbacks you will need to use the callback id
to cancel a specific callback.

**test plan:**
Tests were updated, ran all tests.
@flarnie
Copy link
Contributor Author

flarnie commented May 2, 2018

Just fixed things up and rebased. ✨

delete registeredCallbackIds[callbackId];
isCurrentlyRunningCallback = false;
// Still throw it, but not in this frame.
setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I'm satisfied with this method of handling errors. We shouldn't swallow them, but don't want to throw when we may be in the middle of a queue of callbacks.
Exposing an 'onError' handler eventually maybe? Or taking an 'onError' option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this in person and considered using postMessage to either throw the error sooner or even to create a postMessage event for each callback, so that errors could be thrown without interrupting the queue of callbacks getting called.

@sebmarkbage we would like you to make the call on whether the setTimeout is good enough for now, or should we explore using postMessage or some other approach?

I also considered saving the errors and just throwing them once we finish whatever is in the queue.

// 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} = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ok if we use a Map here. React itself already has a Map dependency.

let previouslyScheduledCallbackConfig;
if (scheduledCallbackConfig !== null) {
// If we have previous callback config, save it and handle it below
previouslyScheduledCallbackConfig = scheduledCallbackConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acdlite pointed out that having this block, then setting up the next callback, and then calling previouslyScheduledCallback below is confusing. Going to restructure this for clarity, so that we handle all logic related to the previous callback in one conditional block instead of two.

expect(callbackLog.length).toBe(3);
expect(callbackLog[0]).toBe('A');
expect(callbackLog[1]).toBe('B');
expect(callbackLog[2]).toBe('C');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @acdlite for pointing out we can assert expect(callbackLog).toEqual(['A', 'B', 'C']);. Had forgotten that matcher checks every item of arrays and objects for equality.

**what is the change?:**
We previously had the following logic:
- pull any old callback out of the 'scheduledCallback' variable
- put the new callback into 'scheduledCallback'
- call the old callback if we had found one.

Splitting out the logic for handling the old callback into two blocks
was hard to read, so we restructured as so:
- if no old callback was found, just put the new callback into the
'scheduledCallback' variable.
- Otherwise, if we do find an old callback, then:
  - pull any old callback out of the 'scheduledCallback' variable
  - put the new callback into 'scheduledCallback'
  - call the old callback

**why make this change?:**
Code clarity

**test plan:**
Ran the tests
registeredCallbackIds.delete(callbackId);
isCurrentlyRunningCallback = false;
// Still throw it, but not in this frame.
setTimeout(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebmarkbage would like your input here, I think this is the last thing to decide before landing this PR. My previous comment here:


We discussed this in person and considered using postMessage to either throw the error sooner or even to create a postMessage event for each callback, so that errors could be thrown without interrupting the queue of callbacks getting called.

@sebmarkbage we would like you to make the call on whether the setTimeout is good enough for now, or should we explore using postMessage or some other approach?

I also considered saving the errors and just throwing them once we finish whatever is in the queue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not in this frame?

This will mess with debugging since it'll be a caught exception so you can't break point on it. Suspense will destroy the use of "pause on caught exception".

Another technique is to call this recursively with a try/finally and you don't need the catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked a bit more and decided the 'try/finally' trick won't work, because a 2nd error thrown in the queue will get swallowed.

Going to look at alternatives, based on what we currently do in React to handle errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2018-05-03 at 10 04 07 am
screen shot 2018-05-03 at 10 03 59 am

registeredCallbackIds.delete(callbackId);
isCurrentlyRunningCallback = false;
// Still throw it, but not in this frame.
setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not in this frame?

This will mess with debugging since it'll be a caught exception so you can't break point on it. Suspense will destroy the use of "pause on caught exception".

Another technique is to call this recursively with a try/finally and you don't need the catch.

// ignore cancelled callbacks
return;
}
isCurrentlyRunningCallback = true;
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 not necessary. Just reset the list before you start working.

@flarnie
Copy link
Contributor Author

flarnie commented May 3, 2018

Ok - after more discussion we decided to re-architect this as follows:

  • Expose a 'flushSerialUpdates' method, which would synchronously flush serial updates which are pending.
  • When you subscribe multiple serial updates in a row, just keep them in a queue. We don't need to flush them as they are added.

Here is a pseudocode example of the new model:

// event handler...
flushSerialUpdates(); // ensure nothing else is pending so that DOM is in stable state when we run our update
// ... 
scheduleSerialCallback(callbackA); // pendingSerialCallbacks = [callbackA];
// ... 
scheduleSerialCallback(callbackB); // pendingSerialCallbacks = [callbackA, callbackB];
// end of event handler
// in the idle tick we call callbackA, and if time also callbackB

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.

5 participants