[pull] main from react:main - #641
Merged
Merged
Conversation
The build size comparison comment was posted by Danger, which authenticated with a personal access token hardcoded in `scripts/tasks/danger.js`. That token has since been revoked, so sizebot has been posting nothing at all (due to e.g. https://github.com/react/react/actions/runs/32181295467/job/95855395224?pr=37315). This change rebuilds it on the short-lived `GITHUB_TOKEN` that Actions mints per run and a new workflow only responsible for rendering untrusted JSON input as markdown in a PR comment. A straight token swap would not have worked. Fork pull requests did receive sizebot comments, but only because the token was in checked-out source: the sizebot job runs on the `pull_request` trigger, where a fork's `GITHUB_TOKEN` is read-only and cannot comment. The comment therefore moves to a new `workflow_run` workflow, `runtime_sizebot_comment.yml`, which runs in this repository with a writable token no matter where the pull request came from. It posts a placeholder when a build is requested and rewrites it in place when the build completes, fails, is cancelled, or is held for maintainer approval. The measurement stays on the unprivileged side of that boundary which are recorded as raw sizes into a `sizebot-results` artifact, and the new workflow downloads only that JSON and renders it from a default-branch checkout. The job holding `pull-requests: write` never unpacks a build produced by a fork, which matters because the existing base-build download justifies using an unverified artifact on the grounds that the job has restricted permissions. Thresholds, the critical bundle list, and the comment template all live on the trusted side, and the renderer validates every field it reads out of the artifact so that a crafted build path cannot inject markdown. The pull request number is resolved from the API rather than from the artifact, since a number read from fork-controlled data would let any contributor post a bot comment on an arbitrary pull request. Resolving that number needs a branch lookup rather than any of the obvious approaches. `workflow_run.pull_requests` is empty for fork runs, and neither `commits/{sha}/pulls` nor the search API indexes fork pull request head commits, so the workflow looks the pull request up by `owner:ref` instead. A comment is only ever left alone in one situation: when it already describes the pull request's current head and the event being handled belongs to an older commit. Everything else is written, and marked stale whenever the report does not describe the current head. That single rule covers both an old run finishing after a force push and a new build superseding a report already on display, and in the latter case the previous numbers stay visible instead of being blanked back to a placeholder. The results file carries a `version` field. Its writer is whatever `compare-sizes.js` a pull request branch happens to carry, while its reader is on the default branch, so the two can mismatch and the renderer needs to be able to say so instead of misrendering a table. Porting the table fixed a longstanding bug in `change()`. Testing `decimal < 0.0001` reported every size decrease as unchanged, which is why `signDisplay: 'exceptZero'` never had a negative number to render: a 709.04 kB to 708.68 kB drop printed as `=`. It now compares the magnitude. Co-authored-by: Claude Code (kimi-k3[1m]) <noreply@anthropic.com>
Flight has two ways to write a string: 1. Small ones are inlined into the JSON model. 2. Large ones (>= 1024 chars) are outlined into a binary text row so they don't get double-encoded and double-parsed. Neither is ever deduplicated. That's most visible in client reference metadata, where a route repeats the same bundler chunk URLs across every client reference (for example, vercel/next.js#95559). **This PR adds a dedupe map for strings inside import metadata.** A string is written once into its own row, and every occurrence is a reference to it. ## How it works When we're about to write a string in import metadata at least as long as the threshold, we look it up in the request's map: - **If it's not in the map:** Emit a row containing the string, store that row's reference in the map, and write the reference here. - **If it is:** Write the reference. So every string goes on the wire once, and every occurrence costs a few bytes. An earlier version waited for the second occurrence before outlining, which is the right default for arbitrary strings where most never repeat (it's what #27537 does for objects). Import metadata is the opposite case: a chunk is listed by every client module that lives in it, so a chunk name that appears once is the exception. In the bench app's three routes, every chunk string at least 16 characters long appears more than 20 times and none appears once. Outlining on first sight saves the inline copy, and a string that never repeats costs 7 bytes more than inlining it. Import metadata needs its own map and its own queue. The client resolves a client reference as soon as it parses the import row, and import chunks flush ahead of model rows, so the string row has to be in the same queue to arrive first. The client needs no protocol changes. It already resolves `$N` references, and a row holding a string resolves to that string. It does get a check that import metadata never blocks on a row that hasn't arrived, which is the other end of the queue ordering above, and `getOutlinedModel` stops allocating a path array for references without one. Model strings are left alone. An earlier version of this PR deduped them too, but we're not going to do this now per review (maybe in a follow-up). Metadata on the debug channel is also left alone: it's a separate serialization path, and deduping across the two would make the main payload depend on whether a debug channel is attached. ## Threshold The trigger is 16, low compared to what a model-side threshold would want, because import metadata is repetitive but its parts are short. How much this saves depends on how many client references share a chunk list, so measuring one string on its own is misleading. For a chunk path of realistic length today: | references sharing the chunk | before | after | |---|---|---| | 1 | 80 B | 87 B | | 2 | 160 B | 122 B | | 3 | 240 B | 157 B | | 5 | 401 B | 228 B | | 10 | 813 B | 416 B | | 40 | 3273 B | 1526 B | | 80 | 6594 B | 3047 B | (Import and string rows only.) It costs 7 bytes at 1 reference and wins from 2. Chunk paths in this app are 47 characters, so a threshold of 48 or higher saves nothing at all here. That's why it's 16: picking a number just under one bundler's path length gives you something that quietly stops working on the next bundler. The map is bounded by the combined length of the strings it holds, 32 KiB. Once the budget is spent, new strings are written inline every time while strings already outlined keep deduping. That makes the savings depend on the order strings are first seen: a shared chunk URL first encountered after 32 KiB of unique module ids won't be deduped. That's main's behavior, so it's a missed win rather than a regression, but a manifest-heavy dev route could hit it. ## Byte measurements Three routes of a Next.js app, serial requests: | route | Flight | document | document (gzip) | |---|---|---|---| | `/dashboard` | −48.4% (710.1 → 366.5 KB) | −34.3% | −7.8% | | `/docs` | −5.8% (555.2 → 523.1 KB) | −5.0% | −0.7% | | `/blog` | −5.4% (878.8 → 831.7 KB) | −4.4% | −1.7% | The difference between the routes is how many client references each one has. On `/dashboard` the import rows shrink from 388.0 KB to about 32 KB with the row *count* unchanged at 114, because every client reference repeats the same 49 chunk URLs. gzip already collapses repeated strings, so −48.4% raw is only −7.8% compressed. The bytes still have to be escaped, encoded and copied before they reach the compressor, which is where most of the speedup below comes from. ## Speed measurements Benchmarked end-to-end through a Next.js app on Vercel Sandbox VMs (x86 Xeon), 16 boots, paired ABBA within each boot, boot as the unit of replication. Base is the merge-base with main, `eafeac09`; candidate is the current head, `e0b4614c`. | cell | effect | 95% CI | p | |---|---|---|---| | `/dashboard` serial req/s | **+16.9%** | ±1.7 | <0.0001 | | `/dashboard` serial p95 latency | −18.2% | ±2.4 | <0.0001 | | `/dashboard` serial TTFB | −23.2% | ±1.2 | <0.0001 | | `/dashboard` under load req/s | **+17.0%** | ±3.4 | <0.0001 | | `/dashboard` under load median latency | −13.9% | ±2.5 | <0.0001 | | `/docs` serial req/s | +3.0% | ±1.4 | 0.0003 | | `/docs` serial TTFB | −3.1% | ±1.0 | <0.0001 | | `/blog` serial req/s | +2.4% | ±1.0 | 0.0001 | | `/blog` serial median latency | −2.4% | ±0.7 | <0.0001 | No detected difference: `/blog` and `/docs` under load (p=0.13–0.56). All 16 boots are positive on both `/dashboard` cells. The `/dashboard` headline has now been measured in four separate 16-boot runs across four heads of this branch and is p<0.0001 in each; the small routes cleared p<0.01 only on this head, after the serializer change below, having sat at p=0.02–0.06 on the three earlier heads. The previous head, `4569e1d6`, which outlined on the second occurrence rather than the first, measured +14.9% ±2.3 on `/dashboard` serial req/s and −17.3% ±5.7 on TTFB against the same base. Those intervals overlap the ones above, so the switch is not a measurable speedup on its own; the bytes it saves are about 1% of the payload. In a real browser on `/dashboard` (measured on an earlier commit of this branch, `aed4d523`), hydration is −2.8% ±1.3 (p=0.0003) / −2.4% ±0.9 (p=0.0001) and LCP is −5.3% ±2.0 (p<0.0001) / −3.6% ±2.6 (p=0.009). Client navigation is under the noise floor in both. ### Where the time goes 32 CPU profiles, taken after the timed runs with an identical request count in both arms, so absolute sampled milliseconds are comparable. One pass per boot, no replication statistics — directional, not a claim. These profiles are from `aed4d523`. The current head also walks the metadata into a copy before a plain `stringify`, after a detour through a `stringify` replacer that measured 2.3× slower in isolation (a replacer function takes V8 off its fast path for the whole call); the `transformImportMetadata` frame below is a fair proxy for the current cost. Cheaper: | base | candidate | frame | | ---: | ---: | --- | | 23.6 s | 5.7 s | ReactDOM `preinitScript` | | 24.9 s | 10.6 s | `serializeClientReference` | | 81.8 s | 67.3 s | `utf8Write` | | 93.9 s | 80.8 s | `createFromString` | | 50.7 s | 39.2 s | Next's `htmlEscapeJsonString` | More expensive: | base | candidate | frame | | ---: | ---: | --- | | 0 | 9.3 s | `transformImportMetadata` | | 2.0 s | 10.3 s | `getOutlinedModel` (SSR-side Flight client) | | 7.7 s | 11.7 s | `parseModelString` | | 154.8 s | 158.2 s | `resolveModelToJSON` | About +31 s of new work against −79 s inside the runtime bundle and −45 s in node's buffer and string layer. `getOutlinedModel` resolving references is the mechanism working, not a warning sign. Next.js runs a Flight client on the server to read its own payload, and a `/dashboard` payload goes from 0 references inside import rows to 4964, so a frame that barely ran before now runs once per reference. Each call is a lookup on a row that has already been initialized: the string row goes into the import queue ahead of the import row that reads it, so it has always arrived and nothing blocks. `parseModelString` grows for the same reason. `preinitScript` doesn't get cheaper from writing fewer bytes. It does two dictionary lookups keyed by the chunk URL per call, and the call count and argument values are unchanged — the resolved models are identical. What changes is string identity: in the base build every one of the 5013 chunk-URL occurrences is a fresh string out of `JSON.parse` whose hash has to be computed before the lookup, and with dedupe the 49 distinct URLs are parsed once and every reference yields the same string, so V8's cached hash makes the repeat lookups nearly free. Some of the `htmlEscapeJsonString` and buffer-layer drops have the same cause. ### React-level CPU in isolation The e2e numbers above include everything downstream of React (escaping, encoding, compression, the SSR client). To see React's own serialization cost, 114 import rows of dashboard-shaped metadata (49 shared 74-character chunk names per row) were rendered against one request on the production bundles with a no-op destination, arms interleaved, median of 5 rounds × 200: | | main | this PR | |---|---|---| | 49 names shared by all rows | 0.502 ms | **0.322 ms** (−36%) | | 5586 unique names, nothing to dedupe | 0.477 ms | 0.771 ms (+62%) | The second row is the worst case for this change, a manifest where every chunk name appears once. It costs about 40 ns per unique string, plus about 130 ns for each row the budget lets it outline, against a payload that is otherwise unchanged. The metadata is serialized by copying it with the strings already replaced and then calling plain `JSON.stringify`; a `stringify` replacer function would keep V8 off its fast path for the whole call (measured 2.3× slower than plain in isolation, even writing a sixth of the bytes). The copy covers plain JSON only and falls back to the replacer for anything else (`toJSON`, class instances, keys that exist on `Object.prototype`, depth over four, which is how cycles end up throwing stringify's own error). Equivalence of the two paths was checked by a harness that runs both on identical requests and compares the JSON and the resulting request state: 2,656,142 cases, including exhaustive enumeration of small trees over adversarial atoms, 100k seeded random values, and the cases from two independent adversarial reviews — 0 divergences outside four stated assumptions that no bundler manifest violates (no Proxies, no index accessors polluted onto `Array.prototype`, no primitive wrappers with a swapped prototype, side-effect-free property access). ## Cost where there's nothing to dedupe React's own `flight-ssr-bench` fixture has about ten client modules and no repeated chunk paths, so the dedupe never fires and the change can only cost. It costs a little, if anything. Over 16 boots at `aed4d523` the Flight+Fizz Node sync variant was +0.9% ±0.7 on median inject time (p=0.008), worse on 14 of 16 boots. On the current head the four Flight+Fizz inject cells are between +0.4% and +0.8% on the median, none below p=0.07; across all 88 fixture metrics (Fizz and Flight+Fizz, Node and Edge, sync and async, inject and HTTP at c=1/c=10) nothing reaches p<0.01 and `heapMb` is flat to ±0.1%. So the no-dedupe cost is somewhere around half a percent of inject time on this fixture, at the edge of what it can resolve. I couldn't localize it past that. It isn't the per-request `Map`, which is about 22 ns against a 14 ms render, and it isn't allocation — `gcMs` and `heapMb` are flat. Using the Fizz-only variants as a within-boot control, since nothing in `ReactFlightServer.js` can reach them, the Flight-specific residual on that cell is +0.8% ±0.5 and the other three variants scatter around zero (+0.4%, +0.1%, −0.2%). A build that re-inlines `escapeStringValue` back into the string branch, which is the only change here that runs for every string in the model rather than only for import metadata, doesn't recover it either (+0.2% ±0.3 on the same cell, another 16 boots). So this looks like code layout rather than a specific added operation, and it's near the resolution limit of the fixture. <details> <summary>Verification</summary> - Both arms' payloads for `/dashboard` were parsed and their `$`-references resolved recursively, then deep-compared: the resolved models are identical. The 49 extra model rows are exactly the 49 distinct chunk URLs. All 114 import rows match after resolution. - Arms fingerprint distinctly (`a898f40a7bbd` vs `87fb4b7ba15e`), so the two builds are genuinely different. - Build fingerprints differ between arms (`04440a11435d` vs `43d09027ce58`) and the arm version strings carry the expected shas. - Per-boot deltas are printed by the harness; on `/dashboard` serial req/s all 16 boots are positive (range +11.2% to +22.9%). - The bench fixture sets a deployment id, so every chunk URL carries a `?dpl=` query param that exactly doubles its length (74 chars vs 37). An app without one would see roughly half the absolute byte saving on this route. The CPU wins that come from string identity rather than byte count should degrade less than proportionally, but that wasn't measured. - Not measured: payloads that exceed the 32 KiB tracking budget, and whether 16 is optimal rather than merely low enough. </details> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )