Closed Bug 925949 Opened 6 years ago Closed 6 years ago

AWFY regressions: 12% Octane-Regexp on Sep 20

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 + verified
firefox28 + verified

People

(Reporter: nbp, Assigned: h4writer)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
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?
Actually esp. importanted for the optimized "str_replace_regexp_remove". Would it be possible to not need to call "zeroLastIndex"
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?
Attached patch bug925949-regexp-regression (obsolete) — Splinter Review
Thanks Waldo. This indeed removes the regression introduced!
Assignee: nobody → hv1989
Attachment #825585 - Flags: review?(jwalden+bmo)
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).
Attachment #825585 - Flags: review?(jwalden+bmo) → review+
Attached patch Updated patchSplinter Review
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: /
Attachment #825585 - Attachment is obsolete: true
Attachment #825888 - Flags: review+
Attachment #825888 - Flags: approval-mozilla-aurora?
(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.
Attachment #825888 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1e69387b6d8b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
Keywords: verifyme
(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?
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.
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
Hannes, excellent. Thank you for the numbers.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.