Closed Bug 1049258 Opened 6 years ago Closed 6 years ago

Make it easier to collect frame uniformity results

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S4 (12sep)

People

(Reporter: mchang, Assigned: mchang)

References

Details

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

Attachments

(4 files, 9 obsolete files)

3.35 MB, text/plain
Details
2.99 MB, text/plain
Details
44 bytes, text/x-github-pull-request
Details | Review
6.46 KB, patch
mchang
: review+
Details | Diff | Splinter Review
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).
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)
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+
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
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.
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)
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 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-
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)
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+
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+
Keywords: perfcheckin-needed
Attachment #8483722 - Flags: review?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/c1cc52b986f8
Status: ASSIGNED → RESOLVED
Closed: 6 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 → ---
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/082148b5e2d4
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
Depends on: 1065241
You need to log in before you can comment on or make changes to this bug.