1055821 - Add layer construction logging

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Right now it's too difficult to find out why a layer is being constructed in such a way.

This patch adds structured logging per layers that surface the decisions that lead to a layer being constructed in a particular way. Currently it focuses on thebes/color/image layers decisions but it can be extended to cover more.

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2387,5 @@
> +  if (mVisibleRegion.IsEqual(nsIntRegion(aVisibleRect)) &&
> +             mVisibleRegion.IsEqual(aClippedOpaqueRegion) &&
> +             clipMatches &&
> +             aItem->SupportsOptimizingToImage()) {
> +    mImage = static_cast<nsDisplayImageContainer*>(aItem);

Feel free to tell me to break this into another patch.

I don't really know if we want to be doing this or if this should never happen because covered items should be removed earlier (by what piece of code exactly if so?).

Here's an example with the b2g system background. These items go into the same layer:
I/Gecko   (16366):   CanvasBackgroundColor p=0xaf276c60 f=0xb05f77a0(Canvas(html)(-1)) bounds(0,0,19200,34160) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  uniform (opaque 0,0,19200,34160) (rgba 0,0,0,255) layer=0xaf242b80
I/Gecko   (16366):   BackgroundColor p=0xaf276cb0 f=0xaf739090(HTMLScroll(div)(1)) bounds(0,0,19200,34160) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  uniform (opaque 0,0,19200,34160) (rgba 0.000000,0.000000,0.000000,1.000000) layer=0xaf242b80
I/Gecko   (16366):   Background p=0xaf276d20 f=0xaf739090(HTMLScroll(div)(1)) bounds(0,0,19200,34160) visible(0,0,19200,34160) componentAlpha(0,0,0,0) clip(0,0,19200,34160)  (opaque 0,0,19200,34160) layer=0xaf242b80

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2387,5 @@
> +  if (mVisibleRegion.IsEqual(nsIntRegion(aVisibleRect)) &&
> +             mVisibleRegion.IsEqual(aClippedOpaqueRegion) &&
> +             clipMatches &&
> +             aItem->SupportsOptimizingToImage()) {
> +    mImage = static_cast<nsDisplayImageContainer*>(aItem);

Yes, let's do this as a separate patch :)

It seems valid though, and you can make it aClippedOpaqueRegion.Contains(mVisibleRegion) instead to cover cases where the image is bigger than the content already in the layer.

I think we can also do this in the same condition that sets mImage with an empty visible region, rather than having it separate.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8475506 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2301,4 @@
>                              const nsIntRect& aDrawRect,
>                              const DisplayItemClip& aClip)
>  {
> +  FLB_LOG_THEBES_DECISION(this, "Accumulating dp=%p, f=%p against tld=%p\n", aItem, aItem->Frame(), this);

Dump aItem->Name() here too? Like "%s(%p)", aItem->Name(), aItem maybe.

Names are much easier to read than pointers.

Comment 4

[Benoit Girard (:BenWa)]

I can add it, but the information is already in the log. It does save a lookup however.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=49da09974994

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Add layer construction logging. r=mattwoodrow

Comment 7

[Benoit Girard (:BenWa)]

checkin-needed to coalesce with simple changes.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a61a6eb04d6

Comment 9

[Ed Morley [:emorley]]

Reverted for build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46557049&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46557043&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c545905a87a9

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Add layer construction logging. r=mattwoodrow

This is what I get for being too conservative with build requests.

Comment 11

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73a965cfceba

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73a965cfceba