inline str_replace with string, string as arguments

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

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

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
Assignee

Updated

6 years ago
Depends on: 951947
Assignee: nobody → romain.perier
Assignee

Comment 1

6 years ago
See my initial patch in attachment.
Assignee

Comment 2

6 years ago
Adding setMovable and !isEffectFul attributes
Attachment #8359701 - Attachment is obsolete: true
Assignee

Updated

6 years ago
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)
Assignee

Comment 5

6 years ago
Attachment #8359838 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8360382 - Flags: review?(jdemooij)
Assignee

Comment 6

6 years ago
Refreshed and rebased on trunk
Attachment #8360382 - Attachment is obsolete: true
Attachment #8360382 - Flags: review?(jdemooij)
Assignee

Updated

6 years ago
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
Assignee

Updated

6 years ago
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+
Assignee

Comment 10

6 years ago
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 ?
Assignee

Comment 11

6 years ago
Fixing build issue with old toolchains
Assignee

Updated

6 years ago
Keywords: checkin-needed
Attachment #8360995 - Flags: checkin+ → checkin-
Assignee

Updated

6 years ago
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.