Closed Bug 1372602 Opened 7 years ago Closed 7 years ago

The hidden window spends time creating a compositor / texture factory identifier that it will never use

Categories

(Core :: Graphics: Layers, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Performance Impact medium
Tracking Status
firefox57 --- fixed

People

(Reporter: mconley, Assigned: mchang)

Details

(Whiteboard: [tpi:+][QRC][QRC_Analyzed])

Attachments

(1 file)

Here's a profile during start up, before first paint:

https://perfht.ml/2s6SYNx

We're spending 11ms waiting for a sync message response to come back from the compositor. This is for the hidden window, which is never actually displayed. I suspect the hidden window doesn't actually need this stuff.
Please update the Priority if this is a P1.
Priority: -- → P3
Whiteboard: [qf] → [qf][tpi:+]
Whiteboard: [qf][tpi:+] → [qf][tpi:+][QRC][QRC_NeedAnalysis]
Ehsan explained to me that this isn't macOS-only. It sounds like removing the hidden window isn't a short-term option but getting rid of this compositor communication seems to make sense.

11 ms during startup seems like a big enough win to make this at least [qf:p2].

Ehsan mentioned we have support for headless widgets now and that maybe we could use that to avoid getting a compositor for it?
OS: Mac OS X → All
Hardware: Unspecified → All
Whiteboard: [qf][tpi:+][QRC][QRC_NeedAnalysis] → [qf:p2][tpi:+][QRC][QRC_NeedAnalysis]
Seems this shouldn't be in Widget:Cocoa
Component: Widget: Cocoa → Widget
Component: Widget → Graphics: Layers
Mason, Please take a look at the profile proved by Mconley.
Assignee: nobody → mchang
I took a look at this. We lazily create a compositor when layout thinks we have to paint. Since we create a compositor through ipdl, and we're already painting right now, we need to make sure the compositor is ready to go before we start painting. This call acts as our check to make sure that we create the compositor and then we can create the correct layers backend in the content process. We can't really kick this off later since we need to paint right now.

In the second part of the profile, we see nsChildView::UpdateWindowDraggingRegion which requires specific paint flags [1], we still schedule a composite during startup. I think the bug here is that layout shouldn't be asking for a paint in this case, then we'd never create the compositor. 

[1] http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3675
Does comment 5 make sense to you or am I missing something special with the hidden window?
Flags: needinfo?(mstange)
That makes sense, and it's quite surprising. So it seems like this is something that needs to be fixed on the widget or layout side.
Flags: needinfo?(mstange)
Just an update. We schedule a paint because the hidden window isn't actually hidden. It shares a bunch of stuff with browser.xul, which causes the paint. I'm still trying to find out how we actually make the hidden window hidden.
I think this is right? When we create the hidden window [1], we create a hidden top level window. The window creates a bunch of child widgets, which have window type child [2]. When we go to paint the hidden window, we're actually trying to paint one of the child widgets rather than the actual top level window.

Views seem to be living inside windows as paintable areas of content. I think if a window isn't visible, a view in that window shouldn't be visible as well.

[1] https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsAppShellService.cpp#139
[2] http://searchfox.org/mozilla-central/source/widget/nsWidgetInitData.h#20
Attachment #8880603 - Flags: review?(mstange)
The hidden window doesn't show up on windows. The first thing we paint is browser.xul (assuming the sanity test already passed).
Whiteboard: [qf:p2][tpi:+][QRC][QRC_NeedAnalysis] → [qf:p2][tpi:+][QRC][QRC_Analyzed]
Comment on attachment 8880603 [details] [diff] [review]
Make child widgets not visible if the window around it isn't visible

Review of attachment 8880603 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the extremely slow review here. This looks fine.

I've done some testing and couldn't find any problems.
Attachment #8880603 - Flags: review?(mstange) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa9361b1968
Make child widgets not visible if the window around it isn't visible on OS X. r=mstange
https://hg.mozilla.org/mozilla-central/rev/8fa9361b1968
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Andrew Overholt [:overholt] from comment #2)
> Ehsan mentioned we have support for headless widgets now and that maybe we
> could use that to avoid getting a compositor for it?

AFAIU, this bug did not take that route. Is there a bug to investigate/implement your proposal?
Flags: needinfo?(overholt)
(In reply to Florian Bender from comment #15)
> (In reply to Andrew Overholt [:overholt] from comment #2)
> > Ehsan mentioned we have support for headless widgets now and that maybe we
> > could use that to avoid getting a compositor for it?
> 
> AFAIU, this bug did not take that route. Is there a bug to
> investigate/implement your proposal?

302 Ehsan
Flags: needinfo?(overholt) → needinfo?(ehsan)
Mason wrote the patch, it's unclear to me if he investigated the other approach and whether that would have worked or whether there was a problem with it.
Flags: needinfo?(ehsan)
(In reply to Florian Bender from comment #15)
> (In reply to Andrew Overholt [:overholt] from comment #2)
> > Ehsan mentioned we have support for headless widgets now and that maybe we
> > could use that to avoid getting a compositor for it?
> 
> AFAIU, this bug did not take that route. Is there a bug to
> investigate/implement your proposal?

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #17)
> Mason wrote the patch, it's unclear to me if he investigated the other
> approach and whether that would have worked or whether there was a problem
> with it.

Can you please clarify and/or point to the bug?
Flags: needinfo?(mchang)
(In reply to Florian Bender from comment #18)
> (In reply to Florian Bender from comment #15)
> > (In reply to Andrew Overholt [:overholt] from comment #2)
> > > Ehsan mentioned we have support for headless widgets now and that maybe we
> > > could use that to avoid getting a compositor for it?
> > 
> > AFAIU, this bug did not take that route. Is there a bug to
> > investigate/implement your proposal?
> 
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #17)
> > Mason wrote the patch, it's unclear to me if he investigated the other
> > approach and whether that would have worked or whether there was a problem
> > with it.
> 
> Can you please clarify and/or point to the bug?

Sorry, I don't understand your question. Comment 5 is the approach with the patch attached to this bug. Can you please clarify your question? Thanks!
Flags: needinfo?(mchang) → needinfo?(fb+mozdev)
Performance Impact: --- → P2
Whiteboard: [qf:p2][tpi:+][QRC][QRC_Analyzed] → [tpi:+][QRC][QRC_Analyzed]
Flags: needinfo?(fb+mozdev)
You need to log in before you can comment on or make changes to this bug.