1360167 - WindowsUIUtils::GetInTabletMode blocks the UI thread during startup

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Florian Quèze [:florian]]

See this profile captured on a quantum reference device: https://perfht.ml/2pp9yJ9
31ms are spent during startup in WindowsUIUtils::GetInTabletMode (called from http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/browser/base/content/browser.js#5420)

The time seems to be spent by the Windows API doing I/O to load a dll.

Comment 1

[Thomas Nguyen (:tnguyen)]

Hi Jim,
I wonder if we can use RoGetActivationFactory
https://msdn.microsoft.com/en-us/library/windows/desktop/br224648(v=vs.85).aspx
then we could load combase.dll in background thread eariler.
Not quite sure about GetActivationFactory, but seems it's a wrapper of RoGetActivationFactory

Comment 2

[Jim Mathies [:jimm]]

Downgrading this since I don't have an assignee currently. We'll look at the suggestion in comment 1 once we have an owner.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

I intend to take a look into this, but this is a pretty new area for me. Could you suggest someone who could help me get started?

Comment 4

[Robin Steuber (they/them) [:bytesized]]

So it looks like this call happens pretty early in startup [1][2] and causes pretty significant changes to the chrome UI. This is making it difficult for me to figure out how to load this in the background. However, maybe we could fail the initial check in onLoad and fire the observer later from a background thread.

If not, we are likely stuck with the early check since we can't assure that we'll have the data when the main browser window going through layout for the first time.

@dao What do you think? Would failing initially and firing later be acceptable?

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#5464
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/browser.js#1345

Comment 5

[Florian Quèze [:florian]]

(In reply to Kirk Steuber [:bytesized] from comment #4)
> So it looks like this call happens pretty early in startup [1][2]

browser.js isn't really early in startup, all the add-on manager initialization happens before that.

Would it be possible to start getting this value in a background thread early in startup, and make the main thread call blocking only if we haven't got the value yet from the background thread?

> @dao What do you think? Would failing initially and firing later be
> acceptable?

I'll let Dao give you a final answer on this, but I think it would cause flickering.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to Kirk Steuber [:bytesized] from comment #4)
> > So it looks like this call happens pretty early in startup [1][2]
> 
> browser.js isn't really early in startup, all the add-on manager
> initialization happens before that.
> 
> Would it be possible to start getting this value in a background thread
> early in startup, and make the main thread call blocking only if we haven't
> got the value yet from the background thread?

This is what I would suggest too.

> > @dao What do you think? Would failing initially and firing later be
> > acceptable?
> 
> I'll let Dao give you a final answer on this, but I think it would cause
> flickering.

Yep, sounds like it.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

How do I reproduce this? I cannot seem to profile startup since the profiler extension starts deactivated.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

You can set the MOZ_PROFILER_STARTUP env var to do that

Comment 9

[Robin Steuber (they/them) [:bytesized]]

Does this continue to reproduce the same way on the reference device? I have tried about a half dozen times and WindowsUIUtils::GetInTabletMode only showed for 1 sample, and only about half the times I tried (the other times WindowsUIUtils::GetInTabletMode didn't occur in any sample). And the sample that shows up in my profiles is usually for the function |RoGetActivationFactory|, where the 31 samples in the posted profile are all for some |Release| function that has never appeared in any of my profile samples.

Comment 10

[Florian Quèze [:florian]]

(In reply to Kirk Steuber [:bytesized] from comment #9)
> Does this continue to reproduce the same way on the reference device? I have
> tried about a half dozen times and WindowsUIUtils::GetInTabletMode only
> showed for 1 sample, and only about half the times I tried (the other times
> WindowsUIUtils::GetInTabletMode didn't occur in any sample). And the sample
> that shows up in my profiles is usually for the function
> |RoGetActivationFactory|, where the 31 samples in the posted profile are all
> for some |Release| function that has never appeared in any of my profile
> samples.

This is something that appears in cold startup profiles. You need to reboot the computer each time, and Firefox needs to be the first application loaded after you log in. Like anything that involves IO, it can't be reproduced consistently, and the time in ms is meaningless.

Comment 11

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1360167 - Move dll loading from WindowsUIUtils::GetInTabletMode off of the main thread

WindowsUIUtils::GetInTabletMode blocks the UI thread during startup while the dll needed to determine if Windows is using Tablet mode is loaded. This patch moves that loading process off of the main thread, with a synchronous fallback if the worker thread is too slow.

Review commit: https://reviewboard.mozilla.org/r/161110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161110/

Comment 12

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8890053 [details]
Bug 1360167 - Move dll loading from WindowsUIUtils::GetInTabletMode off of the main thread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161110/diff/1-2/

Comment 13

[Robin Steuber (they/them) [:bytesized]]

jimm, I hope that you are a good person to review this. I am a bit unsure of my usage of LazyIdleThread and am hoping that you can take a look at it and make sure I am using it correctly. I am particularly concerned about my (lack of) cleanup of that thread. I considered setting |mIdleThread = nullptr| in |SetTabletModeState()| to trigger cleanup, but I am not sure exactly what happens if I do that and the worker thread was still working.

I also felt like there should really be a |mIdleThread->Cancel()| method to call from |SetTabletModeState()|, but I am not sure how to cancel it other than just telling the other thread not to do its work. Since this would require a mutex, I am not sure whether it is worth the overhead or not.

Let me know what you think.

Comment 14

[Robin Steuber (they/them) [:bytesized]]

I should also note that I tested this on the reference machine that I borrowed. I was able to reproduce the original problem (although only for 32-bit for some reason). With this patch applied, |WindowsUIUtils::UpdateTabletModeState| no longer seems to show up in any samples in the profiler.

When I run it on my own machine, however, the Main thread consistently beats the idle thread causing the idle thread to determine the value but then discard it when it sees that the Main thread already has a value.

Side note: I was told when borrowing the reference machine that I could sort of simulate the "cold startup" conditions described by Florian in Comment 10 by using a program called RamMap, which has a command called "Empty Standby List". Apparently this command clears the cached dlls, forcing them to be reloaded when they are used next. I believe that it works because I was only able to reproduce this problem after using this tool.

Comment 15

[Robin Steuber (they/them) [:bytesized]]

I think there may actually be a better way to do this. I am removing the review request until I finish investigating.

Comment 16

[Robin Steuber (they/them) [:bytesized]]

I have ended up getting a wide variety of suggestions as to how this patch ought to be written. To me, this seems like the cleanest solution available:

 - Kick off thread using |NS_NewRunnableFunction| and |NS_NewThread|.
 - Do work on the worker thread, then dispatch an event (derived from nsIRunnable) back to the main thread with the result
 - Back on the main thread, record the result and clean up references to the worker thread

Meanwhile, the main thread will fallback on synchronous behavior (its current behavior) if the result is not ready in time.

This leaves (I think) just one question that I do not know the answer to: In the event that the main thread falls back on synchronous behavior, what do we do about the worker thread? It seems wrong to just leave it running when the work it set out to do has already been done. Or maybe it is ok to let it do that? I am just not sure.


Although there are clearly other ways to accomplish my goal, I would prefer not to make any significant departures from my stated plan unless there is a good reason to do so (performance/code reuse). Otherwise, I am afraid that I will continue to get hung up on what approach to take and never make it to implementation.

Comment 17

[Jim Mathies [:jimm]]

Why don't we roll this in with the work in bug 1367416 (or block on that and try to reuse what we implement there).

Comment 18

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

This is fixed by bug 1367416 including read ahead for twinapi.dll (and twinapi.appcore.dll being included there likely also contributes).