925949 - AWFY regressions: 12% Octane-Regexp on Sep 20

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Regression visible on all platform, noticeable on x86/x64:
  http://arewefastyet.com/#machine=11&view=breakdown&suite=octane

Regression Range:
  http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b54e6e52f47944c0bb00841e3a9a30c3f26ab30a&tochange=cee18252461e629417e72e6a81a9db8b98235add

This is likely a regression caused by one of the 3 patches of Bug 501739 which landed in this regression range.

Comment 1

[Jeff Walden [:Waldo]]

Bleh.  So, what does octane-regexp use, that might have been affected by those changes?  We need to answer that first, then we can move on to considering how to fix this.  CCing Yaron who did a bunch of the work here.

My tentative guess is that JSObject::setProperty has a bunch of generic-case overhead that's hurting us, in pursuit of efforts to make .exec/.match/.replace/.etc. work properly when .lastIndex has been made non-writable.  If so, we could detect that more simply/quickly/special-casedly by obj->nativeLookupPure(NameToId(cx->names().lastIndex))->writable(), then setting the referenced slot directly.  But that's a guess.  First we need to figure out which method's changes caused the regression, then we need to figure out where the extra time is going to cause the regression.

Comment 2

[Hannes Verschore [:h4writer]]

I ran a bisect and the problem is:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a179552e0d48

So the score increase is only because now we have to zero the lastIndex. Would it be possible to do this only when needed or even not at all?

Comment 3

[Hannes Verschore [:h4writer]]

Actually esp. importanted for the optimized "str_replace_regexp_remove". Would it be possible to not need to call "zeroLastIndex"

Comment 4

[Jeff Walden [:Waldo]]

No, not possible.  The spec says we have to set the .lastIndex to 0.  We can't not do that.

However, if we can make the setting process faster, there's that.  If we check obj_->is<RegExpObject>() && obj->nativeLookup(cx, cx->names().lastIndex)->writable(), then we can safely obj_->as<RegExpObject>().zeroLastIndex().  What effect does that fast-path have on the numbers here?

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: bug925949-regexp-regression

Thanks Waldo. This indeed removes the regression introduced!

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 825585 [details] [diff] [review]
bug925949-regexp-regression

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

Just comment nitpicks.

I take it we'll want to uplift this to aurora, right?  I can sell myself on that pretty easily.

::: js/src/jsstr.cpp
@@ +1787,5 @@
>      bool zeroLastIndex(JSContext *cx) {
>          if (!regExpIsObject())
>              return true;
>  
> +        // Fastpath for the RegExpObject, since setProperty is expensive.

// Use a fast path for same-global RegExp objects with writable
// lastIndex.

@@ +1788,5 @@
>          if (!regExpIsObject())
>              return true;
>  
> +        // Fastpath for the RegExpObject, since setProperty is expensive.
> +        if (obj_->is<RegExpObject>() && obj_->nativeLookup(cx, cx->names().lastIndex)->writable()){

Need a space between ){.

@@ +1794,5 @@
> +            return true;
> +        }
> +
> +        // Use setProperty for everything else. That way the writability
> +        // of "lastIndex" is tested properly in all cases.

// Handle everything else generically (including throwing if .lastIndex is non-writable).

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e69387b6d8b

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Updated patch

r+ from previous patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 501739
User impact if declined: decreased octane score and lower performance on normal regexp operations
Testing completed (on m-c, etc.): m-i just landed, jit-tests are good
Risk to taking this patch (and alternatives if risky): very low risk. Start of aurora cycle, small change
String or IDL/UUID changes made by this patch: /

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e69387b6d8b

I see a 11% improvement. Actuall regression was 11.7%. So we are loosing 0.7% due to the extra check that is needed to conform to the spec.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e69387b6d8b

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f6d7f3350c3

Comment 12

[Tracy Walker [:tracy]]

I don't see how to read through the noise in the graph. Could someone more familiar with these perf tests, verify this is fixed? Thank you.

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #12)
> I don't see how to read through the noise in the graph. Could someone more
> familiar with these perf tests, verify this is fixed? Thank you.

What do you want to verified exactly? The regression on aurora? Well I can assure you that the speed regression isn't totally fixed. We were able to get better performance due to not doing some required things, according to the spec. So now we have to do more and we will be slower. This fix only improves the situation somewhat for octane-regexp. I would think this restores our performance between 90%-99% of the original (shortcutted and wrong) score.

I can give you exact numbers if needed?

Comment 14

[Tracy Walker [:tracy]]

Hannes, no problem, I understand that properly fixing this wouldn't return the perf to the pre-regression state. I just couldn't tell where the 90%+ improvement is on the octane-RegExp graph.  Is the improvement from the peak around Sept. 26 to the first couple of tests on Nov. 7 mostly due to this fix?  Numbers from Aurora and Nightly would help, that or hints for how to read the graph to see this fixes affect, thanks.

Comment 15

[Hannes Verschore [:h4writer]]

Before regression: rev 3192173b2dd0

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-aurora/js/src/v8$ js --ion-parallel-compile=on run-regexp.js 
RegExp: 4334
RegExp: 4334
RegExp: 4300
RegExp: 4321 
RegExp: 4338

Not patched: rev 27d0165a1097

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-aurora/js/src/v8$ js --ion-parallel-compile=on run-regexp.js 
RegExp: 3886
RegExp: 3875
RegExp: 3878
RegExp: 3833
RegExp: 3890

Aurora patched: rev 8f6d7f3350c3

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-aurora/js/src/v8$ js --ion-parallel-compile=on run-regexp.js 
RegExp: 4178
RegExp: 4191
RegExp: 4178
RegExp: 4182
RegExp: 4186
RegExp: 4182

Nightly patched: rev 1e69387b6d8b

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-inbound/js/src/v8$ js --ion-parallel-compile=on run-regexp.js 
RegExp: 4133
RegExp: 4153 
RegExp: 4133
RegExp: 4174
RegExp: 4174

Comment 16

[Tracy Walker [:tracy]]

Hannes, excellent. Thank you for the numbers.