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

The parsing optimizations span multiple validation and duplicate-handling paths and warrant final human review.

Review effort: Lite
Findings: None

Resolved since last review (3)

• This change removes the previous per-byte Dbg(dbg_ctl_http, ...) messages for…
• This new public helper dereferences string, hash_out, and all_valid_out unconditionally.…
• hdrtoken_tokenize takes an int string_len, but field_name.size() is size_t. In this file…

[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.

[bneradt]

[P2] Include supplied corpus cases in the timed benchmarks

Running the default stats mode with --corpus-file or --corpus-dir parses those cases once in the validation loop above, but every request benchmark below selects a built-in case by name. The response and MIME benchmarks do the same. Consequently, supplying captured traffic does not measure its request/response/MIME parsing performance, despite the harness documenting that custom corpora feed both modes. Please add timed benchmarks for the loaded cases (or a timed corpus pass), so A/B results can actually reflect the supplied traffic.

[moonchen]

Fixed in 0a79b43 — each target now also gets a corpus (N blocks) benchmark that times one pass over the loaded cases only, alongside the unchanged built-ins. Request-targets from loaded requests feed a url: corpus benchmark too.

[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