1433003 - Use digestBytes and not digestUTF8 in CryptoWrapper.prototype.ciphertextHMAC

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

After poking around with the profiles from bug 1429250, I noticed/remembered that the top js function during a sync is Utils.digestUTF8. On syncs that don't spend all their time syncing a a bunch of addons (which is slow when it does anything, most of the time being spent waiting around -- although bug 633062 improved this a great deal last I checked), it seems like around 7%-8% of the total time of a sync is spent doing digestUTF8 (at least on my machine https://perfht.ml/2GfJuGy -- IIRC it was even more if I apply the patch in 663062 first to remove the few event loops we still have, but even without that it's still a significant amount of time).

This makes some sense, as it's a function computing a crypographic hash, and it seems plausible that sync is limited by the speed at which we can do crypto.

But when you look at the combined stack in the profile, not all the time is spent hashing. Some of it seems unaccounted for, and a decent amount is spent in xpconnect (NativeArrayToJS). Digging further, it turns out that the thing returning the native array is the `this._utf8Converter.convertToByteArray` call, and the unaccounted stuff is hard to say (I'm guessing it was in encoding-rs, which wasn't symbolicated properly or something, but I could be wrong and just missing it in the profile).

The thing is, the argument we pass to Utils.digestUTF8 is the base64-encoded ciphertext. Since it's base64 encoded, we know all the bytes are within the ASCII range, so there's no need to do any utf8 decoding, and so we can use `digestBytes` instead. (Unfortunately, digestBytes doesn't turn the string into a byte array particularly optimally, so in order to use that and improve performance, it needs a small amount of tweaking).

After all of this, the function is about 4x faster on a first sync (273ms before, 1166ms after). Afterwards, it tends to take about 2% of the time of the sync on syncs comparable to the one linked above.

Here are two profiles for comparison, you'll want to compare the time spent in digestUTF8 in before, to the time spent in digestBytes in after.

- before: https://perfht.ml/2GdyklB
- after: https://perfht.ml/2GfYBje

In order to determine whether or not this was even worth filing a bug for I wrote the patch, so patch incoming. (Admittedly, this possibly isn't the best use of my time, since its a pretty small gain, but hey, its 7PM here)

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1433003 - Optimize sync by using digestBytes instead of digestUTF8.

Review commit: https://reviewboard.mozilla.org/r/215494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215494/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8945275 [details]
Bug 1433003 - Optimize sync by using digestBytes instead of digestUTF8.

https://reviewboard.mozilla.org/r/215494/#review221156

LGTM, thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89ce3d9a44f6
Optimize sync by using digestBytes instead of digestUTF8. r=markh

Comment 4

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/89ce3d9a44f6