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

bench.js: Include Node results without the extra string conversion #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cakoose
Copy link

@cakoose cakoose commented Jan 19, 2019

I was surprised when I saw the extra string conversion in the benchmark
code. I think it may surprise others as well.

Though people who care about performance will probably be using Buffers
as input and output, I think it's worth making it clearer what the
benchmark is actually measuring.

It also helps explain why the performance difference is so large.

I was surprised when I saw the extra string conversion in the benchmark
code.  I think it may surprise others as well.

Though people who care about performance will probably be using Buffers
as input and output, I think it's worth making it clearer what the
benchmark is actually measuring.

It also helps explain why the performance difference is so large.
@lovell
Copy link
Owner

lovell commented Jan 19, 2019

Hello, thanks for this, as you say this module is fairly clear in the use case being Buffer-to-Buffer (de)coding.

If the benchmarks were to include test cases that either consume or respond with Strings then perhaps this module should provide new functions such as encodeAsString and decodeFromString to match if that's an API people would expect to be available.

In addition, if you're aware of a native Buffer method in Node to perform base64 encoding/decoding without the use of intermediate Strings then that would make a better benchmark test.

@cakoose
Copy link
Author

cakoose commented Jan 19, 2019

I do think that most people who come across this package will mistakenly think that it does the exact same thing as Buffer.from. That's definitely what I did when I came across it a few weeks ago.

And separately, I think it's generally valuable to give people a rough idea of what the Buffer-to-string conversion overhead is. Might make them rethink some of the things they're doing elsewhere :-)

Maybe separate charts for encode and decode will keep things readable?

Also reworked the benchmark charting code a bit.
@cakoose
Copy link
Author

cakoose commented Jan 19, 2019

Updated the pull request with a bunch of changes.

  • Split into two charts.
  • Tried to come up with intuitive names for Node built-in encode/decode implementations. Not thrilled with what I ended up with.
  • Made "bench.js" spit out something that's a little easier to copy paste.
  • Put benchmark results in an entirely separate file.

Not sure if these changes align with your preferences. Lemme know if you'd like me to do things differently. Or if it's easier for you, feel free to rework everything yourself -- authorship on the final commit is not important to me.

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.

2 participants