Closed Bug 1529596 Opened 9 months ago Closed 9 months ago

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

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: florian, Assigned: froydnj)

References

Details

(Whiteboard: [fxperf:p1])

Attachments

(3 files)

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.

See also bug 1471668, which has similar findings.

Priority: -- → P3

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

Blocks: 1522877
Whiteboard: [fxperf] → [fxperf:p1]

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

Assignee: nobody → nfroyd

This condition logically belongs before we try creating anything.

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

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

Pushed by nfroyd@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
See Also: → 1471668
You need to log in before you can comment on or make changes to this bug.