1364007 - 2.1 - 10.63% cart / tresize / tsvg_static / tsvgx (linux64, osx-10-10, windows7-32) regression on push 0b1371055c7f890ed8cc265726747a35c67ead8f (Wed May 10 2017)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ionuț Goldan [:igoldan]]

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

Regressions:

 11%  cart summary osx-10-10 opt      34.39 -> 38.05
  5%  tresize windows7-32 opt         10.43 -> 10.92
  4%  tsvg_static summary windows7-32 pgo e10s58.46 -> 60.50
  3%  tsvgx summary linux64 pgo e10s  317.32 -> 325.55
  2%  tsvgx summary linux64 pgo       344.42 -> 351.65

Improvements:

 16%  tcanvasmark summary osx-10-10 opt      6,390.21 -> 7,399.08
  4%  tsvgx summary windows7-32 pgo          497.53 -> 477.99
  3%  tsvgx summary windows7-32 pgo e10s     461.03 -> 446.90
  3%  tsvgx summary linux64 opt              489.15 -> 474.31
  2%  tsvgx summary linux64 opt e10s         442.98 -> 436.03


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=6498

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

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, 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

[Ionuț Goldan [:igoldan]]

There is a discrepancy between pgo and opt builds on linux64 platform. One says it's a regression, the other one that is an improvement.
You can ignore it. Further investigations may relate this discrepancy to other issues.
The other stats hold.

:lsalzman I know this got backed out, then backed in with an extra fix. Could you please confirm these regressions, and state whether you have an improvement or whether we should accept this?

Comment 2

[Ionuț Goldan [:igoldan]]

I'm presenting the stats in chronological order. What you see in the 1st comment, are the initial stats, which are a little out of date.

Comment 3

[Ionuț Goldan [:igoldan]]

These are the stats collected after the backout was done:

== Change summary for alert #6510 (as of May 10 2017 17:01 UTC) ==

Regressions:

 15%  tcanvasmark summary osx-10-10 opt      7,433.41 -> 6,332.12
  3%  tsvgx summary linux64 opt              474.67 -> 489.77

Improvements:

 10%  cart summary osx-10-10 opt      38.37 -> 34.41
  6%  tart summary osx-10-10 opt      11.16 -> 10.53

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6510

Comment 4

[Ionuț Goldan [:igoldan]]

And these are the results collected after the second push:

== Change summary for alert #6515 (as of May 10 2017 20:21 UTC) ==

Regressions:

 12%  cart summary osx-10-10 opt      34.47 -> 38.57

Improvements:

 16%  tcanvasmark summary osx-10-10 opt      6,357.54 -> 7,349.08
  4%  tsvgx summary linux64 opt              490.04 -> 471.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6515

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: clip drawing of native Cocoa widgets in gfxQuartzNativeDrawing

We apparently were not clipping the drawing area for native Cocoa widgets to the dirty rect.

So when we end up asking Skia for a borrowed CG context, there may complex clips (such as multiple rects) lying around that force it to create a layer for the CG context to draw into. This layer will encompass a much larger portion of the window (or the entire window) than the dirty rect requires.

This thus clips to the dirty rect in the hope that either the clip simplifies to a single rect or the area of the complex clip is substantially reduced, thus reducing the size of any allocated layers.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8866948 [details] [diff] [review]
clip drawing of native Cocoa widgets in gfxQuartzNativeDrawing

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

This will result in unbalanced Push/PopClip calls if allocating mTempDrawTarget fails. Can you push the clip in all cases so that it's more obvious to see that they're balanced?

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: clip drawing of native Cocoa widgets in gfxQuartzNativeDrawing

Cleaned up the failure handling bits to look more sane.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e6f320ab5d
clip drawing of native Cocoa widgets in gfxQuartzNativeDrawing. r=mstange

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Pulsebot from comment #8)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/54e6f320ab5d
> clip drawing of native Cocoa widgets in gfxQuartzNativeDrawing. r=mstange

This patch fixes the cart talos regression on Mac, tested locally and on try.

The other incidental small Windows regressions just look like PGO optimizer differences due to significant code churn in Skia since we last updated. We will just accept those at least since they're within acceptably small ranges/margin of error.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/54e6f320ab5d