Closed
Bug 1366874
Opened 7 years ago
Closed 7 years ago
[meta] WindowConstructor is expensive during startup
Categories
(Core :: Widget: Win32, defect, P2)
Tracking
()
RESOLVED
FIXED
Performance Impact | high |
People
(Reporter: florian, Unassigned)
References
Details
(Keywords: meta, Whiteboard: [tpi:+])
See this profile: https://perfht.ml/2qdobMq
This is when creating the hidden window.
mozilla::widget::IMEHandler::Initialize() (and more specifically mozilla::widget::TSFTextStore::Initialize()) takes a while here, with a very expensive CoCreateInstance call (I'm not familiar enough with the Windows APIs to figure out exactly which service we are creating an instance of). It looks like it does main thread IO in CAssemblyCacheFile::Open(unsigned char * *) and CAssemblyCacheFile::~CAssemblyCacheFile(). Could this be done off main thread?
Then OleInitialize takes 21ms. Again, could this be done off main thread?
Finally we have 88ms spent in nsUXThemeData::InitTitlebarInfo() calling GetSystemMetrics. This seems to be to get data that will be overwritten very soon when the first actual browser window will be opened, with a call to nsUXThemeData::UpdateTitlebarInfo (that spends another 12ms in NtUserGetWindowCompositionAttribute). Could we initialize this placeholder data lazily so that we don't pay this cost if the data isn't going to be used?
Jimm, could you please figure out what's actionable in this?
Flags: needinfo?(jmathies)
Comment 1•7 years ago
|
||
We should break this up I think.
For starters we spend a lot of time doing initialization in TSFTextStore::Initialize() [1]. Masayuki, do you think any of this can be delayed or done on demand in some way?
http://searchfox.org/mozilla-central/source/widget/windows/TSFTextStore.cpp#5989
Flags: needinfo?(jmathies) → needinfo?(masayuki)
Comment 2•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> Then OleInitialize takes 21ms. Again, could this be done off main thread?
No, it has to be run on the thread that is instantiating and accessing the COM object.
But one thing that we probably *can* do is completely eliminate that call; The main thread is already initialized for COM via mscom::MainThreadRuntime.
Comment 3•7 years ago
|
||
...though I guess it probably does some initialization overtop of just COM initialization?
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 4•7 years ago
|
||
Yeah, it inits a host of random COM based windows services like clipboard and drag and drop. We need this call.
Comment 5•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> See this profile: https://perfht.ml/2qdobMq
>
> This is when creating the hidden window.
>
> mozilla::widget::IMEHandler::Initialize() (and more specifically
> mozilla::widget::TSFTextStore::Initialize()) takes a while here, with a very
> expensive CoCreateInstance call (I'm not familiar enough with the Windows
> APIs to figure out exactly which service we are creating an instance of). It
> looks like it does main thread IO in CAssemblyCacheFile::Open(unsigned char
> * *) and CAssemblyCacheFile::~CAssemblyCacheFile(). Could this be done off
> main thread?
No, TSF is managed every thread (according to MSDN):
https://msdn.microsoft.com/en-us/library/windows/desktop/ms628979(v=vs.85).aspx
Even if we create them from another thread, how do we activate it in the main thread before any input of users? I guess that it's impossible.
On the other hand, some of the initialization could be put off.
I think that initializing each pref cache can be delayed until first use:
http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/widget/windows/TSFTextStore.cpp#6130-6163
sInputProcessorProfiles is used only by TSFStaticSink and debug code. TSFStaticSink is used for logging and hacking TSF bugs. So, we could put off to initialize it until first use:
http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/widget/windows/TSFTextStore.cpp#6008-6017
QIed interfaces could be put off until first use:
http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/widget/windows/TSFTextStore.cpp#6036-6054
And two COM objects are necessary only when user uses composition, so, their initialization could be put off:
http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/widget/windows/TSFTextStore.cpp#6063-6083
I think that creating and initializing disabled context couldn't be put off because it will be used immediately after that.
http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/widget/windows/TSFTextStore.cpp#6085-6104
Additionally, these initialization must be good to test TSF enough work on the environment.
In other words, after we put off some initialization until first use, we cannot test if TSF completely work on the environment and fallback to IMM mode forcibly. However, there shouldn't be such environment actually. So, I guess that this won't be a matter. I'll file follow up bugs to fix each issue.
Flags: needinfo?(masayuki)
Updated•7 years ago
|
Priority: -- → P2
Summary: WindowConstructor is expensive during startup → [meta] WindowConstructor is expensive during startup
Whiteboard: [qf:p1] → [qf:p1][tpi:+]
Comment 6•7 years ago
|
||
I fixed all bugs of TSFTextStore which can be fixed easy. How did they affect the score?
Flags: needinfo?(florian)
Reporter | ||
Comment 7•7 years ago
|
||
Here are 2 cold startup profiles of WindowConstructor with the current (June 20) nightly on the quantum reference hardware: http://bit.ly/2ryURBB (10ms)
http://bit.ly/2rzeY2L (15ms)
Note that IO cost is random, so I can't really give a 'score'. Overall, WindowConstructor is still visible, but seems to have improved significantly :-).
Flags: needinfo?(florian)
Comment 8•7 years ago
|
||
Thank you.
Looks like that ITfThreadMgr::Activate() is sometimes slow (taking 5ms in the former, but only 1ms in the latter). For risk management, we shouldn't touch this anymore (although I guess it can be put off).
Otherwise, there are no points which I can work on.
Reporter | ||
Comment 9•7 years ago
|
||
Another cold profile captured today on the reference hardware: http://bit.ly/2tVuJ5A
Comment 10•7 years ago
|
||
This is marked as [meta] but has no more open dependencies. Is there work left here? Masayuki's comment 8 indicates there's nothing in his realm.
Flags: needinfo?(florian)
Reporter | ||
Comment 11•7 years ago
|
||
Someone who understands this code should look at the profile in comment 9 to see if there are any additional possible wins. If there are none, then we should resolve this bug.
Flags: needinfo?(florian)
Comment 12•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> Someone who understands this code should look at the profile in comment 9 to
> see if there are any additional possible wins. If there are none, then we
> should resolve this bug.
Thanks, Florian. Jim, do you know who could do that?
Flags: needinfo?(jmathies)
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jmathies)
Resolution: --- → FIXED
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1][tpi:+] → [tpi:+]
You need to log in
before you can comment on or make changes to this bug.
Description
•