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.

Sign up for GitHub

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

Popping context is O(1) in SSR #13019

Merged
merged 1 commit into from
Jun 11, 2018
Merged

Popping context is O(1) in SSR #13019

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 48 additions & 27 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,8 +641,10 @@ class ReactDOMServerRenderer {
previousWasTextNode: boolean;
makeStaticMarkup: boolean;

providerStack: Array<?ReactProvider<any>>;
providerIndex: number;
contextIndex: number;
contextStack: Array<ReactContext<any>>;
contextValueStack: Array<any>;
contextProviderStack: ?Array<ReactProvider<any>>; // DEV-only

constructor(children: mixed, makeStaticMarkup: boolean) {
const flatChildren = flattenTopLevelChildren(children);
Expand All @@ -667,46 +669,65 @@ class ReactDOMServerRenderer {
this.makeStaticMarkup = makeStaticMarkup;

// Context (new API)
this.providerStack = []; // Stack of provider objects
this.providerIndex = -1;
this.contextIndex = -1;
this.contextStack = [];
this.contextValueStack = [];
if (__DEV__) {
this.contextProviderStack = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why'd we change the previous providerStack to contextStack?

Or maybe, why do we also need contextProviderStack in DEV? Couldn't our DEV warning just use:

index > -1 &&
provider.type._context === (this.contextStack: any)[index],

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why'd we change the previous providerStack to contextStack?

From the PR:

I also have a DEV-only stack for providers. I could've used it for context objects too, but I decided it's nice to avoid some property access on every pop when we need to read the context object.

I meant that if I keep providers on the stack, I need to do a bunch of object access (provider.type._context) just to get to the context object. Storing it directly avoids that. I don't use provider itself anyway for anything other than DEV validation. This is similar to how we store Fibers on the stack only in DEV for validation.

Or maybe, why do we also need contextProviderStack in DEV? Couldn't our DEV warning just use:
index > -1 &&
provider.type._context === (this.contextStack: any)[index],

This only validates that the context type matches so it's less restrictive. We could have a bug where we accidentally pop the wrong provider object but miss it because it has the same type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also have a DEV-only stack for providers. I could've used it for context objects too, but I decided it's nice to avoid some property access on every pop when we need to read the context object.

It's not obvious to me that the cost of maintaining a second stack is less than the cost of a property access. I guess it's not a big deal either way, since it's DEV only though.

This only validates that the context type matches so it's less restrictive. We could have a bug where we accidentally pop the wrong provider object but miss it because it has the same type.

Context (type), Provider, and Consumer have a 1-to-1-to-1 relationship, no?

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's not obvious to me that the cost of maintaining a second stack is less than the cost of a property access. I guess it's not a big deal either way, since it's DEV only though.

Exactly, I was mostly microoptimizing for prod.

Context (type), Provider, and Consumer have a 1-to-1-to-1 relationship, no?

"Provider" is an object in this case. I guess providerElement would have been a clearer naming. So it's literally <MyProvider /> that gets pushed and popped, not MyProvider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I see what you're saying now (for both cases) 😄 Makes sense. Thanks for explaining.

}
}

/**
* Note: We use just two stacks regardless of how many context providers you have.
* Providers are always popped in the reverse order to how they were pushed
* so we always know on the way down which provider you'll encounter next on the way up.
* On the way down, we push the current provider, and its context value *before*
* we mutated it, onto the stacks. Therefore, on the way up, we always know which
* provider needs to be "restored" to which value.
* https://github.com/facebook/react/pull/12985#issuecomment-396301248
*/

pushProvider<T>(provider: ReactProvider<T>): void {
this.providerIndex += 1;
this.providerStack[this.providerIndex] = provider;
const index = ++this.contextIndex;
const context: ReactContext<any> = provider.type._context;
const previousValue = context._currentValue;

// Remember which value to restore this context to on our way up.
this.contextStack[index] = context;
this.contextValueStack[index] = previousValue;
if (__DEV__) {
// Only used for push/pop mismatch warnings.
(this.contextProviderStack: any)[index] = provider;
}

// Mutate the current value.
context._currentValue = provider.props.value;
}

popProvider<T>(provider: ReactProvider<T>): void {
const index = this.contextIndex;
if (__DEV__) {
warning(
this.providerIndex > -1 &&
provider === this.providerStack[this.providerIndex],
index > -1 && provider === (this.contextProviderStack: any)[index],
'Unexpected pop.',
);
}
this.providerStack[this.providerIndex] = null;
this.providerIndex -= 1;
const context: ReactContext<any> = provider.type._context;

// Find the closest parent provider of the same type and use its value.
// TODO: it would be nice to avoid this being O(N).
let contextPriorProvider = null;
for (let i = this.providerIndex; i >= 0; i--) {
// We assume this Flow type is correct because of the index check above
// and because pushProvider() enforces the correct type.
const priorProvider: ReactProvider<any> = (this.providerStack[i]: any);
if (priorProvider.type === provider.type) {
contextPriorProvider = priorProvider;
break;
}
}
if (contextPriorProvider !== null) {
context._currentValue = contextPriorProvider.props.value;
} else {
context._currentValue = context._defaultValue;
const context: ReactContext<any> = this.contextStack[index];
const previousValue = this.contextValueStack[index];

// "Hide" these null assignments from Flow by using `any`
// because conceptually they are deletions--as long as we
// promise to never access values beyond `this.contextIndex`.
this.contextStack[index] = (null: any);
this.contextValueStack[index] = (null: any);
if (__DEV__) {
(this.contextProviderStack: any)[index] = (null: any);
}
this.contextIndex--;

// Restore to the previous value we stored as we were walking down.
context._currentValue = previousValue;
}

read(bytes: number): string | null {
Expand Down