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(webidl): optimize createRecordConverter() #12286

Merged
merged 4 commits into from
Oct 4, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Oct 1, 2021

Cuts self-time by ~6x, 172ns/iter => 22ns/iter benched on 1M Response builds / HeadersInit calls

This should be functionally identical to the old code since:

  1. for...in loops on objects iterate over all enumberable string keys of an object (including inherited)
  2. Then we filter out inherited keys with .hasOwnProperty(...)

Note: WebIDL / WPT is coupled to a subpar implementation so our hands are tied, unless an isProxy fast path check isn't too expensive

Benchmarks

No fast path:
resp_w_bh:           	n = 1000000, dt = 1.295s, r = 772201/s, t = 1294ns/op
Fast path with isProxy check:
resp_w_bh:           	n = 1000000, dt = 1.146s, r = 872600/s, t = 1146ns/op
Forced fast path:
resp_w_bh:           	n = 1000000, dt = 1.112s, r = 899281/s, t = 1112ns/op

Pending on #12288

@lucacasonato
Copy link
Member

lucacasonato commented Oct 1, 2021

This is not equal. for .. in also enumerates prototype fields, while ReflectOwnKeys does not.

{
  const obj = {
    a: 1,
    get b() {
      return 2;
    },
  };
  Reflect.getPrototypeOf(obj).c = 3;

  for (const key in obj) {
    const val = obj[key];
    console.log("for .. in:", key, val);
  }

  // for .. in: a 1
  // for .. in: b 2
  // for .. in: c 3
}



{
  const obj = {
    a: 1,
    get b() {
      return 2;
    },
  };
  Reflect.getPrototypeOf(obj).c = 3;

  const keys = Reflect.ownKeys(obj);
  for (const key of keys) {
    const val = obj[key];
    console.log("Reflect.ownKeys:", key, val);
  }

  // Reflect.ownKeys: a 1
  // Reflect.ownKeys: b 2
}

@lucacasonato
Copy link
Member

Actually, NVM. Inherited keys are filtered out via hasOwnProperty

AaronO added a commit to AaronO/deno that referenced this pull request Oct 1, 2021
Another binding ... but it's required for denoland#12286 and could be useful elsewhere
AaronO added 2 commits October 1, 2021 20:26
Cuts self-time by ~6x, 172ns/iter => 22ns/iter benched on 1M Response builds / HeadersInit calls
@AaronO AaronO force-pushed the perf/webidl-createRecordConverter branch from 1ca8daf to 7608f77 Compare October 1, 2021 18:26
@AaronO AaronO requested a review from lucacasonato October 1, 2021 18:41
@AaronO AaronO merged commit 5f41f82 into denoland:main Oct 4, 2021
@AaronO AaronO deleted the perf/webidl-createRecordConverter branch October 4, 2021 13:39
ry pushed a commit that referenced this pull request Oct 4, 2021
Cuts self-time by ~6x, 172ns/iter => 22ns/iter benched on 1M Response builds / HeadersInit calls
AaronO added a commit to AaronO/deno that referenced this pull request Oct 5, 2021
In a tweak commit of denoland#12286 I accidentally eliminated the else branch ... running the slow & the fast path providing a worst of both worlds path
AaronO added a commit that referenced this pull request Oct 5, 2021
In a tweak commit of #12286 I accidentally eliminated the else branch ... running the slow & the fast path providing a worst of both worlds path
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Oct 10, 2021
In a tweak commit of denoland#12286 I accidentally eliminated the else branch ... running the slow & the fast path providing a worst of both worlds path
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.

4 participants