perf(ext/event): replace ReflectHas with object lookup #20190

marcosc90 · 2023-08-17T12:14:38Z

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

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

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

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

towards #20167

lucacasonato · 2023-08-17T13:44:33Z

ext/web/02_event.js

@@ -113,7 +112,7 @@ function setStopImmediatePropagation(
 function hasRelatedTarget(
  event,
 ) {
-  return ReflectHas(event, "relatedTarget");
+  return !!event.relatedTarget;


This is not the same thing. If relatedTarget coerces to null, this falsely returns false. Use "relatedTarget" in event

Can we just return false here? We don't have any DOM events.

I know. I used in initially but there's a lint rule against in usage. So went with object lookup because in these changes there wasn't any difference given the possible values and usage of hasRelatedTarget.

Being honest, couldn't find any reasoning as to why Reflect.has was being used. Looked in Node.js implementation and the spec. relatedTarget as far as I know should be either null or a target. So the current implementation with Reflect.has was returning true for relatedTarget === null which IMO is not correct, and later asigning null to eventRelatedTarget, which is the same thing is doing now but is doing it by using the other branch of the ternary operator

const eventRelatedTarget = hasRelatedTarget(eventImpl) ? eventImpl.relatedTarget : null;

Can we just return false here? We don't have any DOM events.

I was actually going to change this later with

const eventRelatedTarget = eventImpl.relatedTarget ?? null;

And most likely ?? null is not even needed.

There are a lot of properties and paths that are most likely never used currently, but can't find examples of code that is supposed to used those branches so it is hard to know whether it'll break things or not, which is why I'm only making small changes for now.

If no WPTs break, it is safe to change for sure. If they do, it may be safe.

Also: relatedTarget should always be the EventTarget that the event was dispatched on, I think.

This check can be !== null I think

Check is actually not needed given possible relatedTarget values. Removed hasRelatedTarget,

lucacasonato · 2023-08-17T13:44:53Z

ext/web/02_event.js

@@ -972,7 +974,7 @@ class EventTarget {

    const { listeners } = self[eventTargetData];

-    if (!(ReflectHas(listeners, type))) {
+    if (!listeners[type]) {


same here, use in

self[eventTargetData] is only accessible internally so we can probably use whatever benchmarks fastest here. There shouldn't be a risk of improper boolean coercion if we know the field is null | array.

listeners is an internal value which is only set by us, and listeners[type] it's either undefined or []. There's no change in using object lookup or in, and since we have a lint rule against in I used object lookup.

mmastrac · 2023-08-17T14:20:32Z

Could we also add another bench call with an onfoo handler?

marcosc90 · 2023-08-17T14:28:42Z

Could we also add another bench call with an onfoo handler?

You mean with handleEvent? or what exactly

  const listener = {
    handleEvent() { /* */ },
  };

bartlomieju · 2023-08-17T15:07:02Z

@marcosc90 I believe Matt meant for a benchmark like that:

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

mmastrac · 2023-08-17T15:10:22Z

@bartlomieju @marcosc90

Correct, though it requires a call to defineEventHandler to set it up:

(from the websocket code)

defineEventHandler(WebSocket.prototype, "message");
defineEventHandler(WebSocket.prototype, "error");
defineEventHandler(WebSocket.prototype, "close");
defineEventHandler(WebSocket.prototype, "open");

mmastrac · 2023-08-17T15:39:40Z

defineEventHandler might be tricky to access, but I think you can dispatch an abort event to an AbortController's onabort signal like so to test this path:

let s = new AbortController().signal
s.onabort = function() { /* ... */ }

marcosc90 · 2023-08-17T16:06:59Z

@mmastrac I exposed defineEventHandler on the global scope so I can bench it

class WS extends EventTarget{}
defineEventHandler(WS.prototype, "message");

const ws = new WS();
ws.onmessage = (e) => {};

const event = new MessageEvent("message", {
  data: 'foo',
  origin: 'localhost',
});

Deno.bench("event dispatch onmessage", () => {
 ws.dispatchEvent(event);
});

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.23 ns/iter  12,464,844.2  (73.97 ns … 121.41 ns)  81.45 ns  87.38 ns  92.71 ns
event dispatch onmessage      79.63 ns/iter  12,558,573.3   (73.55 ns … 87.74 ns)  80.96 ns  84.11 ns  85.41 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.54 ns/iter   9,752,364.8  (96.53 ns … 249.32 ns) 103.38 ns  116.9 ns 132.43 ns
event dispatch onmessage     106.07 ns/iter   9,427,655.9 (100.12 ns … 120.78 ns) 107.28 ns 111.81 ns 112.02 ns

bartlomieju

LGTM, thanks Marcos! Once we run our internal benches we'll let you know how that impact real world benchmarks.

marcosc90 · 2023-08-18T11:52:20Z

LGTM, thanks Marcos! Once we run our internal benches we'll let you know how that impact real world benchmarks.

I'm making improvements on addEventListener now

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

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

perf(ext/event): replace ReflectHas with object lookup

eda86de

bartlomieju requested a review from lucacasonato August 17, 2023 13:35

lucacasonato reviewed Aug 17, 2023

View reviewed changes

remove hasRelatedTarget

199ba39

bartlomieju approved these changes Aug 18, 2023

View reviewed changes

CI

6aad092

bartlomieju merged commit e1aa514 into denoland:main Aug 18, 2023

marcosc90 deleted the perf-dispatch-event branch August 18, 2023 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ext/event): replace ReflectHas with object lookup #20190

perf(ext/event): replace ReflectHas with object lookup #20190

marcosc90 commented Aug 17, 2023 •

edited

Loading

lucacasonato Aug 17, 2023

mmastrac Aug 17, 2023

marcosc90 Aug 17, 2023 •

edited

Loading

marcosc90 Aug 17, 2023

lucacasonato Aug 18, 2023

lucacasonato Aug 18, 2023

marcosc90 Aug 18, 2023

lucacasonato Aug 17, 2023

mmastrac Aug 17, 2023

marcosc90 Aug 17, 2023 •

edited

Loading

mmastrac commented Aug 17, 2023

marcosc90 commented Aug 17, 2023

bartlomieju commented Aug 17, 2023

mmastrac commented Aug 17, 2023

mmastrac commented Aug 17, 2023

marcosc90 commented Aug 17, 2023

bartlomieju left a comment

marcosc90 commented Aug 18, 2023

perf(ext/event): replace ReflectHas with object lookup #20190

perf(ext/event): replace ReflectHas with object lookup #20190

Conversation

marcosc90 commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosc90 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosc90 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

mmastrac commented Aug 17, 2023

marcosc90 commented Aug 17, 2023

bartlomieju commented Aug 17, 2023

mmastrac commented Aug 17, 2023

mmastrac commented Aug 17, 2023

marcosc90 commented Aug 17, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

marcosc90 commented Aug 18, 2023

marcosc90 commented Aug 17, 2023 •

edited

Loading

marcosc90 Aug 17, 2023 •

edited

Loading

marcosc90 Aug 17, 2023 •

edited

Loading