1239152 - 2-4% session restore OS X talos regressions with Skia Content

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1239151 +++

+++ This bug was initially created as a clone of Bug #1207332 +++

See: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=451ff0ecf4b1&newProject=try&newRevision=c291279314ef&framework=1

Profiles:

Base: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1614925221e3
Skia content: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae74f3fddc2b

Comment 1

[Mason Chang [Inactive] [:mchang]]

This also looks similar to bug 1239151. Between each test, we quit the application and reload the same page. However, this empties the glyph cache and re initializes DrawTargetSkia. This causes two rasterizes to be a bit longer than the CG version. 

Still not sure what to do about the glyph cache, but I'll take a look at speeding up the initialization of DrawTargetSkia. It looks like it's zeroing out some mem and allocating some canvas structures.

Comment 2

[Mason Chang [Inactive] [:mchang]]

On this test, we're hitting the conversion from SurfaceFormat:BGRX to BGRA here: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp?case=true&from=DrawTargetSkia.cpp#926

This can take 1-3 ms locally, whereas normal DrawTarget initialization takes < 0.02 ms.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Memset RGBX surfaces to white

From irc, if a TextureClient is initialized with an RGBX surface, we don't actually memset the data to anything. Skia expects that the alpha component of an RGBX surface should be 0xFF [1]. However, if we actually have an RGBX surface, users of the texture data shouldn't touch the alpha component pixel data. Therefore, we should be able to initialize the alpha component once during texture allocation and the alpha component should still be 255 when we call BorrowDrawTarget(). This would allow us to only init the data once and not reset the alpha component every time BorrowDrawTarget() is called. The underlying assumption is that if the alpha component data is not 255, that's probably a bug.

This patch inits RGBX surfaces in TextureClient to all white, and just verifies that the alpha component is 255 during BorrowDrawTarget() with Skia backends on debug builds.

This fixed the session restore talos regression. See: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9861ec32d823&newProject=try&newRevision=9150117547e6&framework=1

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp?from=DrawTargetSkia.cpp#890

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8710046 [details] [diff] [review]
Memset RGBX surfaces to white

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

Looks good, but I'd like to not make non-skia backends pay the cost

::: gfx/layers/client/TextureClient.cpp
@@ +802,5 @@
>    if (data) {
>      return MakeAndAddRef<TextureClient>(data, aTextureFlags, aAllocator);
>    }
>  
> +  if (aFormat == SurfaceFormat::B8G8R8X8 &&

please add:

&& moz2DBackend == gfx::BackendType::SKIA // skia requires the alpha component of RGBX textures to be 255.


I am not too found of paying for the cost of an touching every texels if the backend doesn't require it.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Memset RGBX surfaces to white

Comment 6

[Mason Chang [Inactive] [:mchang]]

Failing Android reftests with our assertions - https://treeherder.mozilla.org/#/jobs?repo=try&revision=652132d3ab2e

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Memset RGBX surfaces to white

Explicitly set RGBX surfaces to opaque white. If we have a component alpha, explicitly set to transparent black. Also had to move the backend check from TextureClient::CreateTextureForDrawing to TextureClient::CreateForRawBufferAccess since accelerated canvas directly creates a raw buffer access [1].

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp?from=CanvasClient.cpp#139

Comment 8

[Mason Chang [Inactive] [:mchang]]

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5585882c5a&selectedJob=16431272

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8717012 [details] [diff] [review]
Memset RGBX surfaces to white

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

::: widget/nsShmImage.cpp
@@ +113,5 @@
>              break;
>          }
>          goto unsupported;
>      case 16:
>          shm->mFormat = SurfaceFormat::R5G6B5_UINT16;

will fix to memset the 16 bit case as well.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Memset RGBX surfaces to white

Carrying r+, also added Lee's feedback.

Successful try (I think) - https://treeherder.mozilla.org/#/jobs?repo=try&revision=78efdb25c4fa

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/266e1a6642b1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/266e1a6642b1

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: skip memset since XShm is already initialized to zero

Try run: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=376a4dd64173

Comment 14

[Lee Salzman [:lsalzman]]

Oops, wrong try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=376a4dd64173

Comment 15

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8721364 [details] [diff] [review]
skip memset since XShm is already initialized to zero

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

::: widget/nsShmImage.cpp
@@ +149,4 @@
>      }
>      break;
>    case 24:
>      // Only xRGB is supported.

nit: fix this comment since it doesn't match the BGRA format below.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1205efecce10

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1205efecce10