1019585 - Latin1 strings: support string.match, string.search and string.replace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

These are the more complicated functions, because they have to work with regular expressions and have all kinds of optimizations for flat strings and rope matching.

I'm going to handle the simple cases first and we'll see what's needed.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - StringMatch case

Just enough changes to make the StringMatch case work.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - RopeMatch case

Uses rope matching if all leaf nodes have the same encoding. This made it pretty easy to convert the code to handle Latin1 ropes/patterns.

I verified the test calls RopeMatchImpl (and removing the all-leaf-nodes-have-same-encoding check triggers assertion failures).

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8433331 [details] [diff] [review]
Part 1 - StringMatch case

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

::: js/src/jsstr.cpp
@@ +1827,5 @@
>  
>  /* Result of a successfully performed flat match. */
>  class FlatMatch
>  {
> +    RootedAtom pat;

Pre-existing, but can you append _ to pat?  Mixed use of _ looks tacky to me.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8433469 [details] [diff] [review]
Part 2 - RopeMatch case

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

::: js/src/jsstr.cpp
@@ +1398,4 @@
>              return false;
>          while (!r.empty()) {
> +            if (threshold-- == 0 ||
> +                r.front()->hasLatin1Chars() != text->hasLatin1Chars() ||

Might be worth it to hoist text->hasLatin1Chars(); I expect the compiler can't automatically.

Comment 5

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c9dcbc34c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b79650ac7b

There's more to do here, but it depends on the regexp engine. Yarr will hopefully be gone next week so I'll wait for that to avoid breaking it.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3c9dcbc34c0
https://hg.mozilla.org/mozilla-central/rev/f2b79650ac7b

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Fix JSSubString

This patch changes JSSubString from a (chars, length) struct to a (base, offset, length) struct. JSSubString isn't used across GC calls as far as I can see.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8440792 [details] [diff] [review]
Part 3 - Fix JSSubString

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

And now that it contains a raw JSString*, we'll hear about it from Hf if we ever do accidentally introduce GC while one is live. Nice!

Comment 9

[Jan de Mooij [:jandem]]

Part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f9d7d919af

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69f9d7d919af

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 4 - FindDollarIndex

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 5 - BuildDollarReplacement

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 6 - RegExp replacement

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 7 - flattenPattern

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 8 - StrReplaceRegexpRemove

And of course there's an optimization for the case where the replacement is the empty string... So many paths str_replace can take, but I think I got all of them.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8442100 [details] [diff] [review]
Part 4 - FindDollarIndex

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

r=me

Comment 17

[Terrence Cole [:terrence]]

Comment on attachment 8442101 [details] [diff] [review]
Part 5 - BuildDollarReplacement

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

::: js/src/jit-test/tests/latin1/replace.js
@@ +38,5 @@
> +    assertEq(s.replace(pat, "A$`\u1300"), "FooAFoo\u1300baz123");
> +    assertEq(s.replace(pat, "A$&\u1300"), "FooAbar\u1200\u1300baz123");
> +    assertEq(s.replace(pat, "A$'\u1300"), "FooAbaz123\u1300baz123");
> +}
> +testDollarReplacement();

Nice!

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 8442102 [details] [diff] [review]
Part 6 - RegExp replacement

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

r=me

Comment 19

[Terrence Cole [:terrence]]

Comment on attachment 8442103 [details] [diff] [review]
Part 7 - flattenPattern

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

r=me

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 8442104 [details] [diff] [review]
Part 8 - StrReplaceRegexpRemove

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

r=me

Comment 21

[Jan de Mooij [:jandem]]

Thanks for the quick reviews!

Parts 4 and 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ce40562f51
https://hg.mozilla.org/integration/mozilla-inbound/rev/e589c195f61d

Comment 22

[Jan de Mooij [:jandem]]

And parts 6-8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5358ae00940
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8e50b64058
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc071decd42d

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95ce40562f51
https://hg.mozilla.org/mozilla-central/rev/e589c195f61d
https://hg.mozilla.org/mozilla-central/rev/f5358ae00940
https://hg.mozilla.org/mozilla-central/rev/1a8e50b64058
https://hg.mozilla.org/mozilla-central/rev/fc071decd42d

Comment 24

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7ede108faf

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b7ede108faf