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

util: reverse buffers instead of strings #745

Merged
merged 2 commits into from
Apr 5, 2019

Conversation

pinheadmz
Copy link
Member

This PR is just a cleanup from the recent buffers-instead-of-strings switchover.
Although these methods do not get called rapidly (they're mainly for JSON responses and API calls)
@BluSyn was able to observe a performance improvement:

~50% faster when compared over 1000000 iterations
difference is ~1300ms vs. ~600ms over 1m iterations

@pinheadmz pinheadmz added the quick Can be fixed quickly, code change less than 10 lines label Apr 2, 2019
@codecov-io

This comment has been minimized.

@tynes
Copy link
Member

tynes commented Apr 3, 2019

Good find!

@braydonf
Copy link
Contributor

braydonf commented Apr 4, 2019

Is it worth it to include a small benchmark in ./bench?

@pinheadmz
Copy link
Member Author

@braydonf Bench test added, results testing against master:

--> git checkout master
--> node bench/hexstring.js
hexstring: ops=100000, time=0.298963409, rate=334489.09462
--> git checkout hashstring
--> node bench/hexstring.js
hexstring: ops=100000, time=0.140646259, rate=711003.62506

@braydonf
Copy link
Contributor

braydonf commented Apr 5, 2019

Looks like a 2 to 2 1/4 times faster, see about the same results.

@braydonf
Copy link
Contributor

braydonf commented Apr 5, 2019

Was taking a look at when reverse() was introduced (I think since v4) at the Node.js documentation, and the method is apparently not documented, see https://nodejs.org/docs/latest-v10.x/api/buffer.html

@braydonf
Copy link
Contributor

braydonf commented Apr 5, 2019

Okay, so it's not documented because Buffer extends Uint8Array and Uint8Array is documented.

@pinheadmz
Copy link
Member Author

Ok great, thanks for checking. was just going to add, reverse() is working in node version 8.5 and above

@braydonf
Copy link
Contributor

braydonf commented Apr 5, 2019

Yeah, it's available from v4 and above (and possible some of the iojs releases in-between).

@braydonf braydonf merged commit fc7fd3c into bcoin-org:master Apr 5, 2019
braydonf added a commit that referenced this pull request Apr 5, 2019
util: reverse buffers instead of strings
@braydonf braydonf added this to the 2.0.0 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick Can be fixed quickly, code change less than 10 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants