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.
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 |
| 'b', | ||
| 'c', | ||
|
|
||
| // The old reconciler has a quirk where `e` d has slightly lower |
| '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.
Subsequent render+commit?
What are the implications for e.g. useSubscription?
react/packages/use-subscription/src/useSubscription.js
Lines 41 to 56 in d804f99
There was a problem hiding this comment.
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.
The code I linked to above is useSubscription calling setState during the render phase, yes.
There was a problem hiding this comment.
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.
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)) { |
| ? prevBaseTime | ||
| : renderExpirationTime; | ||
| const prevBaseLanes = prevState.baseLanes; | ||
| nextBaseLanes = combineLanes(prevBaseLanes, renderLanes); |
There was a problem hiding this comment.
You missed an opportunity to call this mergeLanes
There was a problem hiding this comment.
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.
| 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.
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.
Calling prepareFreshStack after doing something looks odd. Can you explain why we do it?
There was a problem hiding this comment.
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 | |||
|
|
||
| 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.
How often do you think this will break things?
There was a problem hiding this comment.
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.
Maybe silly question: what are there three lanes for this as opposed to just two?
There was a problem hiding this comment.
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.
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.
"shelf"? 🤨 I'd be interested in hearing the reasoning behind this! 😆 Offscreen seemed pretty clear.
There was a problem hiding this comment.
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.
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.
It does seem related to the concept of keep-alive...
There was a problem hiding this comment.
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
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.
<MaybeOrNotOffscreen /> 😆
| } | ||
|
|
||
| export function getLanesToRetrySynchronouslyOnError(root: FiberRoot): Lanes { | ||
| const everythingButOffscreen = root.pendingLanes & ~OffscreenLane; |
There was a problem hiding this comment.
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.
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.
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.
That makes sense. What type of work would fall under IdleLanes then?
There was a problem hiding this comment.
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 |
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.
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.
Is it possible that b equals to NoLane?
There was a problem hiding this comment.
In practice no, it's always SelectiveHydrationLane but should probably rename/refactor this to something more specific so it's not used elsewhere.
| return everythingButOffscreen; | ||
| } | ||
| if (everythingButOffscreen & OffscreenLane) { | ||
| return OffscreenLane; |
There was a problem hiding this comment.
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
enableNewReconcilerflag. 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
ReactDOMForkedbuild, 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
setStatecontains alanefield, a bitmask with a single bit enabled. This replaces theupdate.expirationTimefield 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
lanesfield, a bitmask with zero or more bits enabled (fiber.expirationTimein the old model); and achildLanesfield (fiber.childExpirationTime).Lanes is an opaque type. You can only perform direct bitmask manipulation inside the
ReactFiberLanemodule. 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->renderLanesupdate.expirationTime->update.lanefiber.expirationTime->fiber.lanesfiber.childExpirationTime->fiber.childLanesroot.firstPendingTimeandroot.lastPendingTime->fiber.pendingLanesStuff 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
useTransitionstates. Only the most recent transition in a sequence of related transitions should be allowed to finish.useMutableSourceless frequently.useTransition. Currently, a single transition can easily block other transitions from finishing, even if they are in totally unrelated parts of the tree.useStatehook. 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.