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

little optimizations #11

Merged
merged 5 commits into from
Apr 9, 2016
Merged

little optimizations #11

merged 5 commits into from
Apr 9, 2016

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Feb 29, 2016

Not so much:

old:

$ SEED=123 node bench-hash.js create-hash ripemd160
...
76, 4466835, 13915.373831775702, 321
77, 5463865, 13728.304020100502, 398
78, 6683439, 13866.056016597511, 482
79, 8175230, 13903.452380952382, 588
80, 10000000, 14285.714285714286, 700

new:

$ SEED=123 node bench-hash.js create-hash ripemd160
...
76, 4466835, 13060.921052631578, 342
77, 5463865, 14045.925449871465, 389
78, 6683439, 14250.402985074626, 469
79, 8175230, 14317.39054290718, 571
80, 10000000, 14684.287812041115, 681

cc @dcousens
related PR: #7

@dcousens
Copy link
Member

TBH that looks to be within statistic variations?

@fanatid
Copy link
Contributor Author

fanatid commented Feb 29, 2016

yes, I also think so
but I should note that I removed byte swapping for words

@dcousens
Copy link
Member

That alone is probably worth it. ACK

@dcousens
Copy link
Member

Though is that less readable/reviewable?

@fanatid
Copy link
Contributor Author

fanatid commented Feb 29, 2016

Probably is little more readable because I remove some extra lines.

@dcousens
Copy link
Member

dcousens commented Apr 9, 2016

Why the close? I somehow missed this.

@dcousens dcousens self-assigned this Apr 9, 2016
@dcousens dcousens reopened this Apr 9, 2016
@dcousens dcousens merged commit 2fcf435 into browserify:master Apr 9, 2016
@dcousens
Copy link
Member

dcousens commented Apr 9, 2016

Thanks @fanatid, ping me next time :).
Agreed, this is much clearer.

@dcousens
Copy link
Member

dcousens commented Apr 9, 2016

but I should note that I removed byte swapping for words

For what it is worth, I misunderstood this entirely, thinking you'd changed the algorithm to not use byte-swapping at all.
Instead, you just now use the standard Buffer functions (!!!), perfect.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants