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

Allow setting custom msgpack library to use #469

Closed
MasterOdin opened this issue Dec 2, 2022 · 3 comments
Closed

Allow setting custom msgpack library to use #469

MasterOdin opened this issue Dec 2, 2022 · 3 comments

Comments

@MasterOdin
Copy link
Contributor

I'd like to add in the ability to swap in a custom msgpack library to use. I'm making a proposal first before opening a PR just to see if there's support. This would somewhat mirror the existing capacity to bring your own packet parser to socket.io.

My proposal for the change would be to add a new key/value to RedisAdapterOptions of something like:

interface MsgpackInterface {
  decode: (data: any) => any;
  encode: (data: any) => any;
}

interface RedisAdapterOptions {
  // other options
  /**
   * MessagePack library to use for marshaling and un-marshalling data out of Redis.
   *
   * @default notepack.io
   */
  msgpack?: MsgpackInterface;
}

and then in the RedisAdapter constructor, have something like this.msgpack = opts.msgpack || msgpack, and then any future call to msgpack in the class would be replaced with this.msgpack. I've loosely tested this within my own application and haven't seen anything break, but I may be missing something on how this is used.

I'd make a similar change for the @socket.io/redis-emitter library as well.

For the motivation for this change, this library currently uses the notepack.io library to handle encoding/decoding messages into/out of redis. In benchmarking our application, we are finding that msgpackr library would give us a decent speed-up for the sorts of payloads we have (We also already use it in a number of other places in our stack):

Encoding (this will take a while):
+----------------------------+----------------+
|                            │ data           |
+----------------------------+----------------+
| notepack                   │ 25,242 ops/sec |
+----------------------------+----------------+
| msgpack-js                 │ 2,192 ops/sec  |
+----------------------------+----------------+
| msgpack-lite               │ 17,457 ops/sec |
+----------------------------+----------------+
| @msgpack/msgpack           │ 21,024 ops/sec |
+----------------------------+----------------+
| msgpackr                   │ 42,637 ops/sec |
+----------------------------+----------------+
| JSON.stringify (to Buffer) │ 16,157 ops/sec |
+----------------------------+----------------+

Decoding (this will take a while):
+--------------------------+----------------+
|                          │ data           |
+--------------------------+----------------+
| notepack                 │ 24,175 ops/sec |
+--------------------------+----------------+
| msgpack-js               │ 18,337 ops/sec |
+--------------------------+----------------+
| msgpack-lite             │ 10,894 ops/sec |
+--------------------------+----------------+
| @msgpack/msgpack         │ 19,320 ops/sec |
+--------------------------+----------------+
| msgparck                 │ 45,733 ops/sec |
+--------------------------+----------------+
| JSON.parse (from Buffer) │ 39,214 ops/sec |
+--------------------------+----------------+

As such, I'd like to have the ability to provide my own msgpack implementation to use for the adapter.

@darrachequesne
Copy link
Member

That sounds reasonable 👍

Minor nitpick: I don't think the interface should include "msgpack" in its name, as any format would work (protobuf for example).

@MasterOdin
Copy link
Contributor Author

Does parser work better for you?

@darrachequesne
Copy link
Member

Does parser work better for you?

Yes that sounds great 👍

darrachequesne pushed a commit that referenced this issue Dec 7, 2022
This PR adds a new parser option to the adapter constructor to allow
setting a custom parser to use, defaulting to using notepack.io. This
would allow someone to use a different msgpack library if they wanted,
or even an entirely different protocol altogether (e.g. protobuf).

Related:

- #462
- #469
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 a pull request may close this issue.

2 participants