Repository navigation
EventTarget #32
Description
Activity
We should consider refactoring EventTarget to not use linked lists to store listeners as that incurs pretty significant overhead.
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.
I've created nodejs/node#46527 which improves performance a bit.
Reacted by Ulises Gascón, Khafra and Carlos FuentesTo 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__: nullwhen defining theEvent.isTrustedproperty:
[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.isTrustedin the constructor but, for example, defining it in theEvent.prototypeincreases 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 asEvent.isTrustedis 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?
- Not setting
What are your thoughts?
Node should follow the spec.
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?
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.What does deno, bun & cloudworkers do?
- 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.
Hm. Tricky. I'm sorry to say I'm neutral on this.
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
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.
Reacted by Marvin HagemeisterReacted by Toni VillenaI'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.
Reacted by Santiago Gimeno, Robert Nagy and Carlos FuentesReacted by Toni VillenaFollowing on this, I've created the following draft PR
Reacted by Carlos Fuentes- added a commit that references this issue
on Mar 20, 2023 - added a commit that references this issue
on Apr 3, 2023 - added a commit that references this issue
on Jul 6, 2023 - added a commit that references this issue
on Jul 6, 2023
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
wsuse their own EventTarget/Event implementations due to performance regressions incurred by switching to the native counterparts (along with other reasons).