1267565 - 2.87 - 4% kraken (linux64, windows7-32, windows8-64, windowsxp) regression on push 09aaae546918 (Mon Apr 25 2016)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Talos has detected a Firefox performance regression from push 09aaae546918. As author of one of the patches included in that push, we need your help to address this regression.

This is a list of all known regressions and improvements related to the push:
https://treeherder.mozilla.org/perf.html#/alerts?id=1010

On the page above you can see an 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(s), 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,linux64,win64 -u none -t dromaeojs[Windows XP,Windows 7,Windows 8],dromaeojs-e10s[Windows XP,Windows 7,Windows 8] --rebuild 5  # add "mozharness: --spsProfile" to generate profile data

(we suggest --rebuild 5 to be more confident in the results)

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.lorg/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 Friday, or the offending patch(es) will be backed out! ***

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

Comment 1

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

:arai, can you help figure out what is going on here and if we can reduce this regression at all?

Comment 2

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

as a note the last kraken regression was bug 1262967, we keep getting worse on this test.

Comment 3

[Hannes Verschore [:h4writer]]

With retriggering on awfy I see:
https://arewefastyet.com/#machine=28&view=single&suite=kraken&start=1461620018&end=1461632967

http://hg.mozilla.org/integration/mozilla-inbound/log?rev=45939565472b%3A%3A4b16661bbae5%20and%20!45939565472b

This means it potentially is:
Tooru Fujisawa <[e-mail]> - Tue, 26 Apr 2016 08:08:45 +0900 - rev 294817
Bug 1263525 - Add dedicated function for std_Array self-hosted intrinsic. r=efaust

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Re-enable JIT inlining for std_Array.

Sorry, forgot to add the code to retrieve templateObject for array_construct to IonBuilder::inlineArray, and the JIT inlining was disabled.

The inlined JIT code was shared by both Array ctor and std_Array, and the implementation was only ArrayConstructor.  Bug 1267565 separated it into ArrayConstructor and array_construct, and forgot to update IonBuilder::inlineArray.  (I only updated GetTemplateObjectForNative...)

Locally, confirmed the regression in stanford-crypto-pbkdf2 from 98.2 to 114.1, and the fix with this patch.

Comment 5

[Tooru Fujisawa [:arai]]

So, this is from bug 1263525.

Comment 6

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

thanks for the quick fix and narrowing it down!

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8745265 [details] [diff] [review]
Re-enable JIT inlining for std_Array.

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

::: js/src/jit/MCallOptimize.cpp
@@ +437,5 @@
>  
>      JSObject* templateObject = inspector->getTemplateObjectForNative(pc, ArrayConstructor);
> +    // This is shared by ArrayConstructor and array_construct (std_Array).
> +    if (!templateObject)
> +        templateObject = inspector->getTemplateObjectForNative(pc, array_construct);

please add a blank line below this hunk. This |if (!templateObject)| is associated with the decl, not the failure state.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5decad670c7a535ed949b1553ff8c8a1b8555ffe
Bug 1267565 - Re-enable JIT inlining for std_Array. r=efaust

Comment 9

[Hannes Verschore [:h4writer]]

Confirmed mostly fixed. Remaining is probably just noise.
https://arewefastyet.com/#machine=28&view=single&suite=kraken&start=1461506398&end=1461761526

Comment 10

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

and alerts are showing the improvement as well:
https://treeherder.mozilla.org/perf.html#/alerts?id=1031

thanks for the quick fix!

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5decad670c7a