Popping context is O(1) in SSR

[gaearon]

#12985 fixed a bug (#12984) but added a performance regression, making context popping O(N) where N is the number of providers on the stack.

This diff changes the server stack to behave more similarly to what we do on the client. I'm using two stacks in practice: one stack for the "context" objects and one for the "values" we need to restore them to on the way up.

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. So I only use the provider stack for push/pop mismatch validation, just like we do in Fiber.

[pull-bot]

Details of bundled changes.

Comparing: d480782...bb1c5f5

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom-server.browser.development.js	+0.7%	+1.3%	101.88 KB	102.61 KB	26.58 KB	26.91 KB	UMD_DEV
react-dom-server.browser.production.min.js	-0.1%	-0.3%	15.11 KB	15.1 KB	5.79 KB	5.78 KB	UMD_PROD
react-dom-server.browser.development.js	+0.8%	+1.4%	91.18 KB	91.91 KB	24.32 KB	24.66 KB	NODE_DEV
react-dom-server.browser.production.min.js	-0.1%	-0.1%	14.48 KB	14.46 KB	5.54 KB	5.53 KB	NODE_PROD
react-dom-server.node.development.js	+0.8%	+1.3%	93.1 KB	93.83 KB	24.87 KB	25.2 KB	NODE_DEV
react-dom-server.node.production.min.js	-0.1%	-0.2%	15.28 KB	15.27 KB	5.83 KB	5.82 KB	NODE_PROD
ReactDOMServer-dev.js	+0.7%	+1.4%	94.96 KB	95.67 KB	24.19 KB	24.53 KB	FB_WWW_DEV
ReactDOMServer-prod.js	-0.5%	-0.5%	31.68 KB	31.54 KB	7.77 KB	7.74 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

[bvaughn]

The general approach of pushing, popping, and restoring looks okay to me.

[bvaughn]

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

[gaearon]

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.

[bvaughn]

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?

[gaearon]

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.

[bvaughn]

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