Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flight][Fizz] tasks that ping in a microtask should render synchronously #28907

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Commits on Apr 24, 2024

  1. [Flight][Fizz] schedule flushing independently from performing work

    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.
    gnoff committed Apr 24, 2024
    Configuration menu
    Copy the full SHA
    a855a5a View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    1c0a3f2 View commit details
    Browse the repository at this point in the history

Commits on Apr 25, 2024

  1. [Flight][Fizz] tasks that ping in a microtask should render synchrono…

    …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.
    gnoff committed Apr 25, 2024
    Configuration menu
    Copy the full SHA
    6c0f74a View commit details
    Browse the repository at this point in the history