Closed Bug 1309442 Opened 3 years ago Closed 3 years ago

Get telemetry for sub-phases of the painting pipeline

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Attached patch patchSplinter Review
The goal of this patch is to gather information to help direct graphics optimization work. There are three phases to painting that, with enough effort, we could make serious dents in: display list construction, layerization, and rasterization. We want to know which efforts will have the greatest value. This patch adds some telemetry to get some insight.

First, it tells us how long top-level content paints are taking. Then, when paints are over 16ms, we break that time down further into components:
 * Display List construction
 * Layerization (FrameLayerBuilder)
 * Rasterization

Rather than accumulate the raw time for these phases, we use the percent of total paint time. In addition we store all permutations of these components, so we can tell whether there's a correlation between, say, displaylist construction and rasterization.

Some sample data and analysis is coming.
Attachment #8800116 - Flags: review?(matt.woodrow)
From a browsing session on my desktop (nyt, imgur, gmail): content paint times.
Same session. In my session, FLB and DL times were always under 10% of the total paint time. So I've focused on the rasterization histogram here.

From these two graphs, assuming this patch is measuring correctly, we can estimate a few things:
  6% of my paints are over 16ms, average is 6ms.
  Average rasterization time is 60% of total paint time.
  Rasterization is usually anywhere from 26-75% of total paint time.

On my session, that leaves about 20% of our paint time unaccounted for, assuming DL+FLB are at most 20% of these long paints and rasterization is on average ~60%.

To compare, I turned gfx.content.always-paint to true and applied a patch from Bill to paint more aggressively. Unsurprisingly, the CONTENT_PAINT_TIME histogram got a lot more large paints (average: 17ms). However the histograms for how phases contribute to this time looked almost exactly the same.

We'll have to wait for this to get reviewed and landed to see whether those patterns are true at large.
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/613ed007ea30
Add telemetry for how phases of the painting pipeline contribute to large frame times. (bug 1309442, r=mattwoodrow)
Attachment #8800116 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/613ed007ea30
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
This seems to have missed data review.
Please note that all new measurements require data collection review:
https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(dvander)
Flags: needinfo?(dvander)
Attachment #8800116 - Flags: review?(gfritzsche)
Comment on attachment 8800116 [details] [diff] [review]
patch

I'm not a data collection peer, redirecting.
Attachment #8800116 - Flags: review?(gfritzsche) → review?(francois)
Comment on attachment 8800116 [details] [diff] [review]
patch

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

::: toolkit/components/telemetry/Histograms.json
@@ +10295,5 @@
> +    "expires_in_version": "56",
> +    "kind": "exponential",
> +    "high": 1000,
> +    "n_buckets": 50,
> +    "description": "Time spent in the paint pipeline for content."

Can you please add the units (ms? seconds?) for this measurement?

@@ +10301,5 @@
> +  "CONTENT_LARGE_PAINT_PHASE_WEIGHT": {
> +    "alert_emails": ["danderson@mozilla.com"],
> +    "bug_numbers": [1309442],
> +    "expires_in_version": "56",
> +    "keyed": true,

Is it necessary to use keys here or do you just need the overall percentage?

If you are using a key, the list of possible keys must be included in the description.

@@ +10305,5 @@
> +    "keyed": true,
> +    "kind": "linear",
> +    "high": 100,
> +    "n_buckets": 10,
> +    "description": "Percentage of time taken by phases in expensive content paints."

Do you mean percentage of time taken by:

1. **phases in expensive content paints** over the total time spent by phases in non-expensive and expensive content paints, or

2. **phases** over all expensive content paints (phases or not)?

It would be good if you could word it in a way that removes this ambiguity.
Attachment #8800116 - Flags: review?(francois) → review-
(In reply to François Marier [:francois] from comment #8)
> Comment on attachment 8800116 [details] [diff] [review]
> patch
> 
> Review of attachment 8800116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +10295,5 @@
> > +    "expires_in_version": "56",
> > +    "kind": "exponential",
> > +    "high": 1000,
> > +    "n_buckets": 50,
> > +    "description": "Time spent in the paint pipeline for content."
> 
> Can you please add the units (ms? seconds?) for this measurement?
> 

The time is in milliseconds.

> @@ +10301,5 @@
> > +  "CONTENT_LARGE_PAINT_PHASE_WEIGHT": {
> > +    "alert_emails": ["danderson@mozilla.com"],
> > +    "bug_numbers": [1309442],
> > +    "expires_in_version": "56",
> > +    "keyed": true,
> 
> Is it necessary to use keys here or do you just need the overall percentage?
> 
> If you are using a key, the list of possible keys must be included in the
> description.
> 

The keys are necessary. The keys are 'dl', 'flb', 'r', and in bug 1466146 I intend to add 'fr'.

> @@ +10305,5 @@
> > +    "keyed": true,
> > +    "kind": "linear",
> > +    "high": 100,
> > +    "n_buckets": 10,
> > +    "description": "Percentage of time taken by phases in expensive content paints."
> 
> Do you mean percentage of time taken by:
> 
> 1. **phases in expensive content paints** over the total time spent by
> phases in non-expensive and expensive content paints, or
> 
> 2. **phases** over all expensive content paints (phases or not)?
> 
> It would be good if you could word it in a way that removes this ambiguity.

Not sure if I entirely understand, I'll try to expand it to be more explicit.

Maybe "Percentages of times for phases in an expensive content paint relative to the time spent in the entire expensive paint"?

I'll put up an fixup patch here and reflag data review.
Attached patch content-paint-desc.patch (obsolete) — Splinter Review
Attachment #8984552 - Flags: review?(matt.woodrow)
Attachment #8984552 - Flags: feedback?(francois)
Attachment #8984552 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8984552 [details] [diff] [review]
content-paint-desc.patch

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

Apologies for the delay in getting back to you. Thanks for the improved descriptions!

Can you also point to an area of the code where these key abbreviations might be defined? Alternatively, you can add a "legend" in brackets at the end of the description.

Finally, has the data review form (https://github.com/mozilla/data-review/blob/master/request.md) ever been filled for this? I can't seem to find it. If it hasn't, then can you please fill it out and attach it as a text file to this bug. You can r? me on it and I'll review and r+ it. Thanks!
Attachment #8984552 - Flags: feedback?(francois) → feedback-
Carrying the r+ from Matt. I've updated this patch to include a legend on the keyed histograms.
Attachment #8984552 - Attachment is obsolete: true
Attachment #8986303 - Flags: review+
Attached file data-review-request.md
Attachment #8986305 - Flags: review?(francois)
Comment on attachment 8986305 [details]
data-review-request.md

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default on, only in pre-release channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are sufficient.
Attachment #8986305 - Flags: review?(francois) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58aab1bf314
Elaborate on descriptions for CONTENT_PAINT histograms. r=mattwoodrow
Depends on: 1518134
You need to log in before you can comment on or make changes to this bug.