Memoize promise listeners to prevent exponential growth #14429
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ReactDOM: size: 🔺+0.7%, gzip: 🔺+0.8% Details of bundled changes.Comparing: 535804f...3b60eef react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
threepointone
approved these changes
Dec 13, 2018
acdlite
force-pushed
the
memoize-promise-listeners
branch
2 times, most recently
from
891fcf2
to
79eb26e
Compare
sebmarkbage
reviewed
Dec 14, 2018
| ); | ||
| // runPlaceholderTests('ReactSuspensePlaceholder (mutation)', () => | ||
| // require('react-noop-renderer'), | ||
| // ); |
There was a problem hiding this comment.
Commented it out while debugging, meant to add back. Although now I just noticed... I screwed up when I converted this to use the test renderer. It's not actually testing anything in concurrent mode anymore 🤦
There was a problem hiding this comment.
I'll fix that in a separate PR since it's unrelated
sebmarkbage
approved these changes
Dec 14, 2018
|
|
||
| // If this boundary just timed out, then it will have a set of thenables. | ||
| // For each thenable, attach a listener so that when it resolves, React | ||
| // attempts to re-render the boundary in the primary (pre-timeout) state. |
There was a problem hiding this comment.
I thought we wanted to do this work in the passive effect? Is there a reason not to?
There was a problem hiding this comment.
Only that it seemed like a riskier change. I'll add a follow-up.
|
You still need to add the lint config to a few more places like RN. |
Previously, React would attach a new listener every time a promise is thrown, regardless of whether the same listener was already attached during a previous render. Because React attempts to render every time a promise resolves, the number of listeners grows quickly. This was especially bad in synchronous mode because the renders that happen when the promise pings are not batched together. So if a single promise has multiple listeners for the same root, there will be multiple renders, which in turn results in more listeners being added to the remaining unresolved promises. This results in exponential growth in the number of listeners with respect to the number of IO-bound components in a single render. Fixes facebook#14220
acdlite
force-pushed
the
memoize-promise-listeners
branch
from
000a8dd
to
3b60eef
Compare
|
"Because React attempts to render every time a promise resolves" ah, so it's intentional. I thought it was a bug when I encountered this in test code (I thought React would wait for all thrown Promises to resolve before attempting to rerender a Suspense. Won't this just cause another suspend again?) |
|
@Kovensky Every time a promise pings it means some part of the tree has been unblocked, so it’s worth rendering again, even before all the promises resolve, to fire the nested requests. |
|
^ Just dropping a note here that the above issues were because of a bad polyfill, and have been 'resolved'. This PR is fine. |
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
* Memoize promise listeners to prevent exponential growth Previously, React would attach a new listener every time a promise is thrown, regardless of whether the same listener was already attached during a previous render. Because React attempts to render every time a promise resolves, the number of listeners grows quickly. This was especially bad in synchronous mode because the renders that happen when the promise pings are not batched together. So if a single promise has multiple listeners for the same root, there will be multiple renders, which in turn results in more listeners being added to the remaining unresolved promises. This results in exponential growth in the number of listeners with respect to the number of IO-bound components in a single render. Fixes facebook#14220 * Memoize on the root and Suspense fiber instead of on the promise * Add TODO to fix persistent mode tests
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
* Memoize promise listeners to prevent exponential growth Previously, React would attach a new listener every time a promise is thrown, regardless of whether the same listener was already attached during a previous render. Because React attempts to render every time a promise resolves, the number of listeners grows quickly. This was especially bad in synchronous mode because the renders that happen when the promise pings are not batched together. So if a single promise has multiple listeners for the same root, there will be multiple renders, which in turn results in more listeners being added to the remaining unresolved promises. This results in exponential growth in the number of listeners with respect to the number of IO-bound components in a single render. Fixes facebook#14220 * Memoize on the root and Suspense fiber instead of on the promise * Add TODO to fix persistent mode tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, React would attach a new listener every time a promise is thrown, regardless of whether the same listener was already attached during a previous render. Because React attempts to render every time a promise resolves, the number of listeners grows quickly.
This was especially bad in synchronous mode because the renders that happen when the promise pings are not batched together. So if a single promise has multiple listeners for the same root, there will be multiple renders, which in turn results in more listeners being added to the remaining unresolved promises. This results in exponential growth in the number of listeners with respect to the number of IO-bound components in a single render.
Fixes #14220