Closed Bug 1035228 Opened 7 years ago Closed 7 years ago

Merge FrameMetrics logging code

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
Attachment #8451663 - Flags: review?(bgirard)
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.
Attachment #8451663 - Flags: review?(bgirard) → review+
(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?
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.
(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 :)
https://hg.mozilla.org/mozilla-central/rev/9b42ae417853
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1039616
You need to log in before you can comment on or make changes to this bug.