1488192 - Return the input when no characters were decoded/encoded in decodeURI/encodeURI

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Attachment: not-for-checkin-count-decode-encode.patch

It looks like {de,en}codeURI[Component] are quite often called on websites even though no characters need to be decoded resp. encoded. 

For example I got the following results when applying the attached patch and then browsing some news and Alexa 50 sites:

(gdb) p Decode_IdenticalTransferCount
$1 = 3592393
(gdb) p Decode_NonIdenticalTransferCountLatin1
$2 = 177340
(gdb) p Decode_NonIdenticalTransferCountUTF16
$3 = 809
(gdb) p Encode_IdenticalTransferCount
$4 = 445947
(gdb) p Encode_NonIdenticalTransferCountLatin1
$5 = 38846
(gdb) p Encode_NonIdenticalTransferCountUTF16
$6 = 2335

with
- Decode_IdenticalTransferCount: decodeURI and decodeURIComponent called and no characters needed to be decoded.
- Decode_NonIdenticalTransferCountLatin1: decodeURI and decodeURIComponent called, some characters were decoded, input was Latin-1.
- Decode_NonIdenticalTransferCountUTF16: decodeURI and decodeURIComponent called, some characters were decoded, input was UTF-16.
- Encode_IdenticalTransferCount: encodeURI and encodeURIComponent called and no characters needed to be encoded.
- Encode_NonIdenticalTransferCountLatin1: encodeURI and encodeURIComponent called, some characters were encoded, input was Latin-1.
- Encode_NonIdenticalTransferCountUTF16: encodeURI and encodeURIComponent called, some characters were encoded, input was UTF-16.

Comment 1

[André Bargull [:anba]]

Attachment: decode-encode-counter.txt

More detailed results for a couple of sites

Comment 2

[André Bargull [:anba]]

Attachment: bug-1488192.patch

Modifies Encode(...) and Decode(...) to append string ranges to the StringBuilder instead of single characters and leave the StringBuilder empty when no characters were decoded/encoded, in which case the callers can return the input string unchanged.


Drive-by changes:
- Remove the unnecessary null-character terminator in |hexBuf| in the Encode() function.
- Modified [1] to call StringBuffer::append(Latin1Char) instead of StringBuffer::append(char16_t), because the former should be slightly faster. (Unless the compiler already figured out that the input is definitely a Latin-1 characters, because |B < 128| is true.)
- Correct the OOM handling in DebugState::debugDisplayURL() to check for |cx->isThrowingOutOfMemory()|. Also added an assertion for "over-recursed" exceptions, which probably don't happen when calling |EncodeURI|, but if they actually do happen (and the assertion fails), we should change the code to handle over-recursed errors similar to OOM errors.

[1] https://searchfox.org/mozilla-central/rev/721842eed881c7fcdccb9ec0fe79e4e6d4e46604/js/src/builtin/String.cpp#3890,3895-3896

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 9005999 [details] [diff] [review]
bug-1488192.patch

Review of attachment 9005999 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, great find. That eliminates a lot of string allocations.

::: js/src/builtin/String.cpp
@@ +3755,3 @@
>  {
> +    if (!sb.empty()) {
> +        str = sb.finishString();

I was wondering about the empty-input-string case, but I see finishString returns cx->names().empty if length == 0, so that will be optimized correctly :)

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c2058bd3ad33ff36f4f32d760e417e53e099cb

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b29fcce16f
Return input if no characters were modified in decode/encodeURI. r=jandem

Comment 6

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

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