Reduce repeated work in HTTP header parsing

[moonchen]

Reduce repeated work in HTTP header parsing.

1. Replace per-byte libc calls in URL compliance validation with a vectorizable, locale-independent ASCII range check.
2. Simplify request-target validation using the stored URL fields.
3. Combine field-name colon scanning, hashing, and character validation into one pass, reusing the existing well-known-string table.
4. Skip duplicate lookup for well-known fields when their presence bit is clear.
5. Append adjacent duplicate fields in constant time instead of searching the duplicate chain.

Includes a header parsing benchmark harness and regression tests for validation, field-name scanning, duplicate attachment, and parser reuse.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

tools/benchmark/benchmark_HdrParse.cc:797

• The --profile unknown-target error message omits the supported "wks-lower"/"wkslower" target even though parse_target() accepts it, which can mislead users.

    Target t = parse_target(profile_target);
    if (t == Target::Unknown) {
      std::fprintf(stderr, "unknown target '%s' (want: request|response|mime|url|wks)\n", profile_target.c_str());
      return 2;

src/proxy/hdrs/HdrToken.cc:606

• hdrtoken_tokenize_prehashed()’s comment says it does an “exact ASCII-case-insensitive byte compare”, but the implementation currently only checks (hash, length) and returns the bucket token without validating the bytes. Either adjust the comment or add the byte-compare; adding the compare also prevents accidental or crafted hash/length collisions from being misclassified as a WKS token.

// WKS lookup for a name whose FNV-1a hash the caller has already computed
// (e.g. fused into the field-name scan). Does the slot/length narrowing plus
// the exact ASCII-case-insensitive byte compare, but no hashing and no
// interned-pointer test, so it is only valid for a non-interned `string`.
int
hdrtoken_tokenize_prehashed(const char *string, int string_len, uint32_t hash, const char **wks_string_out)
{
  uint32_t            slot   = hash_to_slot(hash);
  HdrTokenHashBucket *bucket = &(hdrtoken_hash_table[slot]);

  if ((bucket->wks != nullptr) && (bucket->hash == hash) && (hdrtoken_wks_to_length(bucket->wks) == string_len)) {
    int wks_idx = hdrtoken_wks_to_index(bucket->wks);
    if (wks_string_out) {
      *wks_string_out = bucket->wks;
    }
    return wks_idx;
  }

src/proxy/hdrs/URL.cc:1210

• url_is_mostly_compliant() no longer emits the debug message that previously identified the first offending byte (whitespace/non-printable). That makes troubleshooting strict_uri_parsing=2 failures harder. You can keep the vectorized scan and only do a second scalar scan when an invalid byte was detected, to log the first bad value.

  // Mode 2 accepts exactly the printable, non-space ASCII range 0x21..0x7E --
  // equivalent to the previous isspace()/isprint() pair, but locale-independent
  // and call-free. This runs on every request target under the default
  // strict_uri_parsing=2. OR-reducing an out-of-range flag over the whole target
  // (no early exit, no data-dependent branch) lets the compiler auto-vectorize
  // the scan to the build's SIMD; ATS builds -O3, where clang and GCC both do.
  unsigned char bad = 0;
  for (const char *i = start; i < end; ++i) {
    unsigned char const c  = static_cast<unsigned char>(*i);
    bad                   |= static_cast<unsigned char>((c < 0x21) | (c > 0x7E));
  }
  return bad == 0;

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues remain in duplicate attachment performance and field-name collision handling.

Review effort: Lite
Findings: None

[bneradt]

Reviewed the parser changes, regression tests, and benchmark harness. One benchmark coverage issue is noted inline. This was a source review; I did not run a local build. The current PR checks are green.

[bryancall]

Superseded, see the full review and verdict in the next review. Left in place because GitHub does not allow a submitted review to be deleted.

[bryancall]

❌ Request changes

I reviewed this against base 9c69fe1bcc in a scratch worktree and ran a differential harness over the rewritten predicates on macOS/clang and Linux/GCC 16. The performance case holds up and most of the equivalence claims are measured rather than argued. Nothing I found is a live bug. What I am asking for is small, specific, and concentrated in the comments, which is deliberate: this is header parsing on every request, and three comments state the wrong reason for why something is safe. Two of them make a load-bearing check look redundant.

Verified: the fused name scan is identical

hdrtoken_field_name_scan agrees with the code it replaces over 594,256 modelled cases of the name-handling slice of mime_parser_parse (outcome x wks_idx x field_name x field_value, including the BWS-trim branch), with 0 disagreements, plus 281,112 direct hash and validity parity checks, 0 mismatches.

Corpus: all 135 well-known strings in four case forms, every prefix and extension of every well-known name, exhaustive byte injection at every position of seven base names, and all 65,792 one- and two-byte lines over the full 256-byte alphabet. Two platforms, three locales, both remove_ws_from_field_name settings. The FNV-1a fold is the same seed, same prime, same order, no length mixing.

The colon-split rewrite is exactly equivalent too: split_prefix_at(':') returns an empty view both when there is no colon and when the colon sits at index 0, which is precisely the new colon_idx <= 0 test. I checked that against the real TextView.cc rather than a model of it.

Verified: the URL predicate is identical, and it removes undefined behavior

url_is_mostly_compliant agrees with the old isspace/isprint pair in the C locale on both platforms, over all 256 bytes and all 65,536 two-byte strings.

It diverges only on glibc under ISO-8859-1/15 locales, where the old code accepted bytes 0xA0 through 0xFE. There is no setlocale call anywhere in the tree, so a real traffic_server never leaves the C locale and that divergence is unreachable.

Worth stating in the commit message, because it cuts in your favor: char is signed on both platforms, so the old code handed isprint a negative argument for every byte at or above 0x80, which is undefined behavior. This removes that. It is a fix, not only a speedup.

Still blocking: the validate_hdr_request_target comment states a false equivalence

The comment says get_scheme() is empty iff no well-known scheme index is set and the inline scheme length is zero. That is not what get_scheme() does. It goes through make_part_view, which returns {} whenever ptr == nullptr, regardless of the length. So get_scheme() is also empty for {nullptr, 5}, where m_len_scheme == 0 is false.

I enumerated the full URLImpl field state space: 864 states, 54 disagreements with the old code, every one requiring m_ptr_* == nullptr while m_len_* != 0, in both directions. A null host with nonzero length flips DONE to ERROR. A null scheme with nonzero length flips ERROR to DONE, which turns a rejection into an acceptance.

Restricted to states honoring ptr == nullptr implies len == 0: 378 states, 0 disagreements. That invariant does hold today. Every write to these fields goes through mime_str_u16_set, url_clear, or the guarded UrlPrintHack site in HTTP.cc. So this is not a live bug and I could not construct a request that reaches it.

I am still asking for it. validate_hdr_request_target is exported in HTTP.h, so the next caller gets no warning, and URLImpl::recompute_wks_idx three functions away in the same file guards on m_ptr_scheme != nullptr rather than on the length. The codebase already treats the pointer as the authoritative signal in exactly this situation. Test the pointer alongside the length, or state the invariant the comment is leaning on. Neither costs speed.

Still blocking: the check_for_dups skip is correct, but not for the reason given

The comment says a clear presence bit is "exactly the first negative test mime_hdr_field_find performs for an interned name." At this call site the name is not interned: field->m_ptr_name points into the network read buffer, so hdrtoken_is_wks(field_name.data()) is false and mime_hdr_field_find takes the linear walk, which never consults presence bits. The path the comment cites is not the path this code takes.

The optimization is still sound, for a different reason. Presence bits here are exact, not conservative: mime_hdr_field_detach re-sets the bit for next_dup when a duplicate survives and only unsets it when the last one goes, and MIME_HDR_SANITY_CHECK asserts masksum == mh->m_presence_bits. Bit clear therefore means no live field with that name, so the linear walk would have returned null too.

That exactness is the load-bearing invariant and it is currently unstated. Anyone who later relaxes presence bits to conservative, which is an obvious optimization (skip the unset on detach, accept false positives), breaks this fast path silently, and the comment points them away from the hazard rather than at it.

Still blocking: the tail-append comment describes the wrong function

mime_field_create hands out m_field_slots[m_freetop] and then increments, so field sits at m_freetop - 1 and its predecessor at m_freetop - 2.

The call above it is mime_field_create_for_name, not mime_field_create. It delegates to mime_field_create only when the tail block has room. When the tail block is full it walks mh->m_free_slot and can return a reused, previously detached slot without touching m_freetop at all. The stated arithmetic is a property of one branch of the function actually called, and the comment omits the other.

What excludes that case is the explicit pointer comparison below, &tail_fblock->m_field_slots[tail_fblock->m_freetop - 1] == field. As written, the comment makes that check read as a restatement of arithmetic the reader has just been told is obviously true, which invites a maintainer to delete it as belt and suspenders. It is load-bearing. Please say so.

Still blocking: the default URI-parsing mode lost its diagnostics

Both Dbg lines naming the offending byte are gone from url_is_mostly_compliant. That is the function strict_uri_parsing = 2 runs, and 2 is the default. url_is_strictly_compliant, which almost nobody runs, still logs.

So an operator debugging "why is ATS rejecting this client's request" now gets the byte in the mode nobody uses and silence in the mode everybody uses. This is recoverable for free: the reject path is cold, so a reporting loop behind if (bad != 0) costs nothing on the accept path and keeps the branch-free scan intact.

Still blocking: the MIME fast path needs an ASAN run

All 14 CI checks pass, including all four AuTest shards, and I am not disputing any of that. My point is narrower, and it is about my own coverage rather than a claim that CI is wrong.

The fast_tail_append splice is the one part of this PR I could not verify in isolation, and nothing in the measurements above speaks to it. It needs a live HdrHeap, MIMEFieldBlockImpl, m_presence_bits, recompute_cooked_stuff and the slot accelerators, so a standalone harness cannot reach it. It is also the riskiest change here: it reaches into field-chain invariants and reasons explicitly about a freed HdrHeap coming back from the allocator at the same address in the HTTP/2 trailer path. That hazard is what ASAN is good at and what a passing functional suite is not.

Please run the MIME unit tests and the autests under ASAN before this merges.

Non-blocking: the new URL test is a tautology

url_mostly_compliant_reference in test_URL.cc is (c - 0x21u) > 0x5Du, which is the new semantics restated. It compares new against new and cannot catch a behavior change in either direction. I confirmed it agrees with the implementation over all 65,792 one- and two-byte inputs, which is what it is built to do.

The comparison that would catch a regression is old against new, which is the one I ran by hand above. Consider keeping a copy of the old isspace/isprint pair in the test as the reference, so the next person to touch this gets that check for free.

Non-blocking: the parity test exercises hash parity in 9 of its 15 cases

hdrtoken_tokenize_prehashed(...) == hdrtoken_tokenize(...) is only meaningful when the name resolves to a real index. For X-Custom-Header, sec-ch-ua, sec-fetch-mode, priority, X-My-Header and a, neither side is in the token table, so both return -1 whatever hash you feed in, and a wrong hash would pass. The nine live cases cover six distinct well-known names.

Driving that one assertion over the whole _hdrtoken_strs table would be a few lines and would turn a spot check into a proof. The code is right, as the harness above shows; the test does not yet demonstrate it.

Non-blocking: the fast tail append is the only attach path without the sanity check

mime_hdr_field_attach opens and closes with MIME_HDR_SANITY_CHECK. The fast path bypasses attach entirely and so never runs it. That is the one place a broken dup chain would now go undetected in a debug build, and it is the code that most needs the check.

Non-blocking: two smaller items and two wording nits

hdrtoken_tokenize_prehashed is a new extern that skips the hdrtoken_is_wks() interned-pointer check. Safe at its one call site, and the comment says why, but it is a sharp edge for any future caller passing an interned string. An ink_assert(!hdrtoken_is_wks(name)) would make it self-enforcing.

parsed.size() > INT_MAX returning ParseResult::ERROR is new behavior rather than a refactor. The guard is right and practically unreachable, but it belongs in the commit message.

In the URL.cc comment, "ATS builds -O3" is build-type dependent: -O3 for RelWithDebInfo and Release, not for Debug or ASAN builds, which run this scalar. "Equivalent to the previous isspace()/isprint() pair" is true as evaluated in the C locale, which is worth stating because it is what makes the substitution safe.

Agreed: what is good here

MimeParserTailAppendEquivalence is a real differential test. It builds the same field sequence through the parser and through explicit create-plus-attach, then compares dup chain shape, dup-head flag, well-known index, field count and presence bits across five scenarios. That is the right way to test an "equivalent to the slow path" claim, and it is the kind of test usually missing from a change like this.

MimeParserReuseAcrossHeaders targets the specific hazard its comment names rather than a generic happy path.

The oversized name/value rejection in mime_field_name_value_set checks before either branch mutates the field, so a rejected set is all-or-nothing rather than half-stored. Right shape, and the comment explains it accurately.

Happy to re-review quickly once the comments and the diagnostics are sorted.

[bryancall]

Benchmark: base vs head, measured

Separate from my review, here are A/B numbers for this PR. Posting them because the change is 70 days old and a measured result is more useful than another opinion.

How the arms were built

The benchmark source only exists on this branch, so comparing head's benchmark against anything else risks measuring the harness rather than the parser. Both arms therefore carry byte-identical benchmark source and differ only in src/proxy/hdrs:

• BASE = merge-base 9c69fe1bcc plus one commit that is git checkout pr13376 -- tools/benchmark/benchmark_HdrParse.cc tools/benchmark/CMakeLists.txt. The head version of those files was taken deliberately, not the version from b00bdec54, because ccc437f95 later rewrote the --iters argv parsing.
• HEAD = 7e9255ff4 as-is.

$ git diff --stat 9c69fe1bcc..base-with-bench
 tools/benchmark/CMakeLists.txt        |   6 +
 tools/benchmark/benchmark_HdrParse.cc | 813 +++++++++++++++++++++
 2 files changed, 819 insertions(+)

$ git diff --stat 9c69fe1bcc..base-with-bench -- src/proxy/hdrs/ include/proxy/hdrs/
(empty)

$ git diff --stat base-with-bench pr13376 -- tools/benchmark/
(empty)

Build and invocation, so this is repeatable:

cmake --preset release -DENABLE_BENCHMARKS=ON -DBUILD_EXPERIMENTAL_PLUGINS=OFF
cmake --build build-release --target benchmark_HdrParse -j 28

taskset -c 4 ./build-release/tools/benchmark/benchmark_HdrParse "[bench]" \
    --benchmark-samples 200 --order decl --rng-seed 1 --reporter xml

GCC 16.2.1, CMAKE_CXX_FLAGS_RELEASE=-O3 -DNDEBUG, no sanitizers, both arms clean with zero warnings. Pinned to one core, fixed order and RNG seed, 7 rounds strictly interleaved base/head/base/head. Load average during the runs was 1.0 to 1.1, which is essentially just the pinned process.

Results, ns/op, median of 7 interleaved runs

case	base	head	delta	worst-arm spread
url: realistic	217.38	51.66	-76.2%	13.6%
response: 50 dup fields	2546.03	1558.11	-38.8%	3.0%
request: realistic (zero-copy)	367.62	322.17	-12.4%	3.1%
request: realistic (copy)	449.94	403.36	-10.3%	2.9%
request: modern browser (zero-copy)	643.86	593.93	-7.8%	5.9%
mime: 50 wks-miss fields	4142.33	3907.70	-5.7%	15.7%
request: 100 fields	12344.90	12071.50	-2.2%	9.0%
response: realistic	320.85	316.54	-1.3%	5.5%
mime: realistic fields	295.04	291.39	-1.2%	3.9%
wks: tokenize all field names	2124.73	2146.28	+1.0%	0.7%
wks: tokenize, lowercased (H2)	2124.30	2147.43	+1.1%	0.8%

Per-run figures for the cases that carry the result, so you can judge the spread yourself:

• url: realistic base 191.2 192.9 194.6 217.4 218.9 220.5 220.7, head 50.6 51.2 51.5 51.7 52.0 53.0 53.1. Base is visibly bimodal, two clusters about 14% apart, which I did not chase down. Even against base's fastest cluster the head is about 73% faster.
• response: 50 dup fields base 2509 2519 2530 2546 2549 2549 2585, head 1551 1554 1556 1558 1561 1561 1578. No overlap at all.
• request: realistic (zero-copy) base 362.3 to 373.8, head 317.4 to 325.7. No overlap.
• request: modern browser base min 625.2 is above head max 603.6, so despite base's 5.9% spread the distributions do not overlap.
• mime: 50 wks-miss fields base had one outlier run at 4654.9 inflating the spread. Excluding it: base 4006 to 4252, head 3848 to 4028. The win is likely real but I would not quote a precise figure.

The three cases the PR does not touch move 1 to 2%, which is inside noise. That is the control you want to see.

One measured regression

The standalone WKS tokenize entry point is consistently slower. Since 1% is close to noise, those two cases were re-run alone, 10 interleaved rounds each, 300 samples, on a different core:

taskset -c 6 ./benchmark_HdrParse "[wks]" --benchmark-samples 300 --order decl --rng-seed 1 --reporter xml

• wks: tokenize all field names base median 2129.24 (2127.1 to 2137.8), head median 2153.07 (2143.1 to 2157.6), +1.12%, zero distribution overlap
• wks: tokenize, lowercased (H2 form) base median 2129.38 (2125.2 to 2137.0), head median 2153.78 (2152.1 to 2179.7), +1.15%, zero distribution overlap

So: a reproducible ~1.1% slowdown on standalone hdrtoken_tokenize, across 20 interleaved rounds on two different cores with no overlap between arms. Inferred rather than measured: this is most likely the fused scan-and-hash trading standalone tokenize cost for the fused parse path, which would be consistent with the large wins on exactly the cases that go through that path. I did not profile to confirm the attribution, so treat the cause as a hypothesis and the effect as measured.

It is dwarfed by the wins and I am not asking for anything about it. It is worth knowing it exists.

Unit tests, Release build, head arm

test_proxy_hdrs passes: 457,223 assertions in 62 test cases. Base for reference: 456,999 assertions in 57 cases, so this PR adds 5 cases.

What these numbers do not support

• Real-world impact. This is a microbenchmark of src/proxy/hdrs in isolation. It says nothing about what fraction of a real transaction is header parsing. No h2load or end-to-end proxy run was done.
• Per-commit attribution. Base against head only, not bisected. The URL and dup-field attributions above are inferred from reading the diffs.
• Other compilers. GCC 16.2.1 only. The URL win depends on the compiler auto-vectorizing the new branchless loop, so clang or an older GCC could differ in magnitude. Your comment claims both clang and GCC vectorize at -O3; I verified the GCC half by measurement rather than by reading the generated assembly, and separately confirmed the clang half by compiling the loop standalone at -O3 for arm64 and x86-64, where -Rpass=loop-vectorize reports vectorization width 16 in both.
• Loaded behavior. Single pinned core, single threaded, no cache or memory-bandwidth pressure.

[moonchen]

validate_hdr_request_target — Fixed in d83aaa6: tests the pointer along with the length, so it now matches the getters it replaced exactly.

check_for_dups comment — The original reasoning holds. mime_hdr_field_attach searches with field->name_get(), which returns the interned WKS string whenever m_wks_idx >= 0, not m_ptr_name. So mime_hdr_field_find does take the WKS branch, and its first negative test is this presence bit with the same mask (hdrtoken_str_masks[i] is built from wks_info.mask). Reworded the comment in f58829f to say that directly.

Tail-append comment — Rewritten in 0fe3be7; it now says the pointer check is required because mime_field_create_for_name() can reuse an older slot.

URL diagnostics — I agree that a rejection-only scan would preserve the vectorizable loop. I'd still prefer to leave these diagnostics removed: identifying the offending byte doesn't provide enough troubleshooting value to justify the additional scan and logging logic in this frequently executed validator.

ASAN — test_proxy_hdrs is clean under ASAN. Autests under ASAN with leak detection off: h2_origin_trailers_h1, http2, http2_crlf_header_validation, chunked_encoding, chunked_encoding_h2, chunk_trailer_bare_lf, chunked_not_last, bad_chunked_encoding. There were no AddressSanitizer reports. One http2 case failed locally only because its curl command uses |&, which dash rejects.

Non-blocking:

• The URL test now uses the old isspace/isprint pair as its reference.
• The parity test now sweeps the whole WKS table in three case forms.
• hdrtoken_tokenize_prehashed now asserts !hdrtoken_is_wks(string).
• The URL.cc comment block is removed.
• MIME_HDR_SANITY_CHECK is compiled out in every build (ENABLE_MIME_SANITY_CHECK is never defined), so adding it to the fast path would be a no-op. Left as is.
• Left the INT_MAX guard out of the commit message; a multi-gigabyte header line isn't worth the noise.

[Copilot]

Copilot review overview

🟡 Changes recommended

Add the missing <cctype> include and address the remaining duplicate-chain lookup.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Add missing <cctype> include for toupper and tolower · New

[bryancall]

✅ Approve

Thank you for the careful reply. You fixed everything I asked for except one item you declined for reasons I accept, and you were right on two points where I was wrong. Details below, including those corrections, because the record should show them.

I reviewed the delta since my last review, 7 files and +161/-38, and a reviewer built the head and ran test_proxy_hdrs: 458,218 assertions in 62 test cases, all passing. Each test change was also checked by breaking the code it covers and confirming the test fails.

Verified: validate_hdr_request_target

host_present and scheme_absent now test the pointer along with the length, which matches get_host() and get_scheme() through make_part_view() exactly. The function no longer depends on the invariant that a null pointer always comes with a zero length. The comment is now accurate.

Agreed: the check_for_dups comment was right, and my objection was wrong

mime_hdr_field_attach() searches with field->name_get(), and name_get() returns hdrtoken_index_to_wks(m_wks_idx) whenever m_wks_idx >= 0. So mime_hdr_field_find() does take the well-known branch, and its first test is the same presence bit. I read the m_ptr_name field and missed the substitution in name_get(). My argument was also wrong in a second way: skipping on a clear bit only needs presence bits to have no false negatives, so even conservative bits would have been safe. The reworded comment is accurate.

Verified: the tail-append comment

It now names mime_field_create_for_name() and explains the slot reuse, which matches the free-list path. One small correction of my own: I called the pointer comparison load-bearing. Today the allocator only reuses a free slot that sits after the name's last duplicate, so the check guards against a future allocator change rather than a case that can occur now. It is still worth keeping; "guards against" would be slightly more precise than "required", but that is a nit.

Agreed: the ASAN run

test_proxy_hdrs and the trailer and chunked autests you listed are the right coverage for the fast path, in particular h2_origin_trailers_h1 for the reused-heap case.

Agreed: the sanity check is compiled out

MIME_HDR_SANITY_CHECK expands to (void) unless ENABLE_MIME_SANITY_CHECK is defined, and nothing in the tree defines it. My note that the fast path skipped a check that would catch a broken chain in a debug build was wrong.

Non-blocking: the URL diagnostics

I accept your decision to leave them out. Those were Dbg lines behind a debug tag, not production logging, and my earlier description of what an operator loses overstated it.

If you ever want them back, they can cost nothing on either path. DbgCtl::on() exists, so a report loop behind if (bad != 0 && dbg_ctl_http.on()) runs only when a request is rejected and the tag is enabled, and the scan loop stays branch-free.

Related, and worth one line: removing the whole comment block also removed the only explanation of why the loop has no early exit. The obvious way to restore the diagnostics, a Dbg and return false inside the loop, would quietly stop it vectorizing, and nothing now warns against that.

Non-blocking: the token-table sweep compares the tokenizer with itself

The sweep is a real improvement and catches things the fifteen hand-picked names cannot: a hash that goes wrong only past 17 bytes, and a case-folding error on a letter none of those names contain.

It has one blind spot. hdrtoken_tokenize() now ends with return hdrtoken_tokenize_prehashed(...), so both sides of CHECK(hdrtoken_tokenize_prehashed(...) == hdrtoken_tokenize(...)) go through the same function and fail together. For example, a change that made the prehashed lookup return -1 for all-uppercase well-known names of eight or more characters passes the sweep and the whole suite. TRANSFER-ENCODING is one of those names, and it is one the parser's framing depends on.

Comparing against the known index closes it:

CHECK(hdrtoken_tokenize(name.data(), name_len) == idx);

It passes at head and fails on that change.

Non-blocking: cooked header state on the fast path is untested

Deleting the recompute_cooked_stuff(field) call on the tail-append path leaves the whole suite passing, because MimeParserTailAppendEquivalence never uses a cooked header. With the call removed, Cache-Control: no-cache followed by Cache-Control: max-age=5 loses the max-age. A Cache-Control scenario in the equivalence test, comparing get_cooked_cc_mask() and get_cooked_cc_max_age(), would pin it.

Non-blocking: three benchmark tool items

These are in the standalone tool, not the server, and the numbers I posted came from the built-in cases, which check their results.

• In profile mode an unrecognized argument is dropped, so --profile url --iters=1000 runs the default 5,000,000 iterations and exits 0. Rejecting leftover arguments in profile mode would catch that.
• drive_mime passes remove_ws_from_field_name = false while the response parser passes true, so a corpus response with Server : x makes the new mime: corpus benchmark time a parse that stopped early. Nothing in that path checks the parse result.
• The file header says each target gets a corpus benchmark; the wks target does not.

Non-blocking: test_mime.cc needs <cctype>

Copilot is right about this one. The case-parity test calls toupper and tolower (test_mime.cc:297, :300), but the file includes only <cstdio>, <climits>, <cstdint> and <cstring> from the C library, so it compiles only because some other header pulls <cctype> in. Add #include <cctype> next to the others.

Agreed: the rest

The URL test now uses the old isspace and isprint pair as its reference, passed an unsigned char, with the C locale reasoning written down. The ink_assert(!hdrtoken_is_wks(string)) makes the prehashed entry point enforce its own precondition. Leaving the INT_MAX guard out of the commit message is fine.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 3 · 1

Open (4)

• hdrtoken_tokenize_prehashed is now part of the public header API but will ink_assert if called… · New
• The validation step casts uc back to char, which reintroduces signedness-dependent behavior for… · New
• std::filesystem::directory_iterator increment (++it) and is_regular_file() can throw on… · New
• These calls use unqualified isspace/isprint after including <cctype>. For… · New

Resolved since last review (1)

• Add missing <cctype> include for toupper and tolower