[Flight][Fizz] tasks that ping in a microtask should render synchronously #28907
+178
−79
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
Today work that pings in a microtask in Flight and Fizz will end up being rendered in a new macrotask. This is potentially sub-optimal for cache-locality reasons but it also has an impact on flushing because flushes always happen synchronously after rendering. It would be ideal if tasks that can ping in a microtask get a chance to render before we flush because we can end up with a more compact wire format for flight and we can potentially avoid showing fallbacks for fizz. This commit doesn't actually implement rendering in microtasks. This will come in a follow up change. What this change does do is refactor the flushing controls to be able to schedule flushing in a macrotask separately from the render phase. The appraoch uses a notion of epochs to allow scheduled work to infer if it is still valid. For instance if Float causes a flush to be enqueued but then the next task is a ping leading to a render we don't necessarily want to flush before additional renders are allowed to complete. Additionally if there is already a flush scheduled we don't want an enqueued flush from Float to lead to an additional flush. the request's flushScheduled property is now more narrowly tailored to represent out-of-band flushes enqueued through float and not the general purpose flushing that is done after every work loop. the request now has an epoch property which can be used to determine if we haven't started a new work loop or flush since the task was previously scheduled. In some environments schedulWork is synchronous. All the logic still makes sense for synchronous work even if it can have unecessary overhead (such as checking if we're in the same epoch since this will necessarily be true in sync mode). However sync mode is mostly useful for legacy renderers like renderToString and we should probably move away from in for the browser build anyway so I don't think we ought to concern ourselves with further optimization of the sync case.
…usly This change modifies the ping mechanism for FlightServer and FizzServer to perform work synchronously if infer the ping is happening in a microtask. The heuristic is to consider whether we're already in a "work" phase which is only cleared in a second macrotask scheduled alongside the work task. If we are and the task is the only task in the queue we assume we're in a microtask and immediately retry the task. There is an edge case however that I suspect can crop up where a ping is interleaved between a work task and the flush task. If this happens it will also appear like a microtask and be retried synchronously. While this isn't the intent of the PR this also isn't breaking any semantics so this should be fine.
facebook-github-bot
added
CLA Signed
React Core Team
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.
stacked on #28900
This change modifies the ping mechanism for FlightServer and FizzServer to perform work synchronously if infer the ping is happening in a microtask. The heuristic is to consider whether we're already in a "work" phase which is only cleared in a second macrotask scheduled alongside the work task. If we are and the task is the only task in the queue we assume we're in a microtask and immediately retry the task.
There is an edge case however that I suspect can crop up where a ping is interleaved between a work task and the flush task. If this happens it will also appear like a microtask and be retried synchronously. While this isn't the intent of the PR this also isn't breaking any semantics so this should be fine.
I also updated the flight tests to use the same environment as FizzServer and stopped mocking setImmediate to run synchronously. This was necessary to actually exercise the timing here but it also better tests the Node implementation of FlightServer because it can't hide task scheduling mistakes behind synchronous execution