Repository navigation
node:http2: take inbound header names from the per-VM HTTPHeaderIdentifiers cache - #39469
Conversation
…ifiers cache Bun__h2__materializeHeaders allocated a JSString for every field name of every inbound HEADERS block and then atomized the name a second time for the headers-object put. Pseudo-headers additionally copied the name into a fresh StringImpl, since HTTPHeaderNames.in does not list them. Add the five RFC 9113 pseudo-headers to WebCore::HTTPHeaderIdentifiers as a second X-macro list with the same LazyProperty<JSString> + Identifier shape as the existing entries, restore identifierFor() next to stringFor() and add the pseudo-header overloads, and make the materializer use the cache for every name it recognizes. Unknown names are atomized once and the rawHeaders entry wraps that atom.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesHTTP/2 header materialization now resolves pseudo-headers and standard headers through shared cached identifiers. Unknown names are atomized once. Tests cover value handling, ordering, numeric status conversion, and cached raw header names. HTTP/2 header materialization
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes GC-visited per-VM state and the hot native path for every inbound HTTP/2 header block, a human look would still be worthwhile.
What was reviewed:
- Pseudo-header X-macro names (
Authority/Method/Path/Scheme/Status) don't collide with anyHTTP_HEADERS_EACH_NAMEentry, so the generated members and accessors are unique. - New
LazyPropertyslots are wired intoHTTPHeaderIdentifiers::visit(), which is reached fromZigGlobalObject::visitChildren(ZigGlobalObject.cpp:3410), so the cached pseudo-headerJSStrings are marked. - Removed
getString/RETURN_IF_EXCEPTIONpair is safe:stringFor/identifierFordon't enter user JS, and the PR verified withBUN_JSC_validateExceptionChecks=1. :protocol(RFC 8441) is not in the pseudo-header list; it falls through to the atomize-from-wire branch, which is behaviorally identical to before.
Extended reasoning...
Overview
This PR wires the HTTP/2 inbound header materializer (Bun__h2__materializeHeaders) up to the existing per-VM HTTPHeaderIdentifiers cache so that known header names and the five HTTP/2 pseudo-headers reuse a single cached JSString + Identifier instead of allocating fresh ones per request. It touches three C++ files: extends HTTPHeaderIdentifiers.{h,cpp} with a second X-macro list for pseudo-headers (plus enum, findHTTP2PseudoHeaderName, restored identifierFor overloads, and static_asserts against numHTTPHeaderNames), and rewrites the name-resolution block in H2HeadersMaterializer.cpp to consult the cache and compare enums for the :status/cookie/set-cookie special cases. A ~100-line behavior-preservation test is added to node-http2.test.js.
Security risks
None identified. Input is already-validated lowercase ASCII wire bytes; the change swaps how name strings are obtained without touching validation, bounds, or the packed/meta buffer walk. No new untrusted-data parsing.
Level of scrutiny
High. This is native C++ on a per-request hot path with GC-lifetime implications: new LazyProperty<JSGlobalObject, JSString> slots on a per-VM object that must be visited, and cached JSString cells shared across many arrays. Per the repo's review guidance, native memory safety and GC rooting are the most-blocked category. The change follows the established X-macro pattern exactly (same declaration/init/accessor/visitor macros as the existing 94 entries), which limits risk, but it is not a mechanical change.
Other factors
I verified: (1) no member/accessor name collisions between the two X-macro lists; (2) the new LazyProperty slots are marked via HTTPHeaderIdentifiers::visit, which ZigGlobalObject::visitChildrenImpl already calls; (3) the removed nameStr->getString + RETURN_IF_EXCEPTION is safe because the replacement path (stringFor/identifierFor/Identifier::fromString on Latin1 span) cannot throw a JS exception; (4) the unknown-name path (jsString(vm, ident.string())) wraps the atom's refcounted StringImpl, not a borrowed view. The PR reports passing the full test/js/node/http2/ suite plus 283 upstream Node http2 tests on debug+ASAN and under BUN_JSC_validateExceptionChecks=1. The new test is explicitly behavior-preserving (passes before and after), which is appropriate for a perf-only refactor but means it does not gate the optimization itself. Given the hot-path native scope, deferring to a human reviewer rather than auto-approving.
|
On the two points a human reviewer is likely to hit as well:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:30 PM PT - Aug 17th, 2026
✅ @robobun, your commit 3b3e6f4990f3d5dab468d3322e016c30b6f01e0a passed in 🧪 To try this PR locally: bunx bun-pr 39469That installs a local version of the PR into your bun-39469 --bun |
…aderName enum order
There was a problem hiding this comment.
I reviewed this again after 3b3e6f4 and didn't find any bugs — the earlier note about the HTTPHeaderNames.in order comment is now enforced by the entriesFollowHTTPHeaderNameOrder() static_assert, which is a better fix than rewording. Since this is ~100 lines of native C++ on the per-request h2 hot path that adds GC-visited LazyProperty state, a human sign-off from someone familiar with the h2 engine would still be worthwhile.
What was reviewed:
- Pseudo-header X-macro identifiers (
Authority/Method/Path/Scheme/Status) do not collide with anyHTTP_HEADERS_EACH_NAMEentry, so the generated members/accessors are distinct. - New
m_*StringLazyProperties are added tovisit()and reached fromZigGlobalObject::visitChildren, so the cached JSStrings are marked. isStatus/isCookie/isSetCookieenum comparisons are equivalent to the removed byte comparisons given wire names are validated lowercase;:protocolstill routes through the unknown-name branch and produces the same result as before.- Unknown-name branch:
nameView.span8()is safe (Latin1 view over validated-ASCII wire bytes) and the atom-then-jsString order preserves the same identifier used forputDirect.
Extended reasoning...
Overview
This PR wires Bun__h2__materializeHeaders (the single native pass that turns a decoded HTTP/2 header block into rawHeaders / headers-object / sensitive-list) to the existing per-VM HTTPHeaderIdentifiers cache, and extends that cache with the five RFC 9113 pseudo-headers via a second X-macro list. It restores identifierFor(HTTPHeaderName) (removed as dead in #39420), adds HTTP2PseudoHeaderName overloads, and replaces the :status/cookie/set-cookie string comparisons with enum comparisons. My prior review flagged a misleading comment about the macro list following HTTPHeaderNames.in order; the author replaced it with a compile-time static_assert that entry i equals enum value i, which enforces the invariant rather than documenting it.
Security risks
None identified. Header names reaching this code are already validated lowercase ASCII by the Rust engine before the packed buffer is handed over (per the existing comment at the nameView construction site), so the new findHTTP2PseudoHeaderName and enum comparisons see the same input class as the removed byte comparisons. No user-controllable data indexes the new getter tables — the indices are compiler-assigned enum values from findHTTPHeaderName/findHTTP2PseudoHeaderName, and the static_assert now guarantees the regular-name table can't be misaligned. The unknown-name branch's span8() is called on a Latin1 StringView built from those validated bytes.
Level of scrutiny
High. This runs once per inbound HEADERS/PUSH_PROMISE/trailer block on both client and server, so it's a per-request hot path in a Node-compat surface. It adds GC-visited state (five new LazyProperty<JSGlobalObject, JSString> slots) to a per-VM object marked from ZigGlobalObject::visitChildren, and shares one JSString cell across many rawHeaders arrays. I verified the visitor is extended in the same commit, that the pseudo-header identifier suffixes don't collide with any regular-header entry, and that the pattern matches the existing NodeHTTP.cpp consumer of the same cache — but this is exactly the category (native JSC bindings, GC lifetime, hot path) the repo review guidance flags as most-blocked, so a maintainer look is appropriate rather than bot-only approval.
Other factors
Test coverage is strong for a perf change: one test uses jscDescribe() to assert every rawHeaders name string is atomic (fails on main, passes here — a real fails-for-the-right-reason check), and a second behavior-preservation test pins rawHeaders/headers/sensitive across pseudo/known/unknown/all-digit names including the :status numeric coercion, cookie ; join, and set-cookie array, on two round trips of the same session. Both use port: 0, local loopback, and try/finally cleanup. The PR description reports the full test/js/node/http2/ suite (477 tests) and all 283 upstream Node http2 tests pass on debug+ASAN, plus BUN_JSC_validateExceptionChecks=1. The comment-cop bot's paragraph-comment complaints were all addressed in cd77329, and the failing jsresult-swallow lint is confirmed unrelated (fails the same way on main).
Four X-macro string caches (HTTPHeaderIdentifiers, BunCommonStrings, BunMarkdownTagStrings, the JSStreamsRuntime handler table) each stamp one JSC::LazyProperty init lambda per name plus an unrolled accessor and visitor per name. In the linux-x64 binary that is 251 lambda instances of about 500 to 720 bytes each (130 KB), 44 KB of HTTPHeaderIdentifiers accessors and 51 KB of unrolled visit functions, all of them the same code with a different index or literal. Each table now has one shared init lambda that recovers its slot from the LazyProperty address, an array of literals, and a loop for visit. The per-name accessors stay as one-line inline forwards so callers do not change. HTTPHeaderIdentifiers keeps everything #39469 added (the pseudo-header list, identifierFor for both enums, the enum-order static_asserts); the pseudo-headers live in the same table after the regular names, with a static_assert pinning the offset. Checked with node-http, streams and headers tests on the debug build; the touched TUs also type-check against the release compile flags. CI's size gate gives the real number, the local Rust-only measure cannot see C++.
Follow-up to #39448, which deleted the dead
Http2CommonStringscache. This wires the live inbound path up to the cache that is actually in use.Problem
Bun__h2__materializeHeaders(src/jsc/bindings/H2HeadersMaterializer.cpp) runs once per inbound HEADERS / PUSH_PROMISE / trailer block on both the client and the server. For every field it allocated a newJSStringcell for the name (jsString(vm, String(httpHeaderNameStringImpl(...)))), then read it back and calledIdentifier::fromStringon it for the headers-object put, which is an atom-table lookup per field.:method,:path,:scheme,:authority,:status) are not inHTTPHeaderNames.in, so they took the unknown-name path: a newStringImplcopy plus a newJSStringplus an atomization, on every request and every response.WebCore::HTTPHeaderIdentifiers(src/jsc/bindings/webcore/HTTPHeaderIdentifiers.{h,cpp}, reached throughclientData(vm)->httpHeaderIdentifiers()) already caches oneJSStringand oneIdentifierper known header name for the lifetime of the VM, andBun__NodeHTTP__buildRawHeadersArrayinNodeHTTP.cppalready uses it. The HTTP/2 path did not, and the cache had no pseudo-header entries. ItsidentifierFor()was removed as dead code in Remove dead code from react_compiler, the WebCore/IDL bindings, install_jsc, node-fallbacks, and misc crates #39420 earlier today.Fix
HTTPHeaderIdentifiers: addHTTP2_PSEUDO_HEADERS_EACH_NAME, a second X-macro list with the five pseudo-headers, and feed it through the same declaration / lazy-init / accessor / visitor macros as the existing entries, so each pseudo-header gets the sameLazyProperty<JSString>+ cachedIdentifierpair.HTTP2PseudoHeaderNameis generated from that list, so the enum and the lookup tables cannot drift apart;findHTTP2PseudoHeaderName()maps a wire name to it (one byte compare for names that do not start with:).identifierFor(VM&, HTTPHeaderName)next tostringFor()and add theHTTP2PseudoHeaderNameoverloads. The regular-name tables are indexed byHTTPHeaderName, so astatic_assertnow checks thatHTTP_HEADERS_EACH_NAMEhas one entry per enum value and in enum order (previously nothing checked either; a misplaced entry would have silently shifted every later slot).H2HeadersMaterializer.cpp: names found in either list take the cachedJSStringandIdentifier, so they allocate nothing and are never atomized per request. Any other name is atomized straight from the wire bytes (the same atom-table lookup as before, minus the intermediateStringImplcopy) and its rawHeadersJSStringwraps that atom, so the unknown path goes from two allocations to one. The:status,cookieandset-cookiespecial cases compare the enums instead of re-comparing the name bytes. Behavior of the produced rawHeaders array, headers object and sensitive list is unchanged.BunHttp2CommonStringsis not brought back.test/js/node/http2/node-http2.test.js,http2 hands out the cached name strings for pseudo-headers and known header names: after one round trip,jscDescribe()frombun:jscreports every name string in rawHeaders, on both the server and the client side, as atomic, which is only true of strings that wrap the atoms the headers object is keyed by (the cache entries, or the per-block atom for unknown names). On main all ten names come out non-atomic (a JSString over WebCore's static name string, or a copy of the wire bytes), so the test fails there and passes with this change.http2 materializes pseudo, known, unknown and all-digit header names the same way on every request: pins the produced rawHeaders / headers object / sensitive list for all four name paths on both sides, including the cookie / set-cookie /:statusspecial cases, and checks that a second round trip on the same session (served from the populated cache) materializes identically. Behavior-preservation test, passes before and after.bun bd test test/js/node/http2/(all 9 files, includingh2-conformance.test.ts): 477 pass, 0 fail, on the debug + ASAN build.test/js/node/test/{parallel,sequential}/*http2*files pass with the debug build.BUN_JSC_validateExceptionChecks=1.Benchmark
Local h2c loop in one process: a
node:http2server and client on 127.0.0.1, 64 streams in flight on one session. Each round trip carries 10 regular fields on the request and 10 on the response (70% names fromHTTPHeaderNames.in, 30%x-*), so with the pseudo-headers it materializes 25 fields per round trip (14 on the server, 11 on the client).bun run build:release(non-LTO) of main at 9f67488 with and without this diff, same machine (16 vCPU container on a shared, busy host), 7 interleaved trials each, 60k timed round trips per trial after a 5k warm-up.The loop measures the whole request path (stream objects, events, framing, HPACK, loopback sockets), of which name materialization is a small slice, so a few percent end to end is the expected size of this change. Per round trip with the 10-header mix (5 pseudo-headers, 15 known names, 5
x-*names) it removes 20JSStringallocations, 10StringImplcopies and 20 atom-table lookups; the 5x-*names keep their lookup and go from two allocations to one.Benchmark script
Background
HTTPHeaderNames.inis WebCore's list of ~94 well-known HTTP header names.findHTTPHeaderName()maps a name to theHTTPHeaderNameenum with a perfect hash. HTTP/2 pseudo-headers are not HTTP header fields, which is why they are not in that list.WTF::AtomStringImplis the per-thread interning table for strings used as property keys. AJSC::Identifieris a handle to an atom;Identifier::fromStringhashes the input and looks it up (allocating a new atom on a miss).JSStringis the GC cell that wraps aStringImpland is what JavaScript sees; severalJSStrings can share oneStringImpl, including an atom.JSC::LazyProperty<Owner, T>is JSC's lazily-initialized GC slot: it stores an initializer until first use, andvisit()marks the cell once created.HTTPHeaderIdentifierslives in the per-VMJSVMClientDataand is marked fromZig::GlobalObject::visitChildren, so the cached strings stay alive for the VM's lifetime and adding entries to the X-macro list extends both the initialization and the marking automatically.JSStringcell across many rawHeaders arrays is not observable from JavaScript: strings are compared by value, and the cells are immutable.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The inbound node:http2 header path in H2HeadersMaterializer.cpp built a fresh JSString and then a separate Identifier for every header name on every HEADERS frame, and the pseudo-headers were absent from HTTPHeaderNames.in, so each request or response also paid for a new StringImpl plus atomization for :method, :path, :scheme, :authority and :status. The fix routes known names through the existing per-VM HTTPHeaderIdentifiers cache, as NodeHTTP.cpp already does, and adds the five pseudo-headers to that cache as lazily initialized strings with cached identifiers, so steady-state materializat…