Closed
Bug 610914
Opened 14 years ago
Closed 14 years ago
Pervasive crypto efficiency improvements in Sync backend
Categories
(Firefox :: Sync, enhancement)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta8+ |
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
Attachments
(1 file, 1 obsolete file)
14.14 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
Don't fetch info/collections twice on startup.
Comment 2•14 years ago
|
||
(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...
Assignee | ||
Comment 3•14 years ago
|
||
> 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.
Component: Firefox Sync: Backend → Firefox Sync: Crypto
QA Contact: sync-backend → sync-crypto
Summary: Pervasive efficiency improvements in Sync backend → Pervasive crypto efficiency improvements in Sync backend
Assignee | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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.
Attachment #495325 -
Attachment is obsolete: true
Attachment #495415 -
Flags: review?(mconnor)
Attachment #495325 -
Flags: review?(mconnor)
Comment 6•14 years ago
|
||
Comment on attachment 495415 [details] [diff] [review]
Memoizing HMAC and encryption key lookups, v1
Let's get this into the fx-sync merge.
Attachment #495415 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 7•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
blocking2.0: --- → beta8+
Comment 8•14 years ago
|
||
Backed out (the WeaveCrypto parts) as it causes Bug 618068
https://hg.mozilla.org/services/fx-sync/rev/6c7c1a5d5967
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•14 years ago
|
||
Definitely need to re-add that multiple decrypts test as soon as we ship... if it breaks, we need to fix.
Comment 10•14 years ago
|
||
What's next here?
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> What's next here?
Philipp backed out my breaking change. I filed a follow-on bug (Bug 618496).
Comment 12•14 years ago
|
||
(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?
Assignee | ||
Comment 13•14 years ago
|
||
(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.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: Firefox Sync: Crypto → Sync
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•