FlushAsyncPaints is not part of CONTENT_PAINT_PHASE_WEIGHT

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

We track how much time we spend waiting on previous frame paints with GFX_OMTP_PAINT_WAIT_TIME and gfx.omtp.paint_wait_ratio, but not as a part of CONTENT_PAINT_PHASE_WEIGHT [1].

My concern here is if we're using CONTENT_PAINT_PHASE_WEIGHT as a metric for which phase of painting to optimize on the main thread, we may be ignoring the cost of rasterization. But at the same time, the cost of FlushAsyncPaints depends on a previous frame's contents, so maybe it's not fair to include here.

As a note, this did not change with the move to blocking in EndTransaction.

[1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/gfx/layers/client/ClientLayerManager.cpp#323
Matt, Bas, do you have any opinions here?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bas)
Can we just add a new bucket for time spent waiting on async paints?
Flags: needinfo?(matt.woodrow)
That sounds reasonable to me if you don't have any concerns about that. I'll write up a patch.
Flags: needinfo?(bas)
Are there any active studies on this histogram that adding this would violate? Otherwise I tested this out briefly locally and it seemed to work well.
Attachment #8983506 - Flags: review?(matt.woodrow)
Comment on attachment 8983506 [details] [diff] [review]
flush-rasterize.patch

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

I'm not aware of anyone that would be negatively affected by this change!

Adding data-review?francois.
Attachment #8983506 - Flags: review?(matt.woodrow)
Attachment #8983506 - Flags: review?(francois)
Attachment #8983506 - Flags: review+
Which histogram is this using? Can you point me to the bug number where the data review for that histogram was added?

If it's already been reviewed and we're just adding another bucket, there's no need to fill out the complete data review request form again.
Flags: needinfo?(rhunt)
The histograms are CONTENT_SMALL_PAINT_PHASE_WEIGHT and CONTENT_LARGE_PAINT_PHASE_WEIGHT. They were added in bug 1309442, and this is just adding another bucket to these histograms.

It looks like the data-review wasn't actually finished in that bug though? David is exited so I'll continue the data review in that bug.
Flags: needinfo?(rhunt)
Comment on attachment 8983506 [details] [diff] [review]
flush-rasterize.patch

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

The initial data review was done in bug 1309442. The extra data collected within this patch doesn't change any of the answers in https://bugzilla.mozilla.org/show_bug.cgi?id=1309442#c14.

datareview+
Attachment #8983506 - Flags: review?(francois) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9f90b47174
Add FlushRasterization as a metric for CONTENT_PAINT_PHASE_WEIGHT. r=mattwoodrow, data-review=francois
https://hg.mozilla.org/mozilla-central/rev/9d9f90b47174
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.