1035917 - Log something to logcat when we fall back to sync-scrolling scenarios

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There are a few places in Gaia where we fall back to sync scrolling because the layout of the page doesn't let us layerize stuff properly. (This is the thing that bug 967844 is aiming to fix). In the meantime, we should add something that logs when we hit these cases, so that Gaia developers can write regression tests for it and make sure they don't add code that introduces more of these scenarios. It might also be useful to have a profile marker for these cases.

tn, is there an easy place in the layout code where we can stick this log without it being too spammy? If not we might be able to put it in the APZC code somewhere.

Comment 1

[Benoit Girard (:BenWa)]

It would be nice if this log could include some information that could be used to trace it back to a cause. Maybe a hint to nearby display item and/or layers if possible.

Comment 2

[Timothy Nikkel (:tnikkel)]

The most obvious place would be if nsDisplayScrollLayer::ShouldFlattenAway returns true. But that code gets called every content paint. We could store a frame or content property the first time we output and use that to limit the output to only once. Is there a nicer way in APZC? If not that should be fine.

Builds with dump painting enabled it would be easy to dump some info, but we can probably still output something in regular opt builds.

Comment 3

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #2)
> Is there a nicer way in APZC?

I don't think so because if we're falling back to sync scrolling, we're not even creating an APZC (having RecordFrameMetrics called etc.).

Comment 4

[Timothy Nikkel (:tnikkel)]

Well we still get RecordFrameMetrics called, just on the (empty) scroll info layer.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

We do actually create an APZC for it, it's just that the ContainerLayer that the APZC is for looks like a scrollinfo layer (i.e. has no children). We might be able to log something if we encounter what looks like a scrollinfo layer but has a displayport set, or something. It wouldn't give us the info BenWa wanted in comment 1 though.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This will dump an NS_WARNING with the reason we fail to create an async scrollable layer. The reason is either that it will cause incorrect clipping on abs pos items, or interleaving of scrollable and non-scrollable items. In builds with paint dumping enabled we also dump the display item and the items it contains.

Comment 7

[Benoit Girard (:BenWa)]

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

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

::: layout/base/nsDisplayList.cpp
@@ +4124,5 @@
> +      }
> +#ifdef MOZ_DUMP_PAINTING
> +      std::stringstream ss;
> +      nsFrame::PrintDisplayItem(aBuilder, this, ss, true, false);
> +      fprintf_stderr(stderr, "%s", ss.str().c_str());

Can we print this on a single line with the following format:
msg[: displayitem]

This will make it easier for tools, like the profiler, to parse it and build higher level features with this data in cleopatra (without additional patches to gecko).

Also use printf_stderr("%s", ss.str().c_str());

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

what Benoit said

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Benoit Girard (:BenWa) from comment #7)
> Comment on attachment 8452635 [details] [diff] [review]
> patch
> 
> Review of attachment 8452635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +4124,5 @@
> > +      }
> > +#ifdef MOZ_DUMP_PAINTING
> > +      std::stringstream ss;
> > +      nsFrame::PrintDisplayItem(aBuilder, this, ss, true, false);
> > +      fprintf_stderr(stderr, "%s", ss.str().c_str());
> 
> Can we print this on a single line with the following format:
> msg[: displayitem]

The display item we are printing is not really that interesting and probably won't be very helpful. It will always be a nsDisplayScrollLayer and the frame type will always be either scroll frame or block frame (depending if we print the scrolled or scroll frame). The benefit of printing the display item is that it will print the child items as well (on subsequent lines), and it is in those that there will actually be useful information I think. I'm very open to any suggestions here.

Comment 10

[Timothy Nikkel (:tnikkel)]

Talked with BenWa on irc. He suggested a common prefix to the error message, and then a blank line following the display list dump to ease parsing.

Comment 11

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/733716435a70

Comment 12

[Benoit Girard (:BenWa)]

The display list logging will get cut off by the crummy adb logcat. We may need to do a follow up to segment it into multiple calls.

Comment 13

[Timothy Nikkel (:tnikkel)]

I think that's okay. It's only there to give an indication of what is failing exactly, and I didn't want the output to be that big anyway. If it turns out to be a problem in practice then sure let's change it.

Comment 14

[Benoit Girard (:BenWa)]

Sounds good to me.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/733716435a70