1429245 - Optimize CommonUtils.bytesAsHex

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

CommonUtils.bytesAsHex [0] shows up on profiles and could be faster [1] if we changed the algorithm to concatenate the hex values to a string directly instead of using a intermediate array.

[0] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/services/common/utils.js#216
[1] https://jsperf.com/bytesashex

Comment 1

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

https://jsperf.com/bytes-as-hex/1 has a slightly more optimized version, uses possibly more realistic strings, and makes more of an attempt to avoid the JIT optimizing away the bulk of the work.

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1429245 - Optimize CommonUtils.bytesAsHex.

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

Comment 3

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

Comment on attachment 8941628 [details]
Bug 1429245 - Optimize CommonUtils.bytesAsHex.

https://reviewboard.mozilla.org/r/211868/#review217798

Clearing review as discussed in IRC.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8941628 [details]
Bug 1429245 - Optimize CommonUtils.bytesAsHex.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211868/diff/1-2/

Comment 5

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

Comment on attachment 8941628 [details]
Bug 1429245 - Optimize CommonUtils.bytesAsHex.

https://reviewboard.mozilla.org/r/211868/#review217800

Looks good to me! Can you double-check that we have test coverage for this function already? I'm pretty sure we do.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7e62d039551
Optimize CommonUtils.bytesAsHex. r=tcsc

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d7e62d039551

Comment 8

[Tom S [:evilpie]]

Why bytes[i].charCodeAt(0) and not bytes.charCodeAt(i) ?

Comment 9

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

I don't think there's a reason. We'd probably want to check that this is just as fast though (I mean, it seems very unlikely it matters), this function was at the top of most profiles during a sync.

Comment 10

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

To be clear, I probably should have caught that during the review :p