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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Florian Quèze [:florian]]

(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.

Comment 3

[Daniel Holbert [:dholbert]]

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).

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 5

[Florian Quèze [:florian]]

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.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Hey dthayer, do you have any cycles to look at this?

Comment 7

[Alex Thayer [:alexical] (she/her)]

Yeah I can take a look at it.

Comment 8

[Alex Thayer [:alexical] (she/her)]

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.

Comment 9

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1388134 - Move some OS.Constants.Path members to lazy init r=baku

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.

Comment 10

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1388134 - Defer load of OS.Constants.Path members r=gsvelto

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

Comment 11

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1388134 - Fix lazy OS.Constants.Path breakage in addons test r=kmag

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 12

[Gabriele Svelto [:gsvelto] (PTO)]

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

Gabriele Svelto [:gsvelto] has approved the revision.

Comment 13

[Andrea Marchesini [:baku]]

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.

Comment 14

[Andrew Swan [:aswan]]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Narcis Beleuzu [:NarcisB]]

Backed out for crashes on [@ nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*)]

Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception,runnable&classifiedState=unclassified&selectedJob=202670266&revision=365ac2b9486fdff60e9362012a6bbb13ce80a5c5

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception,runnable&classifiedState=unclassified&revision=017a4a49c716dd20134ad8ee2009bb988296585f

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=202670266&repo=autoland&lineNumber=1429

Comment 17

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1388134 - Allow getting tmpDir on main thread for anonymous tmp files r=baku

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

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Hey baku, do you think you'll get a chance to look at this today?

Comment 19

[Andrea Marchesini [:baku]]

done!

Comment 20

[Alex Thayer [:alexical] (she/her)]

Thanks baku!

Comment 21

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1388134 - Provide tmp dir from main thread for TemporaryIPCBlobParent r=baku

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

Comment 22

[Alex Thayer [:alexical] (she/her)]

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.

Comment 23

[Andrea Marchesini [:baku]]

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.

Comment 24

[Alex Thayer [:alexical] (she/her)]

Ah, commented on the review before I read this. I'll look into that solution in the morning.

Comment 25

[Alex Thayer [:alexical] (she/her)]

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dd3bdf404bfb
https://hg.mozilla.org/mozilla-central/rev/3f01db01f1d3
https://hg.mozilla.org/mozilla-central/rev/79444293ad73