Add layer construction logging

RESOLVED FIXED in mozilla34

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla34
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8475506 [details] [diff] [review]
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.
Attachment #8475506 - Flags: review?(matt.woodrow)
(Assignee)

Comment 1

4 years ago
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 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 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.
Attachment #8475506 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 4

4 years ago
I can add it, but the information is already in the log. It does save a lookup however.
(Assignee)

Comment 5

4 years ago
Created attachment 8476803 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=49da09974994
Assignee: nobody → bgirard
Attachment #8475506 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8476803 - Flags: review+
(Assignee)

Comment 6

4 years ago
Created attachment 8476813 [details] [diff] [review]
Add layer construction logging. r=mattwoodrow
Attachment #8476803 - Attachment is obsolete: true
Attachment #8476813 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 1056944
(Assignee)

Comment 7

4 years ago
checkin-needed to coalesce with simple changes.
Keywords: checkin-needed
(Assignee)

Comment 10

4 years ago
Created attachment 8477454 [details] [diff] [review]
Add layer construction logging. r=mattwoodrow

This is what I get for being too conservative with build requests.
Attachment #8476813 - Attachment is obsolete: true
Attachment #8477454 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/73a965cfceba
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.