1166678 - 2% Win7/win8 kraken regression on Mozilla-Inbound-Non-PGO on May 20, 2015 from push 1410ca139039

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Talos has detected a Firefox performance regression from your commit 1410ca139039 in bug 1163091.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=48fa8f0a227c&showAll=1

On the page above you can see Talos alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#kraken

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p win32,win64 -u none -t dromaeojs  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a kraken

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Monday, or the offending patch will be backed out! ***

Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

:bhackett, can you take a look at this.  I have a view of the kraken subtests:
win8: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=f7054968c36b&newProject=mozilla-inbound&newRevision=1410ca139039&originalSignature=ec3f1d105a946b4a3b6076740458d89fff334de7&newSignature=ec3f1d105a946b4a3b6076740458d89fff334de7

win7: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=f7054968c36b&newProject=mozilla-inbound&newRevision=1410ca139039&originalSignature=dcdd5758302310a385a9c8743b8ad05b66c47afa&newSignature=dcdd5758302310a385a9c8743b8ad05b66c47afa

it looks like a common problem in stanford-crypto-*; 

to be honest looking back in history on this test:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=%257B%2522project%2522%253A%2522mozilla-inbound%2522%252C%2522signature%2522%253A%2522dcdd5758302310a385a9c8743b8ad05b66c47afa%2522%252C%2522visible%2522%253Atrue%257D&zoom=%7B%7D

there is a stable history, an improvement a around may 15th and this pretty much backs that improvement out.

thoughts?

Comment 2

[Guilherme Lima]

This regression is present on AWFY too, Mac machines and Win 8.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

I think this is the same issue as bug 1146597 comment 35.  We spend a lot of time in the VM running array functions in these stanford-crypto tests, and are hitting ObjectGroup::maybeSweep a lot.  The attached patch makes this an inline function, like I should have done in the first place in bug 1146597.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f31bacc839

Comment 5

[Brian Hackett [Laid off!]]

Attachment: inline Array.slice

The last patch helped some but not that much.  I haven't looked at instruction counts again to see what's up, but it's kind of dumb to be spending so much time in the VM when we can optimize this stuff better from Ion.  This patch optimizes Array.slice, which is used by all the stanford-crypto tests, and improves our kraken score by maybe 2% or so.

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/a5f31bacc839

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8611379 [details] [diff] [review]
inline Array.slice

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

::: js/src/jit/MCallOptimize.cpp
@@ +1057,5 @@
> +    if (callInfo.argc() > 0) {
> +        begin = callInfo.getArg(0);
> +    } else {
> +        begin = MConstant::New(alloc(), Int32Value(0));
> +        current->add(begin->toInstruction());

Nit: this can be |begin = constant(Int32Value(0));|

::: js/src/jsarray.cpp
@@ +2901,5 @@
> +            return 0;
> +    } else if (value > length) {
> +        return length;
> +    }
> +    return (uint32_t) value;

Nit: uint32_t(value); here and below while you're refactoring this.

@@ +2905,5 @@
> +    return (uint32_t) value;
> +}
> +
> +static inline uint32_t
> +NormalizeSliceTerm(int32_t value, uint32_t length)

You don't think it's worth templatizing this and the previous function, so there's just one copy that takes an int32/double value?

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> @@ +2905,5 @@
> > +    return (uint32_t) value;
> > +}
> > +
> > +static inline uint32_t
> > +NormalizeSliceTerm(int32_t value, uint32_t length)
> 
> You don't think it's worth templatizing this and the previous function, so
> there's just one copy that takes an int32/double value?

I tried doing this but the int32_t version needs a uint32_t cast for the comparison.  I didn't think that was correct to do for the double version but experimenting now it looks like it will be ok (casting a double > UINT32_MAX to a uint32 will produce UINT32_MAX) so I'll go back to the template version.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> I tried doing this but the int32_t version needs a uint32_t cast for the
> comparison.  I didn't think that was correct to do for the double version
> but experimenting now it looks like it will be ok (casting a double >
> UINT32_MAX to a uint32 will produce UINT32_MAX) so I'll go back to the
> template version.

... and after reading some more the semantics of this conversion are unspecified by the standard.  I'll just use a template and cast both sides of the comparison to double.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57bd26cc44b9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57bd26cc44b9

Comment 12

[Hannes Verschore [:h4writer]]

Nice!

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 64-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- kraken: -0.76% (improvement)
- kraken: oscillator: -1.21% (improvement)
- kraken: crypto-aes: -7.17% (improvement)
- kraken: crypto-pbkdf2: -5.96% (improvement)
- octane: Richards: 2.34% (improvement)
- asmjs-ubench: mandelbrot-native: -2.9% (improvement) (noasmjs)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b779fae94e88&tochange=0d29fb828423

More details: http://arewefastyet.com/regressions/#/regression/1696890

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

verified the graphs show this as fixed!  Thanks for the work on it:)

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

is there any need to keep this open?  I would like to resolve this bug as fixed.

Comment 15

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.