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

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

merged 1 commit into from Jun 11, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jun 11, 2018

#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
Copy link

pull-bot commented Jun 11, 2018

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

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

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

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.

@gaearon gaearon merged commit ec60457 into facebook:master Jun 11, 2018
@gaearon gaearon deleted the ctx-ssr branch June 11, 2018 19:52
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 13, 2018
193: Update dependency react to v16.4.1 r=magopian a=renovate[bot]

This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1`



<details>
<summary>Release Notes</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request Jun 14, 2018
25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants