When using React.lazy will cause the GPU/CPU to run overloaded, and the page is very slow. #14220
Comments
|
@janryWang please provide an example reproducing the issue, as requested in the issue template. |
|
hi @aweary ,I updated the image, this picture can clearly see that retrySuspendedRoot is a very time consuming task, it will always jam the browser |
|
It would really help to have an actual code example that triggers this. Can you try to extract one from your codebase? Thanks. |
|
@gaearon I also want to give a demo, but my case is a bit too special.But, I can give you my profile.json |
|
Same to me. The page is very slow, then I decide back to Loadable 😞 |
|
@sydinh This doesn’t help diagnose the issue in any way. What would help is if you shared a project that reproduces it. Otherwise “+1” comments just create notification noise and don’t help us solve the issue. |
|
@gaearon This demo can completely reproduce the whole problem, only need to "npm start" https://github.com/janryWang/react-lazy-demo |
|
I’m on a vacation until Monday but will check then. Or maybe somebody will beat me to it. |
|
Ok, thank you, have a good vacation.😆 |
|
Having a look, thanks for the repo! |
|
just fyi I've been digging into this and can reproduce the slowdowns, working on isolating it to something more specific (removing the included |
|
So I managed to make much smaller test case to reproduce this, with some notes . link - https://codesandbox.io/s/ppjm15v9pq WARNING - Anyway. When We fire a SHIT TON of At this point, I've reached the limit of my understanding of the codebase. I'll dive in some more over the weekend and see if I can figure more out.
|
|
also, here's a zipped profile trace if you can't reproduce it on your machine(s) |
|
@threepointone thank you very much |
janryWang
changed the title
|
It looks like it's a problem specifically with Lazy and synchronous rendering. If you take @threepointone's demo and wrap it with
react/packages/react-reconciler/src/ReactFiberLazyComponent.js Lines 42 to 45 in f777d19 Maybe with synchronous rendering there's a threshold where too many pending lazy elements ping the root frequently enough that it causes starvation issues? |
|
Is there a stable solution to resolve this starvation conflict problem? @aweary |
|
@janryWang a depth of 20 in a binary tree equals to about 1048576 lazy react component. If x is the amount of levels in the binary tree, then there are no more than 2^x -1 nodes in the tree. |
|
Indeed, normal use case, the number of nodes in our application will not be greater than 10w, just tried it no problem, but, why do we have to use the ConcurrentMode component to solve the problem? Can it be handled inside React? |
|
I do think this can be handled inside react. Only as a workaround for now, you could use Concurrent Mode, or not use |
No, I wouldn't recommend it even as a workaround. Concurrent Mode is not ready yet, and nobody should be using it in production unless you're willing into other issues. |
|
@janryWang Yes, we want to fix this. Please give us time. Thanks. |
|
Grateful, waiting for good news |
|
@gaearon Can this issue be marked with the "Bug" label to make sure it doesn't get lost? |
|
I understand that it’s frustrating. We’re coming close to the holiday season and many people are on vacations. So this is moving slower than usually. |
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
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
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 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 #14220 * Memoize on the root and Suspense fiber instead of on the promise * Add TODO to fix persistent mode tests
|
Fix for this is now in master. It'll go out in the next release, within a few days if the testing period doesn't uncover any more issues. |
|
Confirmed it's now fixed by bumping the version in @threepointone's repro: https://codesandbox.io/s/j3rov9692y |
|
Ok, so we can't do another release until we've successfully landed it at Facebook. (We always ship to Facebook first to flush out bugs that we didn't catch in our unit tests.) However, Facebook currently is under a code freeze for the rest of the year, due to the holiday season. If this bug is blocking your work, you can try using our unstable canary build of React: |
|
(Psst I'm going to try to land it anyway, since it's a bugfix, but I can't make any promises.) |
|
Just released in 16.7. Sandbox: https://codesandbox.io/s/j3rov9692y |
|
@acdlite This fix has definitely improved the performance, however starting from a depth of '14' there is still some noticeable frame jank. Is this something we'll just have to live with? |
|
That’s >16000 elements. I think that’s fine, I strongly doubt you’ll reach that in your UI. |
* 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
* 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
Do you want to request a feature or report a bug?
bug
What is the current behavior?
page is very slow.
As we can see from this picture, React has been executing a work loop during the rendering suspend.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
What is the expected behavior?
I expect the page not to be stuck
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React16.6.3
The text was updated successfully, but these errors were encountered: