perf: use class instead of object literals with getters

[tsctx]

Slow initialization of object literals with getters.

• main

benchmark               time (avg)             (min … max)       p75       p99      p999
---------------------------------------------------------- -----------------------------
new Request(input)  12'398 ns/iter   (8'100 ns … 4'134 µs) 11'000 ns 34'100 ns    147 µs

• this patch

benchmark               time (avg)             (min … max)       p75       p99      p999
---------------------------------------------------------- -----------------------------
new Request(input)  10'997 ns/iter   (7'000 ns … 3'704 µs)  9'700 ns 34'000 ns    159 µs

[mcollina]

lgtm

[Uzlopak]

Did you also see that makePolicyContainer() seemed to be super slow?

[KhafraDev]

This should be done for all classes (there is no such thing as a "Request Realm Settings Object", there is an "Environment Settings Object").

[KhafraDev]

Did you also see that makePolicyContainer() seemed to be super slow?

It returns an object with a single property. If you're running creating a hundred thousand Requests everything will seem to be "super slow".

[mcollina]

There is nothing absolutely “slow” or “fast”, as those are relative concepts. The only useful measure when doing optimization work is to identify bottlenecks and remove them.

(A tiny fast function called a bazillion times might be causing a bottleneck as much as a huge slow function seldomly called).

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (501f4fa) to head (29ee9ac).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3138      +/-   ##
==========================================
+ Coverage   94.13%   94.17%   +0.03%     
==========================================
  Files          89       90       +1     
  Lines       24158    24320     +162     
==========================================
+ Hits        22742    22904     +162     
  Misses       1416     1416              

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

[KhafraDev]

not in this context, it's called once per Request and is needed for spec compatibility

[tsctx]

not in this context, it's called once per Request and is needed for spec compatibility

What do you mean by that? I've already created a new context for each call.

[KhafraDev]

I was responding to Matteo's comment, nothing to do with this PR

[KhafraDev]

Response has a realm also, which is an Environment Settings Object.

We're also adding a realm to Headers in some cases but it's unused, should probably be removed.

[tsctx]

@KhafraDev ptal

[KhafraDev]

EnvironmentSettingsObject should be shared between Response and Request, it's meant to be the same object. It's fine to lazily create the properties needed for each.

I guess that can lead to further perf improvements, the realm should be the exact same object and it can't change because they're non-transferable and cannot be cloned. The issue there being that the global origin may be mutated, but that can be a getter while policyContainer and whatever else is constant?

[tsctx]

I guess that can lead to further perf improvements, the realm should be the exact same object and it can't change because they're non-transferable and cannot be cloned. The issue there being that the global origin may be mutated, but that can be a getter while policyContainer and whatever else is constant?

If we use getter, there should be no problem.

[KhafraDev]

Sorry, it seems like baseUrl is only set once. It can be set whenever so it should update appropriately.

[KhafraDev]

I don't think that those changes will solve the issue, consider the following:

import { setGlobalOrigin, Request } from 'undici'

setGlobalOrigin('http://localhost:3000')
new Request('/').url // http://localhost:3000

setGlobalOrigin('https://www.google.com')
new Request('/').url // https://www.google.com

If this works, please add a test so we don't regress in the future.

[tsctx]

I don't think that those changes will solve the issue, consider the following:

import { setGlobalOrigin, Request } from 'undici'

setGlobalOrigin('http://localhost:3000')
new Request('/').url // http://localhost:3000

setGlobalOrigin('https://www.google.com')
new Request('/').url // https://www.google.com

If this works, please add a test so we don't regress in the future.

It works correctly before and after the change.

[KhafraDev]

I see why, didn't realize a new object was being created every time. We should be able to reuse a single instance of EnvironmentSettingsObject instead of creating a new realm per Request/Response. This PR is fine to land as-is, I've already made you do enough...