perf(request): optimize if headers are given

[tsctx]

Rationale

For performance improvement:

1. Do not call Object.keys more than once.
2. When copying the headersList, use HeadersList instead of Headers.
3. Avoid call HeadersList if init["headers"] is not undefined
4. When looping through headers, use HeadersList instead of Headers, which eliminates the sorting process and is faster.

Benchmark

// bench.js
const { Request } = require(".");
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 Request() - record", () => {
  new Request("https://example.com/post", {
    headers: headers,
  });
});

suite.add("new Request() - record without constructor", () => {
  const request = new Request("https://example.com/post", {});
  const h = request.headers
  const keys = Object.keys(headers);
  for (let i = 0; i < keys.length; ++i) {
    h.append(keys[i], headers[keys[i]]);
  } 
})

suite.add("new Request() - entires", () => {
  new Request("https://example.com/post", {
    headers: headersEntries,
  });
});

suite.add("new Request() - entires without constructor", () => {
  const request = new Request("https://example.com/post", {});
  const h = request.headers
  for (let i = 0; i < headersEntries.length; ++i) {
    h.append(headersEntries[i][0], headersEntries[i][1]);
  }
});

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

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

• v5.27.2

new Request() - record x 34,814 ops/sec ±3.33% (75 runs sampled)
new Request() - record without constructor x 64,510 ops/sec ±2.22% (85 runs sampled)
new Request() - entires x 34,316 ops/sec ±3.31% (77 runs sampled)
new Request() - entires without constructor x 61,662 ops/sec ±3.00% (79 runs sampled)

• main

new Request() - record x 41,193 ops/sec ±5.16% (75 runs sampled)
new Request() - record without constructor x 65,217 ops/sec ±2.69% (80 runs sampled)
new Request() - entires x 38,771 ops/sec ±2.78% (78 runs sampled)
new Request() - entires without constructor x 68,280 ops/sec ±2.86% (83 runs sampled)

• this patch

new Request() - record x 68,534 ops/sec ±2.08% (80 runs sampled)
new Request() - record without constructor x 69,161 ops/sec ±2.31% (83 runs sampled)
new Request() - entires x 65,713 ops/sec ±2.13% (84 runs sampled)
new Request() - entires without constructor x 67,820 ops/sec ±2.57% (79 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

[tsctx]

For test failures, see #2453.

[KhafraDev]

Don't have time to review but going against the spec is a -1 from me

[mcollina]

Does this pass the same number of wpts?

[tsctx]

@mcollina
please wait a moment

[tsctx]

@mcollina
See here

• main

[fetch]: Completed: 1453, failed: 165, success: 1288, expected failures: 164, unexpected failures: 1, skipped: 71
[FileAPI]: Completed: 406, failed: 16, success: 390, expected failures: 16, unexpected failures: 0, skipped: 3
[mimesniff]: Completed: 1898, failed: 872, success: 1026, expected failures: 872, unexpected failures: 0, skipped: 0
[xhr/formdata]: Completed: 39, failed: 0, success: 39, expected failures: 0, unexpected failures: 0, skipped: 0
[websockets]: Completed: 500, failed: 18, success: 482, expected failures: 18, unexpected failures: 0, skipped: 10

• this patch

[fetch]: Completed: 1453, failed: 165, success: 1288, expected failures: 164, unexpected failures: 1, skipped: 71
[FileAPI]: Completed: 406, failed: 16, success: 390, expected failures: 16, unexpected failures: 0, skipped: 3
[mimesniff]: Completed: 1898, failed: 872, success: 1026, expected failures: 872, unexpected failures: 0, skipped: 0
[xhr/formdata]: Completed: 39, failed: 0, success: 39, expected failures: 0, unexpected failures: 0, skipped: 0
[websockets]: Completed: 500, failed: 18, success: 482, expected failures: 18, unexpected failures: 0, skipped: 10

[tsctx]

It is no different from main.

[mcollina]

lgtm

@KhafraDev where is the spec breakage?

[KhafraDev]

Spec breakage isn't the concern here, it's not following the spec directly that worries me. If for example those lines are changed in the spec, how do we upstream the changes here? It makes maintaining it a nightmare. If the change is completely compatible with the spec, I don't see a reason that the changes couldn't be made in the fetch spec itself, and then made here.

[KhafraDev]

I have no problem with something like setting the value of Object.keys(init).length to a variable, but right now the comments from the spec are no longer what undici is doing. I promise that if we don't follow the spec directly, it'll lead to an unmaintainable mess (ie. look at basically any spec node core implements).

[mcollina]

@KhafraDev these APIs are called by a lot, making a small improvement noticeable perf-wise.

[tsctx]

The new changes lowered maintenance costs by making fewer changes while maintaining performance.
It is now better suited to the specifications than previous PR implementations.

[KhafraDev]

I don't feel comfortable landing this while the CI isn't working, but the change looks fine to me now.

[codecov-commenter]

Codecov Report

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

Comparison is base (e39a632) 85.54% compared to head (a9e108a) 85.62%.
Report is 106 commits behind head on main.

Files	Patch %	Lines
lib/handler/RetryHandler.js	73.04%	31 Missing ⚠️
lib/fetch/index.js	64.51%	11 Missing ⚠️
lib/fetch/headers.js	87.80%	5 Missing ⚠️
lib/api/readable.js	80.00%	4 Missing ⚠️
lib/client.js	92.10%	3 Missing ⚠️
lib/core/util.js	90.47%	2 Missing ⚠️
lib/fetch/request.js	86.66%	2 Missing ⚠️
lib/core/request.js	97.56%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2454      +/-   ##
==========================================
+ Coverage   85.54%   85.62%   +0.07%     
==========================================
  Files          76       77       +1     
  Lines        6858     7056     +198     
==========================================
+ Hits         5867     6042     +175     
- Misses        991     1014      +23     

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