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

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mchang, Assigned: lsalzman)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [talos_regression][gfx-noted])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Comment 1

3 years ago
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.
(Reporter)

Comment 2

3 years ago
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.
Whiteboard: [gfx-noted] → [talos_regression][gfx-noted]
(Reporter)

Comment 3

3 years ago
Created attachment 8710046 [details] [diff] [review]
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
Attachment #8710046 - Flags: review?(nical.bugzilla)
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.
Attachment #8710046 - Flags: review?(nical.bugzilla)
(Reporter)

Comment 5

3 years ago
Created attachment 8711083 [details] [diff] [review]
Memset RGBX surfaces to white
Attachment #8710046 - Attachment is obsolete: true
Attachment #8711083 - Flags: review?(nical.bugzilla)
Attachment #8711083 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Comment 6

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

Comment 7

3 years ago
Created attachment 8717012 [details] [diff] [review]
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
Attachment #8711083 - Attachment is obsolete: true
Attachment #8717012 - Flags: review?(nical.bugzilla)
(Reporter)

Comment 9

3 years ago
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.
Attachment #8717012 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Comment 10

3 years ago
Created attachment 8720312 [details] [diff] [review]
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
Attachment #8717012 - Attachment is obsolete: true
Attachment #8720312 - Flags: review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/266e1a6642b1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 13

3 years ago
Created attachment 8721364 [details] [diff] [review]
skip memset since XShm is already initialized to zero

Try run: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=376a4dd64173
Assignee: mchang → lsalzman
Status: RESOLVED → REOPENED
Attachment #8721364 - Flags: review?(mchang)
Resolution: FIXED → ---
(Reporter)

Comment 15

3 years ago
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.
Attachment #8721364 - Flags: review?(mchang) → review+

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1205efecce10
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.