Last Comment Bug 724207 - Save 15-20ms on startup from unused file attributes fetch when opening files
: Save 15-20ms on startup from unused file attributes fetch when opening files
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 724203
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 22:25 PST by Brian R. Bondy [:bbondy]
Modified: 2012-02-09 10:26 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1. (670 bytes, patch)
2012-02-03 22:25 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v2. (760 bytes, patch)
2012-02-07 06:44 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v2. (850 bytes, patch)
2012-02-07 06:45 PST, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Review

Description Brian R. Bondy [:bbondy] 2012-02-03 22:25:24 PST
Created attachment 594394 [details] [diff] [review]
Patch v1.

nsLocalFile::OpenNSPRFileDesc is called 311 times during startup, this takes 20-30ms of startup time.

We currently call ResolveaAndStat before opening the file, the purpose is just to get the resolved path though.
There are 311 extra unused GetFileAttributeEx calls because of this.
We should instead use the newer Resolve() method which takes almost no time to run.

These results were obtained by a small profiler I'm using which is based on QueryPerformanceCounter and QueryPerformanceFrequency.
Comment 1 neil@parkwaycc.co.uk 2012-02-07 02:29:25 PST
Comment on attachment 594394 [details] [diff] [review]
Patch v1.

>-    nsresult rv = ResolveAndStat();
>+    nsresult rv = Resolve();
Aha, well there's one of them ;-)

>     if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND)
I don't think Resolve() ever returns this, because it doesn't stat.

Alternatively, test mResolveDirty and if it's true then Resolve() failed.
Comment 2 Brian R. Bondy [:bbondy] 2012-02-07 04:26:33 PST
> I don't think Resolve() ever returns this, because it doesn't stat.
> Alternatively, test mResolveDirty and if it's true then Resolve() failed.

I think the check is only really needed to save time on failure conditions, but I think you're right that it should be adjusted. I'll go with testing mResolveDirty
Comment 3 Brian R. Bondy [:bbondy] 2012-02-07 06:44:20 PST
Created attachment 595018 [details] [diff] [review]
Patch v2.

bsmedberg, I'm just going to r? Neil on this since he's already looked at it.  Let me know if you need to sr it though.

> I don't think Resolve() ever returns this, because it doesn't stat.
> Alternatively, test mResolveDirty and if it's true then Resolve() failed.

On second thought, to keep the code path the same we should just remove the 
`&& rv != NS_ERROR_FILE_NOT_FOUND)`.
Comment 4 Brian R. Bondy [:bbondy] 2012-02-07 06:45:27 PST
Created attachment 595019 [details] [diff] [review]
Patch v2.
Comment 5 Brian R. Bondy [:bbondy] 2012-02-08 13:53:16 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4a4146c5683
Comment 6 Ed Morley [:emorley] 2012-02-09 10:26:54 PST
https://hg.mozilla.org/mozilla-central/rev/f4a4146c5683

\o/

Note You need to log in before you can comment on or make changes to this bug.