Reworking Headers impl #5396

jasnell · 2025-10-23T01:33:47Z

Needs careful review. It does improve performance but the impl is a bit more complex. Fiddled around with a number of other perf tweaks but nothing that really moved the needle enough to justify the increased complexity. Not entirely sold on this impl so be brutal in reviews.

codspeed-hq · 2025-10-23T03:52:34Z

CodSpeed Performance Report

Merging #5396 will improve performances by 53.4%

_{Comparing jasnell/http-headers-take-2 (fb81688) with main (15ccfc8)}

Summary

⚡ 2 improvements
✅ 31 untouched
⏩ 9 skipped¹

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`constructor[ApiHeaders]`	90.8 ms	61.9 ms	+46.84%
⚡	`jsonResponse[Response]`	53.2 µs	34.7 µs	+53.4%

9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

jasnell · 2025-10-23T15:10:12Z

Internal tests are failing because the refactor changes the ordering of the headers in places... will need to see if we can preserve the order but first want to see if this has a positive impact on performance overall... waiting for benchmark results to be updated.

jasnell · 2025-10-23T16:42:12Z

Marking as ready for review but there are still some internal test updates I'll need to do on the internal repo.

npaun · 2025-10-23T17:24:05Z

src/wpt/fetch/api-test.ts

  'headers/headers-errors.any.js': {
    comment: 'Our validation of header names is too lax',
    expectedFailures: [
-      'Create headers giving bad header name as init argument',


So we did end up getting a couple of WPT improvements out of it, nice.

Yep, but we'll need to make sure this doesn't count as a breaking change that'll need a compat flag.

danlapid · 2025-10-23T17:31:50Z

Whenever this is done I want as many reviewers as we can get on this, incl Harris, Kenton, Mike, Yagiz, etc.

jasnell · 2025-10-23T21:01:42Z

src/workerd/api/headers.c++

+  other.forEach([this, &js](auto name, auto value) {
+    // We have to copy the strings here but we can avoid normalizing and validating since
+    // they presumably already went through that process when they were added to the
+    // kj::HttpHeader instance.


Verified that kj::HttpHeader is doing validation on these so this should be fine but if we're paranoid enough then we might want to just go ahead and validate here also. Just keep in mind, however, that this constructor is where most of the cost is when creating a Headers for a request.

src/workerd/api/global-scope.c++

src/workerd/api/headers.h

src/workerd/tests/bench-response.c++

src/workerd/api/headers.c++

anonrig · 2025-10-24T20:42:37Z

src/workerd/api/headers.c++

+  for (char c: name) {
+    JSG_REQUIRE(HTTP_TOKEN_CHAR_TABLE[static_cast<uint8_t>(c)], TypeError, "Invalid header name.");
  }


This can be speed up if we eliminated the branch inside this for loop

Have a suggested alternative impl?

anonrig · 2025-10-24T20:42:54Z

src/workerd/api/headers.c++

+
+inline void requireValidHeaderValue(kj::StringPtr value) {
+  for (char c: value) {
+    JSG_REQUIRE(isValidHeaderValueChar(c), TypeError, "Invalid header value.");


Speed up is possible if we eliminate branches inside this for loop.

src/workerd/api/headers.c++

In preparation to re-apply Headers performance improvements

Modify the kj::HttpHeaders instead and then construct the api::Headers from that.

While the common header names and indices are defined in the capnp header, they are unlikely to change often (if at all). To avoid the overhead of building the hash map at runtime and performing the hash lookups, we can hardcode the common header names in an array and use direct indexing to retrieve them. Should at least save a handful of CPU cycles.

Profile is dominated by the cost of constructing the object to serialize rather than the cost of the Response::json_ call itself.

jasnell · 2025-10-24T23:42:42Z

Note for reviewers: while it likely is possible to get more performance gain with additional tweaks on this, the emphasis for review should be on correctness and ensuring that the revised implementation does not introduce new bugs, etc. We can tweak additional performance knobs separately in follow up PRs.

anonrig · 2025-10-25T00:07:13Z

src/workerd/tests/bench-response.c++

  fixture->runInIoContext([&](const TestFixture::Environment& env) {
    auto& js = env.js;
+    // Prepare object to serialize. Do this outside the loop to avoid measuring its repeated
+    // construction cost. What we want to measure is just the cost of the api::Response::json_ call.


can you make these changes on a separate pull request? it will help us to generate incorrect outcomes for this pull request.

A separate commit isn't enough?

jasnell requested review from a team as code owners October 23, 2025 01:33

jasnell marked this pull request as draft October 23, 2025 01:53

jasnell force-pushed the jasnell/http-headers-take-2 branch 2 times, most recently from 7f431f6 to b94d316 Compare October 23, 2025 02:30

jasnell force-pushed the jasnell/http-headers-take-2 branch 4 times, most recently from 8191e31 to 7f84859 Compare October 23, 2025 15:09

jasnell force-pushed the jasnell/http-headers-take-2 branch 2 times, most recently from 21b09a3 to b59e107 Compare October 23, 2025 16:13

jasnell marked this pull request as ready for review October 23, 2025 16:41

jasnell force-pushed the jasnell/http-headers-take-2 branch from 7ac27f1 to 7c52fb0 Compare October 23, 2025 17:02

npaun reviewed Oct 23, 2025

View reviewed changes

jasnell force-pushed the jasnell/http-headers-take-2 branch from 0a4c8f0 to 7c52fb0 Compare October 23, 2025 19:53

jasnell requested review from harrishancock and kentonv October 23, 2025 20:58

jasnell commented Oct 23, 2025

View reviewed changes

jasnell requested a review from mikea October 23, 2025 23:15

anonrig reviewed Oct 24, 2025

View reviewed changes

jasnell added 3 commits October 24, 2025 16:16

Move Headers definition/impl to headers.h/c++

929c41b

In preparation to re-apply Headers performance improvements

Avoid mutating the api::Headers immediately after creation

4227165

Modify the kj::HttpHeaders instead and then construct the api::Headers from that.

jasnell force-pushed the jasnell/http-headers-take-2 branch from 7c52fb0 to 2e794b4 Compare October 24, 2025 23:20

jasnell added 2 commits October 24, 2025 16:40

Refactor header storage

ebc9c18

Refine the jsonResponse bench a bit more

fb81688

Profile is dominated by the cost of constructing the object to serialize rather than the cost of the Response::json_ call itself.

jasnell force-pushed the jasnell/http-headers-take-2 branch from 2e794b4 to fb81688 Compare October 24, 2025 23:40

anonrig reviewed Oct 25, 2025

View reviewed changes

Reworking Headers impl #5396

Are you sure you want to change the base?

Reworking Headers impl #5396

Conversation

jasnell commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5396 will improve performances by 53.4%

Summary

Benchmarks breakdown

Footnotes

Uh oh!

jasnell commented Oct 23, 2025

Uh oh!

jasnell commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danlapid commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jasnell commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasnell commented Oct 23, 2025 •

edited

Loading

codspeed-hq bot commented Oct 23, 2025 •

edited

Loading