Skip to content
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

[Experiment] Lazily propagate context changes #20890

Merged
merged 5 commits into from
Mar 7, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 26, 2021

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 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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 26, 2021
@sizebot
Copy link

sizebot commented Feb 26, 2021

Comparing: 7df6572...258b375

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.02% 123.30 kB 123.32 kB +0.02% 39.67 kB 39.67 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.02% 129.88 kB 129.91 kB = 41.69 kB 41.70 kB
facebook-www/ReactDOM-prod.classic.js +0.32% 407.38 kB 408.70 kB +0.34% 75.56 kB 75.82 kB
facebook-www/ReactDOM-prod.modern.js +0.33% 395.72 kB 397.04 kB +0.35% 73.69 kB 73.95 kB
facebook-www/ReactDOMForked-prod.classic.js +0.32% 407.39 kB 408.71 kB +0.34% 75.56 kB 75.82 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactIs-dev.classic.js +0.87% 10.02 kB 10.10 kB +0.55% 2.71 kB 2.73 kB
facebook-www/ReactIs-dev.modern.js +0.85% 10.28 kB 10.37 kB +0.54% 2.76 kB 2.77 kB
facebook-www/SchedulerTracing-dev.modern.js +0.77% 11.31 kB 11.40 kB +0.72% 2.49 kB 2.50 kB
facebook-www/SchedulerTracing-dev.classic.js +0.77% 11.31 kB 11.40 kB +0.68% 2.49 kB 2.50 kB
facebook-www/ReactART-prod.modern.js +0.52% 253.26 kB 254.58 kB +0.58% 45.22 kB 45.48 kB
facebook-www/ReactART-prod.classic.js +0.51% 260.62 kB 261.94 kB +0.54% 46.49 kB 46.74 kB
facebook-www/ReactART-dev.modern.js +0.42% 688.37 kB 691.24 kB +0.56% 146.37 kB 147.18 kB
facebook-www/ReactART-dev.classic.js +0.41% 698.66 kB 701.54 kB +0.54% 148.49 kB 149.30 kB
facebook-www/ReactDOM-prod.modern.js +0.33% 395.72 kB 397.04 kB +0.35% 73.69 kB 73.95 kB
facebook-www/ReactDOMForked-prod.modern.js +0.33% 395.73 kB 397.05 kB +0.35% 73.69 kB 73.95 kB
facebook-www/ReactDOM-prod.classic.js +0.32% 407.38 kB 408.70 kB +0.34% 75.56 kB 75.82 kB
facebook-www/ReactDOMForked-prod.classic.js +0.32% 407.39 kB 408.71 kB +0.34% 75.56 kB 75.82 kB
facebook-www/ReactDOM-profiling.modern.js +0.32% 415.61 kB 416.93 kB +0.34% 77.18 kB 77.44 kB
facebook-www/ReactDOMForked-profiling.modern.js +0.32% 415.62 kB 416.94 kB +0.34% 77.18 kB 77.44 kB
facebook-www/ReactDOM-profiling.classic.js +0.31% 427.32 kB 428.64 kB +0.34% 79.02 kB 79.29 kB
facebook-www/ReactDOMForked-profiling.classic.js +0.31% 427.33 kB 428.65 kB +0.34% 79.03 kB 79.29 kB
facebook-www/ReactDOM-dev.modern.js +0.28% 1,030.09 kB 1,032.96 kB +0.36% 228.89 kB 229.72 kB
facebook-www/ReactDOMForked-dev.modern.js +0.28% 1,030.10 kB 1,032.97 kB +0.36% 228.64 kB 229.46 kB
facebook-www/ReactDOM-dev.classic.js +0.27% 1,056.35 kB 1,059.22 kB +0.35% 234.07 kB 234.88 kB
facebook-www/ReactDOMForked-dev.classic.js +0.27% 1,056.36 kB 1,059.23 kB +0.35% 233.82 kB 234.63 kB
facebook-www/JSXDEVRuntime-dev.modern.js +0.21% 42.06 kB 42.15 kB +0.16% 11.76 kB 11.78 kB
facebook-www/JSXDEVRuntime-dev.classic.js +0.21% 42.07 kB 42.15 kB +0.16% 11.76 kB 11.78 kB

Generated by 🚫 dangerJS against 63941d5

@@ -3191,6 +3246,17 @@ function beginWork(
}

