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.
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?
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
Attachment #8838997 - Flags: review?(dvander)
2 years ago
See Also: → bug 1340962
Attachment #8838997 - Flags: review?(dvander) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b75eb9cec112 Only send DidComposite to affected tabs. r=dvander
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.
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
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: ? → ---
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.
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+
status-firefox53: affected → fixed
Setting qe-verify- based on Matt's assessment on manual testing needs (see Comment 8).
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+
status-firefox-esr52: affected → fixed
See Also: → bug 1376763
You need to log in before you can comment on or make changes to this bug.