Closed
Bug 1267565
Opened 8 years ago
Closed 8 years ago
2.87 - 4% kraken (linux64, windows7-32, windows8-64, windowsxp) regression on push 09aaae546918 (Mon Apr 25 2016)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | fixed |
People
(Reporter: jmaher, Assigned: arai)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression])
Attachments
(1 file)
1.18 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•8 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Target Milestone: Firefox 49 → ---
Reporter | ||
Comment 1•8 years ago
|
||
:arai, can you help figure out what is going on here and if we can reduce this regression at all?
Flags: needinfo?(arai.unmht)
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Comment 2•8 years ago
|
||
as a note the last kraken regression was bug 1262967, we keep getting worse on this test.
Comment 3•8 years ago
|
||
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 <arai_a@mac.com> - Tue, 26 Apr 2016 08:08:45 +0900 - rev 294817 Bug 1263525 - Add dedicated function for std_Array self-hosted intrinsic. r=efaust
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8745265 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 5•8 years ago
|
||
So, this is from bug 1263525.
No longer blocks: 1267364
status-firefox48:
--- → unaffected
Reporter | ||
Comment 6•8 years ago
|
||
thanks for the quick fix and narrowing it down!
Comment 7•8 years ago
|
||
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.
Attachment #8745265 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5decad670c7a535ed949b1553ff8c8a1b8555ffe Bug 1267565 - Re-enable JIT inlining for std_Array. r=efaust
Comment 9•8 years ago
|
||
Confirmed mostly fixed. Remaining is probably just noise. https://arewefastyet.com/#machine=28&view=single&suite=kraken&start=1461506398&end=1461761526
Reporter | ||
Comment 10•8 years ago
|
||
and alerts are showing the improvement as well: https://treeherder.mozilla.org/perf.html#/alerts?id=1031 thanks for the quick fix!
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5decad670c7a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•