951947 - IonMonkey: inline str_replace

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

octane-regexp has a lot of str.replace(regexp, ""). We should be able to inline them.
This gives some advantages:
1) output is always typed string, so no need for unbox after call
2) we could detect emptyString and even lose another argument from pushing/keeping around

Inlining (without the emptyString hack) gives us already 6% improvement.

Comment 1

[Romain Perier]

I am interested, I would like to work on this bug

Comment 2

[Till Schneidereit [:till]]

(In reply to Romain Perier from comment #1)
> I am interested, I would like to work on this bug

h4writer seems to already have a patch in the works, if I understand comment 0 correctly. If you were to focus on the emptyString part, though, I'm sure that would be appreciated.

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Romain Perier from comment #1)
> I am interested, I would like to work on this bug

Hi Romain, thanks for willing to take this, but I indeed already have finished and polished the patch. I only didn't submit yet. During my last minute checks the performance numbers were strange and I had no time to rerun them yet. So I'll submit the patch after checking it. I haven't done the "emptyString" part indeed. But maybe that could be easier if my part is already submitted. (Though I manual tested and saw no difference with octane2.0-regexp, but it could help somewhere else?)

Comment 4

[Hannes Verschore [:h4writer]]

Oh there is actually something I didn't implement. That would be a nice extra for this patch.
Inlining str_replace(string, string). I only implemented str_replace(regexp, string). I think a follow-up for this patch would be great to implement this. (In this case it would probably also good to wait until I submit this patch as foundation)

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: WIP str_replace(regexp, string)

Comment 6

[Romain Perier]

As I said you on IRC, I am interested by your last proposition (but it is always better to write it to the corresponding bug). Thanks for your initial patch.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: str_replace

Implements fastpath for string.replace(regexp, string)

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 8355267 [details] [diff] [review]
str_replace

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

Looks good, but breaks '$' behavior in the replacement string. There appear to be some tests for this in tests/ecma_3/ and tests/ecma_5/String/.

::: js/src/jit/MCallOptimize.cpp
@@ +1133,5 @@
> +    // Arg 1: String.
> +    if (callInfo.getArg(1)->type() != MIRType_String)
> +        return InliningStatus_NotInlined;
> +
> +    MInstruction *cte = MRegExpReplace::New(alloc(), callInfo.thisArg(), callInfo.getArg(0),

Does this need callInfo.setFoldedUnchecked()?

::: js/src/jit/MIR.h
@@ +4881,5 @@
> +    }
> +    MDefinition *regexp() const {
> +        return getOperand(1);
> +    }
> +    MDefinition *replace() const {

Prefer "replacement". Also in constructors.

::: js/src/jit/VMFunctions.cpp
@@ +13,5 @@
>  #include "jit/JitCompartment.h"
>  #include "vm/ArrayObject.h"
>  #include "vm/Debugger.h"
>  #include "vm/Interpreter.h"
> +#include "vm/RegExpObject.h"

Is this include still required?

@@ +919,5 @@
> +{
> +    JS_ASSERT(!!string);
> +    JS_ASSERT(!!repl);
> +
> +    RootedValue rval(cx, UndefinedValue());

|RootedValue rval(cx);|

@@ +924,5 @@
> +    if (!str_replace_regexp_raw(cx, string, regexp, repl, &rval))
> +        return nullptr;
> +
> +    JS_ASSERT(rval.isString());
> +    return rval.toString();

toString() already asserts isString().

::: js/src/jsstr.cpp
@@ +2810,5 @@
> +        if (!guard.init(cx, regexp))
> +            return false;
> +
> +        RegExpShared &re = guard.regExp();
> +        return str_replace_regexp_remove(cx, string, re, rval);

Should this function now be named StrReplaceRegExpRemove() for consistency?

@@ +2819,5 @@
> +    rdata.elembase = nullptr;
> +    rdata.repstr = replacement->ensureLinear(cx);
> +    if (!rdata.repstr)
> +        return false;
> +    rdata.dollar = nullptr;

This doesn't seem right. The dollar operator allows replacement with reference to the current match (see DoReplace() and InterpretDollar(), both of which are reachable from StrReplaceRegExp()). The replacement string must be scanned for the presence of a '$' character. To avoid scanning constant strings repeatedly, we could have the JIT pass an additional variable to denote the known absence of a '$'.

rdata.lambda should be explicitly set to nullptr, since StrReplaceRegExp() -> DoMatchForReplaceLocal() -> ReplaceRegExp() -> FindReplaceLength(), and FindReplaceLength() checks the value of rdata.lambda.

It might be nice to factor out the ReplaceData initialization code. It doesn't really belong duplicated into this function.

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: str_replace

Addresses the comments raised + I folded bug 956726 in here. I somehow thought MRegExpReplace was already committed, therefore unnecessarily created a new bug report.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8356592 [details] [diff] [review]
str_replace

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

lgtm!

::: js/src/jsstr.cpp
@@ +2110,5 @@
> +
> +        /* We're about to store pointers into the middle of our string. */
> +        JSLinearString *linear = repstr;
> +        dollarEnd = linear->chars() + linear->length();
> +        dollar = js_strchr_limit(linear->chars(), '$', dollarEnd);

It looks like we could do without "linear", and just use "string" everywhere.

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d357af9c538

Comment 12

[Hannes Verschore [:h4writer]]

I'm very very sorry sstangl, but I put the wrong person as reviewer. Should have been you off course!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d357af9c538