Conversation
| passive, | ||
| once, | ||
| signal, | ||
| callback: typeof callback === 'function' ? callback : callback.handleEvent, |
There was a problem hiding this comment.
Can you avoid calling typeof? string comparisons are always slow. I recommend avoiding it as much as you can in any js code.
There was a problem hiding this comment.
I guess I could do callback?.handleEvent ?? callback but I doubt it's faster(?)
| const validatedOptions = { | ||
| __proto__: null, | ||
| passive: Boolean(options.passive), | ||
| once: Boolean(options.once), |
There was a problem hiding this comment.
you already have a options variable, and you're re-creating it. This is unnecessary copying.
There was a problem hiding this comment.
This is essentially implementing a webidl dictionary converter, we do this in undici. Options could also be a proxy & these could be readonly getters so we can't safely overwrite properties on the object
| if (!event) { | ||
| eventTarget.kTestIndices.set(listener.type, [listener]) | ||
| } else if ( | ||
| !ArrayPrototypeSome(event, (value) => { |
There was a problem hiding this comment.
Can you avoid calling arrayprototypesome in any way?
There was a problem hiding this comment.
not really, event handlers are matched by the event listener (the function) and their options. For example:
const et = new EventTarget()
function a () {}
et.addEventListener('name', a, { capture: false })
et.addEventListener('name', a, { capture: true })this adds 2 event listeners rather than overwriting the first one.
|
|
||
| eventList[eventListener].removed = true | ||
|
|
||
| ArrayPrototypeSplice(eventList, eventListener, 1) |
There was a problem hiding this comment.
Splice mutates the whole array. Why don't we use Set to make removing faster?
There was a problem hiding this comment.
reason listed above. EventTarget is poorly designed for optimizations.
| return | ||
| } | ||
|
|
||
| if (listener.callback == null) { |
There was a problem hiding this comment.
you already validated options in addEventListener, and you are also validating it here. This is one of the most impacting thing I assume.
There was a problem hiding this comment.
the listener is optional,
new EventTarget().addEventListener('event', undefined)
This function call can fail with `Z_VERSION_ERROR` if the compiled
library vs loaded library mismatched in version number or in
stream structure size.
In those cases, zlib doesn't initialize the `strm_.msg` field to
null. Therefore, when a `CompressionError` object is created via
`ErrorForMessage()`, it can read a stale or uninitialized `strm_.msg`
pointer that will cause a crash.
Example ASAN report:
```
AddressSanitizer: SEGV on unknown address
#0 __strlen_avx2
string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76
#1 strlen (/work/node/out/Debug/node+0x1a42ab7)
nodejs#2 v8::(anonymous namespace)::StringLength(char const*)
/work/node/out/../deps/v8/src/api/api.cc:7581:16
nodejs#3 v8::(anonymous namespace)::StringLength(unsigned char const*)
/work/node/out/../deps/v8/src/api/api.cc:7587:10
nodejs#4 v8::String::NewFromOneByte(v8::Isolate*,
unsigned char const*, v8::NewStringType, int)
/work/node/out/../deps/v8/src/api/api.cc:7677:3
nodejs#5 node::OneByteString(v8::Isolate*,
char const*, int, v8::NewStringType)
/work/node/out/../src/util-inl.h:166:10
nodejs#6 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::EmitError(node::(anonymous namespace)
::CompressionError const&)
/work/node/out/../src/node_zlib.cc:565:7
nodejs#7 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::CheckError()
/work/node/out/../src/node_zlib.cc:519:5
nodejs#8 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::AfterThreadPoolWork(int)
/work/node/out/../src/node_zlib.cc:543:10
nodejs#9 node::ThreadPoolWork::ScheduleWork()
::'lambda'(uv_work_s*, int)
::operator()(uv_work_s*, int) const
/work/node/out/../src/threadpoolwork-inl.h:57:15
nodejs#10 node::ThreadPoolWork::ScheduleWork()
::'lambda'(uv_work_s*, int)
::__invoke(uv_work_s*, int)
/work/node/out/../src/threadpoolwork-inl.h:48:7
nodejs#11 uv__work_done /work/libuv-1.51.0/src/threadpool.c:330:5
nodejs#12 uv__async_io.part.0
/work/libuv-1.51.0/src/unix/async.c:208:5
```
Signed-off-by: ndossche <nora.dossche@ugent.be>
PR-URL: nodejs#63476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
2 participants
No description provided.