Irregex regressed Peacekeeper "stringFilter" benchmark

VERIFIED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
JavaScript Engine
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Zlip792, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {perf, regression})

Trunk
mozilla33
x86_64
Windows 8.1
perf, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32+ verified, firefox33+ verified, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8438716 [details]
Testing History

Irregex regressed Peackeeper "stringFilter" benchmark by reducing op/sec from 41xxx range to mere 3xxx range.

Sorry for posting this so late, I noticed this regression on May 23rd and its still present in today's Nightly. I was hesitant because I once gave Boris wrong information (not intentionally but due to testing flaw for Peacekeeper on Windows Machine).

Regression Range (mozilla-inbound-win32-pgo): http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=28f1a546126f&tochange=db356f641d04

Good CSET: 28f1a546126f
Bad CSET: db356f641d04

Expected Result:
Either improve like "stringValidateForm" or show little deviation like other tests..

Actual Result:
"stringFilter" test got regressed.

On regression range, cache2 also landed and turned on by default, I didn't test whether it could be due to it as well but my first guess is it was because of irregex. CCed to Honza as well...
(Reporter)

Updated

3 years ago
Blocks: 1024038
According the cache, it's easy to test (if you can).  Push with a change to all.js switching browser.cache.use_new_backend_temp to false.  That's it.

How the test exactly works?  If there is a huge number of loads involved, there could be an influence, but not in a magnitude order anyway.
(Reporter)

Comment 2

3 years ago
By changing "browser.cache.use_new_backend_temp" to false in Bad CSET from about:config, results in same poor op/sec for StringFilter test.. So I can confirm that its not because of Cache2..
Irregex is now prime suspect...

Thanks Honza for mentioning this preference. :-)
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
Keywords: perf, regression
tracking-firefox32: ? → +
tracking-firefox33: ? → +
Cool :)

Comment 4

3 years ago
important
Related bug 605385 and https://bugs.webkit.org/show_bug.cgi?id=64202 (YARR opt for StringFilter). Basically leading and trailing .* are removed from the RegExp. V8 performs a similar optimizations, except only leading .* is removed [1]. This optimization is probably required to improve the benchmark score [1].

[1] https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/regexp.js#225
[2] https://github.com/sloanyang/BrowserBenchmark/blob/master/Peacekeeper/anatomy/js/tests/string/stringFilter.js#L18
Jan: How much does Peacekeeper's stringFilter test case affect Firefox's overall Peacekeeper score? Do we need to fix this performance regression before irregexp (now in Aurora 32) rides the trains to release?
Blocks: 976446, 499198
No longer blocks: 1024038
Status: UNCONFIRMED → NEW
status-firefox32: --- → affected
status-firefox33: --- → affected
Ever confirmed: true
Flags: needinfo?(jdemooij)
(Reporter)

Comment 6

3 years ago
Just to let you know current Nightly's has severe regression in Array, DOM and renderGrid03 & renderPhysics (Rendering) benchmarks along side  String (only this StringFilter).

Array use to score around 3xxxx not its at 26xxx, DOM use to score around 24xxx, it has now 14xxx, only one string regressed due to Irregexp.

All other regressions are not due to irregexp but due to some other patch stuff. I am mentioning regression from other here because there will be noise if you check with current Nightly and compare...
Zlip792: ouch! Thanks for the update. Do you know if there are open bugs for those Peacekeeper regressions? Are they new in Nightly 33?

btw, I'm working with our test automation team to automate Peacekeeper (and other benchmarks) to catch regressions like these sooner.
(Reporter)

Comment 8

3 years ago
Nope, I don't think so they are filed. I will try to find regression range tonight unless someone beats me to it and file them.

