Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(ext/fetch): use new instead of createBranded #20624

Merged
merged 3 commits into from Sep 22, 2023

Conversation

marcosc90
Copy link
Contributor

This PR optimizes fromInner* methods of Request / Header / Response used by Deno.serve and fetch by using new instead of ObjectCreate from createBranded.

The "brand" is created by passing webidl.brand to the constructor instead.

deno/ext/webidl/00_webidl.js

Lines 1001 to 1005 in 142449e

function createBranded(Type) {
const t = ObjectCreate(Type.prototype);
t[brand] = brand;
return t;
}

Benchmark

const createBranded = Symbol("create branded");
const brand = Symbol("brand");
class B {
  constructor(init) {
    if (init === createBranded) {
      this[brand] = brand;
    }
  }
}

Deno.bench("Object.create(protoype)", () => {
  Object.create(B.prototype);
});

Deno.bench("new Class", () => {
  new B(createBranded);
});
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.37.0 (x86_64-unknown-linux-gnu)

benchmark                    time (avg)        iter/s             (min … max)       p75       p99      p995
----------------------------------------------------------------------------- -----------------------------
Object.create(protoype)       8.74 ns/iter 114,363,610.3    (7.32 ns … 26.02 ns)   8.65 ns  13.39 ns  14.47 ns
new Class                     3.05 ns/iter 328,271,012.2      (2.78 ns … 9.1 ns)   3.06 ns   3.46 ns    3.5 ns

@@ -286,6 +286,11 @@ class Headers {

/** @param {HeadersInit} [init] */
constructor(init = undefined) {
if (init === webidl.brand) {
this[webidl.brand] = webidl.brand;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this[init] = init faster? It should have two less property lookups

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, missed that.

Comment on lines 562 to 565
const request = new Request(webidl.brand);
request[_request] = inner;
request[_signal] = signal;
request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard);
Copy link
Member

@mmastrac mmastrac Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the way v8 optimizes classes, it might be faster to pass all of these into the constructor after the brand, so that the object always has the same shape after the constructor returns.

new Request(webidl.brand, inner, signal, () => ...);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run some benchmarks

@@ -40,6 +40,7 @@ const _headerList = Symbol("header list");
const _iterableHeaders = Symbol("iterable headers");
const _iterableHeadersCache = Symbol("iterable headers cache");
const _guard = Symbol("guard");
const _brand = webidl.brand;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! We can do a follow-up if passing constructor args helps as well

@mmastrac mmastrac merged commit 65a94a6 into denoland:main Sep 22, 2023
13 checks passed
@marcosc90 marcosc90 deleted the perf-inner-request branch October 5, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants