perf(ext/event): optimize addEventListener options converter

[marcosc90]

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

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

[lucacasonato]

Unfortunately, this is not correct. For example, passing a boolean as the third argument to Event should throw, but now doesn't.

I'd urge you to look at the implementation of createDictionaryConverter. This output is very general because it has to deal with any possible dictionary, and a hand-written converter can definitely be faster, but you need to take significant care to not regress the correctness.

You can read the spec for how to validate a JS value as a dictionary here: https://webidl.spec.whatwg.org/#es-dictionary

You also need to take care that object member access happens only once per property, and in lexicographical order.

It may be interesting to write a script that can take a webidl dictionary definition, and then emit a "hand-written" dictionary converter for that dictionary - this would benefit all dictionary converters in Deno, not just the event one. Doing one offs like this can very occasionally make sense, but we used to do this, and it was a nightmare to maintain because every-time you discover a logic bug in one converter you need to manually audit all other hand-written converters for this same logic bug. With a code generator, that is not a concern :)

[marcosc90]

Unfortunately, this is not correct. For example, passing a boolean as the third argument to Event should throw, but now doesn't.

You mean .addEventlistener('foo', null, true)? because I tested that (also with all other types) and it does not throw on main.

[marcosc90]

I'd urge you to look at the implementation of createDictionaryConverter. This output is very general because it has to deal with any possible dictionary, and a hand-written converter can definitely be faster, but you need to take significant care to not regress the correctness.

Please provide an input that is not correct, because I run several manual tests comparing Chrome/Node/Firefox implementation vs mine and couldn't find any difference on behavior. Also, all WPT passes correctly. Once I have the incorrect value for options that my patch is not handling correctly I'll do the adjustments.

[lucacasonato]

I haven't run your code, but I think both of these generate different output:

addEventListener('foo', () => {}, { get capture() { console.log("capture"); return true }, get once() { console.log("once"); return true }, get passive() { console.log("passive"); return true }, get signal() { console.log("signal"); return new AbortController().signal } })

wrong order of access, signal is accessed twice

---

Number.prototype.once = true;
addEventListener('foo', () => {}, 1);

capture and once are both true, even though only capture should be true

---

Also, you are correct about the boolean in third argument case. I was mistaken here.

My point is not that this can not be perfectly correct - I trust you can make it correct. The problem is that if all of our converters were hand-written manually like this, we'd have to very time intensively review every single one.

I'm happy to land this one once the cases I brought up are fixed, but note that this should be an exception and in general we should work towards a dictionary converter generator that can generate the code you wrote here, at this performance, automatically and correctly, and for all of our webidl dictionaries. Otherwise I'll spend the next 4 weeks just reviewing dictionary converter correctness, which I don't really want to do 😆

[lucacasonato]

@marcosc90 For the prototype access bug - does it regress performance if you branch on the webidl.type(V) !== "Object" like this:

if (webidl.type(V) !== "Object") return { capture: true, passive: false, once: false, signal: undefined };
const options = {
  capture: !!V.capture,
  passive: !!V.passive,
  once: !!V.once,
};
...

return options;

[marcosc90]

@marcosc90 For the prototype access bug - does it regress performance if you branch on the webidl.type(V) !== "Object" like this:

if (webidl.type(V) !== "Object") return { capture: true, passive: false, once: false, signal: undefined };
const options = {
  capture: !!V.capture,
  passive: !!V.passive,
  once: !!V.once,
};
...

return options;

There's no noticeable performance difference.

[lucacasonato]

Great, then I think that should fix that bug.

[lucacasonato]

Just minor comments, but LGTM now!

[lucacasonato]

Suggested change

	once: !!V?.once,
	passive: !!V?.passive,
	once: !!V.once,
	passive: !!V.passive,

[lucacasonato]

Suggested change

	const signal = V?.signal;
	const signal = V.signal;

[bartlomieju]

LGTM! Thanks Marcos, fantastic results.

addressed