Closed Bug 1797975 Opened 2 years ago Closed 2 years ago

We may be skipping composites that should not be skipped

Categories

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

defect

Tracking

()

RESOLVED FIXED
109 Branch
Performance Impact medium
Tracking Status
firefox109 --- wontfix
firefox110 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

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.

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.

Performance Impact: --- → ?

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.

Performance Impact: ? → medium
  • 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.

Assignee: nobody → nical.bugzilla
Status: NEW → ASSIGNED

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

mIsRendering does not appear to exist (anymore).

Depends on D162295

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

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

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

Attached file Data reviewe request (deleted) —
Attachment #9303910 - Flags: data-review?(chutten)
Pushed by nsilva@mozilla.com: 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
Blocks: 1800178
No longer blocks: 1800178
Blocks: 1776885

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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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+

Attachment #9303910 - Flags: data-review?(chutten) → data-review+
Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ba3cc162a96d Add telemetry for skipped composites. r=bas
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: