1049258 - Make it easier to collect frame uniformity results

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect, P2)

Description

[Mason Chang [Inactive] [:mchang]]

Right now it's a hassle to collect frame uniformity results. We have to enable a pref, then setup an oragutan build, then copy / paste data from logcat to the frame uniformity tool (http://people.mozilla.org/~mchang/FrameUniformity.html).

See if we can either (a) import this data into the current cleopatra profiler or (b) have an interface like the cleopatra profiler where we don't have to spew data to logcat.

Comment 1

[Benoit Girard (:BenWa)]

It looks like this data is printed using printf_stderr. You could start the profiler with sampling turned off (-i 100000). The profile will include the data you need, you will just need to parse it. Even better you can put the data into markers if you want to avoid the parsing. Then when you dump the profile it will include the data you need.

Then you can either (1) modify FrameUniformity.html to look at the FrameUniformaty data in the profile JSON and ignore the rest or (2) integrate FrameUniformaty.html with the cleopatra. Note for (2) we already have FrameUniformaty data that is derived from the layers dump so we might want to consolidate that if we go with option (2).

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Enable Cleopatra front end to read frame uniformity data

I added support in Gecko to dump the layer uniformity data into profiles. These are the modifications in the front end Cleopatra to read the data and create a pretty graph. Still in beta since I still want to filter some data before making it final, so any feedback is appreciated.

One question I had was how can I remove the UniformityInfo markers in the timeline? It is a lot of spam. I was hoping there was a cleaner way than removing it from the marker data. Tahnks!

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Add layer position data to profiles in the SPS profiler

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Sample Frame Uniformity Profile - Master

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Sample Frame Uniformity Profile - Touch Resampling

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8480963 [details] [review]
Enable Cleopatra front end to read frame uniformity data

Looks great with the two issues mentioned fixed.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Add Layer Transforms and Touch Events to SPS Profiler

Uses the SPS Profiler marker payloads to capture touch data and layer transforms.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Sample Profile w/ Master

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Sample Profile w/ Vsync

The most annoying downside to tracking touch data and layer transforms is that since touch events are processed on the main thread and layer transforms on the compositor thread, we have to profile both the compositor and GeckoMain thread to get both touch data and frame uniformity data.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Enable Cleopatra front end to read frame uniformity data

Updated with your feedback. I also generate a category for the frame position data so it doesn't spam the histogram view with markers.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Add Layer Transforms and Touch Events to SPS Profiler

Enable the SPS profiler to carry two new types of payload data: Layer position data and touch data. The patch creates two new classes, one for each payload data and adds the payload when appropriate. My only question is that I wasn't sure when to delete the payload data. Does the SPS profiler delete it automatically when the circular buffer is full? That's what it looked like from my reading. Thanks!

Comment 12

[(no longer active)]

Comment on attachment 8483724 [details] [diff] [review]
Add Layer Transforms and Touch Events to SPS Profiler

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

Clearing the review flag, assuming that you didn't actually want two reviews here. :)

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 8483724 [details] [diff] [review]
Add Layer Transforms and Touch Events to SPS Profiler

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

since you're creating a payload you may also want to make it conditional on if we're profiling to avoid a heap allocation.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +131,5 @@
>    }
>    Point translation = transform.As2D().GetTranslation();
> +  // I think these markers get deleted as we overwrite data or stop the profiler
> +  // Not really sure
> +  ProfilerBacktrace* stack = profiler_get_backtrace();

again do we really want a stack here?

::: tools/profiler/ProfilerMarkers.cpp
@@ +143,5 @@
> +void
> +LayerTransformPayload::streamPayloadImpl(JSStreamWriter& b)
> +{
> +  char buffer[32];
> +  snprintf(buffer, 128, "%p", mLayer);

This is a security bug

::: tools/profiler/ProfilerMarkers.h
@@ +129,4 @@
>    char* mFilename;
>  };
>  
> +class LayerTransformPayload : public ProfilerMarkerPayload

This could use a class comment. It's called a layer transform but it doesn't store a transform?

@@ +145,5 @@
> +  mozilla::layers::Layer* mLayer;
> +  mozilla::gfx::Point mPoint;
> +};
> +
> +class TouchDataPayload : public ProfilerMarkerPayload

class comment.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +395,5 @@
>          break;
>      }
>  
> +    const ScreenIntPoint& touchPoint = aMultiTouch.mTouches[0].mScreenPoint;
> +    ProfilerBacktrace* stack = profiler_get_backtrace();

Do you really need a stack here? This is always on when profiling and will cause a performance overhead with this portion of the code. We do this for restyle/reflow stacks cause it's a common thing to want to know but I'm not convinced the stack here is useful to the general developer.

Either hide it behind a pref or remove it.

@@ +397,5 @@
>  
> +    const ScreenIntPoint& touchPoint = aMultiTouch.mTouches[0].mScreenPoint;
> +    ProfilerBacktrace* stack = profiler_get_backtrace();
> +    TouchDataPayload* payload = new TouchDataPayload(touchPoint, stack);
> +    PROFILER_MARKER_PAYLOAD(touchAction, payload);

This will leak with the profiler disabled. Please add a delete call to PROFILER_MARKER_PAYLOAD when the profiler isn't defined (in GeckoProfiler.h)

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Add Layer Transforms and Touch Events to SPS Profiler

Updated with feedback from comment 13. Biggest changes are that unless the profiler_is_active(), we never do any work to capture touch data or layer position data.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: Add Layer Transforms and Touch Events to SPS Profiler

Woops forgot to refresh the patch. Here's the right one.

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 8485102 [details] [diff] [review]
Add Layer Transforms and Touch Events to SPS Profiler

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

::: tools/profiler/ProfilerMarkers.cpp
@@ +143,5 @@
> +LayerTranslationPayload::streamPayloadImpl(JSStreamWriter& b)
> +{
> +  const size_t bufferSize = 32;
> +  char buffer[bufferSize];
> +  snprintf(buffer, bufferSize, "%p", mLayer);

next time you can use sizeof(bufferSize). IMO that's less error prone.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Thanks! Try build - https://tbpl.mozilla.org/?tree=Try&rev=f494dc033af1

Comment 18

[Mason Chang [Inactive] [:mchang]]

Attachment: Add Layer Transforms and Touch Events to SPS Profiler (v3)

Carrying r+. Had to fix a build issue on linux. Successful try: https://tbpl.mozilla.org/?tree=Try&rev=8072722c649e

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c1cc52b986f8

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1cc52b986f8

Comment 21

[Carsten Book [:Tomcat]]

sorry had to backout this changes since it seems this broke non unified windows like like  https://tbpl.mozilla.org/php/getParsedLog.php?id=47597317&tree=Mozilla-Central

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Add Layer Transforms and Touch Events to SPS Profiler (v4)

Carrying r+. Updated to use PR_snprintf instead of just snprintf for windows builds. Successful try on non uniform builds - https://tbpl.mozilla.org/?tree=Try&rev=4b7a6a27ad65

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/082148b5e2d4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/082148b5e2d4