Check if suspensey instance resolves in immediate task #26427
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
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
suspensey-commits-immediate-load
branch
from
2e42765
to
7f8169b
Compare
acdlite
force-pushed
the
suspensey-commits-immediate-load
branch
from
7f8169b
to
0cfbe6d
Compare
acdlite
force-pushed
the
suspensey-commits-immediate-load
branch
from
0cfbe6d
to
9cc698b
Compare
When rendering a suspensey resource that we haven't seen before, it may have loaded in the background while we were rendering. We should yield to the main thread to see if the load event fires in an immediate task. For example, if the resource for a link element has already loaded, its load event will fire in a task right after React yields to the main thread. Because the continuation task is not scheduled until right before React yields, the load event will ping React before it resumes. If this happens, we can resume rendering without showing a fallback. I don't think this matters much for images, because the `completed` property tells us whether the image has loaded, and we currently never block the main thread for more than 5ms at a time (for now — we might increase this in the future). It matters more for stylesheets because the only way to check if it has loaded is by listening for the load event. This is essentially the same trick that `use` does for userspace promises, but a bit simpler because we don't need to replay the host component's begin phase; the work-in-progress fiber already completed, so we can just continue onto the next sibling without any additional work. As part of this change, I split the `shouldSuspendCommit` host config method into separate `maySuspendCommit` and `preloadInstance` methods. Previously `shouldSuspendCommit` was used for both. This raised a question of whether we should preload resources during a synchronous render. My initial instinct was that we shouldn't, because we're going to synchronously block the main thread until the resource is inserted into the DOM, anyway. But I wonder if the browser is able to initiate the preload even while the main thread is blocked. It's probably a micro-optimization either way because most resources will be loaded during transitions, not urgent renders.
acdlite
force-pushed
the
suspensey-commits-immediate-load
branch
from
9cc698b
to
3fcd918
Compare
gnoff
approved these changes
Mar 20, 2023
There was a problem hiding this comment.
The one thing I'm unclear on is what causes the immediate resumption of the render? In the use case I thought that the thenable was set up to ping but there is nothing like that for the host version here. Maybe I misunderstood and the use case always immediately continues in another task but if the fullfilled value isn't present that communicates that the thenable did not resolve and a fresh stack is needed? Since this isn't integral to the changes you made and they make sense I'll approve
|
It's basically the same as how you're describing except instead of checking react/packages/react-reconciler/src/ReactFiberWorkLoop.js Lines 2364 to 2386 in 3fcd918 |
github-actions bot
pushed a commit
that referenced
this pull request
Mar 20, 2023
When rendering a suspensey resource that we haven't seen before, it may have loaded in the background while we were rendering. We should yield to the main thread to see if the load event fires in an immediate task. For example, if the resource for a link element has already loaded, its load event will fire in a task right after React yields to the main thread. Because the continuation task is not scheduled until right before React yields, the load event will ping React before it resumes. If this happens, we can resume rendering without showing a fallback. I don't think this matters much for images, because the `completed` property tells us whether the image has loaded, and during a non-urgent render, we never block the main thread for more than 5ms at a time (for now — we might increase this in the future). It matters more for stylesheets because the only way to check if it has loaded is by listening for the load event. This is essentially the same trick that `use` does for userspace promises, but a bit simpler because we don't need to replay the host component's begin phase; the work-in-progress fiber already completed, so we can just continue onto the next sibling without any additional work. As part of this change, I split the `shouldSuspendCommit` host config method into separate `maySuspendCommit` and `preloadInstance` methods. Previously `shouldSuspendCommit` was used for both. This raised a question of whether we should preload resources during a synchronous render. My initial instinct was that we shouldn't, because we're going to synchronously block the main thread until the resource is inserted into the DOM, anyway. But I wonder if the browser is able to initiate the preload even while the main thread is blocked. It's probably a micro-optimization either way because most resources will be loaded during transitions, not urgent renders. DiffTrain build for [0131d0c](0131d0c)
kassens
added a commit
to kassens/react
that referenced
this pull request
Mar 20, 2023
…ook#26427)" This reverts commit 0131d0c.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Mar 29, 2023
Summary: This sync includes the following changes: - **[77ba1618a](facebook/react@77ba1618a )**: Bugfix: Remove extra render pass when reverting to client render ([#26445](facebook/react#26445)) //<Andrew Clark>// - **[520f7f3ed](facebook/react@520f7f3ed )**: Refactor ReactDOMComponent to use flatter property operations ([#26433](facebook/react#26433)) //<Sebastian Markbåge>// - **[0131d0cff](facebook/react@0131d0cff )**: Check if suspensey instance resolves in immediate task ([#26427](facebook/react#26427)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 3554c88...77ba161 jest_e2e[run_all_tests] Reviewed By: poteto Differential Revision: D44476026 fbshipit-source-id: c6935d760a068672b714722dee1fd24839c08c4b
jeongshin
pushed a commit
to jeongshin/react-native
that referenced
this pull request
May 7, 2023
Summary: This sync includes the following changes: - **[77ba1618a](facebook/react@77ba1618a )**: Bugfix: Remove extra render pass when reverting to client render ([facebook#26445](facebook/react#26445)) //<Andrew Clark>// - **[520f7f3ed](facebook/react@520f7f3ed )**: Refactor ReactDOMComponent to use flatter property operations ([facebook#26433](facebook/react#26433)) //<Sebastian Markbåge>// - **[0131d0cff](facebook/react@0131d0cff )**: Check if suspensey instance resolves in immediate task ([facebook#26427](facebook/react#26427)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 3554c88...77ba161 jest_e2e[run_all_tests] Reviewed By: poteto Differential Revision: D44476026 fbshipit-source-id: c6935d760a068672b714722dee1fd24839c08c4b
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: This sync includes the following changes: - **[77ba1618a](facebook/react@77ba1618a )**: Bugfix: Remove extra render pass when reverting to client render ([facebook#26445](facebook/react#26445)) //<Andrew Clark>// - **[520f7f3ed](facebook/react@520f7f3ed )**: Refactor ReactDOMComponent to use flatter property operations ([facebook#26433](facebook/react#26433)) //<Sebastian Markbåge>// - **[0131d0cff](facebook/react@0131d0cff )**: Check if suspensey instance resolves in immediate task ([facebook#26427](facebook/react#26427)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 3554c88...77ba161 jest_e2e[run_all_tests] Reviewed By: poteto Differential Revision: D44476026 fbshipit-source-id: c6935d760a068672b714722dee1fd24839c08c4b
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
When rendering a suspensey resource that we haven't seen before, it may have loaded in the background while we were rendering. We should yield to the main thread to see if the load event fires in an immediate task. For example, if the resource for a link element has already loaded, its load event will fire in a task right after React yields to the main thread. Because the continuation task is not scheduled until right before React yields, the load event will ping React before it resumes. If this happens, we can resume rendering without showing a fallback. I don't think this matters much for images, because the `completed` property tells us whether the image has loaded, and during a non-urgent render, we never block the main thread for more than 5ms at a time (for now — we might increase this in the future). It matters more for stylesheets because the only way to check if it has loaded is by listening for the load event. This is essentially the same trick that `use` does for userspace promises, but a bit simpler because we don't need to replay the host component's begin phase; the work-in-progress fiber already completed, so we can just continue onto the next sibling without any additional work. As part of this change, I split the `shouldSuspendCommit` host config method into separate `maySuspendCommit` and `preloadInstance` methods. Previously `shouldSuspendCommit` was used for both. This raised a question of whether we should preload resources during a synchronous render. My initial instinct was that we shouldn't, because we're going to synchronously block the main thread until the resource is inserted into the DOM, anyway. But I wonder if the browser is able to initiate the preload even while the main thread is blocked. It's probably a micro-optimization either way because most resources will be loaded during transitions, not urgent renders. DiffTrain build for commit 0131d0c.
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.
When rendering a suspensey resource that we haven't seen before, it may have loaded in the background while we were rendering. We should yield to the main thread to see if the load event fires in an immediate task.
For example, if the resource for a link element has already loaded, its load event will fire in a task right after React yields to the main thread. Because the continuation task is not scheduled until right before React yields, the load event will ping React before it resumes.
If this happens, we can resume rendering without showing a fallback.
I don't think this matters much for images, because the
completedproperty tells us whether the image has loaded, and during a non-urgent render, we never block the main thread for more than 5ms at a time (for now — we might increase this in the future). It matters more for stylesheets because the only way to check if it has loaded is by listening for the load event.This is essentially the same trick that
usedoes for userspace promises, but a bit simpler because we don't need to replay the host component's begin phase; the work-in-progress fiber already completed, so we can just continue onto the next sibling without any additional work.As part of this change, I split the
shouldSuspendCommithost config method into separatemaySuspendCommitandpreloadInstancemethods. PreviouslyshouldSuspendCommitwas used for both.This raised a question of whether we should preload resources during a synchronous render. My initial instinct was that we shouldn't, because we're going to synchronously block the main thread until the resource is inserted into the DOM, anyway. But I wonder if the browser is able to initiate the preload even while the main thread is blocked. It's probably a micro-optimization either way because most resources will be loaded during transitions, not urgent renders.