Closed Bug 1388134 Opened 3 years ago Closed 2 years ago

mozilla::OSFileConstantsService::InitOSFileConstants does startup main thread IO

Categories

(Toolkit :: OS.File, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: florian, Assigned: dthayer)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p1:f64][fxperf:p2])

Attachments

(5 files)

See this profile: https://perfht.ml/2vIfAsp (132ms during early startup on the ref hardware)

I wonder if we could get these paths lazily instead doing it immediately during early startup.
Whiteboard: [qf] → [qf:p2]
(In reply to Daniel Holbert [:dholbert] from comment #1)
> It looks like this is all from this NS_GetSpecialDirectory call (which is
> probably file I/O, in NTCreateFile):
> 
> >  // Initialize paths->libDir
> >  nsCOMPtr<nsIFile> file;
> >  nsresult rv = NS_GetSpecialDirectory(NS_XPCOM_LIBRARY_FILE, getter_AddRefs(file));
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.
> cpp#268
> 
> This line was added in bug 958354. Adding a loose dependency on that bug.

It's not that specific call, no. The profile shows that NS_GetSpecialDirectory is called indirectly from mozilla::GetPathToSpecialDir.
Oh, thank you.  Looks like we call GetPathToSpecialDir a bunch of times (for different directories), which might be part of why the profiler is hitting it so much.  So really this is from some/all of these calls:
https://dxr.mozilla.org/mozilla-central/rev/47248637eafa9a38dade8dc3aa6c4736177c8d8d/dom/system/OSFileConstants.cpp#287-323

I agree that this seems ripe for a lazy getter function.  We should perhaps make gPaths into a StaticAutoPtr, and it should only be accessed via a lazy getter perhaps...?

needinfo=yoric who has hg blame for some changes around this code (including some handling for cases where gPaths is unexpectedly null, in DelayedPathSetter::Observe and in DefineOSFileConstants -- if those cases actually happen, they might need some special treatment for the lazy-getter-ification).
No longer depends on: 958354
Flags: needinfo?(dteller)
The main difficulty here is that OSFileConstants can theoretically be looked up from any thread. On the other hand, now that we are removing extensions, it should be easy to find out whether we actually use these constants from worker threads. My guess is that we only ever use one or two and that we can probably fastpath these and make the other ones main thread only.
Flags: needinfo?(dteller)
Keywords: perf
Whiteboard: [qf:p2] → [qf:i60][qf:p1]
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
More recent profile captured today on yesterday's nightly: https://perfht.ml/2umlq31 The mozilla::InitOSFileConstants function is now called mozilla::OSFileConstantsService::InitOSFileConstants but the problem otherwise remains the same.
Summary: mozilla::InitOSFileConstants does startup main thread IO → mozilla::OSFileConstantsService::InitOSFileConstants does startup main thread IO
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1][fxperf]
Whiteboard: [qf:f64][qf:p1][fxperf] → [qf:f64][qf:p1][fxperf:p2]
Whiteboard: [qf:f64][qf:p1][fxperf:p2] → [qf:p1:f64][fxperf:p2]
Hey dthayer, do you have any cycles to look at this?
Flags: needinfo?(dothayer)
Yeah I can take a look at it.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(dothayer)
It looks like we can push out tmpDir, homeDir, and userApplicationDir without much trouble. libDir and localProfileDir almost certainly not. profileDir might be able to be pushed out, but not without pushing out what appears to be a very long tail of things that trivially use it early.

I think I'm going to clean up a patch that just delays things for tmpDir, homeDir, and userApplicationDir, and we can look at profileDir in a followup.
These calls to GetPathToSpecialDir are performing some unnecessary IO
during early startup which we'd like to defer. Simply adding the
required ones to the list in osfile_async_front.jsm should mostly get
us there.
In bug 1388134 we're lazifying some members of OS.Constants.Path
to avoid the extra startup IO. userApplicationDataDir is ripe for
being made lazy, except it's read early in CrashManager.jsm. This
defers that until it's used, and adjusts the affected tests.

Depends on D6079
Delaying the loading of some OS.Constants.Path members to reduce startup
IO is breaking the test_system_delay_update.js test, because it leaves
tmpaddon-* files in the user's temp directory. As far as I can tell this
is okay (please correct me if wrong) - but the error in AddonTestUtils
was being avoided because the OS.Constants.Path.tmpDir value was being
read before we override TmpD for the test. So now we are leaving them
to be ignored in the TmpD directory we specified, rather than leaving
them to be ignored in the user's temp directory.

