SuspenseList: Reschedule continuation at same priority #18738
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 346242e:
|
acdlite
force-pushed
the
suspenselist-reschedule
branch
from
f396149 to
976da2f
Compare
There was a problem hiding this comment.
I checked the commit (d77d125) and it didn't really fail without this even before. Probably more defensively.
Conceptually this is the same as retrying upon commit. The key is that we shouldn't retry before committing this level. I.e. we shouldn't be confused about having remaining work and starting that, even if something suspends.
We have a pretty rigid heuristic to not interrupt an in progress render now. This new lanes stuff makes that more important to maintain as a constraint.
So this is probably fine.
If it's not fine, I believe it will be the same as getting an update at the priority that's already rendering. That needs to go into its own lane. You still have to solve that so that will be an alternative solution.
SuspenseList progressively renders items even if the list is CPU bound, i.e. it isn't waiting for missing data. It does this by showing a fallback for the remaining items, committing the items in that have already finished, then starting a new render to continue working on the rest. When it schedules that subsequent render, it uses a slightly lower priority than the current render: `renderExpirationTime - 1`. This commit changes it to reschedule at `renderExpirationTime` instead. I don't know what the original motivation was for bumping the expiration time slightly lower. The comment says that the priorities of the two renders are the same (which makes sense to me) so I imagine it was motivated by some implementation detail. I don't think it's necessary anymore, though perhaps it was when it was originally written. If it is still necessary, we should write a test case that illustrates why.
acdlite
force-pushed
the
suspenselist-reschedule
branch
from
976da2f to
346242e
Compare
SuspenseList progressively renders items even if the list is CPU bound, i.e. it isn't waiting for missing data. It does this by showing a fallback for the remaining items, committing the items in that have already finished, then starting a new render to continue working on the rest.
When it schedules that subsequent render, it uses a slightly lower priority than the current render:
renderExpirationTime - 1.This commit changes it to reschedule at
renderExpirationTimeinstead.I don't know what the original motivation was for bumping the expiration time slightly lower. The comment says that the priorities of the two renders are the same (which makes sense to me) so I imagine it was motivated by some implementation detail. I don't think it's necessary anymore, though perhaps it was when it was originally written. If it is still necessary, we should write a test case that illustrates why.