[Fizz] Reduce chunk push overhead in HTML serialization#36139
[Fizz] Reduce chunk push overhead in HTML serialization#36139switz wants to merge 1 commit intofacebook:mainfrom
Conversation
Batch attribute and style serialization into single string pushes instead of 3-5 separate push calls per attribute. This reduces array operations, chunk count in segment buffers, and downstream Buffer.byteLength and writeChunk/encodeInto call volume. Changes: - pushStringAttribute: 5 pushes → 1 concatenated string push - pushBooleanAttribute: 3 pushes → 1 push - pushStyleAttribute: 4N+1 pushes → 1 push (build full style string) - pushAttribute default/URL/enumerated/numeric cases: 5 pushes → 1 push - processStyleName: returns plain string instead of PrecomputedChunk - pushStartGenericElement/pushStartAnchor: merge close tag with text children into single push when no dangerouslySetInnerHTML - byteLengthOfChunk: use string.length instead of Buffer.byteLength for heuristic byte sizing (outlining threshold, progressive chunks). For ASCII (99%+ of HTML output), length === byte length. Benchmark (Node v24, Apple M1 Max, 1000 iterations, 100 warmup): | Tree | Before | After | Δ | |-----------------------------|-----------|-----------|----------| | Small (10 el) | 37,959 | 48,257 | +27.1% | | Medium (300 el, Suspense) | 2,309 | 3,518 | +52.4% | | Medium (300 el, no Suspense)| 2,404 | 3,725 | +54.9% | | Large (2500+ el, Suspense) | 429 | 626 | +45.9% |
|
Hi @switz! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
gnoff
left a comment
gnoff
left a comment
There was a problem hiding this comment.
If you open a change with just the attribute improvements I think we can land that. The innerHTML stuff needs a little more work and not sure if it's worth it. The byteLength i'd just leave out
| ? chunk.length // Fast path: .length === byte length for ASCII (99%+ of HTML output). | ||
| : // For multi-byte chars this slightly underestimates, which is fine | ||
| // because byteSize is only used for heuristic decisions (outlining | ||
| // threshold and progressive chunk sizing). | ||
| chunk.byteLength; |
There was a problem hiding this comment.
This change should have broken something in ReactFlightServer. Suggests that we have some untested serialization path with large string serialization.
Even if the rationale was true though we can't land this change because the layering implies you get back an accurate byteLength. We'd have to rename to getApproximateByteLength etc...
Unless this is like the entirety of the perf win it doesn't seem worth the effort to rewrite the parts that use this length for approximating heuristics so let's just drop it.
If you want to see what would be broken by this check out emitTextChunk in ReactFlightServer
| target.push(stringToChunk(encodeHTMLTextNode(children))); | ||
| if (innerHTML == null && typeof children === 'string') { | ||
| // Fast path: close tag and emit text child in a single push. | ||
| target.push(stringToChunk('>' + encodeHTMLTextNode(children))); |
There was a problem hiding this comment.
The reason it was written the way it was before is we try to avoid branching on hot paths as much as possible. In this case you are adding an extra if check to every element that has non string children because we check the innerHTML twice instead of once.
There is probably a more dramatic refactor that could avoid the extra conditional and still allow for the avoidance of the extra chunk.
3 participants
I spent some time yesterday digging into RSC performance with Claude. I think there are two much bigger issues to tackle than this (one being web vs. node streams and the other being an architectural bottleneck), but this PR did provide some nice wins in a variety of benchmarks.
I have a separate improvement to Flight on another branch, but I'm a tad more skeptical of that PR. Plus, its gains were more modest.
Benchmark (Node v24, Apple M1 Max, 1000 iterations, 100 warmup):
This is just one small part of the pipeline, so it's not representative of major e2e gains.
Below are Claude's notes–feel free to turn down this PR if you find its efficacy lacking. The
byteLengthOfChunkchange I'm a touch more nervous about, but if it's only used for heuristics this feels okay.Summary
Batches attribute and style serialization into single string pushes instead of 3-5 separate
target.push()calls per attribute. This reduces array operations, chunk count in segment buffers, anddownstream
Buffer.byteLengthandwriteChunk/encodeIntocall volume.Motivation
Profiling
renderToPipeableStreamwith V8's--profshowed thatpushAttributeandpushStartGenericElementwere the top JS hot paths, with the dominant cost being the sheer number oftarget.push()calls per element. A<div>with 6 attributes and a 6-property style object was doing~54 array pushes for attributes alone. Each push adds an item to the segment's chunk array, which then
has to be iterated by
finishedSegmentfor byte sizing and byflushSubtreefor encoding.Changes
Attribute push batching (
ReactFizzConfigDOM.js):pushStringAttribute: 5 pushes (attributeSeparator,name,attributeAssign,escapedValue,attributeEnd) → 1 push with' ' + name + '="' + escapeTextForBrowser(value) + '"'pushBooleanAttribute: 3 pushes → 1 pushpushStyleAttribute: 4N+1 pushes (N = number of style properties) → 1 push. Builds the entirestyle="..."string before pushing.pushAttributedefault case (data-, aria-, etc.), URL attributes, enumerated attributes, numericattributes, overloaded booleans: all consolidated from 3-5 pushes to 1.
processStyleName: returns plain string instead ofPrecomputedChunkto enable string concatenationin style building.
Text child merging (
ReactFizzConfigDOM.js):pushStartGenericElementandpushStartAnchor: when children is a string and there's nodangerouslySetInnerHTML, merge the>with the escaped text into a single string push.Byte length fast path (
ReactServerStreamConfigNode.js):byteLengthOfChunk: usestring.lengthinstead ofBuffer.byteLength(chunk, 'utf8'). For ASCIIcontent (99%+ of HTML attribute/tag output),
.length === byte length. The slight underestimate formulti-byte characters is acceptable since
byteSizeis only used for heuristic decisions (outliningthreshold at 500 bytes, progressive chunk sizing).
Results
Benchmarked with
renderToPipeableStreamon Node v24.14.0, Apple M1 Max. Renders piped to abyte-counting Writable (no I/O). 100 warmup, 1000 measured iterations, <1% coefficient of variation.
Output HTML is byte-identical before and after.
How did you test this change?
All existing Fizz and Server integration tests pass (4,088 tests across 150 suites). Verified HTML
output identity for elements with className, id, style objects, data-* attributes, escaped text content,
void elements, and SVG namespace attributes.