1544019 - Consolidate IO for startupCache folder access

Status: NEW

(Core :: XPCOM, defect)

Description

[:Gijs (he/him)]

Windows IO markers for my profile show:

• 2 stat calls on the directory, both from the CopyMove call that entails an Exists (one for the parent process cache, one for the child process one)
• 7 (!) create/open calls on the directory:
  1. The first and fourth from GetCacheFile which calls create() itself.
  2. The second, third, fifth and sixth due to bug 1544009 and the MoveTo call in ::OpenCache;
  3. The 7th from our own Create call.

This is clearly suboptimal, and probably costs a few ms on spinny disk devices.

Comment 1

[:Gijs (he/him)]

Access is in different places so not sure how hard this would be, tentatively picking medium t-shirt size (1-2 weeks).

Comment 2

[:Gijs (he/him)]

bug 1544009 landed before the startup mainthread IO test. I think out of:

(In reply to :Gijs (he/him) from comment #0)

• 2 stat calls on the directory, both from the CopyMove call that entails an Exists (one for the parent process cache, one for the child process one)

These should be fixed because https://hg.mozilla.org/mozilla-central/rev/8ccd4b2a5fba#l1.136 moved the exists() check behind a check so they shouldn't happen for the MoveTo calls on Windows, and we never did this on *nix, as far as I can tell from reading the linux/unix ::moveto implementation.

• 7 (!) create/open calls on the directory:
  1. The first and fourth from GetCacheFile which calls create() itself.

This is still here.

2. The second, third, fifth and sixth due to bug 1544009 and the [MoveTo](https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/js/xpconnect/loader/ScriptPreloader.cpp#431-432) call in `::OpenCache`;

These should be fixed by that bug.

3. The 7th from our own [`Create`](https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/startupcache/StartupCache.cpp#173-174) call.

This is still here.

I checked the above with the profiler on a more recent build.

So this leaves 3 create calls:

• 2 from https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/js/xpconnect/loader/ScriptPreloader.cpp#387-388, which runs twice - once for the parent and once for the child cache (but both in the parent process)
• 1 from https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/startupcache/StartupCache.cpp#158-160

I'm minded to suggest having a static method on StartupCache() called GetAndMaybeCreateCacheFolder() that gets and creates the folder once, caching the xpcom ref and handing out clones, and call that from the script preloader. :froydnj, does that sound like an OK solution?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to :Gijs (he/him) from comment #2)

I'm minded to suggest having a static method on StartupCache() called GetAndMaybeCreateCacheFolder() that gets and creates the folder once, caching the xpcom ref and handing out clones, and call that from the script preloader. :froydnj, does that sound like an OK solution?

Yes. Eliminating the nasty duplication between the startup cache and the script preloader sounds like enough of a win here.

Comment 4

[Christina Cheung]

Attachment: Bug 1544019 - Added static method to get and create startup cache directory r=Gijs

Comment 5

[Christina Cheung]

Attachment: Bug 1544019 - Added static method to get and create startup cache, clang-formatted directory r=Gijs

Comment 6

[Christina Cheung]

Attachment: Bug 1544019 - Added static method to create startup cache folder, r=Gijs

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:nika, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Gijs, Is this something we still want to push forward with?

Comment 9

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #8)

Gijs, Is this something we still want to push forward with?

This got marked as a perf P2, so I think so? Florian, can you corroborate?

Comment 10

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #9)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #8)

Gijs, Is this something we still want to push forward with?

This got marked as a perf P2, so I think so? Florian, can you corroborate?

Yes, I see nothing that indicates this would have lost importance (but I haven't tried to reproduce recently either).