Make it easier to collect frame uniformity results

RESOLVED FIXED in 2.1 S4 (12sep)

Status

Firefox OS
Performance
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=devtools p=3 s= u=])

Attachments

(4 attachments, 9 obsolete attachments)

3.35 MB, text/plain
Details
2.99 MB, text/plain
Details
44 bytes, text/x-github-pull-request
Details | Review | Splinter Review
6.46 KB, patch
mchang
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 2

4 years ago
Created attachment 8480963 [details] [review]
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!
Attachment #8480963 - Flags: feedback?(bgirard)
(Assignee)

Comment 3

4 years ago
Created attachment 8480965 [details] [diff] [review]
WIP - Add layer position data to profiles in the SPS profiler
(Assignee)

Comment 4

4 years ago
Created attachment 8480966 [details]
Sample Frame Uniformity Profile - Master
(Assignee)

Comment 5

4 years ago
Created attachment 8480967 [details]
Sample Frame Uniformity Profile - Touch Resampling
Comment on attachment 8480963 [details] [review]
Enable Cleopatra front end to read frame uniformity data

Looks great with the two issues mentioned fixed.
Attachment #8480963 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 8483176 [details] [diff] [review]
WIP: Add Layer Transforms and Touch Events to SPS Profiler

Uses the SPS Profiler marker payloads to capture touch data and layer transforms.
Attachment #8480965 - Attachment is obsolete: true
Attachment #8480966 - Attachment is obsolete: true
Attachment #8480967 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8483177 [details]
Sample Profile w/ Master
(Assignee)

Comment 9

4 years ago
Created attachment 8483178 [details]
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.
(Assignee)

Comment 10

4 years ago
Created attachment 8483722 [details] [review]
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.
Attachment #8480963 - Attachment is obsolete: true
Attachment #8483722 - Flags: review?(bgirard)
(Assignee)

Comment 11

4 years ago
Created attachment 8483724 [details] [diff] [review]
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!
Attachment #8483176 - Attachment is obsolete: true
Attachment #8483724 - Flags: review?(ehsan.akhgari)
Attachment #8483724 - Flags: review?(bgirard)

Comment 12

4 years ago
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. :)
Attachment #8483724 - Flags: review?(ehsan.akhgari)
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)
Attachment #8483724 - Flags: review?(bgirard) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 8485101 [details] [diff] [review]
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.
Attachment #8483724 - Attachment is obsolete: true
Attachment #8485101 - Flags: review?(bgirard)
(Assignee)

Comment 15

4 years ago
Created attachment 8485102 [details] [diff] [review]
Add Layer Transforms and Touch Events to SPS Profiler

Woops forgot to refresh the patch. Here's the right one.
Attachment #8485101 - Attachment is obsolete: true
Attachment #8485101 - Flags: review?(bgirard)
Attachment #8485102 - Flags: review?(bgirard)
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.
Attachment #8485102 - Flags: review?(bgirard) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8485330 [details] [diff] [review]
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
Attachment #8485102 - Attachment is obsolete: true
Attachment #8485330 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: perf → checkin-needed
(Assignee)

Updated

4 years ago
Attachment #8483722 - Flags: review?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/c1cc52b986f8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

4 years ago
Created attachment 8486187 [details] [diff] [review]
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
Attachment #8485330 - Attachment is obsolete: true
Attachment #8486187 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/082148b5e2d4
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)

Updated

4 years ago
Depends on: 1065241
You need to log in before you can comment on or make changes to this bug.