Closed Bug 978944 Opened 6 years ago Closed 6 years ago

Use Android native code pbkdf2sha256 (from NativeCrypto library) in FxAccountClient

Categories

(Firefox for Android :: Android Sync, defect, P2)

All
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 30+ ---

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Keywords: perf, Whiteboard: [parallel][polish][qa+])

Attachments

(1 file)

57 bytes, text/x-github-pull-request
rnewman
: review+
mcomella
: review+
Details | Review
For great perf win.  It might be wise to fallback to our Java code in the case when we can't load the library, but I'm not particularly concerned.
The relevant helper is

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/fxa/FxAccountUtils.java#107

and it's invoked from

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/fxa/QuickPasswordStretcher.java#28

Testing this across devices is more difficult than the fix, for sure.
Priority: P1 → P2
Whiteboard: [parallel][polish]
Summary: User NativeCrypto in FxAccountClient → Use Android native code pbkdf2sha256 (from NativeCrypto library) in FxAccountClient
Also measure the win!
Keywords: perf
Whiteboard: [parallel][polish] → [parallel][polish][qa+]
tracking-fennec: --- → 30+
Attached file github PR
mcomella: feel free to steal this r?.

As for quantifying the win: we go from seconds to milliseconds on my Samsung Galaxy SII.  I had to check that we were, in fact, showing the spinner at all.
Attachment #8390755 - Flags: review?(rnewman)
Attachment #8390755 - Flags: feedback?(michael.l.comella)
Attachment #8390755 - Flags: review?(rnewman) → review+
Comment on attachment 8390755 [details] [review]
github PR

r+ w/ nits.
Attachment #8390755 - Flags: feedback?(michael.l.comella) → review+
https://hg.mozilla.org/integration/fx-team/rev/f5109b04e083
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
(In reply to Nick Alexander :nalexander from comment #5)
> https://hg.mozilla.org/integration/fx-team/rev/f5109b04e083

Damn, wrong URL.  How is this not automated yet?

https://hg.mozilla.org/integration/fx-team/rev/fb1c7f3a3a77
(In reply to Nick Alexander :nalexander from comment #3)

> As for quantifying the win: we go from seconds to milliseconds on my Samsung
> Galaxy SII.  I had to check that we were, in fact, showing the spinner at
> all.

\o/
Posted in Bug 915312 inquiring about uplifting this pair. If it knocks down our memory footprint and has that dramatic an effect on the first-time experience for a user, IMO we should consider getting it out in 29 (and maybe the 400msec startup win in Bug 959652, too).

If we made a mistake, we have ~7 weeks to change our minds.
https://hg.mozilla.org/mozilla-central/rev/fb1c7f3a3a77
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Richard Newman [:rnewman] from comment #8)
> Posted in Bug 915312 inquiring about uplifting this pair. If it knocks down
> our memory footprint and has that dramatic an effect on the first-time
> experience for a user, IMO we should consider getting it out in 29 (and
> maybe the 400msec startup win in Bug 959652, too).

There were two things at play in the perf wins: this code *and* a server misconfiguration that caused the remote request to be significantly faster.  I still think we get significant perf wins from this code, just not "blink of an eye".

In any case, I think we should uplift if Bug 915312 lands cleanly on Aurora.
Bug 915312 has been uplifted to Aurora (29).
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.