if (current !== null) {
// TODO: The factoring of this block is weird.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

let parent = workInProgress;
while (parent !== null && (parent.flags & DidPropagateContext) === NoFlags) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're looking for a place to start reviewing, I suggest here

@acdlite acdlite force-pushed the lazy-context-propagation branch 3 times, most recently from 2d4db97 to 557b0ec Compare February 26, 2021 05:50
@acdlite acdlite marked this pull request as ready for review February 26, 2021 05:55
@acdlite
Copy link
Collaborator Author

acdlite commented Feb 26, 2021

@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 () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also review these tests closely to get a sense of the type of issues we might see when rolling this out

@gnoff
Copy link
Collaborator

gnoff commented Feb 26, 2021

@acdlite happy to take a look tomorrow

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 26, 2021

Hmm I don't know. Maybe someone changed an admin setting recently. I'll look into it tomorrow.

@gnoff
Copy link
Collaborator

gnoff commented Feb 26, 2021

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still very much based on your PR and RFC 😊

@acdlite acdlite force-pushed the lazy-context-propagation branch 2 times, most recently from 3b6f9c7 to 8f7ac18 Compare February 26, 2021 07:04
@acdlite acdlite force-pushed the lazy-context-propagation branch 2 times, most recently from 5174c09 to bde8c75 Compare February 26, 2021 21:37
Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(fiber.return, renderLanes);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider not going deeper on propagation here

Suggested change
nextFiber = fiber.sibling

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -210,43 +218,53 @@ export function propagateContextChange<T>(
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I did here before I reverted it:

if (enableLazyContextPropagation && !forcePropagateEntireTree) {
// Bail out if there's already work scheduled
nextFiber = includesSomeLane(fiber.childLanes, renderLanes)
? null
: fiber.child;
} else {
nextFiber = fiber.child;
}

Explanation here: #20890 (comment)

}

let parent = workInProgress;
while (parent !== null && (parent.flags & DidPropagateContext) === NoFlags) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 27, 2021

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 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

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.

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

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.

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

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 alternate model (our next big planned refactor, to improve our memory model and get rid of cycles).

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
Copy link
Collaborator Author

acdlite commented Feb 27, 2021

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 scheduleWorkOnParentPath.

@ntucker
Copy link

ntucker commented Feb 27, 2021

Does this work with variable number of items selected? i.e.,

const fullContext = useContextSelector(Context, context => context.a.map(k => context[k]));

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 27, 2021

@ntucker I think that's more relevant to the Context selectors RFC: reactjs/rfcs#119

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 1, 2021

I did consider that but decided against it because it seems like more work in the common path. What I like about tracking just the fibers where the propagation ends is that it defers as much as possible to the next bailout, which in many cases won't ever happen.

This solves the sibling issue at any level

Could you elaborate on that? Unless I misunderstand you point, the sibling issue only matters for the direct siblings of the node that matched during propagation. We know for sure that they don't have props, state, or context scheduled on them, so they'll definitely bail out when we hit them during the render phase.

EDIT: Never mind, I interpreted your comment as implying that my current implementation doesn't solve the sibling issue at every level, but I think you meant your solution solves it without also needing the DidPropagateContext flag.

I did consider putting the siblings in a set. Which is pretty similar to your proposal of putting the parents in there. But I decided against it because it didn't seem obviously better, and it's worse in that the Set is fatter as you pointed out.

I added a comment with what I think is the best conceptual fix, which is to bail out of those siblings before ever entering the begin phase. The only reason we clone those nodes at all is because they are part of a persistent list with the rest of the children. We can't clone one child without cloning all of them. Though that doesn't mean we need to do a begin/complete; we just happen to rely on them immediately bailing out using the normal path. We could instead add a special path that quickly bails out. But it'd probably require more code. This has come up a several times, though, so it's something to consider as part of a larger rethink of the Fiber tree structure.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 1, 2021

then when bailing out if we are going deeper we pass in an "exhaustedPropagation" flag into cloneChildFibers which sets the DidPropagateContext bitflag on the cloned children

This idea is appealing but I'm hesitant to add anything to that function because there are a few weird exceptions where we don't call cloneChildFibers on the whole set of children. Just pointing this out though because I think it's a good idea.

@gnoff
Copy link
Collaborator

gnoff commented Mar 1, 2021

I may misunderstand how your solution works but this is the sibling issue at every level I am referring to

image
image

then when bailing out if we are going deeper we pass in an "exhaustedPropagation" flag into cloneChildFibers which sets the DidPropagateContext bitflag on the cloned children

This idea is appealing but I'm hesitant to add anything to that function because there are a few weird exceptions where we don't call cloneChildFibers on the whole set of children. Just pointing this out though because I think it's a good idea.

Yeah it's not necessary with the set proposal, i like your idea of using the bailout to govern this rather than the clone function

EDIT: Never mind, I interpreted your comment as implying that my current implementation doesn't solve the sibling issue at every level, but I think you meant your solution solves it without also needing the DidPropagateContext flag.

you are right about my intent but I'm actually not sure how you avoid the illustrated situation above

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 1, 2021

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 null inside bailoutOnAlreadyFinishedWork and reuse the current subtree.

If the green nodes are direct siblings of the blue nodes, then we will set DidPropagateContext at every level. So it will bail out as soon as it checks the parent. Nvm, I see what you're saying. The blue parents don't have DidPropagateContext set because they had work in their subtree. I thought you were saying earlier that we would end up propagating changes to the subtrees twice, but based on your diagram I take it you meant we'll traverse parent contexts that definitely couldn't have changed. You're right, this isn't ideal, but I kinda group this together with the idea that we shouldn't be entering the begin phase for those nodes at all; we should just clone them and immediately exit. But maybe there's a way to optimize this more that doesn't do completely that.

EDIT: Actually never mind again, the part I crossed out is correct. We always call propagateParentContexts inside bailoutOnAlreadyFinishedWork, so the green siblings will stop traversing the return path as soon as they check the parent.


I pushed another change. I got rid of the Set of matched consumers and replaced it with a flag called NeedsPropagation, which gets set any time context is read.

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 deferredPropagation. However, that's not a problem because anything that re-renders due to props or state needs propagation if it bails out, too. So the flag is correct regardless.

So the main downside is that I had to add an extra flag assignment to a non-bailout-specific path (readContext), which I was trying not to do. However, it's only set for the first context node per fiber. And in exchange we get to remove a lot of complexity from the bail out path.

The rest of the DidPropagateContext algorithm is the same as before.

@ghost
Copy link

ghost commented Mar 1, 2021

Nvm, I see what you're saying. The blue parents don't have DidPropagateContext set because they had work in their subtree.

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

we should just clone them and immediately exit.

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 pushed another change.

I'll check it out

edit: posted this from another gh profile but it's still me (gnoff)

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 1, 2021

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.

Comment on lines +525 to +534
if (!isInsidePropagationBailout) {
if ((parent.flags & NeedsPropagation) !== NoFlags) {
isInsidePropagationBailout = true;
} else if ((parent.flags & DidPropagateContext) !== NoFlags) {
break;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 acdlite force-pushed the lazy-context-propagation branch 2 times, most recently from 51f7179 to d1bfb29 Compare March 2, 2021 21:37
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Reviewed the overall strategy together offline, and left a couple non-blockers after reviewing here.

): void {
// Only used by lazy implemenation
if (!enableLazyContextPropagation) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw or are you calling this function in the old impl?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
}
} 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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);

