1404385 - atob() whitespace processing is slow even when not needed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: atob() microbenchmark

The whitespace removal code from bug 711180 always copies the input to a new string, even if there is no whitespace to remove. The attached microbenchmark shows converting 100MB takes over 2 seconds in current Nightly (58.0a1 (2017-09-29)) but around 700ms in current Chrome Canary (63.0.3225.0). This shows up in profiles of screenshots, for instance https://perfht.ml/2fDVOId .

I have a patch for this which simply doesn't make the copy unless needed.

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Don't copy if not needed for whitespace removal

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8913803 [details] [diff] [review]
Don't copy if not needed for whitespace removal

Seems OK on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f223f0aee8642150288bbda5542a04a4b076af

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8913803 [details] [diff] [review]
Don't copy if not needed for whitespace removal

>+    trimmedString.Append(aAsciiBase64String.BeginReading(), cur - start);
Why not pass start as the first param? It is after all const char16_t* start = aAsciiBase64String.BeginReading();


Please ensure there are testcases for this: whitespace at the beginning, somewhere in the middle and at the end.
And also a case when there is only whitespace and no whitespace at all.

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Don't copy if not needed for whitespace removal

New patch with the change recommended in comment 3 and a new test, carrying forward r+ as the code didn't change meaningfully. Some of these test cases were already covered by testing/web-platform/tests/html/webappapis/atob/base64.html but I wanted to be sure I covered all reasonable cases.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0efa71a9a60a
Don't copy if not needed for whitespace removal. r=smaug

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0efa71a9a60a