perf(headers): Improve Headers

[tsctx]

Rationale

This eliminates the Headers performance bottleneck.

• for...of is slower than for(;;).
• Object.entries is slower than Object.keys.
• Reduced the number of variables in the function ByteString.
• When trimming HTTP whitespace, using RegExp is slow, so change to using charCodeAt instead.

Changes

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Benchmark

// bench.js
const Headers = require("./lib/fetch/headers.js").Headers;
const benchmark = require("benchmark");

const suite = new benchmark.Suite();

const headers = {
  "Content-Type": "application/json",
  "X-Content-Type": "application/json",
  Date: "Wed, 01 Nov 2023 00:00:00 GMT",
  "X-NodeJS-Version": "v21.1.0",
  "Powered-By": "NodeJS",
  "Content-Encoding": "gzip",
  "Set-Cookie": "__Secure-ID=123; Secure; Domain=example.com",
  "Content-Length": "150",
  Vary: "Accept-Encoding, Accept, X-Requested-With",
};

const headersEntries = Object.entries(headers);


suite.add("new Headers - record", () => {
  new Headers(headers);
});

suite.add("new Headers - entries", () => {
  new Headers(headersEntries);
});

suite.on("cycle", function (event) {
  console.log(String(event.target));
});

// run async
suite.run({ async: true });

• main

new Headers - record x 144,160 ops/sec ±1.91% (85 runs sampled)
new Headers - entries x 156,706 ops/sec ±1.88% (83 runs sampled)

• this patch

new Headers - record x 179,333 ops/sec ±4.49% (72 runs sampled)
new Headers - entries x 170,316 ops/sec ±2.39% (80 runs sampled)

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[KhafraDev]

How much does performance improve by only removing the regex?

I don't really like the idea of removing the for..of loops because I don't find it maintainable, in the sense that no one will block a PR that uses it over the standard for loop. I don't mind the change and I'm not asking you to revert it though, just mentioning.

[metcoder95]

It seems there's a small regression:

Branch

Main

Running once more to verify

[tsctx]

Updated benchmarks.

[codecov-commenter]

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (11765a0) 85.84%.
Report is 79 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2397      +/-   ##
==========================================
+ Coverage   85.54%   85.84%   +0.29%     
==========================================
  Files          76       76              
  Lines        6858     6911      +53     
==========================================
+ Hits         5867     5933      +66     
+ Misses        991      978      -13     

Files	Coverage Δ
index-fetch.js	100.00% <100.00%> (ø)
lib/api/api-stream.js	100.00% <100.00%> (ø)
lib/api/readable.js	90.15% <ø> (-1.39%)	⬇️
lib/core/connect.js	81.25% <100.00%> (ø)
lib/core/request.js	92.79% <100.00%> (+5.71%)	⬆️
lib/fetch/body.js	97.68% <100.00%> (ø)
lib/fetch/constants.js	97.22% <100.00%> (+0.67%)	⬆️
lib/fetch/file.js	95.45% <100.00%> (+0.05%)	⬆️
lib/fetch/global.js	100.00% <ø> (ø)
lib/fetch/response.js	97.04% <100.00%> (+1.20%)	⬆️
... and 10 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tsctx]

How much does performance improve by only removing the regex?

I don't really like the idea of removing the for..of loops because I don't find it maintainable, in the sense that no one will block a PR that uses it over the standard for loop. I don't mind the change and I'm not asking you to revert it though, just mentioning.

This is a benchmark for RegExp changes only.

new Headers - record x 160,937 ops/sec ±2.01% (85 runs sampled)
new Headers - entries x 165,845 ops/sec ±2.87% (82 runs sampled)

[KhafraDev]

so it looks like a majority of the improvement is from removing the regex and removing Object.entries?

[tsctx]

@KhafraDev
You’re right.

[tsctx]

Hello!
New improvements were added with f0aaa32.
The conversion to ByteString seems to have already been completed by the webidl converter sequence<sequence<ByteString>> or record<ByteString, ByteString>, but it is being converted again.
See the new benchmark below.
9d838c5

new Headers - record x 185,007 ops/sec ±2.16% (82 runs sampled)
new Headers - entries x 165,584 ops/sec ±1.75% (83 runs sampled)

f0aaa32

new Headers - record x 255,866 ops/sec ±2.29% (80 runs sampled)
new Headers - entries x 224,652 ops/sec ±1.87% (88 runs sampled)

[tsctx]

Hi @KhafraDev @Ethan-Arrowood,
Could you please review the code at a time that is convenient for you?

[ronag]

@KhafraDev @Ethan-Arrowood

[Ethan-Arrowood]

lgtm

[KhafraDev]

I have no time to do an actual review for the next 2 days but if the tests pass there is very little that can break.