[Experiment] Lazily propagate context changes #20890
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
lazy-context-propagation
branch
from
6f4d5d5 to
c713608
Compare
|
Comparing: 7df6572...258b375 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
|
acdlite
force-pushed
the
lazy-context-propagation
branch
from
c713608 to
c4b0b76
Compare
| } | ||
|
|
||
| if (current !== null) { | ||
| // TODO: The factoring of this block is weird. |
There was a problem hiding this comment.
The bailout logic in this block is getting pretty confusing. I started to refactor it a bit, in part by extracting it into its own function, but there are a bunch of little cases and I didn't want to open up that can of worms right now, so instead I aimed to change it as little as possible. I think what I landed on is fine for now, and we can work on the correct abstractions if/when the feature lands.
acdlite
force-pushed
the
lazy-context-propagation
branch
from
c4b0b76 to
9bab764
Compare
| } | ||
|
|
||
| let parent = workInProgress; | ||
| while (parent !== null && (parent.flags & DidPropagateContext) === NoFlags) { |
There was a problem hiding this comment.
If you're looking for a place to start reviewing, I suggest here
acdlite
force-pushed
the
lazy-context-propagation
branch
3 times, most recently
from
2d4db97 to
557b0ec
Compare
|
@gnoff I can't request a review from you for some reason, so I'm doing it with a comment. If you have the bandwidth, of course! |
| }); | ||
|
|
||
| // @gate enableCache | ||
| test('context is propagated across retries', async () => { |
There was a problem hiding this comment.
I would also review these tests closely to get a sense of the type of issues we might see when rolling this out
|
@acdlite happy to take a look tomorrow |
|
Hmm I don't know. Maybe someone changed an admin setting recently. I'll look into it tomorrow. |
|
a gave it a quick glance, love the approach reading up parent path to find providers rather than trying to awkwardly unify them. feels very familiar conceptually, super excited to see this stuff start to come about |
| } | ||
| } | ||
| } | ||
| parent = parent.return; |
There was a problem hiding this comment.
guess I can comment on line items, not sure what I was seeing before. anyway this whole thing approach is very elegant. it fits in much cleaner than the way I did it
There was a problem hiding this comment.
Still very much based on your PR and RFC 😊
acdlite
force-pushed
the
lazy-context-propagation
branch
2 times, most recently
from
3b6f9c7 to
8f7ac18
Compare
acdlite
force-pushed
the
lazy-context-propagation
branch
2 times, most recently
from
5174c09 to
bde8c75
Compare
There was a problem hiding this comment.
So lots of thoughts on this one but I'll try to keep this bit of feedback focussed. Your current approach defers propagation until a bailout which limits the propagation space significantly but it still does not allow for updates caused by one context to opportunistically cause a deferral of work for another once you hit a bailout. this is probably fine but this is the biggest distinction i think between our approaches (that and the fact you actually got suspense and off screen working... 😆)
I think trying an approach where you still propagate each provider individually but you constrain the propagation space to only work-free trees will allow you to capture this and maybe solve the issue you had with DidPropagateContext. I left some comments on where a few simple changes could cause you go to from your mode to mine. There is still something off about what I proposed in that you can end up with duplicate propagations for some sub-subtrees when you end up scheduling work on a sibling branch. solvable I hope but could mean my approach is unworkable or at least has different performance characteristics
One last thought is that when I was working on #18262 I somewhat came to the conclusion that lazy context propagation was not worth it and that general "speculative" work was more powerful and in many ways simpler and you could get the benefit of context selectors without lazy context propagation. This stuff is over a year old and I'm dusting off cobwebs in my brain but let me know if you want me to extrapolate on that at all
| alternate.lanes = mergeLanes(alternate.lanes, renderLanes); | ||
| } | ||
| scheduleWorkOnParentPath(fiber.return, renderLanes); | ||
|
|
There was a problem hiding this comment.
consider not going deeper on propagation here
| nextFiber = fiber.sibling |
There was a problem hiding this comment.
The problem with not doing the full traversal is that we can't set DidPropagateContext. We'd need to replace that with something else to avoid propagating the same tree multiple times.
| if ( | ||
| dependency.context === context && | ||
| (dependency.observedBits & changedBits) !== 0 | ||
| ) { |
There was a problem hiding this comment.
I can't comment on it b/c it didn't change but above
if (list !== null) {
consider bailing out on child work
if (
enableLazyContextPropagation &&
includesSomeLane(fiber.childLanes, renderLanes)
) {
// we are going to do work in this subtree anyway, let's skip propagation
// and check siblings
nextFiber = fiber.sibling;
} else if (list !== null) {
There was a problem hiding this comment.
Yeah that's what I did here before I reverted it:
react/packages/react-reconciler/src/ReactFiberNewContext.new.js
Lines 226 to 233 in 8f7ac18
Explanation here: #20890 (comment)
| } | ||
|
|
||
| let parent = workInProgress; | ||
| while (parent !== null && (parent.flags & DidPropagateContext) === NoFlags) { |
There was a problem hiding this comment.
don't bail out on DidPropagateContext and let the parent loop ride to the root every time.
Then track already propagated providers using a Set or restructure entirely and maintain a "hasChanges" provider set that is added and deleted from on provider push / pop
| } | ||
| } | ||
| } | ||
| parent = parent.return; |
There was a problem hiding this comment.
This comment is not related to the others where I'm articulating a different approach, it's just an independent finding
There is an edge case where on any given bailout multiple stacked providers of the same context will be visited before you bail out on root or DidPropagateContext. In these cases changed bits are recalculated even for providers fibers that aren't top of stack. it won't technically break anything b/c the context value itself will always read consistently but it could lead to propagations that do not actually need to happen
|
Thanks for the review! So the change you are suggesting is essentially what I started with, but I had to backtrack for exactly the reason you describe here:
There seems to be this inherent trade-off between deferring propagation until later, and keeping track of which trees have already been propagated. It's tempting to add an additional data structure to track that information but I haven't thought of an efficient way to do that.
I think you would need one of these sets at every level at which you stop propagating. That's a lot of overhead that doesn't seem like it would pay for itself. Unless I misunderstand your suggestion. One change I could try that would be relatively straightforward is to invert the loops so that there's a single tree traversal that checks all the contexts on the stack in one pass. That would make it more like your PR. But what I'm realizing is that most of the optimizations we're discussing are only relevant if you have multiple contexts that changed simultaneously. That's usually not the case — it might be more common than necessary today because of the lack of selectors, but once you do have selectors, things that update simultaneously should be combined into a single context type. Then you get the benefits of a unified traversal. So my current thinking is that we should optimize for the single changed context per render case. I'm going to run some performance experiments to see how much of an impact the current changes have on their own, and then use whatever we find there to inform the next steps.
If by speculative you mean lazily cloning the intermediate work-in-progress nodes, I think that could be promising, but it's also a more significant refactor than we have the bandwidth to support right now. Would probably tackle something like this after we move away from the fiber However, one interesting thing about the current implementation is that if a consumer matches during context propagation, that node is definitely going to re-render. There's no way to bail out of context updates — but, there will be once we have context selectors. And if we run the selectors during propagation, then we can bail out early and skip cloning that way. So that's what I'm going to try. That leaves bailing out of state updates early. We do already have an optimization there that works pretty well, too. So with that in mind, I wonder if lazily cloning really makes that much a difference, since there aren't many remaining cases where we would clone all the way down to a node only to bail out. |
acdlite
force-pushed
the
lazy-context-propagation
branch
from
bde8c75 to
f3fd511
Compare
|
Another idea I had was to refactor the propagation function so that the deepest nodes are checked first; in other words, check when leaving a node instead of when entering it. If something matches, then we can bail out of checking any of its parents. If we do it this way, we could also unify the traversal with |
|
Does this work with variable number of items selected? i.e., |
|
@ntucker I think that's more relevant to the Context selectors RFC: reactjs/rfcs#119 |
|
In your diagram, are the green nodes the siblings of the blue nodes? Why does the top pair have a line connecting them but not the ones below? If the green nodes aren't direct siblings, but "cousins" instead, then we would never visit them during the render phase. We have a hard bailout where we return If the green nodes are direct siblings of the blue nodes, EDIT: Actually never mind again, the part I crossed out is correct. We always call I pushed another change. I got rid of the Set of matched consumers and replaced it with a flag called That means it gets set for any context consumer that was matched during the last propagation. But it also gets set on consumers that were re-rendered due to new props or state. So, it's a superset of what I was previously putting inside So the main downside is that I had to add an extra flag assignment to a non-bailout-specific path ( The rest of the |
acdlite
force-pushed
the
lazy-context-propagation
branch
from
a77c31c to
620f8b7
Compare
Yup this is what i meant, we re-check the contexts of the green nodes when it is impossible for anything to have changed and thus don't need to
Yeah actually why isn't it this simple? mark the fibers with no childwork on cloneChildFibers with a flag and skip them at the very top of beginWork?
I'll check it out edit: posted this from another gh profile but it's still me (gnoff) |
|
It’s theoretically possible but it’d be a significant refactor because although the begin phase can be skipped, there’s still stuff we have to do in the complete phase. That’s why there’s a giant bailout block at the top of beginWork, that mirrors a bunch of stuff that the regular begin phase does. We’d lift out that bailout branch and put it in, say, cloneChildFibers instead. |
| if (!isInsidePropagationBailout) { | ||
| if ((parent.flags & NeedsPropagation) !== NoFlags) { | ||
| isInsidePropagationBailout = true; | ||
| } else if ((parent.flags & DidPropagateContext) !== NoFlags) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ok this actually seems pretty nice. you're super close to my ideal where you could safely lift the selector bailout to the propagation. the only limitation I see is if you propagate through a scheduled update for state then that state update could change the tree where we already visited and did work (i.e. the selector might have closed over props and the state update changed the props the component will see). I wonder if you could force a NeedsPropagation flag to be written when an intermediate node that does not readContext and then stop propagation at any work, not just work the propagation 'found'
There was a problem hiding this comment.
I know we're back to the whole "we're operating on the current tree" problem but perhaps the NeedsPropagation flag is ok to drop on that tree because even if a render restarted it'll only cause additional propagations it won't ever let a necessary one go missing
There was a problem hiding this comment.
if you propagate through a scheduled update for state then that state update could change the tree where we already visited and did work
So this shouldn't happen because we always check that there are no matching childLanes before starting propagation, and if there's an update in a child then its childLanes would have been set.
There was a problem hiding this comment.
so that accounts for prop and state updates. The only other source of work is context, which we accounted for by combining all the contexts into a simultaneous propagation pass.
There was a problem hiding this comment.
So this shouldn't happen because we always check that there are no matching childLanes before starting propagation, and if there's an update in a child then its childLanes would have been set.
🤯 That's right! Did you really do it?!? I think this is all I hoped for 💪
acdlite
force-pushed
the
lazy-context-propagation
branch
2 times, most recently
from
51f7179 to
d1bfb29
Compare
| ): void { | ||
| // Only used by lazy implemenation | ||
| if (!enableLazyContextPropagation) { | ||
| return; |
There was a problem hiding this comment.
Should this throw or are you calling this function in the old impl?
There was a problem hiding this comment.
Since this is probably for GCC stripping it, I'm specifically asking if we should throw here if GCC doesn't strip it for some reason.
There was a problem hiding this comment.
Throwing isn't dead code so it doesn't help dead code elimination. It only helps indirectly by forcing us to wrap the caller in a condition. But I prefer to wrap in both places.
There was a problem hiding this comment.
Yeah, for me that's a feature not a bug. If this were to accidentally be included, it would silently fail this way.
| 'calculateChangedBits: Expected the return value to be a 31-bit ' + | ||
| 'integer. Instead received: 4294967295', | ||
| ); | ||
| } |
There was a problem hiding this comment.
I like having else blocks required for in-line feature flags so you either know what the expected behavior is in the other branch or can explain why it's missing, something like:
| } | |
| } else { | |
| // TODO: Decide what to do with calculateChangedBits in enableLazyContextPropagation | |
| } |
| // sCU is not called in this case because React force updates when a provider re-renders | ||
| expect(shouldComponentUpdateWasCalled).toBe(false); | ||
| if (gate(flags => flags.enableLazyContextPropagation)) { | ||
| expect(shouldComponentUpdateWasCalled).toBe(true); |
There was a problem hiding this comment.
Consider adding a comment similar to the other branch so that when that branch is deleted there's an explanation of the expectation here. Something like:
| expect(shouldComponentUpdateWasCalled).toBe(true); | |
| // sCU is called in this case because React does not force updates when a provider re-renders. | |
| // Instead, we check context changes lazily, after sCU is called. | |
| expect(shouldComponentUpdateWasCalled).toBe(true); |
| "385": "A mutable source was mutated while the %s component was rendering. This is not supported. Move any mutations into event handlers or effects.", | ||
| "386": "The current renderer does not support microtasks. This error is likely caused by a bug in React. Please file an issue." | ||
| "386": "The current renderer does not support microtasks. This error is likely caused by a bug in React. Please file an issue.", | ||
| "387": "Should have a current fiber. This is a bug in React.", |
There was a problem hiding this comment.
What do you think about providing more context here so if we hear about it then we know it's related to the context propagation? As in "Should have a current fiber while propagating context. This is a bug in React." Same for the next error.
There was a problem hiding this comment.
I went with a generic one so that we don't have to create a new code every time we assert that a current fiber is non-null. The stack and/or line number disambiguates.
There was a problem hiding this comment.
Yeah that's fair, but the trade off is that stack frames can change over time, are missing from reporting pipelines, or not reported by users (requiring an extra step). There's also value in having unique error codes for unique callsites for better reporting and easier debugging.
For example here, if this said it was related to lazy propagation, users opting into the feature (or us reading a bug report) would instantly know it was related to this gated feature instead of the process of finding the stack frame, parsing it, looking it up in the code base, and then knowing enough about the code to know it's related to this feature.
|
I'm going to land in two steps so if one of them breaks they can be bisected |
In the lazy context implementation, not all context changes are propagated from the provider, so we can't rely on the propagation alone to mark the consumer as dirty. The consumer needs to compare to the previous value, like we do for state and context. I added a `memoizedValue` field to the context dependency type. Then in the consumer, we iterate over the current dependencies to see if something changed. We only do this iteration after props and state has already bailed out, so it's a relatively uncommon path, except at the root of a changed subtree. Alternatively, we could move these comparisons into `readContext`, but that's a much hotter path, so I think this is an appropriate trade off.
When a context provider changes, we scan the tree for matching consumers and mark them as dirty so that we know they have pending work. This prevents us from bailing out if, say, an intermediate wrapper is memoized. Currently, we propagate these changes eagerly, at the provider. However, in many cases, we would have ended up visiting the consumer nodes anyway, as part of the normal render traversal, because there's no memoized node in between that bails out. We can save CPU cycles by propagating changes only when we hit a memoized component — so, instead of propagating eagerly at the provider, we propagate lazily if or when something bails out. Most of our bailout logic is centralized in `bailoutOnAlreadyFinishedWork`, so this ended up being not that difficult to implement correctly. There are some exceptions: Suspense and Offscreen. Those are special because they sometimes defer the rendering of their children to a completely separate render cycle. In those cases, we must take extra care to propagate *all* the context changes, not just the first one. I'm pleasantly surprised at how little I needed to change in this initial implementation. I was worried I'd have to use the reconciler fork, but I ended up being able to wrap all my changes in a regular feature flag. So, we could run an experiment in parallel to our other ones. I do consider this a risky rollout overall because of the potential for subtle semantic deviations. However, the model is simple enough that I don't expect us to have trouble fixing regressions if or when they arise during internal dogfooding. --- This is largely based on [RFC#118](reactjs/rfcs#118), by @gnoff. I did deviate in some of the implementation details, though. The main one is how I chose to track context changes. Instead of storing a dirty flag on the stack, I added a `memoizedValue` field to the context dependency object. Then, to check if something has changed, the consumer compares the new context value to the old (memoized) one. This is necessary because of Suspense and Offscreen — those components defer work from one render into a later one. When the subtree continues rendering, the stack from the previous render is no longer available. But the memoized values on the dependencies list are. This requires a bit more work when a consumer bails out, but nothing considerable, and there are ways we could optimize it even further. Conceptually, this model is really appealing, since it matches how our other features "reactively" detect changes — `useMemo`, `useEffect`, `getDerivedStateFromProps`, the built-in cache, and so on. I also intentionally dropped support for `unstable_calculateChangedBits`. We're planning to remove this API anyway before the next major release, in favor of context selectors. It's an unstable feature that we never advertised; I don't think it's seen much adoption. Co-Authored-By: Josh Story <[email protected]>
Instead of propagating the tree once per changed context, we can check all the contexts in a single propagation. This inverts the two loops so that the faster loop (O(numberOfContexts)) is inside the more expensive loop (O(numberOfFibers * avgContextDepsPerFiber)). This adds a bit of overhead to the case where only a single context changes because you have to unwrap the context from the array. I'm also unsure if this will hurt cache locality. Co-Authored-By: Josh Story <[email protected]>
Because we now propagate all context providers in a single traversal, we can defer context propagation to a subtree without losing information about which context providers we're deferring — it's all of them. Theoretically, this is a big optimization because it means we'll never propagate to any tree that has work scheduled on it, nor will we ever propagate the same tree twice. There's an awkward case related to bailing out of the siblings of a context consumer. Because those siblings don't bail out until after they've already entered the begin phase, we have to do extra work to make sure they don't unecessarily propagate context again. We could avoid this by adding an earlier bailout for sibling nodes, something we've discussed in the past. We should consider this during the next refactor of the fiber tree structure. Co-Authored-By: Josh Story <[email protected]>
Instead of storing matched context consumers in a Set, we can mark when a consumer receives an update inside `readContext`. I hesistated to put anything in this function because it's such a hot path, but so are bail outs. Fortunately, we only need to set this flag once, the first time a context is read. So I think it's a reasonable trade off. In exchange, propagation is faster because we no longer need to accumulate a Set of matched consumers, and fiber bailouts are faster because we don't need to consult that Set. And the code is simpler.
acdlite
force-pushed
the
lazy-context-propagation
branch
from
d1bfb29 to
258b375
Compare
|
🎉 |
|
yet another one realization https://github.com/edriang/use-context-selection |
7 participants
When a context provider changes, we scan the tree for matching consumers and mark them as dirty so that we know they have pending work. This prevents us from bailing out if, say, an intermediate wrapper is memoized.
Currently, we propagate these changes eagerly, at the provider.
However, in many cases, we would have ended up visiting the consumer nodes anyway, as part of the normal render traversal, because there's no memoized node in between that bails out.
We can save CPU cycles by propagating changes only when we hit a memoized component — so, instead of propagating eagerly at the provider, we propagate lazily if or when something bails out.
Another neat optimization is that if multiple context providers change simultaneously, we don't need to traverse the tree separately for each provider – we can propagate all providers in a single pass. A related benefit is that we can stop propagating as soon as we find a matching consumer. We'll never waste cycles searching for context changes inside a subtree that we would have visited during the render phase regardless.
Most of our bailout logic is centralized in
bailoutOnAlreadyFinishedWork, so this ended up being not that difficult to implement correctly.There are some exceptions: Suspense and Offscreen. Those are special because they sometimes defer the rendering of their children to a completely separate render cycle. In those cases, we must take extra care to propagate all the context changes, not just the first one.
I'm pleasantly surprised at how little I needed to change in this initial implementation. I was worried I'd have to use the reconciler fork, but I ended up being able to wrap all my changes in a regular feature flag. So, we could run an experiment in parallel to our other ones.
I do consider this a risky rollout overall because of the potential for subtle semantic deviations. However, the model is simple enough that I don't expect us to have trouble fixing regressions if or when they arise during internal dogfooding.
This is largely based on RFC #118, by @gnoff. I did deviate in some of the implementation details, though.
The main one is how I chose to track context changes. Instead of storing a dirty flag on the stack, I added a
memoizedValuefield to the context dependency object. Then, to check if something has changed, the consumer compares the new context value to the old (memoized) one.This is necessary because of Suspense and Offscreen — those components defer work from one render into a later one. When the subtree continues rendering, the stack from the previous render is no longer available. But the memoized values on the dependencies list are. This requires a bit more work when a consumer bails out, but nothing considerable, and there are ways we could optimize it even further.
Conceptually, this model is really appealing, since it matches how our other features "reactively" detect changes —
useMemo,useEffect,getDerivedStateFromProps, the built-in cache, and so on.I also intentionally dropped support for
unstable_calculateChangedBits. We're planning to remove this API anyway before the next major release, in favor of context selectors. It's an unstable feature that we never advertised; I don't think it's seen much adoption.