Closed Bug 951947 Opened 6 years ago Closed 6 years ago

IonMonkey: inline str_replace

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 806646
I am interested, I would like to work on this bug
(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.
(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?)
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)
Attached patch WIP str_replace(regexp, string) (obsolete) — Splinter Review
Assignee: nobody → hv1989
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.
Blocks: 956051
Attached patch str_replace (obsolete) — Splinter Review
Implements fastpath for string.replace(regexp, string)
Attachment #8355206 - Attachment is obsolete: true
Attachment #8355267 - Flags: review?(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.
Attachment #8355267 - Flags: review?(sstangl)
Attached patch str_replaceSplinter Review
Addresses the comments raised + I folded bug 956726 in here. I somehow thought MRegExpReplace was already committed, therefore unnecessarily created a new bug report.
Attachment #8355267 - Attachment is obsolete: true
Attachment #8356592 - Flags: review?(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.
Attachment #8356592 - Flags: review?(sstangl) → review+
I'm very very sorry sstangl, but I put the wrong person as reviewer. Should have been you off course!
https://hg.mozilla.org/mozilla-central/rev/4d357af9c538
Status: NEW → 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.