GMPDiskStorage.cpp and GMPServiceParent.cpp do unnecessary I/O syscalls to create directories during startup
Status
()
People
(Reporter: florian, Assigned: florian)
Tracking
({perf})
Firefox Tracking Flags
(firefox68 fixed)
Details
(Whiteboard: [fxperf:p1])
Attachments
(1 attachment)
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 1•3 months ago
|
||
|
|
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
|
|
|
Assignee | |
Comment 3•3 months ago
|
||
Adding a needinfo in case you missed the phabricator review request.
|
||
Comment 4•3 months ago
|
||
(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!
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 | ||
Description
•