perf: Improve normalizeMethod

[tsctx]

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

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

[ronag]

Benchmark?

[KhafraDev]

Changes look fine but lets wait for CI to be fixed before landing.

[tsctx]

Hi @ronag,
See this benchmark.

// bench.js
const { Request } = require("./lib/fetch/request");
const benchmark = require("benchmark");

const suite = new benchmark.Suite();

const input = "https://example.com/post";

const init = {
  method: "POST",
};

suite.add("new Request()", () => {
  new Request(input, init);
});

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

// run async
new Promise((resolve) => setTimeout(resolve, 7000)).then(() =>
  suite.run({ async: true })
);

• main

new Request() x 42,952 ops/sec ±5.67% (81 runs sampled)

• this patch

new Request() x 43,595 ops/sec ±3.76% (81 runs sampled)

[tsctx]

Listing ['delete', 'DELETE', 'get', 'GET', 'head', 'HEAD', 'options', 'OPTIONS', 'post', 'POST', 'put', 'PUT'] below is 2.5 times faster than the current regex implementation.
But otherwise it is about 2.1 times slower.
Most of the time, I use the ones on this list, so it's not a problem.

[ronag]

Patch is missing

[tsctx]

@ronag
Adding a PATCH is nice, but you need two objects.

[ronag]

@ronag Adding a PATCH is nice, but you need two objects.

Sorry? I don't understand.

[tsctx]

@ronag
In short, it comes down to this.

const normalizeMethodRecordBase = {
  delete: "DELETE",
  DELETE: "DELETE",
  get: "GET",
  GET: "GET",
  head: "HEAD",
  HEAD: "HEAD",
  options: "OPTIONS",
  OPTIONS: "OPTIONS",
  post: "POST",
  POST: "POST",
  put: "PUT",
  PUT: "PUT",
};

const normalizeMethodRecord = {
  ...normalizeMethodRecordBase,
  patch: "patch",
  PATCH: "PATCH",
};

Object.setPrototypeOf(normalizeMethodRecordBase, null);

Object.setPrototypeOf(normalizeMethodRecord, null);

function impl_object(method) {
  return (
    normalizeMethodRecord[method] ??
    // Here, don't use normalizeMethodRecord, otherwise `PATCH` becomes `patch`.
    normalizeMethodRecordBase[method.toLowerCase()] ??
    method
  );
}

[ronag]

I don't understand. Why not just add it to the first object?

[ronag]

const normalizeMethodRecordBase = {
  delete: "DELETE",
  DELETE: "DELETE",
  get: "GET",
  GET: "GET",
  head: "HEAD",
  HEAD: "HEAD",
  options: "OPTIONS",
  OPTIONS: "OPTIONS",
  post: "POST",
  POST: "POST",
  put: "PUT",
  PUT: "PUT",
  patch: "PATCH",
  PATCH: "PATCH",
};

[tsctx]

The specification reads as follows.
To normalize a method, if it is a byte-case-insensitive match for DELETE, GET, HEAD, OPTIONS, POST, or PUT, byte-uppercase it.
It does not say that patch is to be PATCH.

[tsctx]

An example specification states.
Using patch is highly likely to result in a 405 Method Not Allowed. PATCH is much more likely to succeed.