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

RESOLVED FIXED in Firefox -esr52

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: mattwoodrow)

Tracking

({regression})

unspecified
mozilla54
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

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

Details

(Whiteboard: gfx-noted)

Attachments

(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:
http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/gfx/layers/ipc/CompositorBridgeParent.cpp#1847

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)
(Assignee)

Comment 2

2 years ago
Created attachment 8838997 [details] [diff] [review]
only-send-did-composite-to-affected-tabs

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 mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75eb9cec112
Only send DidComposite to affected tabs. r=dvander

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b75eb9cec112
Status: NEW → RESOLVED
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)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8838997 [details] [diff] [review]
only-send-did-composite-to-affected-tabs

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]
only-send-did-composite-to-affected-tabs

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

Comment 10

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e2752c5516c1
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]
only-send-did-composite-to-affected-tabs

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

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/29a1ad09a6ec
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.