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

[Fizz] Implement New Context #21255

Merged
merged 9 commits into from
Apr 14, 2021
Merged

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 13, 2021

Fizz is a bit different than either Fiber or the partial renderer in terms of scenarios and tradeoffs. Unlike the partial renderer, we don't just always resume where we previously left off. We can suspend in different sibling trees. Unlike Fiber, we never have to restart with new props and we don't otherwise need the tree we've already past.

We could use a model similar to partial renderer's use of threads, where each context has N number of concurrent threads and a thread is allocated for each suspended task. But that would require a large and growing array for each Context which probably doesn't end up paying for itself.

We could use a Map of contexts associated with each provider. This requires a lot of cloning at the point of each provider. Effectively what legacy context does.

We could also store a loop up list of all parent providers that we backtrack any time we read a Context. That comes at the cost of reading being slower.

The general tradeoffs that we want to favor is making reading cheap so that mostly global dependency injection things are very cheap. We also generally favor rendering at full perf once something is unsuspended. Even though there are many possible suspended points, if you're preload a lot of data then you hopefully can just render straight through large sections without suspending at all.

The approach that I actually went with is similar to the Fiber approach in that we update the _currentValue field on each context and then reading is as cheap as just reading that value.

I also store a linked list of the stack of context providers that we're currently in. This gets heap allocated each time we visit a provider. If we need to spawn a new task, this node is what gets stored on the Task.

When we resume work on a task, we switch context by popping and pushing the delta between the most recently active context and the task's context. I.e. we restore where we were.

The cost of this happens at the point when we switch between tasks. It's cheap (or free) when we're resuming a single task over and over. Such as when the first few root components suspend or when you've reach the last suspended subtree which is the final one blocking completion. It's slower when you're switching back and forth between two deep and far removed siblings but at that point you're not close to reaching completion anyway.

