1797975 - We may be skipping composites that should not be skipped

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Nicolas Silva [:nical]]

This profile was captured by smaug https://share.firefox.dev/3sEYsPE on the first test of the MotionMark suite. Quoting him:

The renderer track seems to usually do normal composites and child process gets DidComposite, but every now and then there is a very short one, and child process doesn't get a notification and is then blocked from running refreshdriver

There are skipped composites in that profile so probably not a timing issue. The skipped composite reason is "Too many pending frames" so the compositor thread thinks webrender is running behind. The code that decides whether that is the case is https://searchfox.org/mozilla-central/rev/968bd894205cf4f579d94ac4e175cc3187458605/gfx/webrender_bindings/RenderThread.cpp#656

The profile shows that webrender's threads are mostly idle so it doesn't look like they are actually running behind when the composites are skipped so it looks like the code that decides when to throttle composites is might not be working properly.

Comment 1

[Nicolas Silva [:nical]]

Getting some telemetry on the volume of skipped composites would be valuable because it's hard to notice when frames are incorrectly skipped every now and then but it can lead to the feeling that things are not smooth when average timings and frames per second look good.

Comment 2

[Bas Schouten (:bas.schouten)]

Not sure how to gauge the performance impact here. But since it's potentially high, but we don't know how often it's happening, I'm going to put it on medium.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Simplify the compositor throttling logic. r=#gfx-reviewers

• Ensure that the pending frame count is decremented by HandleFrameOneDoc even after some of the rare early-outs.
• Always check TooManyPendingFrames in CompositeToTarget.

It is unclear that these will actually catch (all of) the extraneous skipped frames. If anything, the simplification will make further investigation easier.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Rename CompositeIfNeeded into RetrySkippedComposite. r=#gfx-reviewers

The previous name could have related to any situation where a composite is needed such as when animating or receiving transactions, but the function is actually specifically about scheduling a new composite if the previous one was skipped.

Depends on D162294

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Remove outdated comments about mIsRendering. r=#gfx-reviewers

mIsRendering does not appear to exist (anymore).

Depends on D162295

Comment 6

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Reset mSkippedComposite when a composite is sucessfully scheduled. r=#gfx-reviewers

mSkippedComposite's purpose is to keep track of the fact that the window is not up to date because the previous composite was skipped. This informs RetrySkippedComposite that a new frame is needed to get the latest changes rendered. As soon as we successfully schedule a composite know that the latest changes are en route to be rendered so we don't need RetrySkippedComposite to push an additional frame later.

Depends on D162296

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Simplify the pending frame count tracking code. r=#gfx-reviewers

Only transactions that contain the generate_frame flag are tracked by the pending frame and frame build counters.
This patch attempts to make this clearer with two small adjustments:

Firstly by putting the IncPendingFrameCount call right next to Transaction::GenerateFrame.

Secondly, undoing the hack in wr_notifier_wake_up. The latter is called outside of normal rendered/tracked frames and was calling HandleFrameOneDoc which decrements the rendered frame counter. To compensate it had to manually increment both counters via IncPendingFrameCount and manually decrement the built frame counter via DecPendingFrameBuildCount. Instead this patch introduces the aTrackedFrame argument so that HandleFrameOneDoc only fiddles with the counters when needed and wake_up does not have to hack around it.

Depends on D162297

Comment 8

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Replace wr_notifier_nop_frame_done with an argument in new_frame_ready. r=#gfx-reviewers

it started as a single method new_frame_ready with a composite parameter, to be split into two functions that end up calling the same HandleFrameOneDoc with a composite parameter so the extra function doesn't provide anything.

Depends on D162298

Comment 9

[Nicolas Silva [:nical]]

Attachment: Bug 1797975 - Add telemetry for skipped composites. r=Bas

Comment 10

[Nicolas Silva [:nical]]

Attachment: Data reviewe request

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2b96b89802f
Simplify the compositor throttling logic. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/8052df51ea13
Rename CompositeIfNeeded into RetrySkippedComposite. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/39bd3c9f6c8e
Remove outdated comments about mIsRendering. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/6d671869a8d8
Reset mSkippedComposite when a composite is sucessfully scheduled. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/a53e533acf23
Simplify the pending frame count tracking code. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/c3f622f27e59
Replace wr_notifier_nop_frame_done with an argument in new_frame_ready. r=gfx-reviewers,bradwerth

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e2b96b89802f
https://hg.mozilla.org/mozilla-central/rev/8052df51ea13
https://hg.mozilla.org/mozilla-central/rev/39bd3c9f6c8e
https://hg.mozilla.org/mozilla-central/rev/6d671869a8d8
https://hg.mozilla.org/mozilla-central/rev/a53e533acf23
https://hg.mozilla.org/mozilla-central/rev/c3f622f27e59

Comment 13

[Nicolas Silva [:nical]]

Let's wait for a bit and see if the problem still happens (I wasn't able to repro before/after the patches), and also wait for the telemetry probe to land before closing.

Comment 14

[Chris H-C :chutten]

Comment on attachment 9303910 [details]
Data reviewe request

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.

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

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

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

Yes, Nicolas Silva is responsible.

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 all channels.

Does the instrumentation include the addition of any new identifiers?

No.

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

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba3cc162a96d
Add telemetry for skipped composites. r=bas

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ba3cc162a96d