-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
wip: store additional property in WeakMap instead of on the event object directly. #16532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Pretty sure this will erase most perf and memory advantages of event delegation. Is there really no other way? |
|
Why do you think that the WeakMap has worse performance than adding a propety to the event? |
I filed a bug report for Firefox: |
It's a fair question, since modifying the shape of an object also has performance implications. Personally I'm just used to treating In the past @trueadm has referenced js-framework-benchmark a lot, and we could try this PR and an alternative one I just created — #16538 — to see if there's any difference. I remember the benchmark being an absolute nightmare to set up though |
Wouldn't the perf implications be practically nonexistent due to hidden classes? |
Honestly I dunno, outside my area of expertise |
Since #16538 was merged I'll go ahead and close this but am open to an alternative solution if the benchmarks indicate it. Thank you |
There's a difference between deopting an object vs just using a computationally expensive data structure. If you consistently de-opt and object, the VM will bail out trying to make it monomorphic. That's not ideal, but completely acceptable and happens all the time in JS land. WeakMaps are just very expensive computationally due to their need to handle weak references and the burden it has on the GC and the JIT compiler. |
Another approach to fix #16522
Instead adding a
__root
property to the event object itself a WeakMap is used to associate the event with the propagation meta data.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint