GMPDiskStorage.cpp and GMPServiceParent.cpp do unnecessary I/O syscalls to create directories during startup

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

({perf})

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 attachment)

Assignee

Description

3 months ago

The behavior of nsLocalFile::Create(a/b/c) is to attempt to create the final directory a/b/c first, and if that fails because the previous path components don't exist yet, to create them recursively and then create the final directory again.

The current GMP code, when it wants to use something in a directory a/b/c is to first call nsLocalFile->Create(a), then nsLocalFile->Create(a/b) and then nsLocalFile->Create(a/b/c).

This does useless main thread I/O system calls whenever the directories already exist.

Another bad pattern is to call nsLocalFile->Exists (which does main thread I/O) before doing something that will fail cleanly if the file or directory doesn't exist.
Eg.
if (file.exists()) {
file.read()
...
} else {
create file...
}
is inefficient. We should read the file without checking if it exists, and if that fails check the error code to see if the failure is because the file didn't exist.

The 2 cases that I really care about are
<user profile>\gmp
<user profile>\gmp\WINNT_x86-msvc
which are done unconditionally on the main thread during startup before first paint. The first of these 2 calls is obviously unnecessary. The second one is unnecessary too because when we do use that gmp/$platform folder, it's always to create a subfolder, or to remove the folder (I inspected all the callers I could find).

The patch I'm going to attach cleans up bad patterns in a few other cases where it was clear from code inspection. I refrained from doing any change that requires a real underestanding of the code, but I saw a few other places with Exists calls that didn't seem required (I don't know the code enough to know if they happen on the main thread or off main thread; if it's off main thread it doesn't matter much).

Assignee

Comment 2

3 months ago

Note: I'm fairly confident that these changes are correct, but I don't know how to exercise this code, so it would be good if you could either test it yourself locally when reviewing, or suggest which tests to run on try, or both; thanks!

Updated

3 months ago
Priority: -- → P2
Assignee

Comment 3

3 months ago

Adding a needinfo in case you missed the phabricator review request.

Flags: needinfo?(cpearce)
Assignee

Updated

3 months ago
Blocks: 1522877

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

Adding a needinfo in case you missed the phabricator review request.

r+, I did in fact miss this review request. Sorry!

Flags: needinfo?(cpearce)

Comment 5

3 months ago
Pushed by fqueze@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/72c34a516596
avoid unnecessary gmp directory creations during startup, r=cpearce.

Comment 6

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.