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

Optimize events implementation #20167

Closed
bartlomieju opened this issue Aug 15, 2023 · 7 comments
Closed

Optimize events implementation #20167

bartlomieju opened this issue Aug 15, 2023 · 7 comments
Labels
perf performance related web related to Web APIs

Comments

@bartlomieju
Copy link
Member

While working on #20153 @mmastrac discovered that our implementation of Event and related APIs is very slow:
image (4)

We should do some benchmarks to see what is the cost of:

  • creating an instance of Event class
  • adding event listener using addEventListener API
  • dispatching an event using dispatchEvent

Once we have baseline measurements we should figure out where we are losing time and fix each case one-by-one.

@bartlomieju bartlomieju added perf performance related web related to Web APIs labels Aug 15, 2023
@bartlomieju
Copy link
Member Author

@marcosc90 would this be something you'd be interested to take a look at? :)

@marcosc90
Copy link
Contributor

Sure I'll take a look @bartlomieju!

@marcosc90
Copy link
Contributor

@bartlomieju can you share the code/commands to get that call tree? I have some changes I'll push soon, but want to test it out first.

@bartlomieju
Copy link
Member Author

@marcosc90 you can get these results if you build off of 41cad21 commit, create empty.js (which is an empty file :D) and run:

./target/release/deno run --v8-flags='--prof,--prof-sampling-interval=10' ./empty.js

You can then process this file like so:

node --prof-process --preprocess isolate-0x158050000-48087-v8.log | grep -v "Testing v8 version different from logging version" > /tmp/prof.json

And view the file at https://v8.github.io/tools/head/profview/.

That was a pretty exotic setup and I believe if you just have something like

Deno.bench(function eventListener() {
  foo.addEventListener("foo", () => {});
});

that should give a pretty good result as well.

@marcosc90
Copy link
Contributor

Getting Segmentation fault (core dumped) with that command. I'll use Deno.bench for now

@bartlomieju
Copy link
Member Author

Getting Segmentation fault (core dumped) with that command. I'll use Deno.bench for now

Ouch, that's concerning :/

bartlomieju pushed a commit that referenced this issue Aug 17, 2023
This PR optimizes `Event` constructor

- ~Added a fast path for empty `eventInitDict`~ Removed `EventInit`
dictionary converter
- Don't make `isTrusted` a
[LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable)
property. Doing so makes it non-spec compliant but calling
`Object/Reflect.defineProperty` on the constructor is a big bottleneck.
Node did the same a few months ago
nodejs/node#46974. In my opinion, the
performance gains are worth deviating from the spec for a
browser-related property.

**This PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init      36.69 ns/iter  27,257,504.6   (33.36 ns … 42.45 ns)  37.71 ns  39.61 ns  40.07 ns
event constructor               36.7 ns/iter  27,246,776.6   (33.35 ns … 56.03 ns)  37.73 ns  40.14 ns  41.74 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init     380.48 ns/iter   2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns
event constructor             480.33 ns/iter   2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns
```

```js
Deno.bench("event constructor no init", () => {
  const event = new Event("foo");
});

Deno.bench("event constructor", () => {
  const event = new Event("foo", { bubbles: true, cancelable: false });
});
```

towards #20167
bartlomieju pushed a commit that referenced this issue Aug 18, 2023
This PR optimizes event dispatch by replacing `ReflectHas` with object
lookup. I also made `isSlottable` return `false` since AFAIK there
aren't any slottables nodes in Deno

**This PR**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch       80.46 ns/iter  12,428,739.4  (73.84 ns … 120.07 ns)  81.82 ns  86.34 ns  91.18 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch      102.66 ns/iter   9,741,319.6  (96.66 ns … 132.88 ns) 104.18 ns 114.58 ns 118.45 ns
```

```js
const tg = new EventTarget();
const ev = new Event("foo");

const listener = () => {};
tg.addEventListener("foo", listener);

Deno.bench("event dispatch ", () => {
  tg.dispatchEvent(ev);
});
```

towards #20167
bartlomieju pushed a commit that referenced this issue Aug 20, 2023
This PR optimizes `addEventListener` by replacing
`webidl.createDictionaryConverter("AddEventListenerOptions", ...)` with
a custom options parsing function to avoid the overhead of `webidl`
methods

**this PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)       4.87 ns/iter 205,248,660.8     (4.7 ns … 13.18 ns)   4.91 ns    5.4 ns    5.6 ns
addEventListener options converter (signal)         13.02 ns/iter  76,782,031.2   (11.74 ns … 18.84 ns)  13.08 ns  16.22 ns  16.57 ns
```

**main**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)     108.36 ns/iter   9,228,688.6  (103.5 ns … 129.88 ns) 109.69 ns 115.61 ns 125.28 ns
addEventListener options converter (signal)        134.03 ns/iter   7,460,878.1 (129.14 ns … 144.54 ns) 135.68 ns 141.13 ns  144.1 ns
```

