845321 - IonMonkey: Optimize str_replace

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Just like for re.match we can optimize the call of str_replace away when first argument is a regexp, second a string, |this| is a string and when the result of the str_replace isn't used. I'm pretty sure v8 does this and goes even further. This optimization alone gives us 25% on v8-regexp

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Inline str.replace with regexp as first argument

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 718417 [details] [diff] [review]
Inline str.replace with regexp as first argument

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

::: js/src/ion/MCallOptimize.cpp
@@ +73,5 @@
>          return inlineStrCharAt(callInfo);
> +    if (native == str_replace && !CallResultEscapes(pc))
> +        return inlineStrReplace(callInfo);
> +
> +    }

Remove this "}", forgot to remove when putting this in a proper function call.

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 718417 [details] [diff] [review]
Inline str.replace with regexp as first argument

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

The str.replace() callsite still needs to update the RegExpStatics correctly to the last successful match in the string. If the regular expression is non-global, this is the same as an MRegExpTest -- but we don't actually need to perform the test. If the regular expression is global, finding the last match requires executing tests in a loop. In the latter case, it may be useful to just make some replace mode for the regexp statics, moving execution costs into the lazy evaluation function.

It would also be worth checking how much time is spent in JM for v8-regexp -- it could probably benefit from the same change, if we decide to take it.

How sure are we that v8 doesn't execute str.replace()? A modified v8-regexp that only includes replace() calls and that changes the input string to 16-bit chars slows down the v8 engine significantly, so it appeared that they are not ignoring the calls.

::: js/src/ion/MCallOptimize.cpp
@@ +881,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    callInfo.unwrapArgs();
> +
> +    MInstruction *match = MRegExpTest::New(callInfo.getArg(0), callInfo.thisArg());

The return type of str_replace() is String -- if it's being replaced with another instruction with different return type, we should first check that its result is unused. And then, since the result is unused, we should just update the RegExpStatics with lazy information and not actually execute a regexp.

Comment 4

[Hannes Verschore [:h4writer]]

I thought V8 was doing this, but I'm wrong. And running str.replace() without looking to the result doesn't happen in the wild on the internet. So we would only gain some added complexity and improved regexp score by cheating. I.e WONTFIX