1529596 - (Windows) nsLocalFile::Create shouldn't do I/O for each path component

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Florian Quèze [:florian]]

Whenever nsLocalFile::Create is called to ensure a directory exists, our current implementation calls the CreateDirectoryW Windows API (which does main thread I/O) in a loop for each path component: https://searchfox.org/mozilla-central/rev/8f4f3e6fea43f5920f05907e5e477bcd04d1134e/xpcom/io/nsLocalFileWin.cpp#1280,1284

This is inefficient, and quite visible in startup profiles with mainthreadio markers, eg. https://perfht.ml/2DZCEVj

There are 2 things we could improve in my opinion:

• nsLocalFile::Create should attempt to create the final directory first, and fallback to creating the parent directories only if creating the final directory failed with a 'not found' error.
• we should avoid calling nsLocalFile::Create so often on startup. In most cases the directories will only need to be created for fresh profiles, so it should be fine to attempt to write inside the directories and to attempt to create the directories only if attempting to use them directly caused an error. I'll file a separate bug for this.

Comment 1

[Nathan Froyd [:froydnj]]

See also bug 1471668, which has similar findings.

Comment 2

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #0)

• nsLocalFile::Create should attempt to create the final directory first, and fallback to creating the parent directories only if creating the final directory failed with a 'not found' error.

This seems fairly straightforward to add/fix, so let's make this p1.

Comment 3

[Nathan Froyd [:froydnj]]

I have patches to fix this, just need to get Windows tests ironed out.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: Bug 1529596 - part 1 - pull out common NS_ERROR_FILE_ACCESS_DENIED code; r=aklotz

This condition logically belongs before we try creating anything.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: Bug 1529596 - part 2 - pull out code for creating files/directories; r=aklotz

We eventually want to make the common path just attempt file creation,
and only fall back to creating all the ancestor directories if the
initial attempt failed. To do that in a reasonable way, we'll need
re-usable code for the creation code, which is what this patch creates.

Depends on D22359

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: Bug 1529596 - part 3 - avoid directory creation in the common case; r=aklotz

This change sets up nsLocalFileWin to mirror the behavior of
nsLocalFileUnix, which is all-around more reasonable than the behavior
nsLocalFileWin had before. We also, in passing, fix up some unnecessary
error-handling code at the end of Create().

Depends on D22360

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db0f22709ad1
part 1 - pull out common NS_ERROR_FILE_ACCESS_DENIED code; r=aklotz
https://hg.mozilla.org/integration/autoland/rev/b55a2c1df7bb
part 2 - pull out code for creating files/directories; r=aklotz
https://hg.mozilla.org/integration/autoland/rev/de92c0248a0b
part 3 - avoid directory creation in the common case; r=aklotz

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/db0f22709ad1
https://hg.mozilla.org/mozilla-central/rev/b55a2c1df7bb
https://hg.mozilla.org/mozilla-central/rev/de92c0248a0b