Reduce calls to nsLocalFileWin's IsDirectory by not calling it when enumerating directories

RESOLVED FIXED in mozilla13

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla13
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #596530 - Flags: review?(neil)
(Assignee)

Comment 1

5 years ago
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

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
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>\*
(Assignee)

Comment 4

5 years ago
Created attachment 596663 [details] [diff] [review]
Patch v2.
Attachment #596530 - Attachment is obsolete: true
Attachment #596530 - Flags: review?(neil)
Attachment #596663 - Flags: review?(neil)
(Assignee)

Comment 5

5 years ago
Note: You can use FindFirstFileW on an actual file but not when you append \\* as we always do.

Comment 6

5 years ago
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!
Attachment #596663 - Flags: review?(neil) → review+
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/2df4fa51cfbb
https://hg.mozilla.org/mozilla-central/rev/2df4fa51cfbb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.