[Flight] Pack deferred children into continuation rows#36053
[Flight] Pack deferred children into continuation rows#36053mhart wants to merge 3 commits intofacebook:mainfrom
Conversation
Resolve large rendered children arrays into continuation tasks instead of deferring each remaining child one-by-one, while preserving toJSON warnings and deduped references across packed rows. Fixes facebook#35125.
gnoff
left a comment
gnoff
left a comment
There was a problem hiding this comment.
Thanks for submitting. It's an interesting approach. I left a few comments about isolating the serialization changes to see what kind of impact those have on performance by themselves and also a question about the observability of the optimization in userspace.
| if (!isChildrenArray) { | ||
| continue; | ||
| } | ||
| const remaining = sourceArray.slice(i + 1); |
There was a problem hiding this comment.
for very long arrays this will do quite a bit of memory work and much of it potentially repeated since the entire tail is continued into each new task. If we're already making affordances for this notion we might as well support rendering from an index to avoid memory churn
There was a problem hiding this comment.
Have just pushed up a commit that does this approach
I didn't change the createTask signature for the extra properties (as this is the only place they're used), but I can do it that way if prefered.
Also let me know if you'd prefer a separate branch / PR for this instead
| ); | ||
| pingTask(request, continuationTask); | ||
|
|
||
| resolvedArray[i + 1] = serializeLazyID(continuationTask.id); |
There was a problem hiding this comment.
While in React rendering position this is probably semantically equivalent I don't think it works for something like
function ServerComp () {
const items = new Array(500).fill().map((_, i) => <div key={i}>Item {i}</div>);
return <ClientComp items={items} />
}
The ClientComp component will be expecting an array of items but it might receive an array of items OR arrays of items and so forth. Essentially it leaks the row optimization into observable types and would not be caught by implicit typing provided by typescript.
To do this kind of thing we'd either need a special reference type that reconstituted the array on the client or we'd need to know for certain the array was only in render position.
There was a problem hiding this comment.
This is already addressed right? We only enable array packing when the array is in root or children position.
These tests check this:
should not treat plain prop arrays as renderable childrendoes not pack nested child arrays reused by sibling propsdoes not truncate a packed children array reused by another prop
Let me know if I'm missing a test there
There was a problem hiding this comment.
Added a test to specifically cover this case (does not leak packed child continuations into client component array props): 883a1b3
| toJSON: function ( | ||
| this: | ||
| | {+[key: string | number]: ReactClientValue} | ||
| | $ReadOnlyArray<ReactClientValue>, |
There was a problem hiding this comment.
I understand this change is necessary for your approach. it's likely to have a performance impact for cases outside of Arrays too, possibly beneficial, though possibly not given there may be extra object allocation in the new approach. I'd prefer to see the impact of this as a standalone change given a large part of the premise for making the Array change is to improve performance.
There was a problem hiding this comment.
100% agree, I wouldn't want this to impact performance elsewhere. Do you have benchmarks you use for this kind of thing? The benchmarks in the repo don't seem to test RSC/Flight. I have my own benchmarks, but would be good to have some official ones.
Resume packed children continuations from the original rendered array instead of slicing a new tail for each row. This keeps parent-relative references correct while reducing extra row splitting work.
Also align continuation index coercion with numeric string conversion
3 participants
Summary
This changes Flight's large-array splitting behavior so rendered children arrays can be packed into continuation rows instead of deferring every remaining child one-by-one once
MAX_ROW_SIZEis exceeded.In pages with many children (eg, paragraphs, tables) it leads to a 1.45x performance improvement in req/sec on RSC serialization/deserialization in local benchmarks as well as deployed canary Next.js code on Vercel (160ms -> 110ms P95). This is due to the per-row overhead in RSC and the reduced number of rows (3674 rows -> 240 rows).
Before this change, if a rendered children array crossed the row-size threshold, Flight would often emit one lazy row per remaining child. That creates many tiny rows and adds a lot of per-row overhead. With this change,
MAX_ROW_SIZEis still respected, but Flight can hand the remaining tail of the children array to a continuation task, so later rows can still pack multiple children together when they fit.To support this we move the
toJSONhandling out of theJSON.stringifyreplacer path and into an explicit recursive resolution step that uses v8's optimized single-argJSON.stringifycall. This allows us to control resolution, including splitting large rendered children arrays before stringifying.Fixes #35125.
Why this is safe
This is intended to be a serialization strategy change, not a semantic change.
$L...).In other words, this should only reduce unnecessary row fragmentation for large children lists while preserving the same decoded model.
How did you test this change?
Added coverage for:
toJSONvalues.Testing
volta run yarn test --silent --no-watchman ReactFlightDOMEdge-testvolta run yarn test --silent --no-watchman ReactFlight-test