Bug 610914: performance improvements for WeaveCrypto.

services/crypto/modules/WeaveCrypto.js

@@ -499,11 +499,22 @@ WeaveCrypto.prototype = {
         return "" + outputBuffer.readString() + "";
     },
 
+    _importSymKey: function _importSymKey(slot, mechanism, origin, op, keyItem) {
+        let symKey = this.nss.PK11_ImportSymKey(slot, mechanism, origin, op, keyItem.address(), null);
+        if (symKey.isNull())
+            throw Components.Exception("symkey import failed", Cr.NS_ERROR_FAILURE);
+        return symKey;
+    },
+    
+    _freeSymKey: function _freeSymKey(symKey) {
+        if (symKey && !symKey.isNull())
+            this.nss.PK11_FreeSymKey(symKey);
+    },
 
     _commonCrypt : function (input, output, symmetricKey, iv, operation) {
         this.log("_commonCrypt() called");
         // Get rid of the base64 encoding and convert to SECItems.
-        let keyItem = this.makeSECItem(symmetricKey, true);
+        let keyItem = this.makeSECItem(symmetricKey, true, true);
         let ivItem  = this.makeSECItem(iv, true);
 
         // Determine which (padded) PKCS#11 mechanism to use.
@@ -523,10 +534,7 @@ WeaveCrypto.prototype = {
             if (slot.isNull())
                 throw Components.Exception("can't get internal key slot", Cr.NS_ERROR_FAILURE);
 
-            symKey = this.nss.PK11_ImportSymKey(slot, mechanism, this.nss.PK11_OriginUnwrap, operation, keyItem.address(), null);
-            if (symKey.isNull())
-                throw Components.Exception("symkey import failed", Cr.NS_ERROR_FAILURE);
-
+            symKey = this._importSymKey(slot, mechanism, this.nss.PK11_OriginUnwrap, operation, keyItem);
             ctx = this.nss.PK11_CreateContextBySymKey(mechanism, operation, symKey, ivParam);
             if (ctx.isNull())
                 throw Components.Exception("couldn't create context for symkey", Cr.NS_ERROR_FAILURE);
@@ -557,8 +565,7 @@ WeaveCrypto.prototype = {
         } finally {
             if (ctx && !ctx.isNull())
                 this.nss.PK11_DestroyContext(ctx, true);
-            if (symKey && !symKey.isNull())
-                this.nss.PK11_FreeSymKey(symKey);
+            this._freeSymKey(symKey);
             if (slot && !slot.isNull())
                 this.nss.PK11_FreeSlot(slot);
             if (ivParam && !ivParam.isNull())
@@ -693,7 +700,6 @@ WeaveCrypto.prototype = {
         return new this.nss_t.SECItem(this.nss.SIBUFFER, outputData, outputData.length);
     },
 
-
     /**
      * Returns the expanded data string for the derived key.
      */
@@ -751,3 +757,45 @@ WeaveCrypto.prototype = {
     }
     },
 };
+
+// Memoize makeSECItem for symmetric keys.
+WeaveCrypto.prototype.makeSECItem =
+(function (orig) {
+  let memo = {};
+
+  return function(input, isEncoded, memoize) {
+    if (memoize) {
+      let memoKey = "" + input + !!isEncoded;
+      let val = memo[memoKey];
+      if (!val) {
+        val = orig.apply(this, arguments);
+        memo[memoKey] = val;
+      }
+      return val;
+    }
+    return orig.apply(this, arguments);
+  };
+}(WeaveCrypto.prototype.makeSECItem));
+
+WeaveCrypto.prototype._importSymKey =
+(function (orig) {
+  let memo = {}
+  
+  return function(slot, mechanism, origin, op, keyItem) {
+    // keyItem lookup is already memoized, so we can directly use the address.
+    // Slot changes each time. Don't use it as memo key input.
+    let memoKey = "" + "-" + mechanism +
+                  origin + op + "-" + keyItem.address();
+    let val = memo[memoKey];
+    if (!val) {
+      val = orig.apply(this, arguments);
+      memo[memoKey] = val;
+    }
+    return val;
+  };
+}(WeaveCrypto.prototype._importSymKey));
+
+// Yes, this leaks. However, _importSymKey is now memoized, so the average user
+// will have only a single key, which we persist for the lifetime of the
+// session...
+WeaveCrypto.prototype._freeSymKey = function(symKey) {};

services/crypto/tests/unit/test_crypto_crypt.js

@@ -8,7 +8,69 @@ try {
                 .getService(Ci.IWeaveCrypto);
 }
 
-function run_test() {
+function weavecrypto_memo() {
+  if (!cryptoSvc._importSymKey)
+    return
+      
+  let w = new WeaveCrypto();
+  let key = w.generateRandomKey();
+  let keyItem1 = w.makeSECItem(key, true, true);
+  let keyItem2 = w.makeSECItem(key, true, true);
+  do_check_eq(keyItem1, keyItem2);
+  
+  do_check_eq("" + w.nss.PK11_AlgtagToMechanism(w.algorithm),
+              "" + w.nss.PK11_AlgtagToMechanism(w.algorithm));
+  
+  let symKey1 =
+    w._importSymKey(w.nss.PK11_GetInternalKeySlot(),
+                    w.nss.PK11_AlgtagToMechanism(w.algorithm),
+                    w.nss.PK11_OriginUnwrap,
+                    w.nss.CKA_DECRYPT, 
+                    keyItem1);
+  let symKey2 =
+    w._importSymKey(w.nss.PK11_GetInternalKeySlot(),
+                    w.nss.PK11_AlgtagToMechanism(w.algorithm),
+                    w.nss.PK11_OriginUnwrap,
+                    w.nss.CKA_DECRYPT, 
+                    keyItem1);
+  do_check_eq(symKey1, symKey2);
+}
+
+/*
+With memoization:
+make check-one  10.39s user 0.75s system 100% cpu 11.041 total
+nsStringStats
+ => mAllocCount:           1923
+ => mReallocCount:          306
+ => mFreeCount:            1923
+ => mShareCount:           6764
+ => mAdoptCount:            101
+ => mAdoptFreeCount:        101
+<<<<<<<
+
+Without memoization, it crashes after a few thousand iterations... and 5610 take
+make check-one  7.57s user 0.67s system 101% cpu 8.105 total
+nsStringStats
+ => mAllocCount:           1923
+ => mReallocCount:          306
+ => mFreeCount:            1923
+ => mShareCount:           6764
+ => mAdoptCount:            101
+ => mAdoptFreeCount:        101
+<<<<<<<
+*/ 
+function multiple_decrypts(iterations) {
+  let iv = cryptoSvc.generateRandomIV();
+  let key = cryptoSvc.generateRandomKey();
+  let cipherText = cryptoSvc.encrypt("Hello, world.", key, iv);
+  
+  for (let i = 0; i < iterations; ++i) {
+    let clearText = cryptoSvc.decrypt(cipherText, key, iv);
+    do_check_eq(clearText + " " + i, "Hello, world. " + i);
+  }
+}
+  
+function test_encryption() {
   // First, do a normal run with expected usage... Generate a random key and
   // iv, encrypt and decrypt a string.
   var iv = cryptoSvc.generateRandomIV();
@@ -162,3 +224,9 @@ function run_test() {
   do_check_true(failure);
 
 }
+
+function run_test() {
+  weavecrypto_memo();
+  multiple_decrypts(6000);
+  test_encryption();
+}

services/sync/modules/base_records/crypto.js

@@ -323,6 +323,9 @@ function KeyBundle(realm, collectionName, keyStr) {
   Identity.call(this, realm, collectionName, keyStr);
   this._hmac    = null;
   this._encrypt = null;
+  
+  // Cache the key object.
+  this._hmacObj = null;
 }
 
 KeyBundle.prototype = {
@@ -345,11 +348,11 @@ KeyBundle.prototype = {
   
   set hmacKey(value) {
     this._hmac = value;
+    this._hmacObj = value ? Utils.makeHMACKey(value) : null;
   },
   
   get hmacKeyObject() {
-    if (this.hmacKey)
-      return Utils.makeHMACKey(this.hmacKey);
+    return this._hmacObj;
   },
 }
 
@@ -383,7 +386,7 @@ BulkKeyBundle.prototype = {
       let hm = value[1];
       
       this.password = json;
-      this._hmac    = Utils.safeAtoB(hm);
+      this.hmacKey  = Utils.safeAtoB(hm);
       this._encrypt = en;          // Store in base64.
     }
     else {
@@ -413,7 +416,8 @@ SyncKeyBundle.prototype = {
 
   set keyStr(value) {
     this.password = value;
-    this._hmac = null;
+    this._hmac    = null;
+    this._hmacObj = null;
     this._encrypt = null;
     this.generateEntry();
   },
@@ -437,6 +441,12 @@ SyncKeyBundle.prototype = {
     return this._hmac;
   },
   
+  get hmacKeyObject() {
+    if (!this._hmacObj)
+      this.generateEntry();
+    return this._hmacObj;
+  },
+  
   /*
    * If we've got a string, hash it into keys and store them.
    */
@@ -460,7 +470,10 @@ SyncKeyBundle.prototype = {
       
       // Save them.
       this._encrypt = btoa(enc);
-      this._hmac    = hmac;
+      
+      // Individual sets: cheaper than calling parent setter.
+      this._hmac = hmac;
+      this._hmacObj = Utils.makeHMACKey(hmac);
     }
   }
 };

services/sync/tests/unit/test_keys.js

@@ -4,6 +4,30 @@ Cu.import("resource://services-sync/base_records/crypto.js");
 Cu.import("resource://services-sync/constants.js");
 btoa = Cu.import("resource://services-sync/util.js").btoa;
 
+function test_time_keyFromString(iterations) {
+  let k;
+  let o;
+  let b = new BulkKeyBundle();
+  let d = Utils.decodeKeyBase32("ababcdefabcdefabcdefabcdef");
+  b.generateRandom();
+  
+  _("Running " + iterations + " iterations of hmacKeyObject + sha256HMACBytes.");
+  for (let i = 0; i < iterations; ++i) {
+    let k = b.hmacKeyObject;
+    o = Utils.sha256HMACBytes(d, k);
+  }
+  do_check_true(!!o);
+  _("Done.");
+}
+
+function test_repeated_hmac() {
+  let testKey = "ababcdefabcdefabcdefabcdef";
+  let k = Utils.makeHMACKey("foo");
+  let one = Utils.sha256HMACBytes(Utils.decodeKeyBase32(testKey), k);
+  let two = Utils.sha256HMACBytes(Utils.decodeKeyBase32(testKey), k);
+  do_check_eq(one, two);
+}
+
 function test_keymanager() {
   let testKey = "ababcdefabcdefabcdefabcdef";
   
@@ -156,4 +180,8 @@ function run_test() {
   test_keymanager();
   test_collections_manager();
   test_key_persistence();
+  test_repeated_hmac();
+  
+  // Only do 1,000 to avoid a 5-second pause in test runs.
+  test_time_keyFromString(1000);
 }