1503405 - Measure the impact of texture upload on CONTENT_FRAME_TIME

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Bobby Holley (:bholley)]

CONTENT_FRAME_TIME currently shows more frame drops with WR. Picture caching and blob performance work are expected to improve this.

Another possible source of jank here is texture upload. Currently, we wait to upload textures until the frame where we need them. So, for example, scrolling down a grid of images will not upload anything until the viewport intersects the first pixels of the next row, and then upload all the images in a single frame.

We could make this a lot smarter by gradually metering the uploads over several frames as the image approaches the viewport. There are various ways to implement this, but we first need to measure whether it would move the needle.

Matt suggested measuring how long we spend in texture upload for a given frame. Then, when we report CONTENT_FRAME_TIME, we'd record an additional probe called CONTENT_FRAME_TIME_MINUS_TEXTURE_UPLOAD. The difference between the two histograms would provide an upper bound on the win we could expect by optimizing this.

This telemetry should be straightforward to add, and I think we should do it on the soon side so that we can make decisions based on the data.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: request.md

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1503405 - Add telemetry for CONTENT_FRAME_WITH with texture upload excluded. r?jrmuizel

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Created attachment 9025236 [details]
> Bug 1503405 - Add telemetry for CONTENT_FRAME_WITH with texture upload
> excluded. r?jrmuizel

I took a different approach to exposing RenderStats in the patch on bug 1499251. I'd say it's cleaner but more invasive. Thoughts?

Comment 4

[Chris H-C :chutten]

Comment on attachment 9025235 [details]
request.md

Preliminary note:

You state in the request that you wish to permanently monitor the collection, but the patch has the probes expiring in 70. This review response will assume that the probes will expire in 70.

DATA COLLECTION REVIEW RESPONSE:

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

Yes. Standard Telemetry mechanisms apply.

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

Yes. Standard Telemetry mechanisms apply.

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

N/A. Expires in Firefox 70.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

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

Default on for prerelease channels only. (Application code limits collection to those with WebRender, which is presently a subpopulation of this, but it might expand during the course of the collection.)

    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.

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

Yes.

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

Yes. Matt Woodrow is reponsible for removing or renewing this metric before it expires in Firefox 70.

---
Result: datareview+

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> > Created attachment 9025236 [details]
> > Bug 1503405 - Add telemetry for CONTENT_FRAME_WITH with texture upload
> > excluded. r?jrmuizel
> 
> I took a different approach to exposing RenderStats in the patch on bug
> 1499251. I'd say it's cleaner but more invasive. Thoughts?

I'm not super sold on using Result in c++, but I can rebase on top of your patch if you want to land it today.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> > > Created attachment 9025236 [details]
> > > Bug 1503405 - Add telemetry for CONTENT_FRAME_WITH with texture upload
> > > excluded. r?jrmuizel
> > 
> > I took a different approach to exposing RenderStats in the patch on bug
> > 1499251. I'd say it's cleaner but more invasive. Thoughts?
> 
> I'm not super sold on using Result in c++, but I can rebase on top of your
> patch if you want to land it today.

No worries. You can land with your approach.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/939a5605cccf
Add telemetry for CONTENT_FRAME_WITH with texture upload excluded. r=jrmuizel

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/939a5605cccf