Closed Bug 956051 Opened 7 years ago Closed 6 years ago

inline str_replace with string, string as arguments

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: romain.perier, Assigned: romain.perier)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
OS: Linux → All
Hardware: x86_64 → All
Depends on: 951947
Assignee: nobody → romain.perier
See my initial patch in attachment.
Adding setMovable and !isEffectFul attributes
Attachment #8359701 - Attachment is obsolete: true
Attachment #8359838 - Flags: review?(nicolas.b.pierron)
Attachment #8359838 - Flags: review?(jdemooij)
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.
Attachment #8359838 - Flags: review?(nicolas.b.pierron) → review+
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 !!.
Attachment #8359838 - Flags: review?(jdemooij)
Attachment #8359838 - Attachment is obsolete: true
Attachment #8360382 - Flags: review?(jdemooij)
Refreshed and rebased on trunk
Attachment #8360382 - Attachment is obsolete: true
Attachment #8360382 - Flags: review?(jdemooij)
Attachment #8360995 - Flags: review?(jdemooij)
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 :)
Attachment #8360995 - Flags: review?(jdemooij) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: checkin? :nbp
Attachment #8360995 - Flags: checkin?(nicolas.b.pierron)
Whiteboard: checkin? :nbp
Comment on attachment 8360995 [details] [diff] [review]
inlining str_replace(string,string) version 4

https://hg.mozilla.org/integration/mozilla-inbound/rev/7218723f5a9b
Attachment #8360995 - Flags: checkin?(nicolas.b.pierron) → checkin+
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 ?
Fixing build issue with old toolchains
Keywords: checkin-needed
Attachment #8360995 - Flags: checkin+ → checkin-
Attachment #8360995 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a4dc9a0f81ed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.