There's a lot of base64 going on, as well as expensive creation of NSS constructs (see Bug 603489). It would be great to spend some time focusing on efficiency.

Don't fetch info/collections twice on startup.

(In reply to comment #1) > Don't fetch info/collections twice on startup. It sounds like this bug is mostly about the crypto component. If so we should reflect that in Component and Summary and deal with the info/collections problem elsewhere...

> It sounds like this bug is mostly about the crypto component. If so we should > reflect that in Component and Summary and deal with the info/collections > problem elsewhere... Well, one such fetch was introduced in Bug 603489 for key management purposes, and the other one is password verification, so I think it's fair to classify that as a crypto-ish efficiency problem. Good point, though; done.

Here's one. Svc.Crypto.keyFromString is expensive, and it turns out that the result can be safely reused. Rewriting KeyBundle to keep the result of hmacKeyObject() around results in substantial space and time savings: a simple test of fetching that object and calling sha256HMACBytes took 10,000 iterations: New keyFromString each time: make check-one 4.39s user 0.52s system 97% cpu 5.025 total nsStringStats => mAllocCount: 22606 => mReallocCount: 345 => mFreeCount: 22606 => mShareCount: 7058 => mAdoptCount: 253 => mAdoptFreeCount: 253 <<<<<<< vs make check-one 3.28s user 0.39s system 98% cpu 3.733 total nsStringStats => mAllocCount: 12606 => mReallocCount: 345 => mFreeCount: 12606 => mShareCount: 7058 => mAdoptCount: 253 => mAdoptFreeCount: 253 <<<<<<< This gets called for every WBO we decrypt, so 10,000 is realistic for a first sync! Patch attached.

This patch memoizes: * hmacKeyObject, which avoids a keyFromString call on each HMAC verification (once per downloaded WBO) * PK11_ImportSymKey and WeaveCrypto.makeSECItem for symmetric keys, which avoids base64 decoding, NSS futzing, and allocation/deallocation of NSS key objects on every encrypt/decrypt call. Memoization was chosen as the approach to avoid altering the encrypt/decrypt function signatures. When we no longer support 3.x we can dump NSS symmetric keys right into KeyBundle and avoid all this base64 schlepping. Note that the PK11_ImportSymKey memoization technically leaks keys -- we never free the keys stored in the memo. This is of negligible concern to me, because the average Sync user will have precisely one such key, and the memoized lifetime is simply a stable version of the transient key generations that would otherwise take place; i.e., rather than _n_n_n_n_n_n_ we have _------------ 89 tests pass for me. My (albeit informal) repeated decryption and HMAC tests indicate a 25-50% speedup on these operations, and probably enormous memory savings on long syncs. Right now we do tons of redundant key manipulations on every WBO we process, and this memoization eliminates much of the problem. On Monday I'll put together another Fennec try build, see what kind of first sync performance difference we get versus Old Crypto.

Comment on attachment 495415 [details] [diff] [review] Memoizing HMAC and encryption key lookups, v1 Let's get this into the fx-sync merge.

In fx-sync: http://hg.mozilla.org/services/fx-sync/rev/5cf75b5c40c2

Backed out (the WeaveCrypto parts) as it causes Bug 618068 https://hg.mozilla.org/services/fx-sync/rev/6c7c1a5d5967

Definitely need to re-add that multiple decrypts test as soon as we ship... if it breaks, we need to fix.

What's next here?

(In reply to comment #10) > What's next here? Philipp backed out my breaking change. I filed a follow-on bug (Bug 618496).

(In reply to comment #11) > (In reply to comment #10) > > What's next here? > > Philipp backed out my breaking change. I filed a follow-on bug (Bug 618496). So, this no longer blocks b8? Is that right?

(In reply to comment #12) > > Philipp backed out my breaking change. I filed a follow-on bug (Bug 618496). > > So, this no longer blocks b8? Is that right? Philipp is AFK, but as far as I know this has been merged: http://hg.mozilla.org/mozilla-central/rev/88a632f17a9e so I will close.