Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

Perf improvements for sha1.js, sha2.js, and sha2_64b.js #391

Merged
merged 2 commits into from
Jan 15, 2015

Conversation

coruus
Copy link
Contributor

@coruus coruus commented Dec 26, 2014

Please see #322 for the original PR. This is rebased and squashed.

CC @meheffernan @nanaze

This is rebased and squashed. @nanaze: Are the changes sufficient to make this work on Kindle Fire?

closure/goog/crypt/sha1.js: Compute the message schedule on the fly to avoid allocating additional memory (and incurring additional garbage collections) unnecessarily. ~10% speed gain on Chrome 35.

closure/goog/crypt/sha1.js: Don't try to process more bytes from the message than the message's length.

closure/goog/crypt/sha12mc_test.js: Monte Carlo KATs for SHA-1, SHA2-224, SHA2-256, SHA2-384, and SHA2-512. (Note: this test takes a few minutes.)

closure/goog/crypt/sha2.js: Add helper functions for generating and using precomputed message schedules. Port the block-size fast path from Sha1.update to Sha2.update. Compute message schedule on the fly. Remove code for testing for invalid inputs that is not present in Sha1.update.  ~40% speed gain on Chrome 35.

closure/goog/crypt/sha256_test.html: Remove testBadInput(); no similar test is performed in sha1_test.html

closure/goog/crypt/sha256_test.js: Add some KATs.

closure/goog/crypt/sha256_test.js: Add test-case for 54-byte message. Add tests for precomputed message schedule case; cleaned up other tests somewhat.

closure/goog/crypt/sha256_test.js: Rename tests, synchronize with sha1_test.js.

closure/goog/crypt/sha2_64bit.js: Added length fix from sha1.js and sha2.js

new ones repeatedly; update message schedule in-place. 20% performance
@Ubehebe
Copy link
Contributor

Ubehebe commented Jan 16, 2015

@coruus @meheffernan This PR causes at least five unit tests to fail:

closure/crypt/sha12mc_test
com.google.testing.javascript.runner.core.TimeoutError: Test timed out in 240.105 seconds after exceeding timeout of 240 seconds.
G_testRunner not detected: test failed to load
closure/crypt/hmac_test.testHmac
com.google.testing.javascript.runner.core.JavaScriptFailure: ERROR in testHmac
test 1 failed
Expected <b617318655057264e28bc0b6fb378c8ef146be00> (String) but was <0bd1bed50cc0b29f08a8c4390f6667100dd60ef0> (String)
closure/crypt/pbkdf2_test.testPBKDF2
com.google.testing.javascript.runner.core.JavaScriptFailure: ERROR in testPBKDF2
mismatch at index 0: 
Expected <12> (Number) but was <2> (Number)
closure/crypt/sha1_test.testHashing
com.google.testing.javascript.runner.core.JavaScriptFailure: ERROR in testHashing
Expected <da39a3ee5e6b4b0d3255bfef95601890afd80709> (String) but was <0a39a3ee0e6b4b0d0255bfef056018900fd80709> (String)
closure/crypt/sha1_test.testLength
com.google.testing.javascript.runner.core.JavaScriptFailure: ERROR in testLength
Expected <a9993e364706816aba3e25717850c26c9cd0d89d> (String) but was <02957e6f0a07ff52028650bc02c2e22b0030929a> (String)

I've rewritten the history of the project so that this PR was never merged. We need a better process for testing PRs before merging, to avoid headaches like this.

@coruus
Copy link
Contributor Author

coruus commented Jan 17, 2015

Hmm. I'm somewhat perplexed: When I tested this recently, I'm fairly
certain that all tests were passing.

Is this on a specific target, or all targets?

I know that all tests were passing on V8/amd64 when I first submitted the
previous PR...

In that thread, there was a mention of some platform-specific issues,
though?

Do you have a preferred CI setup that I can test against?

@Ubehebe
Copy link
Contributor

Ubehebe commented Jan 20, 2015

@coruus In order to land this PR, you'll need to point me to a CI setup showing all Closure unit tests passing. I'd like to set up Travis for this project, but I can't commit to doing it anytime soon. Maybe you can help?

@coruus
Copy link
Contributor Author

coruus commented Jan 21, 2015

Sounds like fun.
On Tue, Jan 20, 2015 at 11:34 AM Brendan Linn notifications@github.com
wrote:

@coruus https://github.com/coruus In order to land this PR, you'll need
to point me to a CI setup showing all Closure unit tests passing. I'd like
to set up Travis for this project, but I can't commit to doing it anytime
soon. Maybe you can help?


Reply to this email directly or view it on GitHub
#391 (comment)
.

@coruus
Copy link
Contributor Author

coruus commented Jan 29, 2015

Hmm. So, actually, the problem was fairly trivial (and not platform-specific). Here's the diff:

diff --git a/closure/goog/crypt/sha1.js b/closure/goog/crypt/sha1.js
index a1979ea..a96cc96 100644
--- a/closure/goog/crypt/sha1.js
+++ b/closure/goog/crypt/sha1.js
@@ -179,7 +179,7 @@ goog.crypt.Sha1.prototype.compress_ = function(buf, opt_offset) {
   // performance by about 10% on Chrome 35.
   for (i = 16; i < 80; i++) {
     t = W[(i - 3) & 15] ^ W[(i - 8) & 15] ^ W[(i - 14) & 15] ^ W[i & 15];
-    W[i & 15] = (((t << 1) | (t >>> 31)) & 0xfffffff) | 0;
+    W[i & 15] = (((t << 1) | (t >>> 31)) & 0xffffffff) | 0;
     if (i < 40) {
       if (i < 20) {
         f = d ^ (b & (c ^ d));
@@ -206,11 +206,11 @@ goog.crypt.Sha1.prototype.compress_ = function(buf, opt_offset) {
     a = t;
   }

-  this.chain_[0] = ((this.chain_[0] + a) & 0xfffffff) | 0;
-  this.chain_[1] = ((this.chain_[1] + b) & 0xfffffff) | 0;
-  this.chain_[2] = ((this.chain_[2] + c) & 0xfffffff) | 0;
-  this.chain_[3] = ((this.chain_[3] + d) & 0xfffffff) | 0;
-  this.chain_[4] = ((this.chain_[4] + e) & 0xfffffff) | 0;
+  this.chain_[0] = ((this.chain_[0] + a) & 0xffffffff) | 0;
+  this.chain_[1] = ((this.chain_[1] + b) & 0xffffffff) | 0;
+  this.chain_[2] = ((this.chain_[2] + c) & 0xffffffff) | 0;
+  this.chain_[3] = ((this.chain_[3] + d) & 0xffffffff) | 0;
+  this.chain_[4] = ((this.chain_[4] + e) & 0xffffffff) | 0;
 };

@coruus
Copy link
Contributor Author

coruus commented Jan 29, 2015

And see #404 for the fixed version.

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

Successfully merging this pull request may close these issues.

2 participants