Skip to content
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

Optimize EventDispatcher performance with Map and Set #29917

Closed

Conversation

WittyWhizHard
Copy link

Hey, not sure if these changes are what you're looking for, but here's what I did:

I swapped out plain objects and arrays for Map and Set in the EventDispatcher class to store event listeners. The goal is to boost performance, especially when there are a lot of listeners, since Map and Set offer O(1) time complexity for adding, removing, and checking items.

Key Changes:

  • Performance improvement: Should give a nice performance improvement in Edge, and we should see similar improvements in Chrome and Firefox too.
  • API compatibility: The update doesn't break the existing EventDispatcher API, so no need to change anything on the user side I think.
  • Efficiency improvements: This should help reduce overhead and make event dispatching faster, which is especially useful in performance-heavy scenarios like 3D games.
  • Cross-browser compatibility: It's all compatible with the MDN baseline and works across modern browsers.
  • Benchmarks: I ran some benchmarks with performance.now() in the browser and saw good improvements in Edge. Chrome should have similar results.

Let me know what you think!

Replaced the use of plain objects and arrays in the EventDispatcher class with Map and Set for storing event listeners. This change is intended to improve performance, particularly in cases with many event listeners, by leveraging the constant time complexity (O(1)) of Map and Set for add, remove, and check operations.

- Improved performance in Edge browser, with expected benefits in Chrome as well.
- The new implementation maintains compatibility with the EventDispatcher API, so no changes are required in the code that uses it.
- This optimization should help reduce overhead and improve event dispatch efficiency, particularly in performance-critical scenarios such as 3D game development.
- The code is compatible with the MDN baseline and should work across modern browsers.
- Performance benchmarks were conducted using `performance.now()` in a browser, showing measurable improvements in Edge, with similar expectations for Chrome.
- No functional change, as the API remains the same.
update class name
Copy link

github-actions bot commented Nov 17, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.36
79.06
339.36
79.06
+0 B
+0 B
WebGPU 478.14
132.62
478.14
132.62
+0 B
+0 B
WebGPU Nodes 477.61
132.5
477.61
132.5
+0 B
+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.82
112.02
464.63
111.99
-189 B
-31 B
WebGPU 546.83
148.16
546.64
148.14
-189 B
-23 B
WebGPU Nodes 502.71
137.88
502.52
137.86
-189 B
-24 B

@sunag
Copy link
Collaborator

sunag commented Nov 19, 2024

Could you provide us with the numerical results of the test and the code used in the benchmark?

@mrdoob
Copy link
Owner

mrdoob commented Nov 19, 2024

  • Benchmarks: I ran some benchmarks with performance.now() in the browser and saw good improvements in Edge. Chrome should have similar results.

Please, share links to these benchmarks 🙏

@linbingquan
Copy link
Contributor

import { EventDispatcher as OldEventDispatcher } from "./old.js";
import { EventDispatcher as NewEventDispatcher } from "./new.js";

Deno.bench({
  name: "old",
  baseline: true,
  fn() {
    const dispatcher = new OldEventDispatcher();
    dispatcher.addEventListener("event", () => {});
    dispatcher.dispatchEvent("event");
  },
});

Deno.bench({
  name: "new",
  fn() {
    const dispatcher = new NewEventDispatcher();
    dispatcher.addEventListener("event", () => {});
    dispatcher.dispatchEvent("event");
  },
});
benchmark   time/iter (avg)        iter/s      (min … max)           p75      p99     p995
----------- ----------------------------- --------------------- --------------------------
old                 48.1 ns    20,790,000 ( 39.1 ns … 201.3 ns)  49.4 ns  95.1 ns 109.9 ns
new                116.6 ns     8,574,000 (102.7 ns … 297.5 ns) 119.9 ns 191.1 ns 218.7 ns

summary
  old
     2.42x faster than new

@sunag
Copy link
Collaborator

sunag commented Nov 19, 2024

old 2.42x faster than new

It makes sense to me. Usually primitive values ​​are faster, this should also apply to for instead of forEach.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 19, 2024

dispatcher.dispatchEvent("event");

Just to make sure - this benchmark isn't using dispatchEvent properly. The function takes an object like so:

dispatcher.dispatchEvent( { type: 'event' } );

It may still be faster but this "dispatch" call isn't actually doing or testing any actual dispatching right now. I would also benchmark dispatchEvent and addEventListener separately since dispatchEvent will get called significantly more frequently than addEventListener.

I do still expect the old version to be faster, though.

@eXponenta
Copy link
Contributor

eXponenta commented Nov 22, 2024

Any iterators that use Symbol.iterator is slower about 2 times relative indexed for loop.

Map and Set stored as linked hash tree and can't be indexed and because of this uses Symbol.iterator for it.

Iteration more important than add/remove listeners, because events can be fired every frame, but listeners assigned only in app start or when something updated by user input.

Also classic events list about 10-20 listeners, slice over 10-20 is highly optimised operation.

Also Map and Set by designed for frequent update by unknown key set.

Events map is KNOWN keyset. Because we not set random key as event type every update. Listeners map will be high optimised type-shape with strictly known properties.

Classic event name in core: "update".
This mean that we will be created a lot of small objects with references to single shape map inside JS runtime with property name "update".
How this works

Conclusion:
Map and Set isn't good idea for usage for EventDispatcher due to performance degradations in basic usage, IMHO.

...
Side effects: I know that some plugins read _listeners for check what is listener is bounded to current node.
this important for mouse event dispatching because require strict ordering.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 22, 2024

Based on the feedback so far it seems better to close the PR.

@Mugen87 Mugen87 closed this Nov 22, 2024
@Mugen87 Mugen87 added this to the r171 milestone Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants