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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mchang, NeedInfo)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

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

Attachments

(1 attachment)

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:+]

Updated

2 years ago
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

Comment 4

2 years ago
Mason, Please take a look at the profile proved by Mconley.
Assignee: nobody → mchang
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Comment 9

2 years ago
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)
(Assignee)

Comment 10

2 years ago
The hidden window doesn't show up on windows. The first thing we paint is browser.xul (assuming the sanity test already passed).

Updated

2 years ago
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+

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8fa9361b1968
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 15

2 years ago
(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)

Comment 17

2 years ago
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)

Comment 18

2 years ago
(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)
(Assignee)

Comment 19

2 years ago
(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)
You need to log in before you can comment on or make changes to this bug.