```js
const tg = new EventTarget();
const signal = new AbortController().signal;

Deno.bench("addEventListener options converter (undefined)", () => {
  tg.addEventListener("foo", null); // null callback to only bench options converter
});

Deno.bench("addEventListener options converter (signal)", () => {
  tg.addEventListener("foo", null, { signal });
});
```

Towards #20167
bartlomieju added a commit that referenced this issue Aug 20, 2023
```js
Deno.bench(function eventNew() {
  new Event("foo");
});
```

<b>main</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       36.43 ns/iter  27,451,874.9   (35.15 ns … 46.98 ns)  37.68 ns   40.7 ns  41.69 ns

```

<b>this PR</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       13.71 ns/iter  72,958,970.0   (12.85 ns … 31.79 ns)  15.11 ns  16.49 ns   17.5 ns

```

Towards #20167
littledivy pushed a commit to littledivy/deno that referenced this issue Aug 21, 2023
This PR optimizes `Event` constructor

- ~Added a fast path for empty `eventInitDict`~ Removed `EventInit`
dictionary converter
- Don't make `isTrusted` a
[LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable)
property. Doing so makes it non-spec compliant but calling
`Object/Reflect.defineProperty` on the constructor is a big bottleneck.
Node did the same a few months ago
nodejs/node#46974. In my opinion, the
performance gains are worth deviating from the spec for a
browser-related property.

**This PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init      36.69 ns/iter  27,257,504.6   (33.36 ns … 42.45 ns)  37.71 ns  39.61 ns  40.07 ns
event constructor               36.7 ns/iter  27,246,776.6   (33.35 ns … 56.03 ns)  37.73 ns  40.14 ns  41.74 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init     380.48 ns/iter   2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns
event constructor             480.33 ns/iter   2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns
```

```js
Deno.bench("event constructor no init", () => {
  const event = new Event("foo");
});

Deno.bench("event constructor", () => {
  const event = new Event("foo", { bubbles: true, cancelable: false });
});
```

towards denoland#20167
littledivy pushed a commit to littledivy/deno that referenced this issue Aug 21, 2023
This PR optimizes event dispatch by replacing `ReflectHas` with object
lookup. I also made `isSlottable` return `false` since AFAIK there
aren't any slottables nodes in Deno

**This PR**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch       80.46 ns/iter  12,428,739.4  (73.84 ns … 120.07 ns)  81.82 ns  86.34 ns  91.18 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch      102.66 ns/iter   9,741,319.6  (96.66 ns … 132.88 ns) 104.18 ns 114.58 ns 118.45 ns
```

```js
const tg = new EventTarget();
const ev = new Event("foo");

const listener = () => {};
tg.addEventListener("foo", listener);

Deno.bench("event dispatch ", () => {
  tg.dispatchEvent(ev);
});
```

towards denoland#20167
littledivy pushed a commit to littledivy/deno that referenced this issue Aug 21, 2023
…d#20203)

This PR optimizes `addEventListener` by replacing
`webidl.createDictionaryConverter("AddEventListenerOptions", ...)` with
a custom options parsing function to avoid the overhead of `webidl`
methods

**this PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)       4.87 ns/iter 205,248,660.8     (4.7 ns … 13.18 ns)   4.91 ns    5.4 ns    5.6 ns
addEventListener options converter (signal)         13.02 ns/iter  76,782,031.2   (11.74 ns … 18.84 ns)  13.08 ns  16.22 ns  16.57 ns
```

**main**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)     108.36 ns/iter   9,228,688.6  (103.5 ns … 129.88 ns) 109.69 ns 115.61 ns 125.28 ns
addEventListener options converter (signal)        134.03 ns/iter   7,460,878.1 (129.14 ns … 144.54 ns) 135.68 ns 141.13 ns  144.1 ns
```

```js
const tg = new EventTarget();
const signal = new AbortController().signal;

Deno.bench("addEventListener options converter (undefined)", () => {
  tg.addEventListener("foo", null); // null callback to only bench options converter
});

Deno.bench("addEventListener options converter (signal)", () => {
  tg.addEventListener("foo", null, { signal });
});
```

Towards denoland#20167
littledivy pushed a commit to littledivy/deno that referenced this issue Aug 21, 2023
```js
Deno.bench(function eventNew() {
  new Event("foo");
});
```

<b>main</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       36.43 ns/iter  27,451,874.9   (35.15 ns … 46.98 ns)  37.68 ns   40.7 ns  41.69 ns

