moonchen commented on code in PR #13376:
URL: https://github.com/apache/trafficserver/pull/13376#discussion_r4188013266
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -674,21 +675,15 @@ hdrtoken_method_tokenize(const char *string, int
string_len)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
+// WKS lookup for a name whose FNV-1a hash the caller has already computed
+// (e.g. fused into the field-name scan). Matches by slot, hash, and length
like
+// hdrtoken_tokenize, but skips the interned-pointer test, so it is only valid
+// for a non-interned `string`.
int
-hdrtoken_tokenize(const char *string, int string_len, const char
**wks_string_out)
+hdrtoken_tokenize_prehashed(const char *string, int string_len, uint32_t hash,
const char **wks_string_out)
{
ink_assert(string != nullptr);
-
- if (hdrtoken_is_wks(string)) {
- int const wks_idx = hdrtoken_wks_to_index(string);
-
- if (wks_string_out) {
- *wks_string_out = string;
- }
- return wks_idx;
- }
-
- uint32_t const hash = hdrtoken_hash(std::string_view{string,
static_cast<size_t>(string_len)});
+ ink_assert(!hdrtoken_is_wks(string));
Review Comment:
Keeping the assert. It enforces the precondition. Handling interned pointers
here would add back the branch that the fast path skips, and
`hdrtoken_tokenize` handles them before it delegates.
##########
src/proxy/hdrs/HdrToken.cc:
##########
@@ -708,6 +703,59 @@ hdrtoken_tokenize(const char *string, int string_len,
const char **wks_string_ou
return -1;
}
+/*-------------------------------------------------------------------------
+ -------------------------------------------------------------------------*/
+
+// Single-pass field-name scan for the MIME parser. Scans up to `maxlen` bytes
+// of `string` for the ':' delimiter while, in the same pass, accumulating the
+// FNV-1a name hash (identical to hdrtoken_hash) and tracking whether every
byte
+// before ':' is a valid HTTP field-name char. Returns the index of ':' (i.e.
+// the field-name length) or -1 if no ':' appears within `maxlen`. `*hash_out`
+// and `*all_valid_out` describe the bytes scanned before ':' (or all `maxlen`
+// bytes when ':' is absent); both are required.
+int
+hdrtoken_field_name_scan(const char *string, int maxlen, uint32_t *hash_out,
bool *all_valid_out)
+{
+ uint32_t hval = HDRTOKEN_HASH_SEED; // same FNV-1a name hash as
hdrtoken_hash
+ bool all_valid = true;
+ int i = 0;
+
+ for (; i < maxlen; ++i) {
+ unsigned char const uc = static_cast<unsigned char>(string[i]);
+ if (uc == ':') {
+ break;
+ }
+ hval = (hval ^ hdrtoken_ascii_toupper(uc)) * 0x01000193u;
+ all_valid &= (ParseRules::is_http_field_name(static_cast<char>(uc)) != 0);
Review Comment:
Not a bug: `ParseRules::is_http_field_name` indexes with
`static_cast<unsigned char>(c)`, so the round trip through `char` is lossless.
##########
src/proxy/hdrs/unit_tests/test_URL.cc:
##########
@@ -172,6 +173,68 @@ TEST_CASE("ParseRulesMostlyStrictURI", "[proxy][parseuri]")
CHECK(url_is_mostly_compliant(i.uri, i.uri + strlen(i.uri)) == i.valid);
}
+namespace
+{
+// The isspace()/isprint() pair that url_is_mostly_compliant replaced, kept as
+// the reference so this compares old behavior against new instead of restating
+// the new range test. Bytes are passed as unsigned char; the original passed a
+// plain char, which is undefined for the negative values every byte >= 0x80
+// produces. In the C locale, which ATS never leaves (nothing in the tree calls
+// setlocale), this accepts exactly 0x21..0x7E.
+bool
+url_mostly_compliant_reference(const char *start, const char *end)
+{
+ for (const char *p = start; p < end; ++p) {
+ unsigned char const c = static_cast<unsigned char>(*p);
+ if (isspace(c) || !isprint(c)) {
Review Comment:
Correct as written. I will switch these to `std::` in a follow-up to match
`test_mime.cc`.
##########
tools/benchmark/benchmark_HdrParse.cc:
##########
@@ -0,0 +1,927 @@
+/** @file
+
+ Micro-benchmark for HTTP header parsing (src/proxy/hdrs).
+
+ Two modes in one binary:
+
+ * A/B stats mode (default): Catch2 BENCHMARK cases produce mean/median/
+ stddev per (target x corpus) so an optimization can be measured before
+ and after and guarded against regression. Run e.g.:
+ benchmark_HdrParse "[bench]" --benchmark-samples 100
+
+ * Profiling mode (--profile <target>): a tight fixed-count loop with no
+ Catch2 harness overhead, meant to be wrapped by perf/vtune:
+ perf stat -e cycles,instructions \
+ benchmark_HdrParse --profile request --iters 5000000
+
+ Targets: request, response, mime, url, wks.
+
+ A realistic + adversarial corpus is built in. Real captured header blocks can
+ be supplied with --corpus-file FILE or --corpus-dir DIR (blocks split on a
+ blank line, classified request vs response by the first line); these are
added
+ to both modes, and each loaded request's request-target also feeds the url
+ target. In stats mode the request, response, mime, and url targets also get a
+ "corpus" benchmark that times one pass over the loaded cases alone, so its
+ result reflects only the supplied traffic. The wks benchmarks combine field
+ names from the built-in and loaded cases.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "proxy/hdrs/HTTP.h"
+#include "proxy/hdrs/MIME.h"
+#include "proxy/hdrs/URL.h"
+#include "proxy/hdrs/HdrToken.h"
+#include "proxy/hdrs/HdrHeap.h"
+
+#include <charconv>
+#include <chrono>
+#include <cstdint>
+#include <cstdio>
+#include <cstring>
+#include <filesystem>
+#include <fstream>
+#include <sstream>
+#include <string>
+#include <string_view>
+#include <utility>
+#include <vector>
+
+#define CATCH_CONFIG_ENABLE_BENCHMARKING
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/catch_session.hpp>
+#include <catch2/benchmark/catch_benchmark.hpp>
+
+// Defined in tscore; disables thread-local proxy allocators so no event-system
+// thread setup is required (mirrors the hdrs unit-test main).
+extern int cmd_disable_pfreelist;
+
+namespace
+{
+// ----------------------------------------------------------------------------
+// Corpus
+// ----------------------------------------------------------------------------
+
+struct HeaderCase {
+ std::string label;
+ std::string data; // full block including the start line, ends "\r\n\r\n"
+ bool is_response = false;
+};
+
+struct UrlCase {
+ std::string label;
+ std::string data; // a bare request target
+};
+
+struct Corpus {
+ std::vector<HeaderCase> cases;
+ std::vector<std::string> urls; // bare request targets for the url
target
+ std::vector<UrlCase> file_urls; // request targets of the file-loaded
requests
+ std::vector<std::string> wks; // field names (owned) for the wks target
+ // The same names lowercased. HTTP/2 mandates lowercase field names while the
+ // well-known-string table holds canonical mixed case, so roughly half of
real
+ // edge traffic reaches the well-known lookup folded. Without this the wks
+ // target only ever measures the canonical-case path.
+ std::vector<std::string> wks_lower;
+};
+
+// A representative modern browser request and typical responses, alongside the
+// classic fixtures used by the hdrs unit tests.
+constexpr std::string_view REQ_REALISTIC =
+ "GET /assets/app.9f2c.js HTTP/1.1\r\n"
+ "Host: www.example.com\r\n"
+ "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36
(KHTML, like Gecko) "
+ "Chrome/126.0.0.0 Safari/537.36\r\n"
+ "Accept:
text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8\r\n"
+ "Accept-Encoding: gzip, deflate, br, zstd\r\n"
+ "Accept-Language: en-US,en;q=0.9\r\n"
+ "Cookie: session=8f14e45fceea167a5a36dedd4bea2543; theme=dark;
region=us-west-2; ab_bucket=37\r\n"
+ "Referer: https://www.example.com/\r\n";
+ "Connection: keep-alive\r\n"
+ "\r\n";
+
+// A 2026 Chrome navigation request. Unlike REQ_REALISTIC (all classic WKS
+// names), this carries the modern client-hint / fetch-metadata headers
+// (sec-ch-ua*, sec-fetch-*, priority, upgrade-insecure-requests) that are NOT
+// well-known strings, so it exercises the non-WKS name paths (tokenize miss,
+// name validation, duplicate filter) the way real traffic does.
+constexpr std::string_view REQ_MODERN =
+ "GET /app/feed?tab=home HTTP/1.1\r\n"
+ "Host: www.example.com\r\n"
+ "Connection: keep-alive\r\n"
+ "sec-ch-ua: \"Chromium\";v=\"126\", \"Google Chrome\";v=\"126\",
\"Not-A.Brand\";v=\"99\"\r\n"
+ "sec-ch-ua-mobile: ?0\r\n"
+ "sec-ch-ua-platform: \"Windows\"\r\n"
+ "upgrade-insecure-requests: 1\r\n"
+ "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36
(KHTML, like Gecko) "
+ "Chrome/126.0.0.0 Safari/537.36\r\n"
+ "Accept:
text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8\r\n"
+ "Sec-Fetch-Site: same-origin\r\n"
+ "Sec-Fetch-Mode: navigate\r\n"
+ "Sec-Fetch-User: ?1\r\n"
+ "Sec-Fetch-Dest: document\r\n"
+ "Accept-Encoding: gzip, deflate, br, zstd\r\n"
+ "Accept-Language: en-US,en;q=0.9\r\n"
+ "Cookie: session=8f14e45fceea167a5a36dedd4bea2543; theme=dark;
region=us-west-2; ab_bucket=37\r\n"
+ "Priority: u=0, i\r\n"
+ "\r\n";
+
+constexpr std::string_view REQ_CLASSIC = "GET http://www.news.com:80/
HTTP/1.0\r\n"
+ "Proxy-Connection: Keep-Alive\r\n"
+ "User-Agent: Mozilla/4.04 [en] (X11;
I; Linux 2.0.33 i586)\r\n"
+ "Pragma: no-cache\r\n"
+ "Host: www.news.com\r\n"
+ "Accept: image/gif, image/x-xbitmap,
image/jpeg, image/pjpeg, image/png, */*\r\n"
+ "Accept-Language: en\r\n"
+ "Accept-Charset: iso-8859-1, *,
utf-8\r\n"
+ "\r\n";
+
+constexpr std::string_view RESP_REALISTIC = "HTTP/1.1 200 OK\r\n"
+ "Server: ATS/10.1.0\r\n"
+ "Date: Mon, 21 Oct 2013 20:13:21
GMT\r\n"
+ "Content-Type: text/html;
charset=utf-8\r\n"
+ "Content-Length: 12345\r\n"
+ "Cache-Control: max-age=31536000,
public, immutable\r\n"
+ "Vary: Accept-Encoding\r\n"
+ "Age: 42\r\n"
+ "\r\n";
+
+constexpr std::string_view RESP_304 = "HTTP/1.1 304 Not Modified\r\n"
+ "Date: Mon, 21 Oct 2013 20:13:21 GMT\r\n"
+ "Etag: \"6f2c9a1b\"\r\n"
+ "Cache-Control: max-age=31536000\r\n"
+ "\r\n";
+
+constexpr std::string_view URL_REALISTIC =
"http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75";;
+
+// Build the adversarial cases programmatically so their worst-case sizes are
+// obvious and easy to tune.
+std::string
+gen_many_fields(int n)
+{
+ std::string s = "GET /many HTTP/1.1\r\nHost: h\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "X-Custom-Header-" + std::to_string(i) + ": value-" +
std::to_string(i) + "\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+std::string
+gen_long_value(int len)
+{
+ std::string s = "GET /long HTTP/1.1\r\nHost: h\r\nX-Blob: ";
+ s.append(len, 'a');
+ s += "\r\n\r\n";
+ return s;
+}
+
+// Field names that are guaranteed not to be well-known, forcing the
+// hdrtoken_tokenize hash miss + per-char field-name validation path.
+std::string
+gen_wks_miss(int n)
+{
+ std::string s = "GET /miss HTTP/1.1\r\nHost: h\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "X-Zzq-Nonstandard-Field-" + std::to_string(i) + ": v\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+// Many duplicates of a well-known, commonly-repeated field: stresses
+// mime_hdr_field_attach duplicate chaining.
+std::string
+gen_dup_fields(int n)
+{
+ std::string s = "HTTP/1.1 200 OK\r\nDate: Mon, 21 Oct 2013 20:13:21 GMT\r\n";
+ for (int i = 0; i < n; ++i) {
+ s += "Set-Cookie: c" + std::to_string(i) + "=v" + std::to_string(i) + ";
Path=/; HttpOnly\r\n";
+ }
+ s += "\r\n";
+ return s;
+}
+
+std::string
+gen_long_uri(int len)
+{
+ std::string s = "GET http://www.example.com/";;
+ s.append(len, 'x');
+ s += " HTTP/1.1\r\nHost: www.example.com\r\n\r\n";
+ return s;
+}
+
+// The MIME-only target parses a field block with the start line removed.
+std::string_view
+strip_start_line(std::string_view block)
+{
+ auto pos = block.find('\n');
+ return pos == std::string_view::npos ? block : block.substr(pos + 1);
+}
+
+// Collect the field names in a block (for the wks target). Skips the start
line,
+// continuation lines, and lines without a colon.
+void
+collect_field_names(std::string_view block, std::vector<std::string> &out)
+{
+ std::string_view rest = strip_start_line(block);
+ size_t pos = 0;
+ while (pos < rest.size()) {
+ size_t eol = rest.find('\n', pos);
+ std::string_view line = rest.substr(pos, (eol == std::string_view::npos ?
rest.size() : eol) - pos);
+ pos = (eol == std::string_view::npos) ? rest.size() :
eol + 1;
+ if (line.empty() || line == "\r" || line.front() == ' ' || line.front() ==
'\t') {
+ continue; // blank terminator or folded continuation
+ }
+ size_t colon = line.find(':');
+ if (colon != std::string_view::npos && colon > 0) {
+ out.emplace_back(line.substr(0, colon));
+ }
+ }
+}
+
+// Load raw header blocks from a file. Multiple blocks may be separated by a
+// blank line. Line endings are normalized and each block re-terminated with a
+// canonical "\r\n\r\n".
+void
+load_file(const std::filesystem::path &path, std::vector<HeaderCase> &out)
+{
+ std::ifstream in(path, std::ios::binary);
+ if (!in) {
+ std::fprintf(stderr, "warning: cannot open corpus file %s\n",
path.string().c_str());
+ return;
+ }
+ std::stringstream ss;
+ ss << in.rdbuf();
+ std::string content = ss.str();
+
+ // Normalize CRLF -> LF, then split blocks on a blank line ("\n\n").
+ std::string norm;
+ norm.reserve(content.size());
+ for (char ch : content) {
+ if (ch != '\r') {
+ norm += ch;
+ }
+ }
+
+ int idx = 0;
+ size_t pos = 0;
+ while (pos < norm.size()) {
+ size_t sep = norm.find("\n\n", pos);
+ size_t end = (sep == std::string::npos) ? norm.size() : sep;
+ std::string_view chunk = std::string_view(norm).substr(pos, end - pos);
+ // Trim leading/trailing newlines.
+ while (!chunk.empty() && chunk.front() == '\n') {
+ chunk.remove_prefix(1);
+ }
+ while (!chunk.empty() && chunk.back() == '\n') {
+ chunk.remove_suffix(1);
+ }
+ if (!chunk.empty()) {
+ // Re-insert canonical CRLF line endings and terminator.
+ std::string block;
+ for (size_t i = 0; i < chunk.size(); ++i) {
+ if (chunk[i] == '\n') {
+ block += "\r\n";
+ } else {
+ block += chunk[i];
+ }
+ }
+ block += "\r\n\r\n";
+ HeaderCase c;
+ c.label = path.filename().string() + "#" + std::to_string(idx++);
+ c.is_response = block.rfind("HTTP/", 0) == 0; // status line starts with
"HTTP/"
+ c.data = std::move(block);
+ out.push_back(std::move(c));
+ }
+ if (sep == std::string::npos) {
+ break;
+ }
+ pos = sep + 2;
+ }
+}
+
+// The request-target of a request block's "METHOD SP target SP version" start
+// line, or empty if the start line doesn't have exactly that shape.
+std::string_view
+request_target(std::string_view block)
+{
+ std::string_view const line = block.substr(0, block.find("\r\n"));
+ size_t const first = line.find(' ');
+
+ if (first == std::string_view::npos) {
+ return {};
+ }
+ size_t const second = line.find(' ', first + 1);
+ if (second == std::string_view::npos || second == first + 1 || line.find('
', second + 1) != std::string_view::npos) {
+ return {};
+ }
+ return line.substr(first + 1, second - first - 1);
+}
+
+Corpus
+build_corpus(const std::vector<std::filesystem::path> &files, const
std::vector<std::filesystem::path> &dirs)
+{
+ Corpus corp;
+ auto add = [&](std::string_view label, std::string_view data, bool
is_resp) {
+ corp.cases.push_back({std::string(label), std::string(data), is_resp});
+ };
+
+ // Realistic.
+ add("req_realistic", REQ_REALISTIC, false);
+ add("req_modern", REQ_MODERN, false);
+ add("req_classic", REQ_CLASSIC, false);
+ add("resp_realistic", RESP_REALISTIC, true);
+ add("resp_304", RESP_304, true);
+
+ // Adversarial.
+ add("adv_many_fields", gen_many_fields(100), false);
+ add("adv_long_value", gen_long_value(8192), false);
+ add("adv_wks_miss", gen_wks_miss(50), false);
+ add("adv_dup_fields", gen_dup_fields(50), true);
+ add("adv_long_uri", gen_long_uri(4000), false);
+
+ // File-loaded.
+ size_t const builtin_count = corp.cases.size();
+
+ for (const auto &f : files) {
+ load_file(f, corp.cases);
+ }
+ for (const auto &d : dirs) {
+ std::error_code ec;
+ for (auto it = std::filesystem::directory_iterator(d, ec); !ec && it !=
std::filesystem::directory_iterator(); ++it) {
+ if (it->is_regular_file()) {
+ load_file(it->path(), corp.cases);
+ }
Review Comment:
Valid. I will fix it in a follow-up, together with `--corpus-dir` exiting 0
when the directory is missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
![http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75"](http://www.example.com/images/2026/06/some-article/hero.webp?w=1200&q=75"){kind=link}