EventTarget

[KhafraDev]

See nodejs/node#34074

EventTarget is likely still slower than EventEmitter, as there haven't been any changes made to improve performance (at least none that I could find). I don't see anything in the spec which inherently makes EventTarget slower.

Libraries like ws use their own EventTarget/Event implementations due to performance regressions incurred by switching to the native counterparts (along with other reasons).

[benjamingr]

We should consider refactoring EventTarget to not use linked lists to store listeners as that incurs pretty significant overhead.

[anonrig]

We should consider refactoring EventTarget to not use linked lists to store listeners as that incurs pretty significant overhead.

This seems like a good idea to me.

[KhafraDev]

this would also solve #44

[santigimeno]

I've created nodejs/node#46527 which improves performance a bit.

[santigimeno]

To expand a bit on what was commented on yesterday's meeting, here are some interesting numbers taking the commit from nodejs/node#46527 as a baseline:

• Not setting __proto__: null when defining the Event.isTrusted property:

[00:16:07|% 100| 2/2 files | 120/120 runs | 3/3 configs]: Done
                                                                       confidence improvement accuracy (*)   (**)  (***)
events/eventtarget.js listeners=1 n=1000000                                  ***     20.66 %       ±1.06% ±1.40% ±1.81%
events/eventtarget.js listeners=10 n=1000000                                 ***     17.65 %       ±1.07% ±1.42% ±1.85%
events/eventtarget.js listeners=5 n=1000000                                  ***     20.00 %       ±0.85% ±1.13% ±1.45%
worker/messageport.js n=1000000 style='eventemitter' payload='object'                -1.59 %       ±2.51% ±3.32% ±4.28%
worker/messageport.js n=1000000 style='eventemitter' payload='string'                 0.67 %       ±2.25% ±2.97% ±3.83%
worker/messageport.js n=1000000 style='eventtarget' payload='object'          **      3.70 %       ±2.17% ±2.87% ±3.70%
worker/messageport.js n=1000000 style='eventtarget' payload='string'         ***     13.06 %       ±2.54% ±3.36% ±4.32%

• As @addaleax hinted here not defining Event.isTrusted in the constructor but, for example, defining it in the Event.prototype increases performance by a lot:

[00:14:13|% 100| 2/2 files | 120/120 runs | 3/3 configs]: Done
                                                                       confidence improvement accuracy (*)   (**)  (***)
events/eventtarget.js listeners=1 n=1000000                                  ***    186.96 %       ±1.47% ±1.95% ±2.52%
events/eventtarget.js listeners=10 n=1000000                                 ***    104.59 %       ±1.40% ±1.86% ±2.42%
events/eventtarget.js listeners=5 n=1000000                                  ***    141.10 %       ±1.16% ±1.54% ±2.00%
worker/messageport.js n=1000000 style='eventemitter' payload='object'                -0.67 %       ±1.35% ±1.79% ±2.30%
worker/messageport.js n=1000000 style='eventemitter' payload='string'          *     -1.09 %       ±1.03% ±1.36% ±1.75%
worker/messageport.js n=1000000 style='eventtarget' payload='object'         ***     26.86 %       ±1.62% ±2.15% ±2.77%
worker/messageport.js n=1000000 style='eventtarget' payload='string'         ***     45.61 %       ±1.29% ±1.71% ±2.20%

Including the 1st patch wouldn't protect against certain kind of prototype pollution as described here.
OTOH the 2nd patch would break the spec as Event.isTrusted is defined in the standard as LegacyUnforgeable which declares:

the property will be non-configurable and will exist as an own property on the object itself rather than on its prototype.

What are your thoughts?

[KhafraDev]

What are your thoughts?

Node should follow the spec.

[ronag]

I think it's [LegacyUnforgeable] due to security concerns which are not relevant on the backend. For the end-user's perspective I'm not sure it's observable in real-world scenarios? Or am I missing something?

[KhafraDev]

It wouldn't show up in Object.getOwnPropertyDescriptors(new Event('...')). Probably not a common pattern, but would still bring a distinction between node and other environments.

[ronag]

What does deno, bun & cloudworkers do?

[KhafraDev]

• Deno: https://github.com/denoland/deno/blob/65500f36e870b4ada3996b06aa287e30177d21a3/ext/web/02_event.js#L177-L181 (correct behavior)
• Cloudflare workers: https://github.com/cloudflare/workerd/blob/94083609f856bea6e4e7c06d0c177185eea6866f/src/workerd/api/basics.h#L119 (incorrect, not legacy unforgeable)
• I'm assuming bun uses webkit's implementation so it's correct.

[ronag]

Hm. Tricky. I'm sorry to say I'm neutral on this.

[santigimeno]

• Deno: https://github.com/denoland/deno/blob/65500f36e870b4ada3996b06aa287e30177d21a3/ext/web/02_event.js#L177-L181 (correct behavior)

It's interesting to note that Deno has a path in which they avoid being spec compliant exactly because of this: https://github.com/denoland/deno/blob/65500f36e870b4ada3996b06aa287e30177d21a3/ext/web/02_event.js#L194-L196

[KhafraDev]

Interesting, completely missed that, although it's only used internally.

---

Unrelated but I wonder if isTrusted could be moved to the prototype instead, since changing the behavior probably wouldn't break anything? It might be worth opening an issue in the html repo. The issue seems to be with the spec itself which node is following.

[jasnell]

I've never seen a use case for trusted outside of browsers and even there I think those are questionable. I'd be ok with not being fully spec compliant on this point.

[santigimeno]

Following on this, I've created the following draft PR