Closed Bug 1244228 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s - ---
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: jmaher, Assigned: lsalzman)

References

Details

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

Attachments

(1 file, 2 obsolete files)

:lsalzman, can you comment on this regression?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
Thanks for expanding on the scope of this.  Would it help to bisect this to a specific patch or to grab spsProfile files?
Whiteboard: [talos_regression] → [talos_regression][gfx-noted]
tracking-e10s: --- → -
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
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8714332 - Flags: review?(jmuizelaar)
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.
Attachment #8714332 - Attachment is obsolete: true
Attachment #8714332 - Flags: review?(jmuizelaar)
Attachment #8714845 - Flags: review?(jmuizelaar)
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?
Attachment #8714845 - Flags: review?(jmuizelaar) → review-
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.
Attachment #8714845 - Attachment is obsolete: true
Attachment #8715396 - Flags: review?(jmuizelaar)
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.
Attachment #8715396 - Flags: review?(jmuizelaar) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9875981edf2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
I see an improvement on the graphs!  Any concerns with uplifting this to aurora?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
1.5 weeks later...shall we uplift?
Flags: needinfo?(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
Flags: needinfo?(lsalzman)
Attachment #8715396 - Flags: approval-mozilla-aurora?
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.
Attachment #8715396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.