```

<b>this PR</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       13.71 ns/iter  72,958,970.0   (12.85 ns … 31.79 ns)  15.11 ns  16.49 ns   17.5 ns

```

Towards denoland#20167
littledivy pushed a commit that referenced this issue Aug 21, 2023
This PR optimizes `Event` constructor

- ~Added a fast path for empty `eventInitDict`~ Removed `EventInit`
dictionary converter
- Don't make `isTrusted` a
[LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable)
property. Doing so makes it non-spec compliant but calling
`Object/Reflect.defineProperty` on the constructor is a big bottleneck.
Node did the same a few months ago
nodejs/node#46974. In my opinion, the
performance gains are worth deviating from the spec for a
browser-related property.

**This PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init      36.69 ns/iter  27,257,504.6   (33.36 ns … 42.45 ns)  37.71 ns  39.61 ns  40.07 ns
event constructor               36.7 ns/iter  27,246,776.6   (33.35 ns … 56.03 ns)  37.73 ns  40.14 ns  41.74 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                      time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------------- -----------------------------
event constructor no init     380.48 ns/iter   2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns
event constructor             480.33 ns/iter   2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns
```

```js
Deno.bench("event constructor no init", () => {
  const event = new Event("foo");
});

Deno.bench("event constructor", () => {
  const event = new Event("foo", { bubbles: true, cancelable: false });
});
```

towards #20167
littledivy pushed a commit that referenced this issue Aug 21, 2023
This PR optimizes event dispatch by replacing `ReflectHas` with object
lookup. I also made `isSlottable` return `false` since AFAIK there
aren't any slottables nodes in Deno

**This PR**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch       80.46 ns/iter  12,428,739.4  (73.84 ns … 120.07 ns)  81.82 ns  86.34 ns  91.18 ns
```

**main**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark            time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------------- -----------------------------
event dispatch      102.66 ns/iter   9,741,319.6  (96.66 ns … 132.88 ns) 104.18 ns 114.58 ns 118.45 ns
```

```js
const tg = new EventTarget();
const ev = new Event("foo");

const listener = () => {};
tg.addEventListener("foo", listener);

Deno.bench("event dispatch ", () => {
  tg.dispatchEvent(ev);
});
```

towards #20167
littledivy pushed a commit that referenced this issue Aug 21, 2023
This PR optimizes `addEventListener` by replacing
`webidl.createDictionaryConverter("AddEventListenerOptions", ...)` with
a custom options parsing function to avoid the overhead of `webidl`
methods

**this PR**

```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)       4.87 ns/iter 205,248,660.8     (4.7 ns … 13.18 ns)   4.91 ns    5.4 ns    5.6 ns
addEventListener options converter (signal)         13.02 ns/iter  76,782,031.2   (11.74 ns … 18.84 ns)  13.08 ns  16.22 ns  16.57 ns
```

**main**
```
cpu: 13th Gen Intel(R) Core(TM) i9-13900H
runtime: deno 1.36.1 (x86_64-unknown-linux-gnu)

benchmark                                           time (avg)        iter/s             (min … max)       p75       p99      p995
---------------------------------------------------------------------------------------------------- -----------------------------
addEventListener options converter (undefined)     108.36 ns/iter   9,228,688.6  (103.5 ns … 129.88 ns) 109.69 ns 115.61 ns 125.28 ns
addEventListener options converter (signal)        134.03 ns/iter   7,460,878.1 (129.14 ns … 144.54 ns) 135.68 ns 141.13 ns  144.1 ns
```

```js
const tg = new EventTarget();
const signal = new AbortController().signal;

Deno.bench("addEventListener options converter (undefined)", () => {
  tg.addEventListener("foo", null); // null callback to only bench options converter
});

Deno.bench("addEventListener options converter (signal)", () => {
  tg.addEventListener("foo", null, { signal });
});
```

Towards #20167
littledivy pushed a commit that referenced this issue Aug 21, 2023
```js
Deno.bench(function eventNew() {
  new Event("foo");
});
```

<b>main</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       36.43 ns/iter  27,451,874.9   (35.15 ns … 46.98 ns)  37.68 ns   40.7 ns  41.69 ns

```

<b>this PR</b>
```
./target/release/deno bench event_bench.js
cpu: Apple M1 Max
runtime: deno 1.36.1 (aarch64-apple-darwin)

file:///Users/ib/dev/deno/event_bench.js
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
eventNew       13.71 ns/iter  72,958,970.0   (12.85 ns … 31.79 ns)  15.11 ns  16.49 ns   17.5 ns

```

Towards #20167
@bartlomieju
Copy link
Member Author

We had several PRs that improved the performance of Event class. I'm gonna close this issue for now and open a new one if we spot any more problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf performance related web related to Web APIs
Projects
None yet
Development

No branches or pull requests

2 participants