We may be skipping composites that should not be skipped
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
People
(Reporter: nical, Assigned: nical)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/plain
|
chutten
:
data-review+
|
Details |
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.
Assignee | ||
Comment 1•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 2•2 years ago
|
||
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.
Assignee | ||
Comment 3•2 years ago
|
||
- 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.
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
|
||
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
Assignee | ||
Comment 5•2 years ago
|
||
mIsRendering does not appear to exist (anymore).
Depends on D162295
Assignee | ||
Comment 6•2 years ago
|
||
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
Assignee | ||
Comment 7•2 years ago
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
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
Assignee | ||
Comment 9•2 years ago
|
||
Assignee | ||
Comment 10•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Comment 11•2 years ago
|
||
Comment 12•2 years ago
|
||
bugherder |
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
Assignee | ||
Comment 13•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 14•2 years ago
|
||
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•2 years ago
|
||
Comment 16•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•