Twice as many DidComposite messages as vsync messages in the content process

RESOLVED FIXED in Firefox -esr52



2 years ago
2 years ago


(Reporter: billm, Assigned: mattwoodrow)



Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox-esr5253+ fixed, firefox53 fixed, firefox54 fixed)


(Whiteboard: gfx-noted)


(1 attachment)

Telemetry is showing that the content process receives DidComposite messages at twice the rate as vsync messages. Specifically, the #1 and #2 runnables in the content process are:

('PCompositorBridge::Msg_DidComposite', 36314),
('PVsync::Msg_Notify', 19854)

The numbers are in millions of runnables across two days of nightly usage.

Matt told me that this is an unexpected result, so I'm filing it as a bug. We should investigate why there are so many DidComposite notifications.
Priority: -- → P3
Whiteboard: gfx-noted
I figured out why this is. We send the DidComposite message over IPC here:

The problem is that we're sending one message per tab, even for tabs that aren't visible. Even if the messages for invisible tabs do nothing, that's a lot of messages for the IPC system to handle. Also, it looks like the messages for the invisible tabs are doing actual work.

Matt, is this the expected behavior?
Flags: needinfo?(matt.woodrow)

Comment 2

2 years ago
We only need to send DidComposite to a tab if it was contained within the Layer tree that was just composited.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Attachment #8838997 - Flags: review?(dvander)
Attachment #8838997 - Flags: review?(dvander) → review+

Comment 3

2 years ago
Pushed by
Only send DidComposite to affected tabs. r=dvander

Comment 4

2 years ago
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Considering this regressed in 47 we should probably uplift.
Blocks: 1245765
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
Keywords: regression
Likely too late for Fx52 without strong motivation at this point (the RC1 build was already created on Monday).
status-firefox-esr52: --- → affected
tracking-firefox52: --- → ?
tracking-firefox-esr52: --- → ?
Please request Aurora/ESR52 approval on this when you get a chance. Too late for Fx52 at this point.
status-firefox52: affected → wontfix
tracking-firefox52: ? → ---
Flags: needinfo?(matt.woodrow)

Comment 8

2 years ago
Comment on attachment 8838997 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: Unknown, old regression.
[User impact if declined]: Processing more runnables than necessary, wasted CPU cycles.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just removes pointless runnables from being sent.
[String changes made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8838997 - Flags: approval-mozilla-aurora?
Attachment #8838997 - Flags: approval-mozilla-esr52?
Attachment #8838997 - Flags: approval-mozilla-beta?
Attachment #8838997 - Flags: approval-mozilla-aurora?
Comment on attachment 8838997 [details] [diff] [review]

Fix a regression to avoid wasting CPU cycles. Beta53+.
Attachment #8838997 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

2 years ago
status-firefox53: affected → fixed
Setting qe-verify- based on Matt's assessment on manual testing needs (see Comment 8).
Flags: qe-verify-
Comment on attachment 8838997 [details] [diff] [review]

avoid unnecessary ipc, esr52+
Attachment #8838997 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 13

2 years ago
status-firefox-esr52: affected → fixed
tracking-firefox-esr52: ? → 53+
You need to log in before you can comment on or make changes to this bug.