Depends on D6080
Comment on attachment 9009747 [details]
Bug 1388134 - Defer load of OS.Constants.Path members r=gsvelto

Gabriele Svelto [:gsvelto] has approved the revision.
Attachment #9009747 - Flags: review+
Comment on attachment 9009746 [details]
Bug 1388134 - Move some OS.Constants.Path members to lazy init r=baku

Andrea Marchesini [:baku] has approved the revision.
Attachment #9009746 - Flags: review+
Blocks: 1495021
Comment on attachment 9009748 [details]
Bug 1388134 - Fix lazy OS.Constants.Path breakage in addons test r=kmag

Andrew Swan [:aswan] has approved the revision.
Attachment #9009748 - Flags: review+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/238ec26bef30
Move some OS.Constants.Path members to lazy init r=baku
https://hg.mozilla.org/integration/autoland/rev/aeb38f1ace8d
Defer load of OS.Constants.Path members r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/365ac2b9486f
Fix lazy OS.Constants.Path breakage in addons test r=aswan
Depends on: 1495889
Because nsDirectoryService::Get is not thread safe (it caches with a
hash table), this patch allows specifying an already retrieved temp
dir to the anonymous temporary file getters. These were failing
because we pushed back the early calls to retrieve NS_OS_TEMP_DIR,
so the FileBlockCache was doing the load. This caused crashes on
Android since it provides a js-based implementation.

This is not an exhaustive replacement of off-main-thread
nsDirectoryService access, or even off-main-thread
nsAnonymousTemporaryFile access, since there are bugs covering that
and this is simply trying to get our deferment of InitOSFileConstants
through.

Depends on D6081
Hey baku, do you think you'll get a chance to look at this today?
Flags: needinfo?(amarchesini)
done!
Flags: needinfo?(amarchesini)
Thanks baku!
Flags: needinfo?(dothayer)
I missed this in the first patch, but I noticed one crash in the try run
from this. Fairly straightforward like the previous patch in the series -
simply loading the tmp dir off of the main thread and passing it through
to to the parent running off the main thread.

Depends on D8570
Ugh. I'm sorry to do this to you but would you mind taking a look at this patch too, baku? Unfortunately I just noticed this last infrequent crash on Android coming from TemporaryIPCBlobParent.
Flags: needinfo?(amarchesini)
I have a different approach to share:

Currently NS_OpenAnonymousTemporaryFile() is sync and it can be used only on the parent process. On the content process we have AsyncOpenAnonymousTemporaryFile(). What about if we introduce a new function that unifies these 2 and we make it async? Something like:

nsresult
NS_AsyncOpenAnonymousTemporaryFile(const std::function<void(PRFileDesc*)>& aCallback);

This new method should do:

1. if executed on the parent process, call NS_OpenAnonymousTemporaryFile() on the main-thread, then exec the callback on the target thread.

2. if on the content process, just exec: dom::ContentChild::GetSingleton()->AsyncOpenAnonymousTemporaryFile()

If you implement this function, you can remove this block completely:

https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/dom/media/FileBlockCache.cpp#90-109

and replace it with NS_AsyncOpenAnonymousTemporaryFile(...)

Something similar can be done for TemporaryIPCBlob protocol.
At the same time you can deprecated NS_OpenAnonymousTemporaryFile and eventually, remove it.
Flags: needinfo?(amarchesini) → needinfo?(dothayer)
Ah, commented on the review before I read this. I'll look into that solution in the morning.
Flags: needinfo?(dothayer)
I think I'm just going to put this work into a followup and go with the original patches but with tmpDir excluded from the optimizations.
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd3bdf404bfb
Move some OS.Constants.Path members to lazy init r=baku
https://hg.mozilla.org/integration/autoland/rev/3f01db01f1d3
Defer load of OS.Constants.Path members r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/79444293ad73
Fix lazy OS.Constants.Path breakage in addons test r=aswan
Depends on: 1499799
https://hg.mozilla.org/mozilla-central/rev/dd3bdf404bfb
https://hg.mozilla.org/mozilla-central/rev/3f01db01f1d3
https://hg.mozilla.org/mozilla-central/rev/79444293ad73
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.