@@ -374,5 +374,7 @@
"383": "This query has received fewer parameters than the last time the same query was used. Always pass the exact number of parameters that the query needs.",
"384": "Refreshing the cache is not supported in Server Components.",
"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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rickhanlonii rickhanlonii Mar 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 7, 2021

I'm going to land in two steps so if one of them breaks they can be bisected

acdlite and others added 5 commits March 7, 2021 00:37
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 <jcs.gnoff@gmail.com>
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 <jcs.gnoff@gmail.com>
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 <jcs.gnoff@gmail.com>
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 acdlite merged commit c7b4497 into facebook:master Mar 7, 2021
@gnoff
Copy link
Collaborator

gnoff commented Mar 7, 2021

🎉

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 17, 2021
Summary:
This sync includes the following changes:
- **[b9c4a01f7](facebook/react@b9c4a01f7 )**: Allow the streaming config to decide how to precompute or compute chunks ([#21008](facebook/react#21008)) //<Sebastian Markbåge>//
- **[c06d245fc](facebook/react@c06d245fc )**: Update devtools-extensions build script to reflect changes in local b… ([#21004](facebook/react#21004)) //<Hector Rincon>//
- **[14e4fd1ff](facebook/react@14e4fd1ff )**: [Fizz] Move DOM/Native format configs to their respective packages ([#20994](facebook/react#20994)) //<Sebastian Markbåge>//
- **[f2b6bf7c8](facebook/react@f2b6bf7c8 )**: [Fizz] Destroy the stream with an error if the root throws ([#20992](facebook/react#20992)) //<Sebastian Markbåge>//
- **[10cc40018](facebook/react@10cc40018 )**: Basic Fizz Architecture ([#20970](facebook/react#20970)) //<Sebastian Markbåge>//
- **[b7e631066](facebook/react@b7e631066 )**: Stop tracking roots with pending discrete updates ([#20978](facebook/react#20978)) //<Andrew Clark>//
- **[860f673](facebook/react@860f673a7 )**: Remove Blocking Mode (again) ([#20974](facebook/react#20974)) //<Ricky>//
- **[acde65469](facebook/react@acde65469 )**: Unify InputDiscreteLane with SyncLane ([#20968](facebook/react#20968)) //<Ricky>//
- **[6556e2a87](facebook/react@6556e2a87 )**: Cleaned up unused PassiveUnmountPendingDev DEV flag ([#20973](facebook/react#20973)) //<Brian Vaughn>//
- **[60182d64c](facebook/react@60182d64c )**: Cleanup tests using runWithPriority. ([#20958](facebook/react#20958)) //<Ricky>//
- **[e4d4b7074](facebook/react@e4d4b7074 )**: Land enableNativeEventPriorityInference ([#20955](facebook/react#20955)) //<Ricky>//
- **[73e900b0e](facebook/react@73e900b0e )**: Land enableDiscreteEventMicroTasks ([#20954](facebook/react#20954)) //<Ricky>//
- **[41e62e771](facebook/react@41e62e771 )**: Remove runWithPriority internally //<Rick Hanlon>//
- **[431e76e2d](facebook/react@431e76e2d )**: Switch callsites over to update lane priority //<Rick Hanlon>//
- **[e89d74ee6](facebook/react@e89d74ee6 )**: Remove decoupleUpdatePriorityFromScheduler //<Rick Hanlon>//
- **[ca15606d8](facebook/react@ca15606d8 )**: chore(build):  Ensure experimental builds exists on windows ([#20933](facebook/react#20933)) //<Sebastian Silbermann>//
- **[d74559746](facebook/react@d74559746 )**: Use update lane priority to set pending updates on roots ([#20918](facebook/react#20918)) //<Ricky>//
- **[f04bcb813](facebook/react@f04bcb813 )**: [Bugfix] Reset `subtreeFlags` in `resetWorkInProgress` ([#20948](facebook/react#20948)) //<Andrew Clark>//
- **[c7b449798](facebook/react@c7b449798 )**: [Experiment] Lazily propagate context changes ([#20890](facebook/react#20890)) //<Andrew Clark>//
- **[258b375a4](facebook/react@258b375a4 )**: Move context comparison to consumer //<Andrew Clark>//
- **[7df65725b](facebook/react@7df65725b )**: Split getComponentName into getComponentNameFromFiber and getComponentNameFromType ([#20940](facebook/react#20940)) //<Brian Vaughn>//
- **[ee4326357](facebook/react@ee4326357 )**: Revert "Remove blocking mode and blocking root ([#20888](facebook/react#20888))" ([#20916](facebook/react#20916)) //<Andrew Clark>//
- **[de0ee76db](facebook/react@de0ee76db )**: Add unstable_strictModeLevel to test renderer ([#20914](facebook/react#20914)) //<Andrew Clark>//
- **[d857f9e4d](facebook/react@d857f9e4d )**: Land enableSetImmediate feature flag ([#20906](facebook/react#20906)) //<Dan Abramov>//
- **[553440bd1](facebook/react@553440bd1 )**: Remove blocking mode and blocking root ([#20888](facebook/react#20888)) //<Ricky>//
- **[38f392ced](facebook/react@38f392ced )**: typo fix for the word 'Psuedo' ([#20894](facebook/react#20894)) //<Bowen>//
- **[0cf9fc10b](facebook/react@0cf9fc10b )**: Fix React Native flow types ([#20889](facebook/react#20889)) //<Ricky>//
- **[c581cdd48](facebook/react@c581cdd48 )**: Schedule sync updates in microtask ([#20872](facebook/react#20872)) //<Ricky>//
- **[90bde6505](facebook/react@90bde6505 )**: Add SuspenseList to react-is ([#20874](facebook/react#20874)) //<Brian Vaughn>//
- **[8336f19aa](facebook/react@8336f19aa )**: Update React Native types ([#20883](facebook/react#20883)) //<Rubén Norte>//
- **[9209c30ff](facebook/react@9209c30ff )**: Add StrictMode level prop and createRoot unstable_strictModeLevel option ([#20849](facebook/react#20849)) //<Brian Vaughn>//
- **[e5f6b91d2](facebook/react@e5f6b91d2 )**: Add Lane labels to scheduling profiler marks ([#20808](facebook/react#20808)) //<Brian Vaughn>//
- **[c62986cfd](facebook/react@c62986cfd )**: Add additional messaging for RulesOfHooks lint error ([#20692](facebook/react#20692)) //<Anthony Garritano>//
- **[78d2f2d30](facebook/react@78d2f2d30 )**: Fabric-compatible implementation of `JSReponder` feature ([#20768](facebook/react#20768)) //<Valentin Shergin>//
- **[4d28eca97](facebook/react@4d28eca97 )**: Land enableNonInterruptingNormalPri ([#20859](facebook/react#20859)) //<Ricky>//
- **[8af27aeed](facebook/react@8af27aeed )**: Remove scheduler sampling profiler shared array buffer ([#20840](facebook/react#20840)) //<Brian Vaughn>//
- **[af3d52611](facebook/react@af3d52611 )**: Disable (unstable) scheduler sampling profiler for OSS builds ([#20832](facebook/react#20832)) //<Brian Vaughn>//
- **[8fa0ccca0](facebook/react@8fa0ccca0 )**: fix: use SharedArrayBuffer only when cross-origin isolation is enabled ([#20831](facebook/react#20831)) //<Toru Kobayashi>//
- **[099164792](facebook/react@099164792 )**: Use setImmediate when available over MessageChannel ([#20834](facebook/react#20834)) //<Dan Abramov>//
- **[e2fd460cc](facebook/react@e2fd460cc )**: Bailout in sync task if work is not sync ([#20813](facebook/react#20813)) //<Andrew Clark>//
- **[1a7472624](facebook/react@1a7472624 )**: Add `supportsMicrotasks` to the host config ([#20809](facebook/react#20809)) //<Andrew Clark>//
- **[696e736be](facebook/react@696e736be )**: Warn if static flag is accidentally cleared ([#20807](facebook/react#20807)) //<Andrew Clark>//
- **[483358c38](facebook/react@483358c38 )**: Omit TransitionHydrationLane from TransitionLanes ([#20802](facebook/react#20802)) //<Andrew Clark>//
- **[78ec97d34](facebook/react@78ec97d34 )**: Fix typo ([#20466](facebook/react#20466)) //<inokawa>//
- **[6cdc35972](facebook/react@6cdc35972 )**: fix comments of markUpdateLaneFromFiberToRoot ([#20546](facebook/react#20546)) //<neroneroffy>//
- **[47dd9f441](facebook/react@47dd9f441 )**: Remove fakeCallbackNode ([#20799](facebook/react#20799)) //<Andrew Clark>//
- **[114ab5295](facebook/react@114ab5295 )**: Make remaining empty lanes Transition lanes ([#20793](facebook/react#20793)) //<Andrew Clark>//
- **[d3d2451a0](facebook/react@d3d2451a0 )**: Use a single lane per priority level ([#20791](facebook/react#20791)) //<Andrew Clark>//
- **[eee874ce6](facebook/react@eee874ce6 )**: Cross-fork lint: Support named export declaration ([#20784](facebook/react#20784)) //<Andrew Clark>//
- **[3b870b1e0](facebook/react@3b870b1e0 )**: Lane enableTransitionEntanglement flag ([#20775](facebook/react#20775)) //<Andrew Clark>//
- **[d1845ad0f](facebook/react@d1845ad0f )**: Default updates should not interrupt transitions ([#20771](facebook/react#20771)) //<Andrew Clark>//
- **[3499c343a](facebook/react@3499c343a )**: Apply #20778 to new fork, too ([#20782](facebook/react#20782)) //<Andrew Clark>//
- **[3d10eca24](facebook/react@3d10eca24 )**: Move scheduler priority check into ReactDOM ([#20778](facebook/react#20778)) //<Dan Abramov>//
- **[97fce318a](facebook/react@97fce318a )**: Experiment: Infer the current event priority from the native event ([#20748](facebook/react#20748)) //<Dan Abramov>//
- **[6c526c515](facebook/react@6c526c515 )**: Don't shift interleaved updates to separate lane ([#20681](facebook/react#20681)) //<Andrew Clark>//
- **[35f7441d3](facebook/react@35f7441d3 )**: Use Lanes instead of priority event constants ([#20762](facebook/react#20762)) //<Dan Abramov>//
- **[a014c915c](facebook/react@a014c915c )**: Parallel transitions: Assign different lanes to consecutive transitions ([#20672](facebook/react#20672)) //<Andrew Clark>//
- **[77754ae61](facebook/react@77754ae61 )**: Decouple event priority list from event name list ([#20760](facebook/react#20760)) //<Dan Abramov>//
- **[b5bac1821](facebook/react@b5bac1821 )**: Align event group constant naming with lane naming ([#20744](facebook/react#20744)) //<Dan Abramov>//
- **[4ecf11977](facebook/react@4ecf11977 )**: Remove the Fundamental internals ([#20745](facebook/react#20745)) //<Dan Abramov>//
- **[eeb1325b0](facebook/react@eeb1325b0 )**: Fix UMD bundles by removing usage of global ([#20743](facebook/react#20743)) //<Dan Abramov>//
- **[0935a1db3](facebook/react@0935a1db3 )**: Delete consolidateBundleSizes script ([#20724](facebook/react#20724)) //<Andrew Clark>//
- **[7cb9fd7ef](facebook/react@7cb9fd7ef )**: Land interleaved updates change in main fork ([#20710](facebook/react#20710)) //<Andrew Clark>//
- **[dc27b5aaa](facebook/react@dc27b5aaa )**: useMutableSource: Use StrictMode double render to detect render phase mutation ([#20698](facebook/react#20698)) //<Andrew Clark>//
- **[bb1b7951d](facebook/react@bb1b7951d )**: fix: don't run effects if a render phase update results in unchanged deps ([#20676](facebook/react#20676)) //<Sebastian Silbermann>//
- **[766a7a28a](facebook/react@766a7a28a )**: Improve React error message when mutable sources are mutated during render ([#20665](facebook/react#20665)) //<Brian Vaughn>//
- **[a922f1c71](facebook/react@a922f1c71 )**: Fix cache refresh bug that broke DevTools ([#20687](facebook/react#20687)) //<Andrew Clark>//
- **[e51bd6c1f](facebook/react@e51bd6c1f )**: Queue discrete events in microtask ([#20669](facebook/react#20669)) //<Ricky>//
- **[aa736a0fa](facebook/react@aa736a0fa )**: Add queue microtask to host configs ([#20668](facebook/react#20668)) //<Ricky>//
- **[deeeaf1d2](facebook/react@deeeaf1d2 )**: Entangle overlapping transitions per queue ([#20670](facebook/react#20670)) //<Andrew Clark>//
- **[e316f7855](facebook/react@e316f7855 )**: RN: Implement `sendAccessibilityEvent` in RN Renderer that proxies between Fabric/non-Fabric ([#20554](facebook/react#20554)) //<Joshua Gross>//
- **[9c32622cf](facebook/react@9c32622cf )**: Improve tests that use discrete events ([#20667](facebook/react#20667)) //<Ricky>//
- **[d13f5b953](facebook/react@d13f5b953 )**: Experiment: Unsuspend all lanes on update ([#20660](facebook/react#20660)) //<Andrew Clark>//
- **[a511dc709](facebook/react@a511dc709 )**: Error for deferred value and transition in Server Components ([#20657](facebook/react#20657)) //<Sebastian Markbåge>//
- **[fb3f63f1a](facebook/react@fb3f63f1a )**: Remove lazy invokation of segments ([#20656](facebook/react#20656)) //<Sebastian Markbåge>//
- **[895ae67fd](facebook/react@895ae67fd )**: Improve error boundary handling for unmounted subtrees ([#20645](facebook/react#20645)) //<Brian Vaughn>//
- **[f15f8f64b](facebook/react@f15f8f64b )**: Store interleaved updates on separate queue until end of render ([#20615](facebook/react#20615)) //<Andrew Clark>//
- **[0fd6805c6](facebook/react@0fd6805c6 )**: Land rest of effects refactor in main fork ([#20644](facebook/react#20644)) //<Andrew Clark>//
- **[a6b5256a2](facebook/react@a6b5256a2 )**: Refactored recursive strict effects method to be iterative ([#20642](facebook/react#20642)) //<Brian Vaughn>//
- **[3957853ae](facebook/react@3957853ae )**: Re-add "strict effects mode" for legacy roots only ([#20639](facebook/react#20639)) //<Brian Vaughn>//
- **[fceb75e89](facebook/react@fceb75e89 )**: Delete remaining references to effect list ([#20625](facebook/react#20625)) //<Andrew Clark>//
- **[741dcbdbe](facebook/react@741dcbdbe )**: Schedule passive phase whenever there's a deletion ([#20624](facebook/react#20624)) //<Andrew Clark>//
- **[11a983fc7](facebook/react@11a983fc7 )**: Remove references to Deletion flag ([#20623](facebook/react#20623)) //<Andrew Clark>//
- **[2e948e0d9](facebook/react@2e948e0d9 )**: Avoid .valueOf to close #20594 ([#20617](facebook/react#20617)) //<Dima Tisnek>//
- **[2a646f73e](facebook/react@2a646f73e )**: Convert snapshot phase to depth-first traversal ([#20622](facebook/react#20622)) //<Andrew Clark>//
- **[fb3e158a6](facebook/react@fb3e158a6 )**: Convert ReactSuspenseWithNoopRenderer tests to use built-in cache ([#20601](facebook/react#20601)) //<Andrew Clark>//
- **[e0fd9e67f](facebook/react@e0fd9e67f )**: Use update lane priority in work loop ([#20621](facebook/react#20621)) //<Ricky>//
- **[58e830448](facebook/react@58e830448 )**: Remove custom error message from hook access error ([#20604](facebook/react#20604)) //<Andrew Clark>//
- **[9043626f0](facebook/react@9043626f0 )**: Cache tests: Make it easier to test many caches ([#20600](facebook/react#20600)) //<Andrew Clark>//
- **[af0bb68e8](facebook/react@af0bb68e8 )**: Land #20595 and #20596 in main fork ([#20602](facebook/react#20602)) //<Andrew Clark>//
- **[2b6985114](facebook/react@2b6985114 )**: build-combined: Fix failures  when renaming across devices ([#20620](facebook/react#20620)) //<Sebastian Silbermann>//
- **[af16f755d](facebook/react@af16f755d )**: Update DevTools to use getCacheForType API ([#20548](facebook/react#20548)) //<Brian Vaughn>//
- **[95feb0e70](facebook/react@95feb0e70 )**: Convert mutation phase to depth-first traversal ([#20596](facebook/react#20596)) //<Andrew Clark>//
- **[6132919bf](facebook/react@6132919bf )**: Convert layout phase to depth-first traversal ([#20595](facebook/react#20595)) //<Andrew Clark>//
- **[42e04b46d](facebook/react@42e04b46d )**: Fix: Detach deleted fiber's alternate, too ([#20587](facebook/react#20587)) //<Andrew Clark>//
- **[a656ace8d](facebook/react@a656ace8d )**: Deletion effects should fire parent -> child ([#20584](facebook/react#20584)) //<Andrew Clark>//
- **[e6ed2bcf4](facebook/react@e6ed2bcf4 )**: Update package.json versions as part of build step ([#20579](facebook/react#20579)) //<Andrew Clark>//
- **[eb0fb3823](facebook/react@eb0fb3823 )**: Build stable and experimental with same command ([#20573](facebook/react#20573)) //<Andrew Clark>//
- **[e8eff119e](facebook/react@e8eff119e )**: Fix ESLint crash on empty react effect hook ([#20385](facebook/react#20385)) //<Christian Ruigrok>//
- **[27659559e](facebook/react@27659559e )**: Add useRefresh hook to react-debug-tools ([#20460](facebook/react#20460)) //<Brian Vaughn>//
- **[99554dc36](facebook/react@99554dc36 )**: Add Flight packages to experimental allowlist ([#20486](facebook/react#20486)) //<Andrew Clark>//
- **[efc57e5cb](facebook/react@efc57e5cb )**: Add built-in Suspense cache with support for invalidation (refreshing) ([#20456](facebook/react#20456)) //<Andrew Clark>//
- **[00a5b08e2](facebook/react@00a5b08e2 )**: Remove PassiveStatic optimization //<Andrew Clark>//
- **[a6329b105](facebook/react@a6329b105 )**: Don't clear static flags in resetWorkInProgress //<Andrew Clark>//
- **[1cf59f34b](facebook/react@1cf59f34b )**: Convert passive unmount phase to tree traversal //<Andrew Clark>//
- **[ab29695a0](facebook/react@ab29695a0 )**: Defer more field detachments to passive phase //<Andrew Clark>//
- **[d37d7a4bb](facebook/react@d37d7a4bb )**: Convert passive mount phase to tree traversal //<Andrew Clark>//
- **[19e15a398](facebook/react@19e15a398 )**: Add PassiveStatic to trees with passive effects //<Andrew Clark>//
- **[ff17fc176](facebook/react@ff17fc176 )**: Don't clear other flags when adding Deletion //<Andrew Clark>//
- **[5687864eb](facebook/react@5687864eb )**: Add back disableSchedulerTimeoutInWorkLoop flag ([#20482](facebook/react#20482)) //<Ricky>//
- **[9f338e5d7](facebook/react@9f338e5d7 )**: clone json obj in react native flight client host config parser ([#20474](facebook/react#20474)) //<Luna Ruan>//
- **[4e62fd271](facebook/react@4e62fd271 )**: clone json obj in relay flight client host config parser ([#20465](facebook/react#20465)) //<Luna Ruan>//
- **[070372cde](facebook/react@070372cde )**: [Flight] Fix webpack watch mode issue ([#20457](facebook/react#20457)) //<Dan Abramov>//
- **[0f80dd148](facebook/react@0f80dd148 )**: [Flight] Support concatenated modules in Webpack plugin ([#20449](facebook/react#20449)) //<Dan Abramov>//
- **[daf38ecdf](facebook/react@daf38ecdf )**: [Flight] Use lazy reference for existing modules ([#20445](facebook/react#20445)) //<Dan Abramov>//
- **[3f9205c33](facebook/react@3f9205c33 )**: Regression test: SuspenseList causes lost unmount ([#20433](facebook/react#20433)) //<Andrew Clark>//
- **[cdfde3ae1](facebook/react@cdfde3ae1 )**: Always rethrow original error when we replay errors ([#20425](facebook/react#20425)) //<Sebastian Markbåge>//
- **[b15d6e93e](facebook/react@b15d6e93e )**: [Flight] Make PG and FS server-only ([#20424](facebook/react#20424)) //<Dan Abramov>//
- **[40ff2395e](facebook/react@40ff2395e )**: [Flight] Prevent non-Server imports of aliased Server entrypoints ([#20422](facebook/react#20422)) //<Dan Abramov>//
- **[94aa365e3](facebook/react@94aa365e3 )**: [Flight] Fix webpack plugin to use chunk groups ([#20421](facebook/react#20421)) //<Dan Abramov>//
- **[842ee367e](facebook/react@842ee367e )**: [Flight] Rename the shared entry point ([#20420](facebook/react#20420)) //<Dan Abramov>//
- **[dbf40ef75](facebook/react@dbf40ef75 )**: Put .server.js at the end of bundle filenames ([#20419](facebook/react#20419)) //<Dan Abramov>//
- **[03126dd08](facebook/react@03126dd08 )**: [Flight] Add read-only fs methods ([#20412](facebook/react#20412)) //<Dan Abramov>//
- **[b51a686a9](facebook/react@b51a686a9 )**: Turn on double effects for www test renderer ([#20416](facebook/react#20416)) //<Brian Vaughn>//
- **[56a632adb](facebook/react@56a632adb )**: Double Invoke Effects in __DEV__ (in old reconciler fork) ([#20415](facebook/react#20415)) //<Brian Vaughn>//
- **[1a2422337](facebook/react@1a2422337 )**: fixed typo ([#20351](facebook/react#20351)) //<togami2864>//
- **[a233c9e2a](facebook/react@a233c9e2a )**: Rename internal cache helpers ([#20410](facebook/react#20410)) //<Dan Abramov>//
- **[6a4b12b81](facebook/react@6a4b12b81 )**: [Flight] Add rudimentary FS binding ([#20409](facebook/react#20409)) //<Dan Abramov>//
- **[7659949d6](facebook/react@7659949d6 )**: Clear `deletions` in `detachFiber` ([#20401](facebook/react#20401)) //<Andrew Clark>//
- **[b9680aef7](facebook/react@b9680aef7 )**: Cache react-fetch results in the Node version ([#20407](facebook/react#20407)) //<Dan Abramov>//
- **[cdae31ab8](facebook/react@cdae31ab8 )**: Fix typo ([#20279](facebook/react#20279)) //<inokawa>//
- **[51a7cfe21](facebook/react@51a7cfe21 )**: Fix typo ([#20300](facebook/react#20300)) //<Hollow Man>//
- **[373b297c5](facebook/react@373b297c5 )**: fix: Fix typo in react-reconciler docs ([#20284](facebook/react#20284)) //<Sam Zhou>//
- **[1b5ca9906](facebook/react@1b5ca9906 )**: Fix module ID deduplication ([#20406](facebook/react#20406)) //<Dan Abramov>//
- **[5fd9db732](facebook/react@5fd9db732 )**: [Flight] Rename react-transport-... packages to react-server-... ([#20403](facebook/react#20403)) //<Sebastian Markbåge>//
- **[ce40f1dc2](facebook/react@ce40f1dc2 )**: Use assets API + writeToDisk instead of directly writing to disk ([#20402](facebook/react#20402)) //<Sebastian Markbåge>//
- **[b66ae09b6](facebook/react@b66ae09b6 )**: Track subtreeFlags et al with bubbleProperties //<Andrew Clark>//
- **[de75315d7](facebook/react@de75315d7 )**: Track deletions using an array on the parent //<Andrew Clark>//
- **[1377e465d](facebook/react@1377e465d )**: Add Placement bit without removing others ([#20398](facebook/react#20398)) //<Andrew Clark>//
- **[18d7574ae](facebook/react@18d7574ae )**: Remove `catch` from Scheduler build ([#20396](facebook/react#20396)) //<Andrew Clark>//
- **[30dfb8602](facebook/react@30dfb8602 )**: [Flight] Basic scan of the file system to find Client modules ([#20383](facebook/react#20383)) //<Sebastian Markbåge>//
- **[9b8060041](facebook/react@9b8060041 )**: Error when the number of parameters to a query changes ([#20379](facebook/react#20379)) //<Dan Abramov>//
- **[60e4a76](facebook/react@60e4a76fa )**: [Flight] Add rudimentary PG binding ([#20372](facebook/react#20372)) //<Dan Abramov>//
- **[88ef95712](facebook/react@88ef95712 )**: Fork ReactFiberLane ([#20371](facebook/react#20371)) //<Andrew Clark>//
- **[41c5d00fc](facebook/react@41c5d00fc )**: [Flight] Minimal webpack plugin ([#20228](facebook/react#20228)) //<Dan Abramov>//
- **[e23673b51](facebook/react@e23673b51 )**: [Flight] Add getCacheForType() to the dispatcher ([#20315](facebook/react#20315)) //<Dan Abramov>//
- **[555eeae33](facebook/react@555eeae33 )**: Add disableNativeComponentFrames flag ([#20364](facebook/react#20364)) //<Philipp Spiess>//
- **[148ffe3cf](facebook/react@148ffe3cf )**: Failing test for Client reconciliation ([#20318](facebook/react#20318)) //<Dan Abramov>//
- **[a2a025537](facebook/react@a2a025537 )**: Fixed invalid DevTools work tags ([#20362](facebook/react#20362)) //<Brian Vaughn>//
- **[5711811da](facebook/react@5711811da )**: Reconcile element types of lazy component yielding the same type ([#20357](facebook/react#20357)) //<Sebastian Markbåge>//
- **[3f73dcee3](facebook/react@3f73dcee3 )**: Support named exports from client references ([#20312](facebook/react#20312)) //<Sebastian Markbåge>//
- **[565148d75](facebook/react@565148d75 )**: Disallow *.server.js imports from any other files ([#20309](facebook/react#20309)) //<Sebastian Markbåge>//
- **[e6a0f2763](facebook/react@e6a0f2763 )**: Profiler: Improve nested-update checks ([#20299](facebook/react#20299)) //<Brian Vaughn>//
- **[d93b58a5e](facebook/react@d93b58a5e )**: Add flight specific entry point for react package ([#20304](facebook/react#20304)) //<Sebastian Markbåge>//
- **[a81c02ac1](facebook/react@a81c02ac1 )**: Profiler onNestedUpdateScheduled accepts id as first param ([#20293](facebook/react#20293)) //<Brian Vaughn>//
- **[ac2cff4b1](facebook/react@ac2cff4b1 )**: Warn if commit phase error thrown in detached tree ([#20286](facebook/react#20286)) //<Andrew Clark>//
- **[0f83a64ed](facebook/react@0f83a64ed )**: Regression test: Missing unmount after re-order ([#20285](facebook/react#20285)) //<Andrew Clark>//
- **[ebf158965](facebook/react@ebf158965 )**: Add best-effort documentation for third-party renderers ([#20278](facebook/react#20278)) //<Dan Abramov>//
- **[82e99e1b0](facebook/react@82e99e1b0 )**: Add Node ESM Loader and Register Entrypoints ([#20274](facebook/react#20274)) //<Sebastian Markbåge>//
- **[bf7b7aeb1](facebook/react@bf7b7aeb1 )**: findDOMNode: Remove return pointer mutation ([#20272](facebook/react#20272)) //<Andrew Clark>//
- **[369c3db62](facebook/react@369c3db62 )**: Add separate ChildDeletion flag ([#20264](facebook/react#20264)) //<Andrew Clark>//
- **[765e89b90](facebook/react@765e89b90 )**: Reset new fork to old fork  ([#20254](facebook/react#20254)) //<Andrew Clark>//
- **[7548dd573](facebook/react@7548dd573 )**: Properly reset Profiler nested-update flag ([#20253](facebook/react#20253)) //<Brian Vaughn>//
- **[b44e4b13a](facebook/react@b44e4b13a )**: Check for deletions in `hadNoMutationsEffects` ([#20252](facebook/react#20252)) //<Andrew Clark>//
- **[3ebf05183](facebook/react@3ebf05183 )**: Add new effect fields to old fork, and vice versa ([#20246](facebook/react#20246)) //<Andrew Clark>//
- **[2fbcc9806](facebook/react@2fbcc9806 )**: Remove cycle between ReactFiberHooks and ReactInternalTypes ([#20242](facebook/react#20242)) //<Paul Doyle>//
- **[504222dcd](facebook/react@504222dcd )**: Add Node ESM build option ([#20243](facebook/react#20243)) //<Sebastian Markbåge>//
- **[1b96ee444](facebook/react@1b96ee444 )**: Remove noinline directives from new commit phase ([#20241](facebook/react#20241)) //<Andrew Clark>//
- **[760d9ab57](facebook/react@760d9ab57 )**: Scheduling profiler tweaks ([#20215](facebook/react#20215)) //<Brian Vaughn>//
- **[9403c3b53](facebook/react@9403c3b53 )**: Add Profiler callback when nested updates are scheduled ([#20211](facebook/react#20211)) //<Brian Vaughn>//
- **[62efd9618](facebook/react@62efd9618 )**: use-subscription@1.5.1 //<Dan Abramov>//
- **[e7006d67d](facebook/react@e7006d67d )**: Widen peer dependency range of use-subscription ([#20225](facebook/react#20225)) //<Billy Janitsch>//
- **[15df051c9](facebook/react@15df051c9 )**: Add warning if return pointer is inconsistent ([#20219](facebook/react#20219)) //<Andrew Clark>//
- **[9aca239f1](facebook/react@9aca239f1 )**: Improved dev experience when DevTools hook is disabled ([#20208](facebook/react#20208)) //<Alphabet Codes>//
- **[12627f93b](facebook/react@12627f93b )**: Perform hasOwnProperty check in Relay Flight ([#20220](facebook/react#20220)) //<Sebastian Markbåge>//
- **[163199d8c](facebook/react@163199d8c )**: Dedupe module id generation ([#20172](facebook/react#20172)) //<Sebastian Markbåge>//
- **[76a6dbcb9](facebook/react@76a6dbcb9 )**: [Flight] Encode Symbols as special rows that can be referenced by models … ([#20171](facebook/react#20171)) //<Sebastian Markbåge>//
- **[35e53b465](facebook/react@35e53b465 )**: [Flight] Simplify Relay row protocol ([#20168](facebook/react#20168)) //<Sebastian Markbåge>//
- **[16e6dadba](facebook/react@16e6dadba )**: Encode throwing server components as lazy throwing references ([#20217](facebook/react#20217)) //<Sebastian Markbåge>//
- **[c896cf961](facebook/react@c896cf961 )**: Set return pointer when reusing current tree ([#20212](facebook/react#20212)) //<Andrew Clark>//
- **[089866015](facebook/react@089866015 )**: Add version of scheduler that only swaps MessageChannel for postTask ([#20206](facebook/react#20206)) //<Ricky>//
- **[393c452e3](facebook/react@393c452e3 )**: Add "nested-update" phase to Profiler API ([#20163](facebook/react#20163)) //<Brian Vaughn>//
- **[13a62feab](facebook/react@13a62feab )**: Fix path for SchedulerFeatureFlags ([#20200](facebook/react#20200)) //<Ricky>//
- **[7a73d6a0f](facebook/react@7a73d6a0f )**: (Temporarily) revert unmounting error boundaries changes ([#20147](facebook/react#20147)) //<Brian Vaughn>//
- **[c29710a57](facebook/react@c29710a57 )**: fix: useImperativeMethods to useImperativeHandle ([#20194](facebook/react#20194)) //<Jack Works>//
- **[f8979e0e2](facebook/react@f8979e0e2 )**: Revert 'Fabric-compatible implementation of  feature' and have Fabric noop when setJSResponder is called for now ([#21009](facebook/react#21009)) //<Joshua Gross>//
- **[c9f6d0a3a](facebook/react@c9f6d0a3a )**: Sync `ReactNativeTypes` from React Native ([#21015](facebook/react#21015)) //<Timothy Yung>//

Changelog:
[General][Changed] - React Native sync for revisions c3e20f1...c9f6d0a

jest_e2e[run_all_tests]

Reviewed By: PeteTheHeat

Differential Revision: D27051885

fbshipit-source-id: 5b232f6093f5f2527f3b321bc8b5487934e92d70
@budarin
Copy link

budarin commented Apr 22, 2021

yet another one realization https://github.com/edriang/use-context-selection

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Move context comparison to consumer

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.

* [Experiment] Lazily propagate context changes

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 <jcs.gnoff@gmail.com>

* Propagate all contexts in single pass

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 <jcs.gnoff@gmail.com>

* Stop propagating at nearest dependency match

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 <jcs.gnoff@gmail.com>

* Mark trees that need propagation in readContext

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.

Co-authored-by: Josh Story <jcs.gnoff@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants