Refactor of interleaved ("concurrent") update queue #24663
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
interleaved-refactor
branch
from
c4748dc to
eebe492
Compare
|
Comparing: 4ddd8b4...f5b0535 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
acdlite
force-pushed
the
interleaved-refactor
branch
4 times, most recently
from
935574b to
6b0b8f1
Compare
acdlite
force-pushed
the
interleaved-refactor
branch
3 times, most recently
from
671dd1e to
0c92bdd
Compare
rickhanlonii
approved these changes
Jun 3, 2022
| if (root !== null) { | ||
| scheduleUpdateOnFiber(root, fiber, lane, eventTime); |
There was a problem hiding this comment.
Hoisting this up inside of this check will skip checkForNestedUpdates and the insertion effect in render warning. What cases would make it possible to skip those now due to root being null?
There was a problem hiding this comment.
It would only happen when the component is unmounted. Those checks should probably move to enqueueUpdate, though, since that's where similar warnings are.
| @@ -242,7 +242,7 @@ export function enqueueUpdate<State>( | |||
| update.next = update; | |||
| // At the end of the current render, this queue's interleaved updates will | |||
| // be transferred to the pending queue. | |||
| pushInterleavedQueue(sharedQueue); | |||
| pushConcurrentUpdateQueue(sharedQueue); | |||
There was a problem hiding this comment.
Just for my context, what's the thinking behind the name "concurrent queue" (since concurrent is overloaded I want to be sure I understand what exactly this means).
There was a problem hiding this comment.
Means "multithreaded" basically. Agree it's a little confusing that we sometimes use "concurrent" to mean "not synchronous".
| @@ -53,3 +66,119 @@ export function finishQueueingConcurrentUpdates() { | |||
| concurrentQueues = null; | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nice, I like these functions much more 👍
| @@ -426,7 +425,7 @@ export function attemptSynchronousHydration(fiber: Fiber): void { | |||
| } | |||
| case SuspenseComponent: { | |||
| flushSync(() => { | |||
| const root = markUpdateLaneFromFiberToRoot(fiber, SyncLane); | |||
| const root = enqueueConcurrentRenderForLane(fiber, SyncLane); | |||
There was a problem hiding this comment.
I see this replacement in a few places, but enqueueConcurrentRenderForLane does more than just markUpdateLaneFromFiberToRoot, what's the thinking there?
There was a problem hiding this comment.
They do the same thing except enqueueConcurrentRenderForLane waits to mark the update lane until after the current render phase has finished
| pending: ConcurrentUpdate | null, | ||
| }; | ||
|
|
||
| // If a render is in progress, and we receive an update from a concurrent event, |
There was a problem hiding this comment.
What is a "concurrent event"? Like an event that happens while yielding (e.g. a timeout)?
| } else { | ||
| update.next = interleaved.next; | ||
| interleaved.next = update; | ||
| // Don't update the `childLanes` on the return path yet. If we already in |
Was thinking I'd use the |
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
A lot of the logic around scheduling an update needs access to the fiber root. To obtain this reference, we must walk up the fiber return path. We also do this to update `childLanes` on all the parent nodes, so we can use the same traversal for both purposes. The traversal currently happens inside `scheduleUpdateOnFiber`, but sometimes we need to access it beyond that function, too. So I've hoisted the traversal out of `scheduleUpdateOnFiber` into its own function call that happens at the beginning of the `setState` algorithm.
The scope of this module is expanding so I've renamed accordingly. No behavioral changes.
During a setState, the childLanes are updated immediately, even if a render is already in progress. This can lead to subtle concurrency bugs, so the plan is to wait until the in-progress render has finished before updating the childLanes, to prevent subtle concurrency bugs. As a step toward that change, when scheduling an update, we should not update the childLanes directly, but instead defer to the ReactConcurrentUpdates module to do it at the appropriate time. This makes markUpdateLaneFromFiberToRoot a private function that is only called from the ReactConcurrentUpdates module.
(This is the riskiest commit in the stack. Only affects the "new" reconciler fork.) Updates that occur in a concurrent event while a render is already in progress can't be processed during that render. This is tricky to get right. Previously we solved this by adding concurrent updates to a special `interleaved` queue, then transferring the `interleaved` queue to the `pending` queue after the render phase had completed. However, we would still mutate the `childLanes` along the parent path immediately, which can lead to its own subtle data races. Instead, we can queue the entire operation until after the render phase has completed. This replaces the need for an `interleaved` field on every fiber/hook queue. The main motivation for this change, aside from simplifying the logic a bit, is so we can read information about the current fiber while we're walking up its return path, like whether it's inside a hidden tree. (I haven't done anything like that in this commit, though.)
acdlite
force-pushed
the
interleaved-refactor
branch
from
0c92bdd to
bd04069
Compare
acdlite
force-pushed
the
interleaved-refactor
branch
from
690864d to
f5b0535
Compare
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: facebook#18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in facebook#24663. Because this bugfix depends on a moderately risky refactor to the update queue (facebook#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: facebook#18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in facebook#24663. Because this bugfix depends on a moderately risky refactor to the update queue (facebook#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: facebook#18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in facebook#24663. Because this bugfix depends on a moderately risky refactor to the update queue (facebook#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 7, 2022
This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: facebook#18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in facebook#24663. Because this bugfix depends on a moderately risky refactor to the update queue (facebook#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork.
acdlite
added a commit
that referenced
this pull request
Jun 8, 2022
* Add `isHidden` to OffscreenInstance We need to be able to read whether an offscreen tree is hidden from an imperative event. We can store this on its OffscreenInstance. We were already scheduling a commit effect whenever the visibility changes, in order to toggle the inner effects. So we can reuse that. * [FORKED] Bugfix: Revealing a hidden update This fixes a bug I discovered related to revealing a hidden Offscreen tree. When this happens, we include in that render all the updates that had previously been deferred — that is, all the updates that would have already committed if the tree weren't hidden. This is necessary to avoid tearing with the surrounding contents. (This was the "flickering" Suspense bug we found a few years ago: #18411.) The way we do this is by tracking the lanes of the updates that were deferred by a hidden tree. These are the "base" lanes. Then, in order to reveal the hidden tree, we process any update that matches one of those base lanes. The bug I discovered is that some of these base lanes may include updates that were not present at the time the tree was hidden. We cannot flush those updates earlier that the surrounding contents — that, too, could cause tearing. The crux of the problem is that we sometimes reuse the same lane for base updates and for non-base updates. So the lane alone isn't sufficient to distinguish between these cases. We must track this in some other way. The solution I landed upon was to add an extra OffscreenLane bit to any update that is made to a hidden tree. Then later when we reveal the tree, we'll know not to treat them as base updates. The extra OffscreenLane bit is removed as soon as that lane is committed by the root (markRootFinished) — at that point, it gets "upgraded" to a base update. The trickiest part of this algorithm is reliably detecting when an update is made to a hidden tree. What makes this challenging is when the update is received during a concurrent event, while a render is already in progress — it's possible the work-in-progress render is about to flip the visibility of the tree that's being updated, leading to a race condition. To avoid a race condition, we will wait to read the visibility of the tree until the current render has finished. In other words, this makes it an atomic operation. Most of this logic was already implemented in #24663. Because this bugfix depends on a moderately risky refactor to the update queue (#24663), it only works in the "new" reconciler fork. We will roll it out gradually to www before landing in the main fork. * Add previous commit to list of forked revisions
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jun 15, 2022
Summary: This sync includes the following changes: - **[5cc2487e0](facebook/react@5cc2487e0 )**: bump versions for next release ([#24725](facebook/react#24725)) //<Josh Story>// - **[54f17e490](facebook/react@54f17e490 )**: [Transition Tracing] Fix Cache and Transitions Pop Order ([#24719](facebook/react#24719)) //<Luna Ruan>// - **[7cf8dfd94](facebook/react@7cf8dfd94 )**: [Transition Tracing] Create/Process Marker Complete Callback ([#24700](facebook/react#24700)) //<Luna Ruan>// - **[327e4a1f9](facebook/react@327e4a1f9 )**: [Follow-up] Land enableClientRenderFallbackOnTextMismatch //<Andrew Clark>// - **[a8c9cb18b](facebook/react@a8c9cb18b )**: Land enableSuspenseLayoutEffectSemantics flag ([#24713](facebook/react#24713)) //<Andrew Clark>// - **[a8555c308](facebook/react@a8555c308 )**: [Transition Tracing] Add Tracing Marker Stack ([#24661](facebook/react#24661)) //<Luna Ruan>// - **[8186b1937](facebook/react@8186b1937 )**: Check for infinite update loops even if unmounted ([#24697](facebook/react#24697)) //<Andrew Clark>// - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...256aefb jest_e2e[run_all_tests] Reviewed By: cortinico Differential Revision: D37155957 fbshipit-source-id: 4c0afc95abe8fa13c3803584922c8dc0059ff562
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Aug 8, 2022
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
Summary: Sync goes to v18.2 release. I had to manually trigger CircleCI builds because TTL for build artefacts is 30 days. This sync includes the following changes: - **[060505e9d](facebook/react@060505e9d )**: Fix misapplying prod error opt-out ([facebook#24688](facebook/react#24688)) //<Josh Story>// - **[47944142f](facebook/react@47944142f )**: `now` isn't part of the react-reconciler config anymore ([facebook#24689](facebook/react#24689)) //<Mathieu Dutour>// - **[b34552352](facebook/react@b34552352 )**: [Fizz] Support abort reasons ([facebook#24680](facebook/react#24680)) //<Josh Story>// - **[79f54c16d](facebook/react@79f54c16d )**: Bugfix: Revealing a hidden update ([facebook#24685](facebook/react#24685)) //<Andrew Clark>// - **[7e8a020a4](facebook/react@7e8a020a4 )**: Remove extra Server Context argument ([facebook#24683](facebook/react#24683)) //<Sebastian Markbåge>// - **[4f29ba1cc](facebook/react@4f29ba1cc )**: support errorInfo in onRecoverableError ([facebook#24591](facebook/react#24591)) //<Josh Story>// - **[1cd90d2cc](facebook/react@1cd90d2cc )**: Refactor of interleaved ("concurrent") update queue ([facebook#24663](facebook/react#24663)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions d300ceb...9e3b772 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D38496392 fbshipit-source-id: 3ecffc2b3354104562eb23a2643fe0a37a01a7e6
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.
Commits are atomic and designed to be reviewed individually. Will probably end up landing in multiple steps, though, to make it easier to bisect later.
Updates that occur in a concurrent event while a render is already in progress can't be processed during that render. This is tricky to get right. Previously we solved this by adding concurrent updates to a special
interleavedqueue, then transferring theinterleavedqueue to thependingqueue after the render phase had completed.However, we would still mutate the
childLanesalong the parent path immediately, which can lead to its own subtle data races.Instead, we can queue the entire operation until after the render phase has completed. This replaces the need for an
interleavedfield on every fiber/hook queue.The main motivation for this change, aside from simplifying the logic a bit, is so we can read information about the current fiber while we're walking up its return path, like whether it's inside a hidden tree. (I haven't done anything like that in this PR, though.)