Closed Bug 944866 Opened 6 years ago Closed 6 years ago

Introduce a ScrollGraph overlay

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Whiteboard: [qa-])

Attachments

(5 files, 5 obsolete files)

Currently people are using the FPS counter for measuring scrolling. This is *TERRIBLE*, especially on b2g where some frames might come from trivial updates like the status bar or an async animation. 60 FPS, 50 FPS doesn't mean we're smoothly scrolling a layer

This scroll graphs work well with momentum scrolling (trackpad or touch). We could extend this code to show other types of graph for async animation.
Attachment #8340606 - Attachment is patch: false
Attachment #8340606 - Attachment mime type: text/plain → image/gif
Attached patch Messy WIP (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch Messy WIPSplinter Review
Attachment #8340607 - Attachment is obsolete: true
Part 2 wont be ready yet but would be nice to get this out of the way. Right now with OGL it's easy to support the EffectChain (I think?), but maybe it's overkill and we should just have a simple DrawLines with a flat color.
Attachment #8340660 - Flags: review?(bas)
Attachment #8340660 - Attachment is obsolete: true
Attachment #8340660 - Flags: review?(bas)
Attachment #8340661 - Flags: review?(bas)
Comment on attachment 8340661 [details] [diff] [review]
Part 1: Add DrawLines to Compositor.h and implement for OGL

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

I will pretend I am qualified to review this to get you going.

::: gfx/layers/Compositor.h
@@ +297,5 @@
>    /**
> +   * Tell the compositor to draw lines connecting the points. Behaves like
> +   * DrawQuad.
> +   */
> +  virtual void DrawLines(const nsTArray<gfx::Point>& aLines, const gfx::Rect& aClipRect,

I would use vector<> at the call site and pass a pointer and length in here. nsTArray is so yesterday.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1569,5 @@
>      mGLContext->fEnableVertexAttribArray(aTexCoordAttribIndex);
>    }
>  
>    mGLContext->fEnableVertexAttribArray(aVertAttribIndex);
> +  if (mDrawType == LOCAL_GL_LINE_STRIP) {

mDrawType is pretty nasty. How about a DrawQuadInternal where you pass in the draw type?
Attachment #8340661 - Flags: review?(bas) → review+
Part 2 is held back because it's harder to get working for both APZC and non APZC. I may just land it for non APZC now and port to APZC later.
This doesn't support APZC very well. The counters stays at the top left of the displayport which is offscreen when not at the top of the layer/page. I'll push to improve it there in a follow-up.
Comment on attachment 8341499 [details] [diff] [review]
Part 2: Add layers.scroll-graph

This is not the right patch. Its a very cool patch though :)
Attached patch Part 2: Add layers.scroll-graph (obsolete) — Splinter Review
Right patch this time :)
Attachment #8341499 - Attachment is obsolete: true
Attachment #8341499 - Flags: review?(gal)
Attachment #8341562 - Flags: review?(gal)
Comment on attachment 8341562 [details] [diff] [review]
Part 2: Add layers.scroll-graph

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

::: modules/libpref/src/init/all.js
@@ +4240,5 @@
>  pref("layers.max-active", -1);
> +// When a layer is moving it will add a scroll graph to measure the smoothness
> +// of the movement. Data will be output to stderr and can be graph more
> +// accuratly in a spreadsheet. NOTE: This pref triggers composites to refresh
> +// the graph.

Opps, I intended to do this but haven't done it yet.
Comment on attachment 8341562 [details] [diff] [review]
Part 2: Add layers.scroll-graph

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

r=me. You will need to explain to people how this works and what the graph means and want to add a Gaia pref for this? (or ask someone to do it for you).

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +96,5 @@
> +  std::vector<VelocityData> mData;
> +};
> +
> +static void
> +DrawVelGraph(const nsIntRect& aClipRect,

Nit: This fits in one line and would look simpler.

@@ +104,5 @@
> +  gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> +
> +  TimeStamp now = TimeStamp::Now();
> +
> +  void* key = reinterpret_cast<void*>(gfxPlatform::GetPrefLayersScrollGraph);

Want to do a dedicated static char sLayerVelocityUserDataKey; for this?

@@ +120,5 @@
> +  }
> +
> +  nsIntPoint scrollOffset = aLayer->GetEffectiveVisibleRegion().GetBounds().TopLeft();
> +  velocityData->mData.push_back(
> +    LayerVelocityUserData::VelocityData{now, Point(scrollOffset.x,

Nit: I didn't even know this syntax works. Want to use a real constructor? Would make this shorter.

@@ +144,5 @@
> +      return;
> +    }
> +  }
> +
> +  if (aLayer->GetEffectiveVisibleRegion().GetBounds().width < 300 ||

This might not work well for HVGA phones.

@@ +303,5 @@
>      } else {
>        layerToRender->RenderLayer(clipRect);
>      }
> +
> +    if (gfxPlatform::GetPrefLayersScrollGraph()) {

This is racy but who cares.
Attachment #8341562 - Flags: review?(gal) → review+
(In reply to Andreas Gal :gal from comment #16)
> > +  gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> > +
> > +  TimeStamp now = TimeStamp::Now();
> > +
> > +  void* key = reinterpret_cast<void*>(gfxPlatform::GetPrefLayersScrollGraph);
> 
> Want to do a dedicated static char sLayerVelocityUserDataKey; for this?

Ok. Normally I do things like that to save a few bytes in the executable when it doesn't hurt readability much.
(In reply to Andreas Gal :gal from comment #16)
> @@ +144,5 @@
> > +      return;
> > +    }
> > +  }
> > +
> > +  if (aLayer->GetEffectiveVisibleRegion().GetBounds().width < 300 ||
> 
> This might not work well for HVGA phones.
> 

The size of the graph would also be wrong. Let's port this to HVGA phones when we need it. We can always fallback to doing something like 'adb lolcat | grep ScrollGraph' in the time being.

I'll be adding a simple printf to get the data out.
Actually I added the printf_stderr code but comment out since it's too noisy and I'm afraid it might drag down performance. For now we can enable it by rebuilding.
Attachment #8341562 - Attachment is obsolete: true
Attachment #8341748 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/361907c4a2ce

Documentation coming after it merges.
Whiteboard: [leave open]
*sigh* This intermittent failure was my fault:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6fa6ac9d11
Whiteboard: [qa-]
See Also: → 970751
Great tool!

Hi BenWa,
On B2G, render scroll graph on the device is ok, but if we can sent velocityData data to a viewer(a browser viewer on desktop, which attach with profiled B2G device), draw lines and do statistic analysis in that viewer, I think it's easier to address scrolling jank problem in bug 958592, how do you think?
Flags: needinfo?(bgirard)
(In reply to C.J. Ku[:CJKu] from comment #26)
> Great tool!
> 
> Hi BenWa,
> On B2G, render scroll graph on the device is ok, but if we can sent
> velocityData data to a viewer(a browser viewer on desktop, which attach with
> profiled B2G device), draw lines and do statistic analysis in that viewer, I
> think it's easier to address scrolling jank problem in bug 958592, how do
> you think?

typo: it should be bug 979751 - Making more smooth scrolling on scrollable layer
Attached image jank.jpg
Another reason that I suggest to draw graph in another viewer is because even if the graph looks smooth in the graph sprite, it may not really smooth.

We may need a bigger diagram and do statistic analysis to figure out this small factors in scrolling.
Yes. It's planned here:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#150

We should be able to write a small program to convert adb logcat and graph it.

(In reply to C.J. Ku[:CJKu] from comment #28)

I agree. Great example!
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.