1244228 - 4% osx e10s canvasmark regression on inbound (v.46) on Jan 21 from push 36bd96c1cd13

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

I missed this last week, and now I see it on aurora.  I did a bunch of retriggers:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=OS%20X%2010.10%20opt%20Talos%20Performance%20e10s%20Talos%20chrome%20e10s%20T-e10s%28c%29&tochange=e44eb7d3eaba&fromchange=55f1fc505b88

and found that we have a clear point where we regressed:
https://treeherder.mozilla.org/perf.html#/graphs?series=[mozilla-aurora,6d7ba064eedb822927c37ce2cda8d3164ee69604,1]&series=[mozilla-inbound,6d7ba064eedb822927c37ce2cda8d3164ee69604,1]&highlightedRevisions=36bd96c1cd13&zoom=1453318916161.5566,1453465036358.4905,5652.173913043478,6804.347826086957

you can see the subtests of canvasmark here:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=7a3b839f4a8e&newProject=mozilla-inbound&newRevision=36bd96c1cd13&originalSignature=6d7ba064eedb822927c37ce2cda8d3164ee69604&newSignature=6d7ba064eedb822927c37ce2cda8d3164ee69604

this appears to be e10s only and on osx only.

I see this is skia related, I recall some other perf bugs related to skia recently that were wontfix- please comment on this if we can do anything for osx e10s or if we need to accept it.

Comment 1

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

:lsalzman, can you comment on this regression?

Comment 2

[Lee Salzman [:lsalzman]]

(In reply to Joel Maher (:jmaher) from comment #1)
> :lsalzman, can you comment on this regression?

Note that the regression also happens in non-e10s, it's just that we balanced out in the subtests more evenly, but the same pattern is still evident in the subtests if you look:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=0e2ac34b2fe0&newProject=mozilla-central&newRevision=8a3c5c9b1486&originalSignature=96f77827ad799f45f0ebee409f2036c5232e6244&newSignature=96f77827ad799f45f0ebee409f2036c5232e6244

Canvasmark results can be tricky to interpret due to the micro-benchmarkyness of it, so it will take some time for me to investigate this.

Comment 3

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

Thanks for expanding on the scope of this.  Would it help to bisect this to a specific patch or to grab spsProfile files?

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

CanvasMark uses really large textures that exceed the texture size limits we set on OSX (4096x4096). In those cases, we left the source surfaces as raw data source surfaces, which meant that Skia couldn't still do it's own cached texture uploads. Also noteworthy, when a bitmap is too large to upload as a texture, Skia will do tiled uploads of the texture, which is why the caching is important. This is thus fixed by making sure we do wrap the source surface in a Skia bitmap, so that Skia can properly recognize it in its cache.

Also, I make sure to flag any transient textures (shadow draw targets, optimized source surfaces, etc.) we create as cached so they live within the budget limits we set within Skia.

This resolves the performance issues as far as try is concerned: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e4fa6b5ace
Retriggered many times just to be sure.

Try run on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f9bd68b9c76

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

After a minor scare about how exactly SkiaGL's cache was behaving, we decided that the original behavior of this patch did make sense. We decided to keep the behavior as-is, but better document how the cache was behaving to avoid future confusion. We documented our findings in comments in this version of the patch.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8714845 [details] [diff] [review]
fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +98,1 @@
>  {

I don't really like having this function having the additional functionality, but I don't have any suggestions for how to improve it. Maybe if we don't end up using the DataSurface below maybe we don't need it anymore.

@@ +748,5 @@
> +    // If the data was too big to fit in a GrTexture, but is already in a Skia source surface,
> +    // then just reuse it as-is.
> +    if (!texture && data->GetType() == SurfaceType::SKIA) {
> +      return data.forget();
> +    }

If data->GetType() != SurfaceType::SKIA we'll fall into the code below which seems wrong.

@@ +753,4 @@
>  
>      // Create a new SourceSurfaceSkia whose SkBitmap contains the GrTexture.
>      RefPtr<SourceSurfaceSkia> surface = new SourceSurfaceSkia();
> +    if (!surface->InitFromGrTexture(texture, data->GetSize(), data->GetFormat())) {

We should be able to pull the size and format from the bitmap instead of datasourcesurface.

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +82,5 @@
>  }
>  
> +void
> +SourceSurfaceSkia::InitFromBitmap(const SkBitmap& aBitmap,
> +                                  const IntSize& aSize,

Can't we just get aSize and aFormat from the bitmap?

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

Removed the extra parameter from GetBitmapForSurface.

Revised the control flow in OptimizeSourceSurface to be more obvious.

Made InitFromBitmap take the size and format from the bitmap itself.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8715396 [details] [diff] [review]
fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

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

I like this one.

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

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

I see an improvement on the graphs!  Any concerns with uplifting this to aurora?

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Joel Maher (:jmaher) from comment #11)
> I see an improvement on the graphs!  Any concerns with uplifting this to
> aurora?

I would rather wait a week or two to make sure there are no other serious performance concerns or unforeseen behavior that crop up. This patch has only just landed.

Comment 13

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

1.5 weeks later...shall we uplift?

Comment 14

[Lee Salzman [:lsalzman]]

Comment on attachment 8715396 [details] [diff] [review]
fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

Approval Request Comment
[Feature/regressing bug #]: Bug 1240177
[User impact if declined]: Canvas2D performance regressions on OS X and Android.
[Describe test coverage new/current, TreeHerder]: mochitest, reftest, crashtest
[Risks and why]: Might cause browser to use a bit more GPU memory for caching images, since this functionality was erroneously disabled. However, it really should be enabled as otherwise performance is impacted. Seems to have not caused any explosions since sitting on central for a couple weeks.
[String/UUID change made/needed]: None

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8715396 [details] [diff] [review]
fix DrawTargetSkia::OptimizeSourceSurface to still create Skia surfaces for GPU canvases even if creating a GPU surface failed

OS X perf improvement, should help with e10s perf, please uplift to aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca4016cf7300