Closed Bug 1233858 Opened 4 years ago Closed 4 years ago

33-35% Windows tcanvasmark regression on Mozilla-Inbound-Non-PGO on December 18, 2015 from push 018c2a3031f42e2e53f26351d6de570f6febf7bc

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: wlach, Assigned: lsalzman)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression], gfx-noted)

Attachments

(3 files)

Talos has detected a Firefox performance regression from your commit 018c2a3031f42e2e53f26351d6de570f6febf7bc in bug 1082598.  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=018c2a3031f42e2e53f26351d6de570f6febf7bc&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#CanvasMark

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 -u none -t chromez  # 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 tcanvasmark

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
Flags: needinfo?(lsalzman)
this is both win7/winxp in e10s and non-e10s configurations.
Blocks: 1233209
Keywords: perf, regression
Whiteboard: [talos_regression]
Whiteboard: [talos_regression] → [talos_regression], gfx-noted
Looking into it...
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
these are all showing up on PGO as well
Attachment #8702624 - Flags: review?(jmuizelaar) → review+
Attachment #8702627 - Flags: review?(jmuizelaar) → review+
I see r+, is there anything left to do prior to landing?
(In reply to Joel Maher (:jmaher) from comment #7)
> I see r+, is there anything left to do prior to landing?

There are instabilities in Visual Studio builds triggered by this patch that need to be fixed in upstream Skia before this can land. I am working on it still.
This is a simplified version of a change that was upstreamed to Skia at: https://bugs.chromium.org/p/skia/issues/detail?id=4765

It removes the usage of __vectorcall to work around VS 2013 optimizer bugs and makes the xfermodes a bit more inline-able.

Once we update to a newer Skia this patch will no longer be necessary.
Attachment #8705792 - Flags: review?(jmuizelaar)
Attachment #8705792 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/6324deed9c32
https://hg.mozilla.org/mozilla-central/rev/9d6453a68482
https://hg.mozilla.org/mozilla-central/rev/39e699922ca7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
I have verified that we see improvements with this set of patches.  Not back to the original values, but the regression is ~10% now instead of 30%+.  Thanks for reducing this regression!!

I assume this is the new level we need to accept and will rebaseline with an updated Skia?
Flags: needinfo?(lsalzman)
(In reply to Joel Maher (:jmaher) from comment #13)
> I have verified that we see improvements with this set of patches.  Not back
> to the original values, but the regression is ~10% now instead of 30%+. 
> Thanks for reducing this regression!!
> 
> I assume this is the new level we need to accept and will rebaseline with an
> updated Skia?

There has just been significant rearchitecting of Skia's guts upstream, and without basing rewriting a lot of it this is the best we can do.
Flags: needinfo?(lsalzman)
awesome, thanks for the clarification!
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.