1024132 - Irregex regressed Peacekeeper "stringFilter" benchmark

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ahmad Saleem]

Attachment: 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...

Comment 1

[Honza Bambas (:mayhemer)]

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.

Comment 2

[Ahmad Saleem]

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. :-)

Comment 3

[Honza Bambas (:mayhemer)]

Cool :)

Comment 4

[André Bargull [:anba]]

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

Comment 5

[Chris Peterson [:cpeterson]]

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?

Comment 6

[Ahmad Saleem]

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...

Comment 7

[Chris Peterson [:cpeterson]]

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.

Comment 8

[Ahmad Saleem]

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..

Comment 9

[Jan de Mooij [:jandem]]

(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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 11

[Jan de Mooij [:jandem]]

(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...

Comment 12

[Jan de Mooij [:jandem]]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Zlip792, have you had a chance to hunt down the regression range, by any chance?

Comment 14

[Ahmad Saleem]

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?

Comment 15

[Ahmad Saleem]

Attachment: Relevant to comment 14

Comment 16

[Ahmad Saleem]

I wish I could edit my comment..

This bug is still valid, stringFilter is still regressed though... Clearing this... I know I bumped it..

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, that sounds like the JS pgo regression that suddenly went away...  And yes, stringFilter still needs fixing.

Comment 18

[Jan de Mooij [:jandem]]

Brian will be back in < 2 weeks, I may have time to look into this before that.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 20

[Jan de Mooij [:jandem]]

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?

Comment 21

[Brian Hackett [Laid off!]]

(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

Comment 22

[Brian Hackett [Laid off!]]

(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.

Comment 23

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 24

[Jan de Mooij [:jandem]]

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...

Comment 25

[Brian Hackett [Laid off!]]

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

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/19b8c4576669

Comment 27

[Brian Hackett [Laid off!]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Brian Hackett [Laid off!]]

Attachment: rebase on aurora tip

See comment 29

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8459404 [details] [diff] [review]
rebase on aurora tip

beta+ Please land before beta2.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2339e39f5ff4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2339e39f5ff4

Comment 34

[Paul Silaghi, QA [:pauly]]

Win 7 x64:
Reproduced in 33.0a1 (2014-07-09) -> 2457.5
Verified fixed 33.0a2 (2014-08-05) -> 30402.5
32b4 ->  29388