[Flight][Fizz] schedule work async #29551
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
| @@ -22,7 +22,7 @@ export opaque type Chunk = string; | |||
| export type BinaryChunk = $ArrayBufferView; | |||
|
|
|||
| export function scheduleWork(callback: () => void) { | |||
| callback(); | |||
| setImmediate(callback); | |||
There was a problem hiding this comment.
@Jarred-Sumner not sure why this was originally made to be sync but generally we'd want to schedule work after microtasks have had a chance to run. I believe Bun's support for setImmediate makes this the best option in that we don't want to throttle the task but lmk if there is a better option for Bun that I'm unaware of
There was a problem hiding this comment.
We kind of should add another one here. Basically, you can use setImmediate, but it only runs after all the other tasks are complete - and that's tasks, not microtasks.
What if you did process.nextTick? Microtasks are drained after each one of those, and unlike setImmediate it doesn't have a dependency on the (async) event loop.
There was a problem hiding this comment.
interesting, my understanding is Node schedules setImmediate and setTimeout(f, 0) simiarly but we use setImmediate because it isn't throttled like setTimeout may be. In Bun it seems like setImmediate is actually lower priority than setTimeout(f, 0). We can use that for now however if it is also throttled that's not great either since it puts an upper bound on iterations and it unnecessarily delays work start.
We can't use microtasks here because we are going to add a new scheduling primitive for microtasks and we need to be able to ensure some work is scheduled in a macrotask to ensure the microtask work is flushed first.
Would postMessage be better? that's what we're going to use for the browser
There was a problem hiding this comment.
Microtasks are drained after each process.nextTick runs in both node & bun.
The code is essentially (with some try catch and promise rejection handling and async loca storage)
while (tick = next()) {
tick();
drainMicrotasks()
}I think it’s likely you want nextTick in bun and node, or to override nextTick to make yours have higher priority
There was a problem hiding this comment.
Well actually we want it to be a lower priority. If two fetches are resolving we'd prefer if both resolved before we restarted work otherwise we might stall on the second one. So we want a low priority task, we just don't want it to be artificially delayed. I maybe misinterpreted your earlier comment but my understanding is that in Node setImmediate will usually run before setTimeout(..., 0). In Bun is this true too? I wouldn't want it to run later than setTimeout(,0) but I also don't want it to run before IO tasks have had a chance to run which should be prioritized. The concern with setTimeout is that if there are many they may be throttled in some environments
| @@ -43,7 +43,7 @@ export interface Destination { | |||
| } | |||
|
|
|||
| export function scheduleWork(callback: () => void) { | |||
| callback(); | |||
| setTimeout(callback, 0); | |||
There was a problem hiding this comment.
@alunyov I don't know enough about internal FB infra to know whether setImmediate is available all the time or ever. setTimeout is probably not the best choice here but I don't know if we should feature detect setImmediate, use postMessage, or just use setTimeout. Lmk what makes sense for Meta
There was a problem hiding this comment.
Currently there is none I believe. There’s no async I/O that can be scheduled from JS. Only called into.
So I believe resolving a Promise is the only available for now.
| export const scheduleWork: (callback: () => void) => void = | ||
| LocalMessageChannel !== undefined | ||
| ? (callback: () => void) => { | ||
| const channel = new LocalMessageChannel(); |
There was a problem hiding this comment.
MessageChannel support goes back quite far. I wonder if we should just assume it
There was a problem hiding this comment.
Yea probably fine to assume and maybe encourages not abusing this build for other environments. Although we currently abuse it by using it in Deno. https://github.com/facebook/react/blob/main/packages/react-dom/package.json#L64
You don't need to create a new MessageChannel each time. Can use the same one and just post a new message to it like we do in the scheduler.
There was a problem hiding this comment.
the only safe way I think we can do that is to use a queue
gnoff
force-pushed
the
schedule-work-async
branch
from
ec6124b
to
7745cd0
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
7745cd0
to
4386e0e
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
4386e0e
to
a9916e3
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
a9916e3
to
d7bff09
Compare
| global.requestIdleCallback = function (callback) { | ||
| return setTimeout(() => { | ||
| callback({ | ||
| timeRemaining() { | ||
| return Infinity; | ||
| }, | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| global.cancelIdleCallback = function (callbackID) { | ||
| clearTimeout(callbackID); | ||
| }; |
There was a problem hiding this comment.
These haven't been used in a very long time
| scheduleWork(() => { | ||
| request.flushScheduled = false; | ||
| const destination = request.destination; | ||
| if (destination) { | ||
| flushCompletedChunks(request, destination); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This work uncovered a bug where we didn't properly check whether the destination still existed before attempting to flush.
gnoff
force-pushed
the
schedule-work-async
branch
from
d7bff09
to
8b1d407
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
8b1d407
to
98838a2
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
98838a2
to
3ada4db
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
3ada4db
to
53284cd
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
53284cd
to
d5d9149
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
d5d9149
to
54bbd70
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
54bbd70
to
e632056
Compare
gnoff
force-pushed
the
schedule-work-async
branch
from
e632056
to
736d927
Compare
While most builds of Flight and Fizz schedule work in new tasks some do execute work synchronously. While this is necessary for legacy APIs like renderToString for modern APIs there really isn't a great reason to do this synchronously. This change updates all non-legacy uses to be async using the best availalble scheduler at runtime
gnoff
force-pushed
the
schedule-work-async
branch
from
736d927
to
1f062b4
Compare
While most builds of Flight and Fizz schedule work in new tasks some do execute work synchronously. While this is necessary for legacy APIs like renderToString for modern APIs there really isn't a great reason to do this synchronously. We could schedule works as microtasks but we actually want to yield so the runtime can run events and other things that will unblock additional work before starting the next work loop. This change updates all non-legacy uses to be async using the best availalble macrotask scheduler. Browser now uses postMessage Bun uses setTimeout because while it also supports setImmediate the scheduling is not as eager as the same API in node the FB build also uses setTimeout This change required a number of changes to tests which were utilizing the sync nature of work in the Browser builds to avoid having to manage timers and tasks. I added a patch to install MessageChannel which is required by the browser builds and made this patched version integrate with the Scheduler mock. This way we can effectively use `act` to flush flight and fizz work similar to how we do this on the client. DiffTrain build for commit b526a0a.
Stacked on #29551 Flight pings much more often than Fizz because async function components will always take at least a microtask to resolve . Rather than scheduling this work as a new macrotask Flight now schedules pings in a microtask. This allows more microtasks to ping before actually doing a work flush but doesn't force the vm to spin up a new task which is quite common give n the nature of Server Components
Stacked on #29551 Flight pings much more often than Fizz because async function components will always take at least a microtask to resolve . Rather than scheduling this work as a new macrotask Flight now schedules pings in a microtask. This allows more microtasks to ping before actually doing a work flush but doesn't force the vm to spin up a new task which is quite common give n the nature of Server Components DiffTrain build for commit 1e1e5cd.
While most builds of Flight and Fizz schedule work in new tasks some do execute work synchronously. While this is necessary for legacy APIs like renderToString for modern APIs there really isn't a great reason to do this synchronously.
We could schedule works as microtasks but we actually want to yield so the runtime can run events and other things that will unblock additional work before starting the next work loop.
This change updates all non-legacy uses to be async using the best availalble macrotask scheduler.
Browser now uses postMessage
Bun uses setTimeout because while it also supports setImmediate the scheduling is not as eager as the same API in node
the FB build also uses setTimeout
This change required a number of changes to tests which were utilizing the sync nature of work in the Browser builds to avoid having to manage timers and tasks. I added a patch to install MessageChannel which is required by the browser builds and made this patched version integrate with the Scheduler mock. This way we can effectively use
actto flush flight and fizz work similar to how we do this on the client.