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)
Core
Graphics: Layers
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mconley, Assigned: mchang)
Details
(Whiteboard: [tpi:+][QRC][QRC_Analyzed])
Attachments
(1 file)
1.19 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•7 years ago
|
||
Please update the Priority if this is a P1.
Priority: -- → P3
Whiteboard: [qf] → [qf][tpi:+]
Updated•7 years ago
|
Whiteboard: [qf][tpi:+] → [qf][tpi:+][QRC][QRC_NeedAnalysis]
Comment 2•7 years ago
|
||
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]
Updated•7 years ago
|
Component: Widget → Graphics: Layers
Comment 4•7 years ago
|
||
Mason, Please take a look at the profile proved by Mconley.
Assignee: nobody → mchang
Assignee | ||
Comment 5•7 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•7 years ago
|
||
Does comment 5 make sense to you or am I missing something special with the hidden window?
Flags: needinfo?(mstange)
Comment 7•7 years ago
|
||
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•7 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•7 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•7 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•7 years ago
|
Whiteboard: [qf:p2][tpi:+][QRC][QRC_NeedAnalysis] → [qf:p2][tpi:+][QRC][QRC_Analyzed]
Comment 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a893d5e870fe099bcb35180d9a40b95a3d62b38c
Comment 13•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8fa9361b1968
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 15•7 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)
Comment 16•7 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? 302 Ehsan
Flags: needinfo?(overholt) → needinfo?(ehsan)
Comment 17•7 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•7 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•7 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)
Updated•2 years ago
|
Performance Impact: --- → P2
Whiteboard: [qf:p2][tpi:+][QRC][QRC_Analyzed] → [tpi:+][QRC][QRC_Analyzed]
Updated•2 years ago
|
Flags: needinfo?(fb+mozdev)
You need to log in
before you can comment on or make changes to this bug.
Description
•