956051 - inline str_replace with string, string as arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Romain Perier]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131212154635

Steps to reproduce:

We should be able to inline str.replace(str1, str2), when both arguments are of type string.

Bug https://bugzilla.mozilla.org/show_bug.cgi?id=951947 has already shown some advantages with this technic.

Comment 1

[Romain Perier]

Attachment: inlining str_replace(string,string) version 1

See my initial patch in attachment.

Comment 2

[Romain Perier]

Attachment: inlining str_replace(string,string) version 2

Adding setMovable and !isEffectFul attributes

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8359838 [details] [diff] [review]
inlining str_replace(string,string) version 2

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

LGTM, Nice work :)

::: js/src/jit/MIR.h
@@ +4901,5 @@
>  
>    public:
> +
> +    AliasSet getAliasSet() const {
> +        return AliasSet::None();

Nice :)

Is this true for regexp?

::: js/src/jit/VMFunctions.cpp
@@ +927,5 @@
>  }
>  
> +JSString *
> +string_replace(JSContext *cx, HandleString string, HandleString pattern, HandleString repl)
> +{

style-nit: We mostly use CaMl case.

This code is probably coming from some legacy C-like interface, as these functions are declared in VMFunction.h, we are not expecting to have them exposed to anything else than the Jit, so we should use the CaMl case here and above (regexp_replace).

::: js/src/jsstr.cpp
@@ +2882,5 @@
> +static const uint32_t ReplaceOptArg = 2;
> +
> +bool
> +js::str_replace_string_raw(JSContext *cx, HandleString string, HandleString pattern,
> +                       HandleString replacement, MutableHandleValue rval)

style-nit: indent at the level of the opening brace.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8359838 [details] [diff] [review]
inlining str_replace(string,string) version 2

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

Resetting review. A single review is fine usually, unless you're doing something really complicated or are touching different parts of the code base.

::: js/src/jit/MCallOptimize.cpp
@@ +1199,5 @@
>  
>      callInfo.setImplicitlyUsedUnchecked();
>  
> +    MInstruction *cte;
> +    if (callInfo.getArg(0)->type() == MIRType_String)

Nit: multi-line if-body (and else-body), so use { } (with { on same line as if/else)

::: js/src/jit/MIR.h
@@ +4900,5 @@
>      }
>  
>    public:
> +
> +    AliasSet getAliasSet() const {

As Nicolas said, I think this should be in MStringReplace. RegExps can do weird things, see the g (global) modifier for instance.

::: js/src/jit/VMFunctions.cpp
@@ +930,5 @@
> +string_replace(JSContext *cx, HandleString string, HandleString pattern, HandleString repl)
> +{
> +    JS_ASSERT(!!string);
> +    JS_ASSERT(!!pattern);
> +    JS_ASSERT(!!repl);

Nit: remove the !!.

Comment 5

[Romain Perier]

Attachment: inlining str_replace(string,string) version 3

Comment 6

[Romain Perier]

Attachment: inlining str_replace(string,string) version 4

Refreshed and rebased on trunk

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8360995 [details] [diff] [review]
inlining str_replace(string,string) version 4

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

LGTM, though I only skimmed it because it got review from nbp already :)

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8360995 [details] [diff] [review]
inlining str_replace(string,string) version 4

https://hg.mozilla.org/integration/mozilla-inbound/rev/7218723f5a9b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd443bb2208

https://tbpl.mozilla.org/php/getParsedLog.php?id=33173590&tree=Mozilla-Inbound

Comment 10

[Romain Perier]

What I am supposed to do to fix it ? It builds and works just fine on linux and mac here...
what is the architecture for b2g_mozilla-inbound_emulator-debug_dep ? x86_64 or ARM ?

Comment 11

[Romain Perier]

Attachment: inlining str_replace(string,string) version 5

Fixing build issue with old toolchains

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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