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

Reuse Encoder/Decoder instance? #172

Open
buu700 opened this issue Apr 29, 2021 · 6 comments
Open

Reuse Encoder/Decoder instance? #172

buu700 opened this issue Apr 29, 2021 · 6 comments

Comments

@buu700
Copy link

buu700 commented Apr 29, 2021

The readme says that encoding is 20% faster when reusing an Encoder instance. If that's the case, is there any reason not to just memoize them in encode (or at least reuse a default instance when no options are set)?

@gfx gfx added the Docs label Apr 30, 2021
@gfx
Copy link
Member

gfx commented Apr 30, 2021

That's a good question. There's an example in test/reuse-instances.test.ts, but they only accepts positional parameters right now, so I might change their interfaces to accept named parameters just like as encode({...}) and decode({...}).

@buu700
Copy link
Author

buu700 commented Apr 30, 2021

Oh, just to clarify, I wasn't asking about the documentation. I was suggesting an optimization to the default encode and decode functions / wondering whether there was a non-obvious reason that that hadn't been done.

@gfx
Copy link
Member

gfx commented Apr 30, 2021

Ah, sorry! Misunderstood.

There're some reasons that we cannot memorize them:

  • instance.*Async() are not async-await safe (or not thread-safe)
  • the instance has lots of configuration parameters so it's difficult to memoize

@buu700
Copy link
Author

buu700 commented Apr 30, 2021

Got it, that makes sense. If memoizing isn't an option, what about just pre-initializing a default instance that gets used when options is undefined? That would at least cover the most common case (I'm assuming).

@gfx gfx removed the Docs label Apr 30, 2021
@gfx
Copy link
Member

gfx commented Apr 30, 2021

Sounds good. Will try it.

@buu700
Copy link
Author

buu700 commented Apr 30, 2021

Nice, thanks! I'd be happy to submit a PR if that would be helpful.

buu700 added a commit to buu700/msgpack-javascript that referenced this issue May 4, 2021
buu700 added a commit to buu700/msgpack-javascript that referenced this issue May 5, 2021
buu700 added a commit to buu700/msgpack-javascript that referenced this issue May 5, 2021
buu700 added a commit to buu700/msgpack-javascript that referenced this issue May 5, 2021
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

No branches or pull requests

2 participants