[Flight] Walk parsed JSON instead of using reviver for parsing RSC payload#35776
[Flight] Walk parsed JSON instead of using reviver for parsing RSC payload#35776timneutkens wants to merge 5 commits intofacebook:mainfrom
Conversation
| if (typeof value === 'string') { | ||
| // We can't use .bind here because we need the "this" value. | ||
| return parseModelString(response, this, key, value); | ||
| if (value[0] === '$') { |
There was a problem hiding this comment.
I don't think this check is an optimization tbh. it's checked in parseModelString and will just return the value there.
There was a problem hiding this comment.
The check is duplicated, but the point is avoiding the function call. parseModelString is a large function with many switch cases, so neither GCC nor V8 will inline it. Without the guard, every non-$ string (class names, text content, attribute values, etc.) pays for argument setup, stack frame push/pop, the same value[0] === '$' check inside, and return. With the guard, those strings never enter the function. I haven't measured the impact though, so it would be good to verify with your benchmarks.
| return value; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| if (value[0] === REACT_ELEMENT_TYPE) { |
There was a problem hiding this comment.
Check here is needed because the other case is that it's an object that needs to traverse deeper.
| // Pass a wrapper object as parentObject to match the original JSON.parse | ||
| // reviver behavior, where the root value's reviver receives {"": rootValue} | ||
| // as `this`. This ensures parentObject is never null when accessed downstream. | ||
| return response._walkJSON(parsed, {'': parsed}, ''); |
There was a problem hiding this comment.
Not super happy about this extra object being created, it matches the JSON.parse behavior though.
|
|
||
| // Don't inline this call because it causes closure to outline the call above. | ||
| this._fromJSON = createFromJSONCallback(this); | ||
| this._walkJSON = createWalkParsedJSON(this); |
There was a problem hiding this comment.
The old code stored a callback on the instance because JSON.parse's reviver has a fixed (key, value) signature. There was no way to pass response as a parameter, and .bind() wasn't an option because the reviver relies on this being set to the parent object by the engine.
With the walker, we control the function signature, so response can just be a normal parameter. This could be a plain module-scope function. No factory, no instance field, no type change needed.
There was a problem hiding this comment.
Got it! The initial version did exactly what you're suggesting. Only changed it to using this approach to match the existing code a little closer. Will update it! Nice
| } | ||
| if (value[0] === REACT_ELEMENT_TYPE) { | ||
| // React element tuple | ||
| return parseModelTuple(response, value); |
There was a problem hiding this comment.
Curious about the performance comparison here vs. the initial version's element shortcut. The shortcut avoided the generic loop for element arrays entirely, processing type/key/props with type-specific knowledge (e.g. skipping _walkJSON for a plain string type or null key), avoiding '' + i string conversion per index, and not re-reading fields from the array through parseModelTuple. With the post-loop approach, element arrays go through the full generic walk, then parseModelTuple re-accesses all fields that were just walked. Was there a measured reason to drop the shortcut, or was it just for simplicity?
There was a problem hiding this comment.
Was there a measured reason to drop the shortcut, or was it just for simplicity?
Yeah the reason is the existing tests fail if the loop is moved afterwards 😄
The reason is that value[0] is a string, not the REACT_ELEMENT_TYPE symbol.
I checked and in the various benchmarks this change did not make things meaningfully slower while it preserves the existing behavior / check which is why moving it made sense for this PR.
I think we can change the check the string instead and this fast path can be unlocked, but I'm not familiar enough with this code to properly asses that it would be safe to make that assumption. Hence matching the existing behavior that passes the tests.
| parseModel(response: Response, json) { | ||
| return JSON.parse(json, response._fromJSON); | ||
| const parsed = JSON.parse(json); | ||
| return response._walkJSON(parsed, {'': parsed}, ''); |
There was a problem hiding this comment.
This method should be removed rather than updated. Once _fromJSON/_walkJSON is removed from Response in favor of a plain module-scope function, there's nothing for this to call. ReactFlightClient.js never imports parseModel from ReactFlightClientConfig either, and the tests that directly use this module never call it.
There was a problem hiding this comment.
Great, I'm all for that (details: #35776 (comment))
3 participants
Summary
Follow-up to vercel/next.js#89823 with the actual changes to React.
Replaces the
JSON.parsereviver callback ininitializeModelChunkwith a two-step approach: plainJSON.parse()followed by a recursivewalkParsedJSON()post-process. This yields a ~75% speedup in RSC chunk deserialization.Problem
createFromJSONCallbackreturns a reviver function passed as the second argument toJSON.parse(). This reviver is called for every key-value pair in the parsed JSON. While the logic inside the reviver is lightweight, the dominant cost is the C++ → JavaScript boundary crossing — V8'sJSON.parseis implemented in C++, and calling back into JavaScript for every node incurs significant overhead.Even a trivial no-op reviver
(k, v) => vmakesJSON.parse~4x slower than bareJSON.parsewithout a reviver:Change
Replace the reviver with a two-step process:
JSON.parse(resolvedModel)— parse the entire payload in C++ with no callbacks_walkJSON— recursively walk the resulting object in pure JavaScript to apply RSC transformations (resolving$-prefixed special strings, constructing React elements, handlinginitializingHandlerprotocol)The
_walkJSONfunction includes additional optimizations over the original reviver:parseModelStringwhen the string starts with$, skipping the vast majority of strings (class names, text content, etc.)["$", type, key, props]) upfront and processes each field with knowledge of its expected type, avoiding generic traversalResults
You can find the related applications in the Next.js PR as I've been testing this on Next.js applications.
Table as Server Component with 1000 items
Before:
After:
Table as Client Component with 1000 items
Before:
After:
Nested Suspense
Before:
After:
Even more nested Suspense (double-level Suspense)
Before:
After:
How did you test this change?
Ran it across many Next.js benchmark applications.
The entire Next.js test suite passes with this change.