(This could use some more tests of various suspending and resuming scenarios that are specific to Fizz and isn't covered by existing tests. The coverage isn't great yet. But didn't want to spend too much time on it before I unblock other work on top.)

This implements a reverse linked list tree containing the previous
contexts.
This algorithm pops the contexts back to a shared ancestor on the way down
the stack and then pushes new contexts in reverse order up the stack.
This is primarily intended to be used to support renderToString with a
separate build than the main one. This allows them to be nested.
@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Apr 13, 2021
// for that. Therefore this algorithm is recursive.
// 1) First we pop which ever snapshot tree was deepest. Popping old contexts as we go.
// 2) Then we find the nearest common ancestor from there. Popping old contexts as we go.
// 3) Then we reapply new contexts on the way back up the stack.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sophiebits You might enjoy this algorithm.

I also considered only storing the "value" and not depth/previousValue. That can be used to create a mode that always pushes all contexts - unless the new context is a strict superset. That could make the whole thing a bit lighter and still be fast when it only unsuspends in strict parent->child order.

But I think the pure common ancestor approach is probably worth it.

@@ -1148,6 +1319,16 @@ function performWork(request: Request): void {
fatalError(request, error);
} finally {
ReactCurrentDispatcher.current = prevDispatcher;
if (prevDispatcher === Dispatcher) {
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Apr 13, 2021

Choose a reason for hiding this comment

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

This case is meant to handle something like:

function Foo() {
  renderToString(<Bar />);
}
renderToString(<Foo />);

In a follow up.

@@ -48,6 +48,10 @@ import hasOwnProperty from 'shared/hasOwnProperty';
import sanitizeURL from '../shared/sanitizeURL';
import isArray from 'shared/isArray';

// Used to distinguish these contexts from ones used in other renderers.
// E.g. this can be used to distinguish legacy renderers from this modern one.
export const isPrimaryRenderer = true;
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 plan on building renderToString and Fizz in separate renderers so this will be used to deal with this case:

function Foo() {
  renderToString(<Bar />);
}
renderToWritable(<Foo />);

@sizebot
Copy link

sizebot commented Apr 13, 2021

Comparing: dbadfa2...5acef5f

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 = 122.68 kB 122.68 kB = 39.38 kB 39.38 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.25 kB 129.25 kB = 41.46 kB 41.46 kB
facebook-www/ReactDOM-prod.classic.js = 412.18 kB 412.18 kB = 76.23 kB 76.23 kB
facebook-www/ReactDOM-prod.modern.js = 400.23 kB 400.23 kB = 74.32 kB 74.32 kB
facebook-www/ReactDOMForked-prod.classic.js = 412.18 kB 412.18 kB = 76.23 kB 76.23 kB
oss-experimental/react-server/cjs/react-server.development.js +11.42% 86.30 kB 96.15 kB +12.47% 21.14 kB 23.77 kB
oss-stable/react-server/cjs/react-server.development.js +11.42% 86.30 kB 96.15 kB +12.47% 21.14 kB 23.77 kB
oss-experimental/react-server/cjs/react-server.production.min.js +10.06% 13.69 kB 15.07 kB +7.88% 4.76 kB 5.13 kB
oss-stable/react-server/cjs/react-server.production.min.js +10.06% 13.69 kB 15.07 kB +7.88% 4.76 kB 5.13 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +5.70% 191.48 kB 202.40 kB +6.08% 44.04 kB 46.71 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +5.69% 182.25 kB 192.61 kB +6.04% 43.48 kB 46.10 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +5.68% 182.30 kB 192.66 kB +6.03% 43.58 kB 46.21 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +4.84% 28.33 kB 29.70 kB +4.03% 9.55 kB 9.94 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +4.79% 28.51 kB 29.88 kB +3.75% 9.66 kB 10.02 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +4.79% 28.63 kB 30.00 kB +3.97% 9.58 kB 9.96 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server/cjs/react-server.development.js +11.42% 86.30 kB 96.15 kB +12.47% 21.14 kB 23.77 kB
oss-stable/react-server/cjs/react-server.development.js +11.42% 86.30 kB 96.15 kB +12.47% 21.14 kB 23.77 kB
oss-experimental/react-server/cjs/react-server.production.min.js +10.06% 13.69 kB 15.07 kB +7.88% 4.76 kB 5.13 kB
oss-stable/react-server/cjs/react-server.production.min.js +10.06% 13.69 kB 15.07 kB +7.88% 4.76 kB 5.13 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +5.70% 191.48 kB 202.40 kB +6.08% 44.04 kB 46.71 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +5.69% 182.25 kB 192.61 kB +6.04% 43.48 kB 46.10 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +5.68% 182.30 kB 192.66 kB +6.03% 43.58 kB 46.21 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +4.84% 28.33 kB 29.70 kB +4.03% 9.55 kB 9.94 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +4.79% 28.51 kB 29.88 kB +3.75% 9.66 kB 10.02 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +4.79% 28.63 kB 30.00 kB +3.97% 9.58 kB 9.96 kB

Generated by 🚫 dangerJS against 5acef5f


export function popProvider<T>(context: ReactContext<T>): void {
const prevSnapshot = currentActiveSnapshot;
invariant(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why check this in prod?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It follows the principle that if it's getting checked by runtime type checks here anyway, it likely isn't a perf cost (other than the extra code). The prevSnapshot.context access after does that anyway.

} else if (next === null) {
popAllPrevious(prev);
} else if (prev.depth === next.depth) {
popToNearestCommonAncestor(prev, next);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The naming of this and the next one confused me because they don't only pop, but also push the missing ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I thought that when I was naming them. Not sure what to call it. Maybe ...AndThenPush.

// We must still be deeper.
popNextToCommonLevel(prev, parentNext);
}
pushNode(next);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed? Same question re: initial pop in the previous method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case one is deeper than the other there won't be the same amount of pop/push calls. If the previous is deeper, we need to pop more times than we push. If the next is deeper, we need to push more than we pop.

@sebmarkbage sebmarkbage merged commit 4f76a28 into facebook:master Apr 14, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 20, 2021
Summary:
This sync includes the following changes:
- **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>//
- **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>//
- **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>//
- **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>//
- **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>//
- **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>//
- **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>//
- **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>//
- **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>//
- **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>//
- **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>//
- **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>//
- **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>//
- **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>//
- **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>//
- **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>//
- **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>//
- **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>//
- **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>//
- **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>//
- **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>//
- **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>//
- **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>//
- **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>//
- **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>//
- **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>//
- **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>//
- **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>//
- **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>//
- **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>//
- **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>//
- **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>//
- **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>//
- **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>//
- **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>//
- **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>//
- **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>//
- **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>//
- **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>//
- **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>//
- **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>//
- **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>//
- **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>//
- **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>//
- **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>//
- **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>//
- **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>//

Changelog:
[General][Changed] - React Native sync for revisions c9aab1c...f7cdc89

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D27740113

fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Add NewContext module

This implements a reverse linked list tree containing the previous
contexts.

* Implement recursive algorithm

This algorithm pops the contexts back to a shared ancestor on the way down
the stack and then pushes new contexts in reverse order up the stack.

* Move isPrimaryRenderer to ServerFormatConfig

This is primarily intended to be used to support renderToString with a
separate build than the main one. This allows them to be nested.

* Wire up more element type matchers

* Wire up Context Provider type

* Wire up Context Consumer

* Test

* Implement reader in class

* Update error codez
@gaearon gaearon mentioned this pull request Mar 29, 2022
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.

4 participants