[Scheduler] shouldYield should always return false from inside an expired task #17968
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The mock Scheduler that we use in our tests has its own fake timer implementation. The `unstable_advanceTime` method advances the timeline. Currently, a call to `unstable_advanceTime` will also flush any pending expired work. But that's not how it works in the browser: when a timer fires, the corresponding task is added to the Scheduler queue. However, we will still wait until the next message event before flushing it. This commit changes `unstable_advanceTime` to more closely resemble the browser behavior, by removing the automatic flushing of expired work. ```js // Before this commit Scheduler.unstable_advanceTime(ms); // Equivalent behavior after this commit Scheduler.unstable_advanceTime(ms); Scheduler.unstable_flushExpired(); ``` The general principle is to prefer separate APIs for scheduling tasks and flushing them. This change does not affect any public APIs. `unstable_advanceTime` is only used by our own test suite. It is not used by `act`. However, we may need to update tests in www, like Relay's.
The current behavior of shouldYield is unspecified except in cases that happen to be relied on by the React work loop's implementation. However, some of the unspecified cases are important and the consequences of assuming the wrong behavior are really bad. For example, it's natural to assume that `shouldYield` returns false when called from within an expired task. But the current behavior usually does that, unless there's a higher priority task or a main thread task (i.e. at the end of the 5ms message loop interval). This fixes `shouldYield` so that it always returns false inside an expired task, regardless of the other tasks in the queue.
Since the queue is already sorted by priority, it's faster and less error-prone for `shouldYield` to check if the current task is at the head of the list. The only caveat is that a higher priority task may have been canceled, but is still part of the queue. This is because we don't always remove canceled tasks from the queue immediately, because deletion of an arbitrary node from a binary heap is O(log n). We address this by popping high priority canceled tasks from the queue upon the next call to `shouldYield`. Since this is an edge case, and since it prevents us from yielding unnecessarily, the shifted cost is worth it. It's not extra work because we would have done this anyway the next time we enter the Scheduler work loop. Another argument in favor is that it's better to do this work during a yieldy, default priority task than at the beginning of a blocking, high priority task.
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 776b4e4:
|
Collaborator
|
It seems to me that shouldYield is already way above its budget for such a hot function. I'd expect a single atomic read basically. It also seems to me that we're getting rid of the notion of "expiration time" in the scheduler since we won't have a timeoutMs option and the native one won't work this way anyway so this doesn't translate. I think we should just simplify this check to only yield every 5ms regardless if higher pri tasks are scheduled or anything else. |
sebmarkbage
requested changes
Feb 12, 2020
Collaborator
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
I believe the last thing we said was that we would instead make shouldYield always return true after 5ms regardless. This makes the code smaller and the hot path the fastest we can make it.
Collaborator
|
Closing as stale. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #17967
The current behavior of shouldYield is unspecified except in cases that happen to be relied on by the React work loop's implementation. However, some of the unspecified cases are important and the consequences of assuming the wrong behavior are really bad.
For example, it's natural to assume that
shouldYieldreturns false when called from within an expired task. But the current behavior usually does that, unless there's a higher priority task or a main thread task (i.e. at the end of the 5ms message loop interval).This fixes
shouldYieldso that it always returns false inside an expired task, regardless of the other tasks in the queue.