Closed dcousens closed 10 years ago
did you benchmark these changes?
I think I used the bl = this._block
pattern because it was slightly, but consistently faster.
I didn't, but this function is only called once in the hash's cycle, by comparison to the _update()
, this would take a negligible amount of the run time.
I'm happy to switch it back, but I'd bet this change wouldn't even be demonstrable in the noise created by _update()
.
edit: Perhaps we should add a benchmark suite for tests in future if that is desired?
yes, whether or not it can be demonstrated is the essential thing. I have some benchmarks for js crypto over here: https://github.com/dominictarr/crypto-bench (but it doesn't have sha512 yet)
I would like to benchmark the changes, but for now I've simply re-introduced that behaviour in https://github.com/dcousens/sha.js/commit/2358b025c2f826ab2d391f59ebf83c1e05132c01, albeit with clearer variable names.
Will squash if requested.
:+1:
Any further queries/qualms @dominictarr ?
This is great! Is there something that needs to be improved with this? If not, would you be willing to accept the PR? Thanks!
Just checking up on this @dominictarr, is there anything else that needs to be done?
okay this is merged into 2.2.2
Cheers!
I saw you had some trouble with the tests, apologies, I could have sworn I'd already done the changes you added.
Alas, I must have lost them when splitting up the PRs. Thanks though :)
okay no problem
On Tue, Sep 16, 2014 at 4:21 PM, Daniel Cousens notifications@github.com wrote:
Cheers! :)
I saw you had some trouble with the tests, apologies, I could have sworn I'd done the changes you added.
Alas, I must have lost them when splitting up the PRs. Thanks though :)
— Reply to this email directly or view it on GitHub https://github.com/dominictarr/sha.js/pull/8#issuecomment-55740701.
This pull request adds an implementation for SHA-512 into
sha.js
.To do so required fixing the logic in
hash.js
which, while it worked for"sha256"
and friends, was incorrect as per the NIST paper.Overall, not too many changes, mostly just the addition of the SHA512 implementation. I used node's
sha512
implementation to generate the test vectors.I intend to use these vectors: http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip But just haven't had the time to parse them yet.
Happy to amend this pull request per any feedback as necessary. I mostly just want to get this into
crypto-browserify
as soon as possible.