-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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
Initial Lanes implementation #18796
Initial Lanes implementation #18796
Conversation
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 1114b6e:
|
Details of bundled changes.Comparing: 3c7d52c...1114b6e react-dom
Size changes (experimental) |
Details of bundled changes.Comparing: 3c7d52c...1114b6e react-dom
Size changes (stable) |
|
||
function getLowestPriorityLane(lanes: Lanes): Lane { | ||
// This finds the most significant non-zero bit. | ||
// TODO: Consider alternate implementations. Could use a map. Is Math.log2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Math.clz32
could work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn’t realize so many browsers supported this. We can use a fallback when it’s not there. Good idea, thanks!
'b', | ||
'c', | ||
|
||
// The old reconciler has a quirk where `e` d has slightly lower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo "e
d"
'b', | ||
'c', | ||
|
||
// The old reconciler has a quirk where `e` d has slightly lower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo "e
d"
'setState updater', | ||
// In the new reconciler, updates inside the render phase are | ||
// treated as if they came from an event, so the update gets | ||
// shifted to a subsequent render. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsequent render+commit?
What are the implications for e.g. useSubscription
?
react/packages/use-subscription/src/useSubscription.js
Lines 41 to 56 in d804f99
// If parameters have changed since our last render, schedule an update with its current value. | |
if ( | |
state.getCurrentValue !== getCurrentValue || | |
state.subscribe !== subscribe | |
) { | |
// If the subscription has been updated, we'll schedule another update with React. | |
// React will process this update immediately, so the old subscription value won't be committed. | |
// It is still nice to avoid returning a mismatched value though, so let's override the return value. | |
valueToReturn = getCurrentValue(); | |
setState({ | |
getCurrentValue, | |
subscribe, | |
value: valueToReturn, | |
}); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSubscription doesn’t call setState during the render phase right? I think the only place we do this intentionally is useOpaqueIdentifier. Everywhere else we warn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code I linked to above is useSubscription
calling setState
during the render phase, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a hook update on itself. Those are fine. Should come up with some disambiguating term.
"Render phase" updates here refers to updates on another component, or a class component on itself. The ones that would trigger the warnAboutRenderPhaseUpdatesInDEV
warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Ok thanks for clarifying 😄
// The pending update priority was cleared at the beginning of | ||
// beginWork. We're about to bail out, but there might be additional | ||
// updates at a lower priority. Usually, the priority level of the | ||
if (!includesSomeLane(renderLanes, updateLanes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
? prevBaseTime | ||
: renderExpirationTime; | ||
const prevBaseLanes = prevState.baseLanes; | ||
nextBaseLanes = combineLanes(prevBaseLanes, renderLanes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed an opportunity to call this mergeLanes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s never too late
@@ -249,7 +250,7 @@ export function updateContainer( | |||
} | |||
} | |||
const suspenseConfig = requestCurrentSuspenseConfig(); | |||
const expirationTime = requestUpdateExpirationTime(current, suspenseConfig); | |||
const expirationTime = requestUpdateLane(current, suspenseConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const expirationTime = requestUpdateLane(current, suspenseConfig); | |
const lane = requestUpdateLane(current, suspenseConfig); |
@@ -500,34 +468,33 @@ export function scheduleUpdateOnFiber( | |||
// This is the result of a discrete event. Track the lowest priority | |||
// discrete update per root so we can flush them early, if needed. | |||
if (rootsWithPendingDiscreteUpdates === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if/else needs to move up into the else
above now (see PR #18797)
startWorkOnPendingInteractions(root, lanes); | ||
} else if (lanes !== root.wipLanes) { | ||
startWorkOnPendingInteractions(root, lanes); | ||
prepareFreshStack(root, lanes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling prepareFreshStack
after doing something looks odd. Can you explain why we do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call both of these functions right before starting a new render at a new level. I don't think the order matters since prepareFreshStack
doesn't touch root.memoizedInteractions
, but I'll rearrange them for consistency with the other branches.
@@ -2769,9 +2532,14 @@ export function pingSuspendedRoot( | |||
pingCache.delete(wakeable); | |||
} | |||
|
|||
// TODO: Instead of passing the current time as an argument, we should read it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit "time"
|
||
let subscriber; | ||
|
||
try { | ||
subscriber = __subscriberRef.current; | ||
if (subscriber !== null && root.memoizedInteractions.size > 0) { | ||
const threadID = computeThreadID(root, committedExpirationTime); | ||
// FIXME: More than one lane can finish in a single commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do you think this will break things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn’t sure about this. If it’s not already a problem then this doesn’t make it worse, though, since multiple expirations times could finish in the same batch, too.
This change looks great, by the way. Really excited about the new direction. |
const SyncBatchedUpdateRangeEnd = 2; | ||
|
||
export const InputDiscreteHydrationLane: Lane = /* */ 0b000000000000000000000000000100; | ||
const InputDiscreteLanes: Lanes = /* */ 0b000000000000000000000000011100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe silly question: what are there three lanes for this as opposed to just two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is for a trick we use during hydration. If we receive an update to a tree that hasn’t been hydrated yet, we restart at a slightly higher priority to hydrate it, then continue with the update on top. So every async priority has a hydration lane associated with it.
The other extra lane was reserved for a trick that SuspenseList does where it leaves behind work that is slightly lower priority, but that turned out to not be necessary, so I removed it: #18738
Regardless, this whole range is going to go away when we make discrete inputs synchronous, so we can redistribute them to other priorities.
const IdleUpdateRangeStart = 27; | ||
const IdleUpdateRangeEnd = 29; | ||
|
||
export const OffscreenLane: Lane = /* */ 0b100000000000000000000000000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a heads up, @sebmarkbage and I were thinking about renaming Offscreen
to something else (like maybe Shelf). Not sure if that will affect your naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"shelf"? 🤨 I'd be interested in hearing the reasoning behind this! 😆 Offscreen seemed pretty clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seb thinks that having an Offscreen component when something is not off screen is confusing. He is worried that people might misunderstand the name and assume that you only need to use Offscreen
component if the elements are offscreen, which might result in people not wrapping something with Offscreen until the elements are hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<KeepAlive>
, a la Vue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "shelf" might be a bit of a colloquialism that isn't very intuitive for a lot of people.
Isn't Offscreen
always for things that might be on or off the screen? (e.g. list items that might be scrolled out of viewports, tab containers that might be hidden)
Is the concern that you'd always have an Offscreen
wrapper around even the visible items in that case (since we don't support reparenting)?
Maybe something like Conditional
would be more accurate/intuitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem related to the concept of keep-alive
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that a public API called Offscreen
is confusing. Similar to when we were trying to think of a name for the Suspense
component.
The naming considerations are slightly different for this lane, though. The thing that makes OffscreenLane
special, aside from it being really low priority, is that we're allowed to not finish it (i.e. it can tear), because the work at that level is not visible to the user.
react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Lines 1654 to 1659 in 17448e1
// TODO: Move this check out of the hot path by moving `resetChildLanes` | |
// to switch statement in `completeWork`. | |
(completedWork.tag === LegacyHiddenComponent || | |
completedWork.tag === OffscreenComponent) && | |
completedWork.memoizedState !== null && | |
!includesSomeLane(renderLanes, (OffscreenLane: Lane)) |
A better solution, though, might be to move resetChildLanes
(the function in that snippet) to the switch statement inside completeWork
. Then the only thing remarkable about OffscreenLane
is that it has the lowest priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<MaybeOrNotOffscreen />
😆
} | ||
|
||
export function getLanesToRetrySynchronouslyOnError(root: FiberRoot): Lanes { | ||
const everythingButOffscreen = root.pendingLanes & ~OffscreenLane; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we retrying IdleLanes
if we are not retrying OffscreenLanes
? I tried to grep to see where we were using IdleLands
outside of schedulerPriorityToLanePriority
and I couldn't find other use cases, so I might be missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to include anything that might include a pending update that would fix the error that we’re trying to recover from. Offscreen never includes updates, only deferred subtrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to look at it is: we don't want to include Offscreen because that work is expensive; for example, it includes hydrating the remaining dehydrated boundaries. So since we know including Offscreen won't fix the error, anyway, might as well exclude it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. What type of work would fall under IdleLanes
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates that don't have to finish with any particular urgency. Like maybe a background refresh of data. Honestly, we don't have a ton of use cases in practice yet. It might end up merging with the "long transition" lanes.
__DEV__ | ||
? [ | ||
'Child with ID', | ||
// Fallbacks are immdiately committed in TestUtils version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: immediately
53c3611
to
4689b2c
Compare
These tests were written in such a way that when a wakeable has multiple listeners, only the most recent listener is notified. I switched them to use a promise instead of mutating a ref.
A refactor of our concurrency and scheduling architecture. Replaces the ExpirationTime type with a new concept, called Lanes. See PR facebook#18796 for more information. All of the changes I've made in this commit are behind the `enableNewReconciler` flag. Merging this to master will not affect the open source builds or the build that we ship to Facebook. The only build that is affected is the `ReactDOMForked` build, which is deployed to Facebook **behind an experimental flag (currently disabled for all users)**. We will use this flag to gradually roll out the new reconciler, and quickly roll it back if we find any problems. Because we have those protections in place, what I'm aiming for with this initial PR is the **smallest possible atomic change that lands cleanly and doesn't rely on too many hacks**. The goal has not been to get every single test or feature passing, and it definitely is not to implement all the features that we intend to build on top of the new model. When possible, I have chosen to preserve existing semantics and defer changes to follow-up steps. (Listed in the section below.) (I did not end up having to disable any tests, although if I had, that should not have necessarily been a merge blocker.) For example, even though one of the primary goals of this project is to improve our model for parallel Suspense transitions, in this initial implementation, I have chosen to keep the same core heuristics for sequencing and flushing that existed in the ExpirationTimes model: low priority updates cannot finish without also finishing high priority ones. Despite all these precautions, **because the scope of this refactor is inherently large, I do expect we will find regressions.** The flip side is that I also expect the new model to improve the stability of the codebase and make it easier to fix bugs when they arise.
3d0790a
to
1114b6e
Compare
// bother waiting until the root is complete. | ||
(wipLanes & suspendedLanes) === NoLanes | ||
) { | ||
getHighestPriorityLanes(wipLanes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a 'get' function but we don't use the return value from it though that's intended, but is that will get confusing?
|
||
export function higherPriorityLane(a: Lane, b: Lane) { | ||
// This works because the bit ranges decrease in priority as you go left. | ||
return a !== NoLane && a < b ? a : b; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that b
equals to NoLane
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice no, it's always SelectiveHydrationLane
but should probably rename/refactor this to something more specific so it's not used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch :)
return everythingButOffscreen; | ||
} | ||
if (everythingButOffscreen & OffscreenLane) { | ||
return OffscreenLane; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this branch is unreachable. Maybe root.pendingLanes & OffscreenLane
?
i need a longer technical document :P~ |
Maybe I need an article about lanes to explain 5W1H,bigwig~~~~~ |
is there an longer technical document about lanes ? Really need it! @acdlite |
This is ready for review. I'm going to do another pass to add comments and look for anything unusual I may have missed, but overall I'm happy with its current state.
All of the changes I've made in this PR are behind the
enableNewReconciler
flag. Merging this to master will not affect the open source builds or the build that we ship to Facebook.The only build that is affected is the
ReactDOMForked
build, which is deployed to Facebook behind an experimental flag (currently disabled for all users). We will use this flag to gradually roll out the new reconciler, and quickly roll it back if we find any problems.Because we have those protections in place, what I'm aiming for with this initial PR is the smallest possible atomic change that lands cleanly and doesn't rely on too many hacks. The goal has not been to get every single test or feature passing, and it definitely is not to implement all the features that we intend to build on top of the new model. When possible, I have chosen to preserve existing semantics and defer changes to follow-up steps. (Listed in the section below.)
(I did not end up having to disable any tests, although if I had, that should not have necessarily been a merge blocker.)
For example, even though one of the primary goals of this project is to improve our model for parallel Suspense transitions, in this initial implementation, I have chosen to keep the same core heuristics for sequencing and flushing that existed in the ExpirationTimes model: low priority updates cannot finish without also finishing high priority ones.
Despite all these precautions, because the scope of this refactor is inherently large, I do expect we will find regressions. The flip side is that I also expect the new model to improve the stability of the codebase and make it easier to fix bugs when they arise.
Technical description of Lanes
This is not a comprehensive description. That would take up many pages. What I've written here is a brief overview intended to help React team members translate from the old model (Expiration Times) to the new one (Lanes). I can write a longer technical document once more of follow-up steps done.
There are two primary advantages of the Lanes model over the Expiration Times model:
In the old model, to decide whether to include a given unit of work in the batch that's being worked on, we would compare their relative priorities:
This worked because of a constraint we imposed where lower priority tasks were not allowed to complete unless higher priority tasks are also included. Given priorities A > B > C, you couldn't work on B without also working on A; nor could you work on C without working on both B and A.
This constraint was designed before Suspense was a thing, and it made some sense in that world. When all your work is CPU bound, there's not much reason to work on tasks in any order other than by their priority. But when you introduce tasks that are IO-bound (i.e. Suspense), you can have a scenario where a higher priority IO-bound task blocks a lower-priority CPU-bound task from completing.
A similar flaw of Expiration Times is that it's limited in how we can express a group of multiple priority levels.
Using a Set object isn't practical, in terms of either memory or computation — the existence checks we're dealing with are extremely pervasive, so they need to be fast and use as little memory as possible.
As a compromise, often what we'd do instead is maintain a range of priority levels:
Setting aside that this requires two separate fields, even this is quite limiting in its expressiveness. You can express a closed, continuous range of tasks. But you can't represent a finite set of distinct tasks. For example, given a range of tasks, how do you remove a task that lies in the middle of the range? Even in cases where we had designed a decent workaround, reasoning about groups of tasks this way became extremely confusing and prone to regressions.
At first by design, but then more by accident, the old model coupled the two concepts of 1) prioritization and 2) batching into a single data type. We were limited in our ability to express one except in terms that affected the other.
In the new model, we have decoupled those two concepts. Groups of tasks are instead expressed not as relative numbers, but as bitmasks:
The type of the bitmask that represents a task is called a
Lane
. The type of the bitmask that represents a batch is calledLanes
. (Note: these names are not final. I know using a plural is a bit confusing, but since the name appears all over the place, I wanted something that was short. But I'm open to suggestions.)In more concrete React terms, an update object scheduled by
setState
contains alane
field, a bitmask with a single bit enabled. This replaces theupdate.expirationTime
field in the old model.On the other hand, a fiber is not associated with only a single update, but potentially many. So it has a
lanes
field, a bitmask with zero or more bits enabled (fiber.expirationTime
in the old model); and achildLanes
field (fiber.childExpirationTime
).Lanes is an opaque type. You can only perform direct bitmask manipulation inside the
ReactFiberLane
module. Elsewhere, you must import a helper function from that module. This is a trade off, but one that I think is ultimately worth it, since dealing with Lanes can be very subtle, and colocating all the logic will make it easier for us to tweak our heuristics without having to do a huge refactor (like this one) every time.Commonly seen Expiration Time fields, translated to Lanes
renderExpirationtime
->renderLanes
update.expirationTime
->update.lane
fiber.expirationTime
->fiber.lanes
fiber.childExpirationTime
->fiber.childLanes
root.firstPendingTime
androot.lastPendingTime
->fiber.pendingLanes
Stuff I intentionally omitted from this initial PR
Things that probably block us from rolling this out at Facebook
Future improvements that will build on top
Not a comprehensive list
useTransition
states. Only the most recent transition in a sequence of related transitions should be allowed to finish.useMutableSource
less frequently.useTransition
. Currently, a single transition can easily block other transitions from finishing, even if they are in totally unrelated parts of the tree.useState
hook. We may do this via entanglement, or we may take advantage of the fact that there are a finite number of non-overlapping batches that an update might render into and precompute each one whenever an additional pending update is scheduled before the queue is flushed.