It will be great to automate Peacekeeper testing..
(In reply to Chris Peterson (:cpeterson) from comment #5)
> Jan: How much does Peacekeeper's stringFilter test case affect Firefox's
> overall Peacekeeper score? Do we need to fix this performance regression
> before irregexp (now in Aurora 32) rides the trains to release?

stringValidateForm got faster (< 400k to > 530k), because it has some regular expressions Yarr couldn't compile. It'd be nice to fix the stringFilter regression, but I don't think it's necessary for Aurora/32.

Brian, maybe we can port the hack mentioned in comment 4?
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
> but I don't think it's necessary for Aurora/32.

That sort of depends on how zero-tolerance a policy we have for peacekeeper regressions.  The way it's scored, a 12x regression in one test is much worse than a 25% improvement in another test...
(In reply to Boris Zbarsky [:bz] from comment #10)
> That sort of depends on how zero-tolerance a policy we have for peacekeeper
> regressions.  The way it's scored, a 12x regression in one test is much
> worse than a 25% improvement in another test...

Ah! I misread comment 0 as "from 4xxx range to mere 3xxx" instead of "from 41xxx range to mere 3xxx".

That changes things yes...
(In reply to Zlip792 from comment #6)
> Just to let you know current Nightly's has severe regression in Array, DOM
> and renderGrid03 & renderPhysics (Rendering) benchmarks along side  String
> (only this StringFilter).
> 
> Array use to score around 3xxxx not its at 26xxx, DOM use to score around
> 24xxx, it has now 14xxx, only one string regressed due to Irregexp.
> 
> All other regressions are not due to irregexp but due to some other patch
> stuff. I am mentioning regression from other here because there will be
> noise if you check with current Nightly and compare...

This could be the Windows PGO regression I mentioned in bug 1030706 comment 5. The patch there should help.
Zlip792, have you had a chance to hunt down the regression range, by any chance?
Flags: needinfo?(zlip.792)
(Reporter)

Comment 14

3 years ago
Sorry Boris, I didn't manage to get back to this bug and neither does check last few days Nightly on Peacekeeper, although I have good news which that current today's Nightly does not suffer from any regression any longer which I mentioned on June 25 in comment 6.

Its not my testing fluke, I am attaching my attempt of today trying to find regression range and you can compare both test results (I tested today's Nightly twice to confirm, it fixed regressions) and see that there was definitely regressed performance.

I have suggestion that should I make read only or whatever Google Spreadsheet with each day Peacekeeper testing of my own and post its link in meta bug, if it could help you in anyway?
Flags: needinfo?(zlip.792)
(Reporter)

Comment 15

3 years ago
Created attachment 8449078 [details]
Relevant to comment 14
(Reporter)

Comment 16

3 years ago
I wish I could edit my comment..

This bug is still valid, stringFilter is still regressed though... Clearing this... I know I bumped it..
Ah, that sounds like the JS pgo regression that suddenly went away...  And yes, stringFilter still needs fixing.
Brian will be back in < 2 weeks, I may have time to look into this before that.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 19

3 years ago
Created attachment 8454898 [details] [diff] [review]
patch

This patch improves my stringFilter score from ~3400 to ~30000.  It is similar to how v8 optimizes this benchmark --- a one slot cache is used, and only RegExp.test is improved.
Assignee: nobody → bhackett1024
Attachment #8454898 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Comment on attachment 8454898 [details] [diff] [review]
patch

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

Nice! Just some comments.

::: js/src/builtin/RegExp.cpp
@@ +680,5 @@
> +static inline bool
> +StringHasDotStar(HandleLinearString str, size_t index)
> +{
> +    // Return whether the portion of the string at the specified index is '.*'
> +    return str->latin1OrTwoByteChar(0) == '.' && str->latin1OrTwoByteChar(1) == '*';

Use index and index + 1 instead of 0 and 1.

@@ +724,5 @@
> +            return false;
> +
> +        source = AtomizeString(cx, newSource);
> +        if (!source)
> +            return false;

Would it be possible to restructure the code so that we don't need to allocate 4 strings in the leading + trailing .* case?

@@ +770,5 @@
> +        status = ExecuteRegExp(cx, alternate, input, matches, DontUpdateRegExpStatics);
> +
> +        if (status == RegExpRunStatus_Success) {
> +            // Update the RegExpStatics to reflect the original RegExp we were
> +            // trying to execute, and not the alternate one.

Why is this needed when we pass DontUpdateRegExpStatics to ExecuteRegExp? Does that indicate something else?
Attachment #8454898 - Flags: review?(jdemooij)
(Assignee)

Comment 21

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #20)
> @@ +770,5 @@
> > +        status = ExecuteRegExp(cx, alternate, input, matches, DontUpdateRegExpStatics);
> > +
> > +        if (status == RegExpRunStatus_Success) {
> > +            // Update the RegExpStatics to reflect the original RegExp we were
> > +            // trying to execute, and not the alternate one.
> 
> Why is this needed when we pass DontUpdateRegExpStatics to ExecuteRegExp?
> Does that indicate something else?

If we pass DontUpdateRegExpStatics to ExecuteRegExp then the statics will not get updated at all, which we don't want because |RegExp.lastMatch| etc. need to reflect any successful last call to test().  So this block of code updates the statics to reflect the original RegExp which was passed in, and not the one with leading/trailing .* stripped out.


The RegExpStatics need to be updated after a successful test() call to reflect the result of that regexp execution (for |RegExp.lastMatch| etc. to work).  We used to always pass in UpdateRegExpStatics to the ExecuteRegExp call to make sure this happens, but if we did that here  we provide
(Assignee)

Comment 22

3 years ago
(In reply to Brian Hackett (:bhackett) from comment #21)
> The RegExpStatics need to be updated after a successful test() call to
> reflect the result of that regexp execution (for |RegExp.lastMatch| etc. to
> work).  We used to always pass in UpdateRegExpStatics to the ExecuteRegExp
> call to make sure this happens, but if we did that here  we provide

Whoops, zombie comment draft.
(Assignee)

Comment 23

3 years ago
Created attachment 8455492 [details] [diff] [review]
patch
Attachment #8454898 - Attachment is obsolete: true
Attachment #8455492 - Flags: review?(jdemooij)
Comment on attachment 8455492 [details] [diff] [review]
patch

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

::: js/src/builtin/RegExp.cpp
@@ +684,5 @@
> +    return str->latin1OrTwoByteChar(index) == '.' && str->latin1OrTwoByteChar(index + 1) == '*';
> +}
> +
> +static bool
> +TryFillRegExpTestCache(JSContext *cx, HandleObject regexp, RegExpTestCache &cache,

There are a lot of regexp->as<RegExpObject>() calls below, it'd be nice if we could use Handle<RegExpObject*> (or add HandleRegExpObject to gc/Rooting.h) to avoid those. Same in regexp_test_raw.

@@ +738,3 @@
>  /* Separate interface for use by IonMonkey. */
>  bool
>  js::regexp_test_raw(JSContext *cx, HandleObject regexp, HandleString input, bool *result)

This method is only called by Ion, while the generic regexp_test_impl calls ExecuteRegExp with a CallArgs (and it's the only caller of that function). It might be worth folding ExecuteRegExp(..CallArgs..) into regexp_test_impl, and then have it call regexp_test_raw, so that we use the same code path everywhere.

But OTOH the optimization has some runtime/allocation overhead as well, so it might not be unreasonable to do it only in (hot) Ion code...
Attachment #8455492 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 25

3 years ago
I think both of those refactorings would be nice, but they would require touching more code and I'd like to keep this change pretty minimal for (hopefully) an aurora uplift.

https://hg.mozilla.org/integration/mozilla-inbound/rev/19b8c4576669
https://hg.mozilla.org/mozilla-central/rev/19b8c4576669
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 27

3 years ago
Comment on attachment 8455492 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Irregexp port
[User impact if declined]: Massive regression on PeaceKeeper benchmark
[Describe test coverage new/current, TBPL]: new test
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8455492 - Flags: approval-mozilla-aurora?
Attachment #8455492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite+
Needs rebasing for Aurora uplift.
Flags: needinfo?(bhackett1024)
Keywords: branch-patch-needed
Comment on attachment 8455492 [details] [diff] [review]
patch

This isn't going to make it onto Aurora before the uplift at this point. Please request beta approval once you've got a rebased patch ready to go.
Attachment #8455492 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 30

3 years ago
Created attachment 8459404 [details] [diff] [review]
rebase on aurora tip

See comment 29
Attachment #8459404 - Flags: approval-mozilla-beta?
Flags: needinfo?(bhackett1024)

Updated

3 years ago
Flags: needinfo?(jdemooij)
Comment on attachment 8459404 [details] [diff] [review]
rebase on aurora tip

beta+ Please land before beta2.
Attachment #8459404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2339e39f5ff4
status-firefox32: affected → fixed
Keywords: branch-patch-needed
Keywords: verifyme
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2339e39f5ff4
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Win 7 x64:
Reproduced in 33.0a1 (2014-07-09) -> 2457.5
Verified fixed 33.0a2 (2014-08-05) -> 30402.5
32b4 ->  29388
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
status-firefox33: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.