1035228 - Merge FrameMetrics logging code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

There's a FrameMetrics serializer in LayersLogging.cpp used in layer dumps, and there's a more comprehensive serializer in AsyncPanZoomController.cpp used for APZC logging. We should combine the two to reduce duplicated code.

Comment 1

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

Attachment: Patch

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8451663 [details] [diff] [review]
Patch

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

Note that nsPrintfCString is very slow. Having slow logging code means we can't use it while tracking down performance problems.

::: gfx/layers/LayersLogging.cpp
@@ +120,3 @@
>  {
>    aStream << pfx;
> +  AppendToString(aStream, m.mCompositionBounds, "{ cb=");

Optional: What about using slightly longer acronyms compBound?

@@ +130,5 @@
> +    AppendToString(aStream, m.GetDisplayPortMargins(), " dpm=");
> +    aStream << nsPrintfCString(" um=%d", m.GetUseDisplayPortMargins()).get();
> +    AppendToString(aStream, m.GetRootCompositionSize(), " rcs=");
> +    AppendToString(aStream, m.mViewport, " v=");
> +    aStream << nsPrintfCString(" z=(ld=%.3f r=%.3f cr=%.3f z=%.3f ts=%.3f)",

Especially here. I don't know what ld stands for.

::: gfx/layers/LayersLogging.h
@@ +105,5 @@
> +               const char* pfx="", const char* sfx="")
> +{
> +  aStream << pfx;
> +  aStream << nsPrintfCString(
> +    "(l=%f, t=%f, r=%f, b=%f)",

I often prefer to see l, t, r, b converted to x,y,w,h in my logging even if the underlying representation is not that but I think this is really subjective so feel free to ignore.

Comment 3

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

(In reply to Benoit Girard (:BenWa) from comment #2)
> Note that nsPrintfCString is very slow. Having slow logging code means we
> can't use it while tracking down performance problems.

Ok. This stuff doesn't get run unless logging is enabled anyway, so it shouldn't show up on production build profiles. A lot of the LayersLogging.cpp and LayersLogging.h code uses nsPrintfCString though so if that's really slow we should replace it with streaming the individual fields rather than using printf format strings.

> ::: gfx/layers/LayersLogging.cpp
> @@ +120,3 @@
> >  {
> >    aStream << pfx;
> > +  AppendToString(aStream, m.mCompositionBounds, "{ cb=");
> 
> Optional: What about using slightly longer acronyms compBound?
> 
> @@ +130,5 @@
> > +    AppendToString(aStream, m.GetDisplayPortMargins(), " dpm=");
> > +    aStream << nsPrintfCString(" um=%d", m.GetUseDisplayPortMargins()).get();
> > +    AppendToString(aStream, m.GetRootCompositionSize(), " rcs=");
> > +    AppendToString(aStream, m.mViewport, " v=");
> > +    aStream << nsPrintfCString(" z=(ld=%.3f r=%.3f cr=%.3f z=%.3f ts=%.3f)",
> 
> Especially here. I don't know what ld stands for.
> 

The detailed FrameMetrics dump is already ~460 characters long. While I could make a lot of these acronyms longer it would flood the log even more than it already does. Specially with the detailed fields, that will only get enabled if people manually enable the APZC logging and in that case whoever is looking at the log will probably know what it means (or not need to care about it). I'm not sure there's that much value in making the acroynms longer.

> ::: gfx/layers/LayersLogging.h
> @@ +105,5 @@
> > +               const char* pfx="", const char* sfx="")
> > +{
> > +  aStream << pfx;
> > +  aStream << nsPrintfCString(
> > +    "(l=%f, t=%f, r=%f, b=%f)",
> 
> I often prefer to see l, t, r, b converted to x,y,w,h in my logging even if
> the underlying representation is not that but I think this is really
> subjective so feel free to ignore.

I think in the case of margins it might be confusing to see x/w instead of l/r. Does w include l+r, or would it just be r?

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b42ae417853

Comment 5

[Botond Ballo [:botond]]

If I understand correctly what this patch is doing, it will make lines corresponding to scrollable layers in a layer dump quite a bit longer by including a number of additional FrameMetrics fields.

Is this really desirable? I find layer dumps very noisy as they are.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> If I understand correctly what this patch is doing, it will make lines
> corresponding to scrollable layers in a layer dump quite a bit longer by
> including a number of additional FrameMetrics fields.
> 
> Is this really desirable? I find layer dumps very noisy as they are.

Oh whoops, I missed the 'detailed' flag. Never mind then :)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b42ae417853