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
@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
https://gist.github.com/janryWang/cab49587a69e53a65e333e3ea7c0c718
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
@gaearon @aweary Is there any problem that has been found?
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 next
component library etc).
So I managed to make much smaller test case to reproduce this, with some notes . link - https://codesandbox.io/s/ppjm15v9pq WARNING - depth=7
/isLazy=true
WILL LOCK YOUR BROWSER. have a look at the code and comments (and possibly run it) to see the behaviour.
Anyway.
When isLazy=false
, the tree should render just fine. I was able to render depth=13
without _much_ trouble. But when isLazy=true
, it renders fine till depth=5
. when depth=6
, it hits a some kind of cliff and locks the CPU/GPU up for MANY seconds. with depth=7
, chrome has to be force closed.
We fire a SHIT TON of retrySuspendedRoot
calls after the initial burst of js eval. Even though the calls take <0.5μs, react appears to yield back to the thread for 10μs, and then call it again.
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)
Profile-20181116T162852.json.zip
@threepointone thank you very much
@acdlite @gaearon @aweary Is there any problem that has been found?
It looks like it's a problem specifically with Lazy and synchronous rendering. If you take @threepointone's demo and wrap it with React.unstable_ConcurrentMode
it works fine with even taller trees.
retrySuspenseRoot
gets called over and over again as long as the Promise read from the lazy element hasn't been called yet.
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
When I change depth=20 , chrome has to be force closed.
@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.
>1 milion lazy loaded react components seems to be an extreme use case.
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 .lazy
so extensively. We'll definitely fix this soon.
Only as a workaround for now, you could use Concurrent Mode
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?
This issue is preventing us from using react lazy in production due to major browser locking.
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.
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: react@canary
or [email protected]
. I can't guarantee this build won't have other bugs. If you do run into issues, please let us know. Sorry for the delay! We'll get this released as soon as we can when regular work resumes in January.
(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.
Most helpful comment
Just released in 16.7. Sandbox: https://codesandbox.io/s/j3rov9692y