[Flight][Fizz] schedule flushing independently from performing work #28900
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
force-pushed
the
refactor-performwork
branch
from
d71af94
to
9f01b18
Compare
| <head> | ||
| <link rel="preconnect" href="foo" /> | ||
| </head> |
There was a problem hiding this comment.
This demonstrates a small semantic difference in this PR. the foo preconnect resolves in a microtask which now runs before the flush. I updated the test to perform an additional preconnect new a new macrotask and it demonstrates that there is no attempted write to the stream after the render has completed.
| export function startWork(request: Request): void { | ||
| request.flushScheduled = request.destination !== null; | ||
| if (supportsRequestStorage) { | ||
| scheduleWork(() => requestStorage.run(request, performWork, request)); | ||
| requestStorage.run(request, startPerformingWork, request); |
There was a problem hiding this comment.
I don’t quite trust this thing here. It assume a lot about the scheduling correctly following the local storage implementation. Which 1) it’s implemented as AsyncLocalStorage at all which might not be the case 2) that the scheduling is implemented using something that follows the storage 3) the scheduling isn’t batched across requests.
There was a problem hiding this comment.
Yeah this makes sense. I also think then I should run the pings inside request storage explicitly too. We're sort of relying on them to be carried in an async context implicitly today but there isn't harm (other than a little perf) in running it in a scope explicitly
Not for this PR but I expect it to be necessary for the follow up which maybe should land together which is to do some microtask pings synchronously rather than scheduling a new work task. We could loosen our definition of a "microtask ping" to be anything that pings after the main work task but before the next flush task but this is quite racey and while it might not have any practical downsides it doesn't model what I'm trying to model If you're wanting to support > 1000 year long running renders I can mod the epoch. There is still a chance of collision of course but now it's a rarity of rarities. but I was satisfied by the maximum allowed running time. At some point you'd also run out of id's though you'd probably go through those a bit slower than task counters 🙃 |
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
force-pushed
the
refactor-performwork
branch
from
9f01b18
to
31c2742
Compare
|
I pushed an update that models this using a schedule state. I think there are still some timing implications once we move to having microtask pings retry synchronously because we won't be able to tell the difference between a ping in a microtask from the current work macrotask vs a ping in a macrotask that is interleaved between the the work macrotask and the flush macrotask. That said this may be somewhat arbitrary. It may be rare and also it just shifts a bit what is flushed but the semantics should stlil hold. The one case that might be problematic is if it is possible for an IO task to ping before the first flush. We may want to favor flushing the first work loop without allowing any IO to ping first. I don't think this can happen in Node.js because I believe setImmediate has the right behavior but I'm not confident about this for edge becuase it isn't clear to me that consecutive setTimeout tasks won't be interleaved. Maybe we can use postMessage or some other scheduling in edge that gives us greater confidence in the ordering |
gnoff
force-pushed
the
refactor-performwork
branch
from
31c2742
to
b25f3d4
Compare
gnoff
force-pushed
the
refactor-performwork
branch
from
b25f3d4
to
1c0a3f2
Compare
|
closing in favor of #29491 |
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.