Last Comment Bug 726481 - Reduce calls to nsLocalFileWin's IsDirectory by not calling it when enumerating directories
: Reduce calls to nsLocalFileWin's IsDirectory by not calling it when enumerati...
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:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-12 15:45 PST by Brian R. Bondy [:bbondy]
Modified: 2012-02-17 05:34 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1. (1.44 KB, patch)
2012-02-12 15:45 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v2. (2.76 KB, patch)
2012-02-13 06:08 PST, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Review

Description Brian R. Bondy [:bbondy] 2012-02-12 15:45:43 PST
Created attachment 596530 [details] [diff] [review]
Patch v1.

IsDirectory is called when enumerating directories unnecessarily.  
We can instead check it after the call to FindFirstFile which automatically returns the file attributes with it.
Comment 1 Brian R. Bondy [:bbondy] 2012-02-12 15:47:31 PST
Most of the time when this function is called it is called on a directory, so in that case we basically do 2 calls to fetch file attributes.
Comment 2 neil@parkwaycc.co.uk 2012-02-13 02:00:33 PST
(In reply to Brian R. Bondy from comment #0)
> IsDirectory is called when enumerating directories unnecessarily.  
> We can instead check it after the call to FindFirstFile which automatically
> returns the file attributes with it.
But that returns the attributes of the found file, not the directory that we're enumerating. If people subsequently call IsDirectory on the found file, then yes, there is potential for optimisation there, but it would be nontrivial.
Comment 3 Brian R. Bondy [:bbondy] 2012-02-13 04:57:46 PST
The first found file of a directory should be "." or the directory itself so this patch should work. 

Actually I think we can simplify this patch even more because OpenDir always adds \\* to the path so it would always return an error for a file. 

FindFirstFile returns ERROR_DIRECTORY if you pass <filepath>\* and it returns ERROR_PATH_NOT_FOUND when you have a <filepath_that_doesn't_exist>\*
Comment 4 Brian R. Bondy [:bbondy] 2012-02-13 06:08:09 PST
Created attachment 596663 [details] [diff] [review]
Patch v2.
Comment 5 Brian R. Bondy [:bbondy] 2012-02-13 06:09:24 PST
Note: You can use FindFirstFileW on an actual file but not when you append \\* as we always do.
Comment 6 neil@parkwaycc.co.uk 2012-02-14 14:26:47 PST
Comment on attachment 596663 [details] [diff] [review]
Patch v2.

Bah, I'm disappointed that bug 359808 didn't remove all the nsDir stuff from bug 162361 particularly given that they had the same author and reviewer!
Comment 7 Brian R. Bondy [:bbondy] 2012-02-16 05:51:02 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/2df4fa51cfbb
Comment 8 Ed Morley [:emorley] 2012-02-17 05:34:18 PST
https://hg.mozilla.org/mozilla-central/rev/2df4fa51cfbb

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