Land update lane priorities #20919
Closed
Land update lane priorities #20919
Conversation
This file contains 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
facebook-github-bot
added
CLA Signed
React Core Team
rickhanlonii
commented
Mar 3, 2021
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js
Outdated
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js
Outdated
Show resolved
Hide resolved
|
Comparing: d745597...0388bba Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
rickhanlonii
force-pushed
the
rh-land-decouple
branch
from
24beceb
to
2e48166
Compare
rickhanlonii
commented
Mar 8, 2021
There was a problem hiding this comment.
One notable impact of this change is that anything that was reading Scheduler.getCurrentPriorityLevel expecting it to be the current update priority will be broken. With these changes, nothing internal should be impacted, but maybe some callers like www could.
| @@ -1057,7 +1056,7 @@ function commitUnmount( | |||
| finishedRoot: FiberRoot, | |||
| current: Fiber, | |||
| nearestMountedAncestor: Fiber, | |||
| renderPriorityLevel: ReactPriorityLevel, | |||
| renderPriorityLevel: LanePriority, | |||
There was a problem hiding this comment.
Switched all these methods to pass around a LanePriority now. After this, the only references to this type are in ReactFiberLanes (for converting between), SchedulerWithReactIntegration, and the DevTools.
| const schedulerPriority = | ||
| priorityLevel === NoLanePriority | ||
| ? NormalPriority | ||
| : lanePriorityToSchedulerPriority(priorityLevel); |
There was a problem hiding this comment.
DevTools is expecting the scheduler priority, so I'm converting to it here. I'm not sure what the plan should be for this - i.e should we update devtools to use the updateLanePriority instead of the scheduler priority? cc @bvaughn for interest
| @@ -395,7 +392,7 @@ export function requestUpdateLane(fiber: Fiber): Lane { | |||
| if ((mode & BlockingMode) === NoMode) { | |||
| return (SyncLane: Lane); | |||
| } else if ((mode & ConcurrentMode) === NoMode) { | |||
| return getCurrentPriorityLevel() === ImmediateSchedulerPriority | |||
There was a problem hiding this comment.
Notable update here - now we can just check if the updateLanePriority is Sync, otherwise use batched.
| @@ -486,7 +480,7 @@ function requestRetryLane(fiber: Fiber) { | |||
| if ((mode & BlockingMode) === NoMode) { | |||
| return (SyncLane: Lane); | |||
| } else if ((mode & ConcurrentMode) === NoMode) { | |||
| return getCurrentPriorityLevel() === ImmediateSchedulerPriority | |||
| return getCurrentUpdateLanePriority() === SyncLanePriority | |||
There was a problem hiding this comment.
Ditto here for the retry lane.
| priorityLevel === ImmediateSchedulerPriority) | ||
| // Only updates greater than default considered discrete, even inside a discrete event. | ||
| higherLanePriority(updateLanePriority, DefaultLanePriority) !== | ||
| DefaultLanePriority |
There was a problem hiding this comment.
Another notable change - instead of checking the scheduler priority, check the update lane priority.
| pendingPassiveEffectsRenderPriority = | ||
| renderPriorityLevel === NoLanePriority | ||
| ? DefaultLanePriority | ||
| : renderPriorityLevel; |
There was a problem hiding this comment.
I switched this over to use the updateLanePriority. Plan is to remove this in a follow up.
acdlite
approved these changes
Mar 8, 2021
rickhanlonii
force-pushed
the
rh-land-decouple
branch
3 times, most recently
from
9efed01
to
4e6de2a
Compare
rickhanlonii
force-pushed
the
rh-land-decouple
branch
from
0388bba
to
41e62e7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
Depends on #20918
This change lands an experiment we ran to decouple the scheduler priority from React updates by creating an internal update priority that's tracked separately from the Scheduler.
Motivation
The original thinking was that users would control the priority of updates via the
schedulerpackage, which would expose methods for controlling the priority of scheduled tasks. This made more sense at the beginning of the old expiration times model because an update priority roughly corresponded with the expiration time of that task, but as we've evolved our understanding or concurrent mode and moved to the lanes implementation, there's less of a 1-1 correspondence between task priority and update priority. It also made more sense based on certain implementations of a browser scheduler, which has not made it into the spec, making our model of how the scheduler would be used out of date.Instead, we now expose functions directly from React to control the priority of React updates such as
startTransitionwhich de-prioritizes updates andflushSyncwhich prioritizes updates. In practice, we've found that these two methods alone cover all of the use cases for needed for task prioritization, along with heuristics inside of React to decide when updates depend on each other or need rebased.As an added benefit, this change also simplifies the layering between the scheduler and React because users or events can now directly change the update priority in React, instead of going around React to update the priority in the scheduler with React reading the scheduler priority in various critical paths. This makes the scheduler more of a prollyfill for native scheduling, with React and users using the scheduler simply as a mechanism to schedule tasks.