mNeedsComposite counter is based on faulty assumption; can result in spurious overcompositing on Android

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: kats, Assigned: kats)

Tracking

({perf})

unspecified
mozilla64
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments)

Spinoff from https://bugzilla.mozilla.org/show_bug.cgi?id=1432019#c24:

The mNeedsComposite counter in CompositorVsyncScheduler tracks the number of composite requests that the scheduler has received since the last vsync. On Android, it also forces a composite immediately if that number is >= 2 [1]. I added this code back in bug 1225950, but it has a faulty assumption that in the normal course of events we'll only get one composite request (i.e. one increment of mNeedsComposite) per vsync interval.

This assumption is wrong, because APZ can have multiple scroll animations going, or might receive multiple touch events within a single vsync interval, each of which could call ScheduleComposite. In this case mNeedsComposite can increment rapidly and trigger extra composites in between vsyncs. This is unnecessary. It would be better to change mNeedsComposite to be a timestamp-based mechanism rather than a simple counter. I wrote a WIP at [2] that is what I had in mind but it needs more testing.

[1] https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/gfx/layers/ipc/CompositorVsyncScheduler.cpp#166-173
[2] https://hg.mozilla.org/try/rev/60bf653625f1ed8bebde7a56961538b9974e06d8
Keywords: perf
OS: Unspecified → Android
Summary: mNeedsComposite counter is based on faulty assumption; can result in spurious overcompositing → mNeedsComposite counter is based on faulty assumption; can result in spurious overcompositing on Android

Updated

10 months ago
Priority: -- → P2
The mNeedsComposite counter was used to force a composite immediately if
the scheduler received a number of composite requests without actually
getting a vsync. It was necessary on Fennec because of main-thread
contention. However it was wrong because it assumes only a single
composite gets requested per vsync interval, which is not true. Instead
of using a counter this patch uses a timestamp to ensure that we only
force the vsync after two vsync intervals have elapsed.

Depends on D8765

Comment 4

8 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b032c73f6f69
Expose the vsync interval via the CompositorVsyncSchedulerOwner interface. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/23ac21b643c2
Replace the mNeedsComposite counter with a timestamp. r=sotaro

Comment 5

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b032c73f6f69
https://hg.mozilla.org/mozilla-central/rev/23ac21b643c2
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Duplicate of this bug: 1499841
You need to log in before you can comment on or make changes to this bug.