Skip to content

implement node:events in javascript - #2604

Merged
Jarred-Sumner merged 13 commits into
mainfrom
dave/events
Apr 18, 2023
Merged

Jarred-Sumner merged 13 commits into
mainfrom
dave/events

Conversation

@paperclover

@paperclover paperclover commented Apr 10, 2023 •

Copy link
Copy Markdown
Contributor

Currently, node:events is implemented in C++, which may sound fast but ends up being much slower than a pure JS implementation as the rapid hopping between native code and JS with listener callbacks is a lot of overhead. This PR is not only porting the node.js events into JS as closely as possible, but also trying to get it as fast as possible.

Currently node.js own events implementation beats us, but we've gone from being 4x slower to something more like 2x.

Still debating which implementation details are best, such as

  • Object vs Map: they lead to different gains in different tests
  • An optimization where having only one listener is not an array but just the function itself. This seems to be slower than just having and looping over a length=1 array.

Also, setting real-world benchmarks that tests actual usage instead of than a single ".emit()". Microbenchmarks are handy but we need to consider the bigger picture.

required todo:

  • Replace the C++ module out for events.exports.js
  • Find the best implementations for addListener and emit
    • the array clone we have to do seems to be the slow part?
  • EventEmitter#once (implemented but check if can be made faster)
    • optimization: compare arrow fn vs the current bind solution.
  • EventEmitter#removeListener (reimplement)
  • EventEmitter#prependListener
  • EventEmitter#listenerCount
  • EventEmitter#types
  • EventEmitter.once (async iterator)
  • EventEmitter.setMaxListeners
  • EventEmitter.getEventListeners
  • error event
  • capturing rejections
  • validators

things might not do in this PR

  • Remove the C++ implementation, it's still in the code somewhere.
  • EventEmitter.on (async iterator)
  • EventEmitterAsyncResource

Fixes #2190

@paperclover paperclover changed the title implement node:events in javscript implement node:events in javascript Apr 10, 2023
@Jarred-Sumner

This comment was marked as resolved.

@paperclover paperclover changed the title implement node:events in javascript implement node:events in javascript + improve node createHash performance Apr 13, 2023
@paperclover paperclover changed the title implement node:events in javascript + improve node createHash performance implement node:events in javascript Apr 14, 2023
@github-actions

github-actions Bot commented Apr 15, 2023 •

Copy link
Copy Markdown
Contributor

✅ zig fmt errors have been resolved. Thank you.

#fe3260916318ebe8a9840666f3955bb23fc29162
zig v0.11.0-dev.2571+31738de28

@paperclover

Copy link
Copy Markdown
Contributor Author

3 opened issues for less important but missing features. passing all tests for everything else though

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

looks like there are unintentional changes to submodules

can you reset those?

Comment thread test/package.json Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Comment thread src/bun.js/events.exports.js Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make Array and Object come from primordials here

Suggested change
var { isPromise } = import.meta.primordials;
var { isPromise, Array, Object } = import.meta.primordials;

@Jarred-Sumner
Jarred-Sumner merged commit 96a2ed1 into main Apr 18, 2023
@Jarred-Sumner
Jarred-Sumner deleted the dave/events branch April 18, 2023 21:59
Jarred-Sumner added a commit that referenced this pull request Apr 21, 2023
paperclover added a commit that referenced this pull request Apr 24, 2023
paperclover added a commit that referenced this pull request May 17, 2023
paperclover added a commit that referenced this pull request May 23, 2023
paperclover added a commit that referenced this pull request May 30, 2023
paperclover added a commit that referenced this pull request Jun 1, 2023
paperclover added a commit that referenced this pull request Jun 1, 2023
Jarred-Sumner pushed a commit that referenced this pull request Jun 1, 2023
…2913)

* Revert "Revert "use a lazyily initialized stream for `node:crypto` `createHash` (#2652)""

This reverts commit 613bb48.

* Revert "Revert "implement `node:events` in javascript (#2604)""

This reverts commit a4d0a19.

* oops

* fix entrypoints stuff

* fix hash copy

* use native events for node streams and crypto

* requested changes

* oops

* make discord.js work

* fix webkit hash

* headers tojson
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.

EventEmitter performance issue causes Bun's node:crypto to be 20% slower than node.js's node:crypto

2 participants