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

RESOLVED FIXED in mozilla33

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: tnikkel)

Tracking

unspecified
mozilla33
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Flags: needinfo?(tnikkel)
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.
(Assignee)

Comment 2

4 years ago
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.
Flags: needinfo?(tnikkel)
(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.).
(Assignee)

Comment 4

4 years ago
Well we still get RecordFrameMetrics called, just on the (empty) scroll info layer.
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8452635 [details] [diff] [review]
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.
Assignee: nobody → tnikkel
Attachment #8452635 - Flags: review?(roc)
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 on attachment 8452635 [details] [diff] [review]
patch

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

what Benoit said
Attachment #8452635 - Flags: review?(roc) → review+
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
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.
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.
(Assignee)

Comment 13

4 years ago
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.
Sounds good to me.
https://hg.mozilla.org/mozilla-central/rev/733716435a70
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1047069
Blocks: 1026271
You need to log in before you can comment on or make changes to this bug.