Closed Bug 1337548 Opened 7 years ago Closed 7 years ago

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


(Core :: Graphics, defect, P3)




Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 --- fixed
firefox54 --- fixed


(Reporter: billm, Assigned: mattwoodrow)



(Keywords: regression, Whiteboard: gfx-noted)


(1 file)

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)
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+
Pushed by
Only send DidComposite to affected tabs. r=dvander
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Considering this regressed in 47 we should probably uplift.
Likely too late for Fx52 without strong motivation at this point (the RC1 build was already created on Monday).
Please request Aurora/ESR52 approval on this when you get a chance. Too late for Fx52 at this point.
Flags: needinfo?(matt.woodrow)
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+
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+
You need to log in before you can comment on or make changes to this bug.