Conversation
… stays O(1) HTTPHeaderMap stored uncommon header names in a Vector scanned linearly on every get/set/append/has/delete, so N distinct names cost O(N) per operation and O(N^2) to build or read once. Add a lazily-built ASCIICaseInsensitiveHash map from name to vector index, constructed only once the vector passes 64 entries; below that the existing linear scan runs unchanged. The index aliases the String already held by each vector entry, so it costs one bucket per name rather than a second copy. Release build, N distinct uncommon names, append each then get each: N=50 16.6us -> 15.8us N=255 354us -> 65.5us N=1000 4.5ms -> 233us N=12k 868ms -> 6ms
|
Updated 3:38 AM PT - Jul 22nd, 2026
✅ @robobun, your commit c7cfa47ee03539b0750a200d562b2c70e314700c passed in 🧪 To try this PR locally: bunx bun-pr 35087That installs a local version of the PR into your bun-35087 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This does not address #20493. That issue is about |
WalkthroughHTTPHeaderMap now lazily indexes uncommon headers with ASCII case-insensitive keys, maintains the index across mutations, invalidates it when storage changes, and accounts for its memory. FetchHeaders uses direct map assignment, with tests covering correctness and lookup scaling. ChangesUncommon header indexing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/webcore/HTTPHeaderMap.h`:
- Around line 257-260: Update uncommonHeaders() so exposing m_uncommonHeaders
permanently disables indexed lookup for the lifetime of the escaped mutable
reference; do not allow get() or related lookup paths to rebuild the index
afterward. Prefer enforcing this through the existing index state and lookup
logic, or replace the raw reference with mutation APIs that invalidate safely.
In `@test/js/web/fetch/headers.test.ts`:
- Around line 626-632: Update the explanatory comment above the lookup
performance probes to state that linear-search work is roughly 15× larger for
the 8N-entry map, reflecting that the N-entry case scans all positions while the
larger case scans only its final N positions. Keep the description of the O(1)
hash-index comparison and self-calibrating ratio unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c5e5082-10af-40d5-8b77-a9b4c73837e3
📒 Files selected for processing (3)
src/jsc/bindings/webcore/HTTPHeaderMap.cppsrc/jsc/bindings/webcore/HTTPHeaderMap.htest/js/web/fetch/headers.test.ts
…e map directly The only caller of the non-const accessor was FetchHeaders::fill filling an empty map, which is now a straight copy of the source HTTPHeaderMap. Removing the accessor means no caller can hold a mutable vector reference across a lookup that rebuilds the index. Also correct the linear-scan ratio in the test comment (probing the last N of an 8N-entry map averages ~7.5N scan depth vs ~N/2 for the N-entry map, so ~15x, not 8x).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked: all remaining uncommonHeaders() callers (NodeHTTP.cpp, JSFetchHeaders.cpp) are read-only iteration, so dropping the mutable overload is safe; setInternalHeaders was a plain move-assign, so the FetchHeaders::fill copy-assign is behavior-equivalent; and the HashMap holds its own String ref, so removeAt(index) before m_uncommonHeadersIndex->remove(name) doesn't dangle the key.
Extended reasoning...
This is a performance optimization to a core WebCore data structure (HTTPHeaderMap) that adds a lazily-built secondary index with new copy/move ctors, invalidation logic across every mutation path, and a wall-clock timing test. The C++ looks correct on inspection and I ruled out a few specific concerns (noted in the message), but the change is neither simple nor mechanical — index-coherence bugs here would surface as wrong header values in HTTP responses, so it warrants a human look rather than auto-approval. The one finding posted inline is a test-quality nit about the timing assertion degenerating to an absolute bound on release builds.
At N=1500 gets the release-build baseline was ~0.3ms, so the 1ms floor turned the ratio assertion into an absolute tLarge < 3ms bound. Cycle 20000 gets through the same N names instead; the release baseline is now ~1.3ms, the floor is gone, and the ratio self-calibrates on every build. best-of-2 absorbs single scheduler blips.
There was a problem hiding this comment.
I didn't find correctness issues, but this adds a new data member with copy/move/clear lifecycle to HTTPHeaderMap (present on every Request/Response) and picks a threshold constant on a hot path, so it's worth a maintainer sign-off on the approach.
What was reviewed:
- Index lifecycle across copy ctor/assign, move,
clear(),isolatedCopy(),shrinkToFit()— all leave or reset the index consistently with the vector. removeUncommonHeader: vectorremoveAtshifts before the index is walked; the HashMap key is a refcountedStringso it survives the vector entry's destruction; decrement loop matches the shift.- Removed mutable
uncommonHeaders(): remaining callers (NodeHTTP.cpp,JSFetchHeaders.cpp, the iterator) all reach it viaconst HTTPHeaderMap&so they resolve to the const overload. FetchHeaders::fillrewrite is equivalent to the old appendVector+move path (setInternalHeaderswas just a move-assign).
Extended reasoning...
Overview
Adds a lazily-built HashMap<String, unsigned, ASCIICaseInsensitiveHash> to HTTPHeaderMap that indexes m_uncommonHeaders by name once the vector exceeds 64 entries, so per-name operations stay O(1) instead of degrading to O(N). Touches HTTPHeaderMap.{h,cpp} (new member, custom copy ctor/assign, defaulted move, findUncommonHeaderIndex/appendUncommonHeader helpers, index maintenance in removeUncommonHeader, memoryCost accounting), FetchHeaders.cpp (replaces the vector-append fill path with direct copy-assign, which also let the mutable uncommonHeaders() accessor be deleted), and adds two tests to headers.test.ts.
Security risks
None identified. The change is an internal lookup optimization; no new user-controlled input reaches new parsing or allocation logic. Header names are already validated by isValidHTTPToken before reaching these paths, so the empty-String HashMap sentinel is unreachable.
Level of scrutiny
Moderate-to-high. HTTPHeaderMap sits under every Headers, Request, and Response; the added unique_ptr field and custom copy/move machinery run on the common path even when the index is never built. The threshold of 64 and the decision to maintain (rather than invalidate) the index on delete are design choices a maintainer should confirm. The Performance section of the landing-PRs doc applies.
Other factors
Correctness looks solid: every mutation site that touches m_uncommonHeaders now routes through appendUncommonHeader/removeUncommonHeader or leaves indices unchanged; clear(), copy-assign, and isolatedCopy() all reset/omit the index; shrinkToFit() doesn't need to (indices are stored, not pointers). The removed mutable accessor's remaining call sites all go through internalHeaders() which returns const&, so they bind to the const overload. The correctness test exercises get/has/set/append/delete across the threshold with mixed case and post-delete shifts. The timing test was reworked in c7cfa47 after earlier feedback so the baseline clears 1ms on release and the ratio genuinely self-calibrates (~1.1 pass vs ~11 regression, threshold 3) — reasonable, though timing-ratio tests always carry some residual flake risk. Deferring so a human can sign off on the hot-path design and the +8 bytes per map.
|
Build #77589: |
2 participants
HTTPHeaderMapstores uncommon header names in aVector<UncommonHeader>and everyget/set/append/has/deleteon an uncommon name didm_uncommonHeaders.findIf(equalIgnoringASCIICase(...)), so the cost of each operation grew with the number of distinct names already present. Building aHeaderswith N distinct uncommon names, or reading each once, was O(N^2).Real servers cap ingress headers well below this, but user code can build a
Headersof any size programmatically (header-mirroring proxies, exporters, tooling that stuffs metadata through the fetch API).Fix
Add a lazily-built
HashMap<String, unsigned, ASCIICaseInsensitiveHash>that maps each uncommon name to its vector index. The index is only constructed oncem_uncommonHeaders.size()passes 64; below that the existing linear scan runs unchanged (one extra null check), so the common few-headers case keeps its current fast path. The index stores theStringalready held by the vector entry, so it costs one bucket per name rather than a second copy of each name. Mutations keep the index in sync: append records the new slot, remove drops the key and decrements the shifted slots (same O(N) as the vector shift), copy/clear/the mutableuncommonHeaders()accessor drop the index so it rebuilds on the next lookup.Numbers (release build, append N distinct uncommon names then
geteach once)Verification
test/js/web/fetch/headers.test.tsgains two tests under "many distinct uncommon names":append/get/has/set/delete/copy, including mixed case and lookups after deletes have shifted vector slots, so a stale index would return the wrong value;All of
headers.test.ts,headers.undici.test.ts,headers-case.test.ts, theHeadersblock offetch.test.ts,deno/fetch/headers.test.ts,response.test.tsandcookies.test.tspass unchanged.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file