887437 - Optimize encodeURIComponent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

encodeURIComponent perf is critical for Kraken stanford-crypto-sha256.

Currently, for every char, Encode does a js_strchr on this table:

    {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
     'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
     'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
     'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm',
     'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
     '-', '_', '.', '!', '~', '*', '\'', '(', ')', 0};

Using a few range checks instead wins at least 12% on this test. Ideally we'd use the char as index in an array of booleans.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

The conversion from array-of-chars to array-of-booleans was a bit tedious but after that it was pretty straight-forward.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 768218 [details] [diff] [review]
Patch

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

Nice!

::: js/src/jsstr.cpp
@@ +4419,2 @@
>              if (!sb.append(c))
>                  return false;

We can remove the check now here, since you reserved it. Though I know there hasn't been a concensus about removing/leaving this. So I'm fine either way.

@@ +4564,5 @@
>      Rooted<JSLinearString*> str(cx, ArgToRootedString(cx, args, 0));
>      if (!str)
>          return false;
>  
> +    return Encode(cx, str, js_isUriUnescaped, js_isUriReservedPlusPound, args.rval());

Is there a particular reason the arguments got switched? Is that just because "str_encodeURI_Component" has js_isUriUnescaped on the first place?

Comment 4

[Jan de Mooij [:jandem]]

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f664c8b66d


(In reply to Hannes Verschore [:h4writer] from comment #3)
> We can remove the check now here, since you reserved it.

We reserve input.length chars, but encoding can result in a longer output string, for instance "#" -> "%23"

> Is there a particular reason the arguments got switched? Is that just
> because "str_encodeURI_Component" has js_isUriUnescaped on the first place?

Yeah that's one reason, but it's also a micro-optimization: js_isUriUnescaped includes more characters (and common ones like a-z, A-Z, 0-9) so it's a bit more efficient to test these first.

Comment 5

[Phil Ringnalda (:philor)]

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