Closed Bug 1239152 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mchang, Assigned: lsalzman)

References

Details

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

Attachments

(2 files, 3 obsolete files)

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.
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]
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
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)
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
Attachment #8710046 - Attachment is obsolete: true
Attachment #8711083 - Flags: review?(nical.bugzilla)
Attachment #8711083 - Flags: review?(nical.bugzilla) → review+
Failing Android reftests with our assertions - https://treeherder.mozilla.org/#/jobs?repo=try&revision=652132d3ab2e
Attached patch Memset RGBX surfaces to white (obsolete) — Splinter Review
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)
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+
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+
https://hg.mozilla.org/mozilla-central/rev/266e1a6642b1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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 → ---
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+
https://hg.mozilla.org/mozilla-central/rev/1205efecce10
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.