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

RESOLVED FIXED in Firefox 64

Status

()

RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: florian, Assigned: dthayer)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {perf})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [qf:p1:f64][fxperf:p2])

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
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]
Comment hidden (obsolete)
(Reporter)

Comment 2

2 years ago
(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)

Updated

a year ago
Whiteboard: [qf:p2] → [qf:i60][qf:p1]

Updated

a year ago
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]

Updated

a year ago
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
(Reporter)

Comment 5

a year ago
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.
(Reporter)

Updated

a year ago
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]
(Assignee)

Updated

10 months ago
Whiteboard: [qf:f64][qf:p1][fxperf] → [qf:f64][qf:p1][fxperf:p2]

Updated

10 months ago
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)
(Assignee)

Comment 7

7 months ago
Yeah I can take a look at it.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(dothayer)
(Assignee)

Comment 8

6 months ago
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.
(Assignee)

Comment 9

6 months ago
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.
(Assignee)

Comment 10

6 months ago
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
(Assignee)

Comment 11

6 months ago
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+
(Assignee)

Updated

6 months ago
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+

Comment 15

6 months ago
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
(Assignee)

Updated

6 months ago
Depends on: 1495889
(Assignee)

Comment 17

5 months ago
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)
(Assignee)

Comment 20

5 months ago
Thanks baku!
Flags: needinfo?(dothayer)
(Assignee)

Comment 21

5 months ago
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
(Assignee)

Comment 22

5 months ago
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)
(Assignee)

Comment 24

5 months ago
Ah, commented on the review before I read this. I'll look into that solution in the morning.
Flags: needinfo?(dothayer)
(Assignee)

Comment 25

5 months ago
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

5 months ago
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
(Assignee)

Updated

5 months ago
Depends on: 1499799

Comment 27

5 months ago
bugherder
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
Last Resolved: 5 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.