Defer mozilla::intl::GetSystemLocales() until later in startup
Categories
(Toolkit :: Telemetry, enhancement, P3)
Tracking
()
Performance Impact | medium |
People
(Reporter: emmamalysz, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: perf:startup)
Attachments
(1 file)
4.53 MB,
application/x-7z-compressed
|
Details |
In the attached profile, mozilla::intl::GetSystemLocales()
loads several system DLLs on startup, including Windows.Globalization.dll. Zibi, do you think this is feasible to defer this work to a later point in startup?
Comment 1•4 years ago
|
||
I don't see any GetSystemLocale
or getSystemLocale
in the attached profile.
What's the stack of the call? Which process?
Reporter | ||
Comment 2•4 years ago
|
||
Here's the callstack:
mozilla::freestanding::patched_LdrLoadDll(wchar_t*, unsigned long*, _UNICODE_STRING*, void**) (firefox.exe)
LoadLibraryExW (kernelbase.dll)
LoadLibraryWithLogging(LoadOrFreeWhy,unsigned short const *,unsigned long,HINSTANCE__ * *) (combase.dll)
CClassCache::CDllPathEntry::LoadDll(DLL_INSTANTIATION_PROPERTIES &,long (*&)(_GUID const &,_GUID const &,void * *),long (*&)(HSTRING__ *,IActivationFactory * *),long (*&)(void),HINSTANCE__ * &) (combase.dll)
CClassCache::CDllPathEntry::Create(DLL_INSTANTIATION_PROPERTIES &,bool,CClassCache::CDllPathEntry * &) (combase.dll)
CClassCache::GetOrLoadWinRTInprocClass(IWinRTRuntimeClassInfo *,IActivationFactory * *) (combase.dll)
RoGetActivationFactory (combase.dll)
mozilla::intl::OSPreferences::ReadSystemLocales(nsTArray<nsTString<char> >&) (xul.dll)
mozilla::intl::OSPreferences::GetSystemLocales(nsTArray<nsTString<char> >&) (xul.dll)
mozilla::intl::OSPreferences::GetSystemLocale(nsTSubstring<char>&) (xul.dll)
XPTC__InvokebyIndex (xul.dll)
static XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xul.dll)
XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*) (xul.dll)
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (xul.dll)
js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (xul.dll)
js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject *>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) (xul.dll)
Interpret(JSContext*, js::RunState&) (xul.dll)
js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (xul.dll)
js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (xul.dll)
js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject *>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) (xul.dll)
Interpret(JSContext*, js::RunState&) (xul.dll)
If you load the files into https://procmon-analyze.github.io/, you can search for Windows.Globalization.dll
you can see this happening ~33s in.
Comment 3•4 years ago
|
||
Ok, so it seems like the call comes from JS then? I'd love to understand what calls it in JS to reason about whether it has to be that early.
It should be some call to systemLocale
getter.
Updated•4 years ago
|
Comment 4•4 years ago
•
|
||
Here's a stack with JS frames (not sure if it's the only possible one, but it's the only one in a Gecko profile I just took):
mozilla::glue::LoaderObserver::OnBeginDllLoad C:\Windows\System32\Windows.Globalization.dll
getSystemLocale
_getOSData
_getSystem
EnvironmentCache
getGlobal
get currentEnvironment
js::RunScript
annotateEnvironment
TelemetryStartup.prototype.observe
js::RunScript
XPCWrappedJS method call
Category observer notification - profile-after-change (TelemetryStartup)
XREMain::XRE_main
(root)
I suspect we could defer initializing the EnvironmentCache
until after startup is settled.
Comment 5•4 years ago
|
||
Alessio, am I right in thinking the async bits of EnvironmentCache()
can be deferred further until after startup is all settled?
Comment 6•4 years ago
|
||
Yeah, ok! Thanks Doug!
Seems like the question is for the Telemetry people how lazy we can be with that call.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 7•4 years ago
|
||
It depends on who depends on this information for crash reporting. We have a bug on file (bug 1516260) for moving the entirety of annotateEnvironment
until after profile-after-change
at least, but didn't get clarity from crash report stakeholders on whether that'd be problematic. If you can clarify this we can move it all later, not just locale stuff.
From the POV of the opinions of the team, in general we're open to moving these quite a bit later. Telemetry's gotten quite good at waiting for the pieces of the puzzle it needs before doing anything, so it shouldn't (though it'll need verification) have any ill effect on core data reporting mechanism.
However, this will likely increase shutdown timeout crashes as it'll increase the amount of work we'll need to do during the shutdown of shortish sessions.
Comment 8•4 years ago
|
||
I'm not 100% sure I understand this comment, so could you tell me if this is correct?
A) If we defer some telemetry initialization, and then the browser crashes for some reason before that telemetry initialization is completed, the crash reporter will not be able to submit that information if it has not been initialized.
B) If we defer some telemetry information, and the user initiates a browser shutdown before that is completed, telemetry will have to wait for it
to be completed before it can send pings. This will degrade shutdown performance and increase shutdown hang crashes.
I think my brain got confused because you mentioned crashes twice, but for different reasons. Is that correct?
Assuming it is, for A) it looks like from bug 1516260 that we've rung the bell as much as we can for crash reporter stakeholders and everyone who did respond was okay with it. For B), the question boils down to whether we would like to trade shutdown performance for startup performance? In which case, the answer is a clear yes. The ratio might not be one for one, but it should be in that ballpark given that a nontrivial amount of this work occurs on the main thread. For the work that occurs off main thread, we're still doing IO, which is a strong bottleneck for users on spinny disks. I'll ask the performance PM if he's okay with all this, assuming I've understood the problem correctly, but I don't imagine there should be much debate.
Does all of this sound correct, reasonable?
Comment 9•4 years ago
|
||
This is not a DOM Content Processes bug. Moving to Telemetry component.
Comment 10•4 years ago
|
||
(In reply to Doug Thayer [:dthayer] from comment #8)
I'm not 100% sure I understand this comment, so could you tell me if this is correct?
You've got it! Be sure to budget in some time for data validation -- we wouldn't want to find out that my assumptions about Telemetry's robustness to async Environment init don't always hold in the wild.
Updated•4 years ago
|
Updated•2 years ago
|
Description
•