PR_GetFileInfo much slower on Windows than native system call

RESOLVED FIXED in 4.7

Status

NSPR
NSPR
P2
normal
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Christian Hudon, Assigned: jimm)

Tracking

(Blocks: 1 bug, {perf})

other
x86
Windows NT
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050210 Firefox/1.0 (Debian package 1.0+dfsg.1-6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050210 Firefox/1.0 (Debian package 1.0+dfsg.1-6)

Using NSPR to replace a series of custom #ifdef WINDOWS, etc. for code that
needs to be portable, we get a big slowdown in our functions that return the
modification time, etc. on Windows. The reason is that the Windows version of
the function PR_GetFileInfo{,64} does a wildcard expansion and then returns the
stat for the first filename. The Unix version doesn't do this.

Suggested fixes:

- Remove the wildcard code for Windows, to make the function consistent with its
namesake on other platforms
- Provide an optional way of telling the function not to do wildcard expansion
- Scan the string for wildcard characters on Windows and do the FindFirstFile,
FindClose thing only if there are wildcards in the filename.


Reproducible: Always

Steps to Reproduce:

Comment 1

13 years ago
I suspect that you misunderstood our code.
PR_GetFileInfo64 doesn't do a wildcard
expansion.  In fact, it fails if the pathname
contains the wildcard character "*".  See
http://lxr.mozilla.org/nspr/source/nsprpub/pr/src/md/windows/w95io.c#774.

The reason it calls FindFirstFile is that
FindFirstFile returns the information needed
by PR_GetFileInfo64.  If there is a better
function to call, please let me know.

(I remember _stat calls FindFirstFile, too.)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

13 years ago
Hmm. For Windows 98 and up, GetFileAttributesEx seems to provide everything that
is needed to fill out the PRFileInfo structure:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/base/win32_file_attribute_data_str.asp

... so at the very least this function should be ok for ntio.c

If you want to support Windows 95 too, the following each seem give a subset of
the information contained in the PRFileInfo structure:
GetFileInformationByHandle (together with CreateFile and the
FILE_FLAG_BACKUP_SEMANTICS flag), GetFileAttributes, GetFileSize{,Ex}, GetFileTime

As a hack, we replaced calls to PR_GetFileInfo with calls to PR_Open followed by
PR_GetOpenFileInfo and we get a speedup of about 10 in the code that scans
files' mtimes. The only inconvenient is that this doesn't work on directories
(on Windows).

Comment 3

13 years ago
Christian, do you have time to put together
a patch using GetFileAttributesEx?  Seems like
we can look up the address of GetFileAttributesEx.
If it doesn't exist, then fall back on the current
FindFirstFile code.

I seem to remember PR_GetFileInfo was originally
implemented using stat() on Windows.  It had some
problem, and I found that MSVC C runtime library
uses FindFirstFile to implement stat().  That's
how I got the idea of using FindFirstFile().
(Reporter)

Comment 4

13 years ago
Actually, while browsing some more in MSDN, I found something that does exactly
this in the Windows SDK. Assuming you have a Windows SDK that isn't from the
last millenium (aka stock VC++ 6.0 without any updates), you can do the following:

#define WANT_GETFILEATTRIBUTESEX_WRAPPER
#include <NewAPIs.h>

and then you can use GetFileAttributesEx, and if it isn't supported by the
version of Windows you're running on (Win95, NT4), it'll emulate it using
FindFirstFile for you.

How does that sound?

Comment 5

13 years ago
Shanmu, could you take a stab at this bug?  Thanks.
Assignee: wtchang → shanmus
Status: ASSIGNED → NEW

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 6

12 years ago
Wan-Teh, I was thinking that the w95io.c will be used for Windows 95 (which
needs the wrapper, and hence the need for NewAPIs.h) while the rest of the OSs
use ntio.c. If this is true I can include the NewAPIs.h in w95io.c and replace
the FindFirstFile with GetFileAttributesEx on ntio.c. 

Comment 7

12 years ago
Created attachment 193357 [details] [diff] [review]
Patch to replace FindFirstFile with GetFileAttributesEx.

This patch looks for the address of the GetFileAttributesEx at runtime. 
If successful, then GetFileAttributesEx is used instead of FindFirstFile. 
If the OS doesn't support this then FindFirstFile is used.

Comment 8

12 years ago
Shanmu: ntio.c is used for the "WINNT" configuration
of NSPR, and w95io.c is used for the "WIN95" configuration
of NSPR.

The "WINNT" configuration only works on the NT-derived
Windows platforms: NT 4.0, 2000, XP, and Server 2003.

The "WIN95" configuration works on all Windows platforms.
It should have been named "Generic Win32".

I am not that familiar with the relationship of Platform
SDK to the various versions of MSVC.  So let's avoid
introducing a dependency on Platform SDK.

Comment 9

12 years ago
Comment on attachment 193357 [details] [diff] [review]
Patch to replace FindFirstFile with GetFileAttributesEx.

Thanks for the patch, Shanmu.

This patch has a serious flaw.	It needs to call GetFileAttributesEx
using the function's address, which is returned by the GetProcAddress
call.  You are calling GetFileAttributesEx directly.  When the code
runs on older Windows platforms, nspr4.dll will generate a symbol
not found error.

You may call FindFirstFile directly because that symbol is present
in all Windows platforms.

A second problem is that you should do this checking of GetFileAttributsEx
in NSPR initialization so that it is only done once.
Attachment #193357 - Flags: review-

Comment 10

12 years ago
Created attachment 197599 [details] [diff] [review]
Proposing a new patch.

The check for the existence of the routine GetFileAttributesEx is done in NSPR
initialization (in the routine _PR_NT_INIT_IO function). If the OS supports the
GetFileAttributesEx call, the address of this call is stored in the variable
getFileAttributesEx, which is used in the _PR_MD_GETFILEINFO64 call to invoke
GetFileattributesEx. 
This has been done for both w95io.c and ntio.c file.

Wan-Teh, let me know how this fix looks.

Comment 11

12 years ago
Comment on attachment 197599 [details] [diff] [review]
Proposing a new patch.

Shanmu, please regenerate this patch in the unified
context diff format (cvs diff -u).

Updated

12 years ago
Blocks: 307815

Comment 12

12 years ago
Created attachment 197690 [details] [diff] [review]
patch using cvs diff -u.

I generated the unified patch. Please review this.
Attachment #193357 - Attachment is obsolete: true
Attachment #197599 - Attachment is obsolete: true

Comment 13

12 years ago
Comment on attachment 197690 [details] [diff] [review]
patch using cvs diff -u.


>-    hFindFile = FindFirstFile(fn, &findFileData);
>+    if (getFileAttributesEx)
>+        hFindFile = getFileAttributesEx(fn, GetFileExInfoStandard, &findFileData);
>+    else
>+        hFindFile = FindFirstFile(fn, &findFileData);
>     if (INVALID_HANDLE_VALUE == hFindFile) {

GetFileAttributesEx returns BOOL so that the above isn't quite right although it might work (by chance). Besides, |FindClose(hFindFile)| down the road should be enclosed with |if (!getFileAttributesEx)|

Comment 14

12 years ago
-> me for review and check-in

I think this is contributing to roughly 5% of startup time (if not more) on the first launch of Firefox after booting my machine.
Assignee: shanmus → darin
Status: ASSIGNED → NEW

Comment 15

12 years ago
To be more precise: that's how much time we spend inside FindFirstFile.

Updated

12 years ago
Status: NEW → ASSIGNED
QA Contact: wtchang → nspr

Comment 16

11 years ago
(In reply to comment #14)
 
> I think this is contributing to roughly 5% of startup time (if not more) on the
> first launch of Firefox after booting my machine.

Darin, is this still the case after bug 162361 fix was landed? I thought most of calls to PR_GetFileInfo  come from xpcom/io. When fixing bug 162361, I used  GetFileAttributesEx where it's available. 

Nonetheless, this issue has to be addressed for NSPR. 

Comment 17

11 years ago
-> reassign to default owner
Assignee: darin.moz → wtc
Status: ASSIGNED → NEW
Flags: blocking1.9?
Keywords: perf
(Assignee)

Comment 18

10 years ago
Mike S. asked me to pick this one up, so I'm snagging it.
Assignee: wtc → jmathies

Comment 19

10 years ago
Moving to blocking list - if comment 16 means this doesn't have much of an impact that we should remove it...
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 20

10 years ago
Just curious, does nspr have the same minimum platform requirements as Firefox? (Win2K supports GetFileAttributesEx) Can't we avoid the GetProcAddress work at this point? 

Comment 21

10 years ago
I am not sure if NSPR still needs to support Windows 9x.  It is less work to just do the
GetProcAddress than polling the NSPR customers for their opinions though.
(Assignee)

Comment 22

10 years ago
> Darin, is this still the case after bug 162361 fix was landed? I thought most
> of calls to PR_GetFileInfo come from xpcom/io. When fixing bug 162361, I used 
> GetFileAttributesEx where it's available.

This doesn't appear to be the case. I'll post an updated patch here in a sec but break points on these routines do not hit at all during firefox's boot or during normal operation. Some routines in here do, like file seek, write and read, the file info calls don't appear to be in much use at all.  
(Assignee)

Comment 23

10 years ago
Created attachment 295963 [details] [diff] [review]
getfileinfo patch v.1

A couple comments - I didn't see any need for the win95 additions since getfileattribex is'nt supported there, so I removed it. I also cleaned up the winnt code a bit, making the assumption if getfileattribex is present and fails, the file is not present so there's no need to fall back on findfirstfile.
Attachment #197690 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
Re comments 19, I think the blocking flag on this can be removed, but I'll hold off on that until folks have had a chance to comment.

Comment 25

10 years ago
Comment on attachment 295963 [details] [diff] [review]
getfileinfo patch v.1

Jim: NSPR has two separate implementations for Windows.  For historical reasons they're called
"WIN95" and "WINNT", and the "WIN95" implementation is the one used by the Mozilla clients.
So the changes to ntio.c won't be used by the Mozilla clients.  You need the changes to w95io.c
from the original patch.
(Assignee)

Comment 26

10 years ago
Cool, thanks. I'll take a look and roll another patch.
(Assignee)

Comment 27

10 years ago
Created attachment 296001 [details] [diff] [review]
getfileinfo patch v.2

Ok, here we go. Note I still don't see the need for a blocking flag on this, these routines are not heavily used. Lxr gives an idea of the coverage - 

http://lxr.mozilla.org/mozilla/ident?i=PR_GetFileInfo64
http://lxr.mozilla.org/mozilla/ident?i=PR_GetFileInfo

I did find a good unit test (testfile.c in nspr) that leveraged these routines, debugged that, stepped through the code and everything was working correctly.
Attachment #295963 - Attachment is obsolete: true
Attachment #296001 - Flags: review?(wtc)

Comment 28

10 years ago
(In reply to comment #22)
> > Darin, is this still the case after bug 162361 fix was landed? I thought most
> > of calls to PR_GetFileInfo come from xpcom/io. When fixing bug 162361, I used 
> > GetFileAttributesEx where it's available.
> 
> This doesn't appear to be the case. I'll post an updated patch here in a sec
> but break points on these routines do not hit at all during firefox's boot or
> during normal operation. Some routines in here do, like file seek, write and
> read, the file info calls don't appear to be in much use at all.  
> 

Is there anything else interesting in there we should look at?
(Assignee)

Comment 29

10 years ago
> Is there anything else interesting in there we should look at?

The most common call in use here during boot is the read file routine which is fine. We could tweak some of our file buffer sizes a bit, like MFL_CHECKSUM_BUFSIZE in nsFastLoadFile and PREF_READ_BUFFER_SIZE in nsPrefService, both of which make repeated calls with smallish buffer sizes. Other than the read file routine though we really don’t spend much time in here.

Comment 30

10 years ago
(In reply to comment #29)
> > Is there anything else interesting in there we should look at?
> 
> The most common call in use here during boot is the read file routine which is
> fine. We could tweak some of our file buffer sizes a bit, like
> MFL_CHECKSUM_BUFSIZE in nsFastLoadFile and PREF_READ_BUFFER_SIZE in
> nsPrefService, both of which make repeated calls with smallish buffer sizes.
> Other than the read file routine though we really don’t spend much time in
> here.
> 

Can you get bugs on file for that?  Sounds like an easy win..
(Assignee)

Comment 31

10 years ago
> Can you get bugs on file for that?  Sounds like an easy win..

Done! Bug 411579. 

Comment 32

10 years ago
Comment on attachment 296001 [details] [diff] [review]
getfileinfo patch v.2

Jim, thanks for the patch.  I did a quick review.  I have
some suggested changes and a question.

1. _PR_NT_CheckExistence should be renamed something like
_PR_NT_InitGetFileInfo to more accurately describe what it
does.

2. The new code you added should follow NSPR's indent offset
(four spaces).  Also, if-else is formatted like this:
    if (condition) {
        code;
    } else {
        code;
    }

3. In _win95.h, the new code (typedef and declarations) is not
added at the best place in the file.

4. The old code handles root directories and pathnames ending
in a slash (/) specially, as noted in the block comment at the
beginning of the old code.  Did you test the new code with
root directories and pathnames ending in a slash?
(Assignee)

Comment 33

10 years ago
>4. The old code handles root directories and pathnames ending
>in a slash (/) specially, as noted in the block comment at the
>beginning of the old code.  Did you test the new code with
>root directories and pathnames ending in a slash?

Yes it handles both.
(Assignee)

Comment 34

10 years ago
Created attachment 297200 [details] [diff] [review]
getfileinfo patch v.3

Ok, here's a new rev with some clean up and better commenting.
Attachment #296001 - Attachment is obsolete: true
Attachment #297200 - Flags: review?(wtc)
Attachment #296001 - Flags: review?(wtc)
(Assignee)

Comment 35

10 years ago
Created attachment 297202 [details] [diff] [review]
getfileinfo patch v.3

Sorry for the bug spam. I decided to take that last comment out in the init call, it's really not needed and wasn't very clear.
Attachment #297200 - Attachment is obsolete: true
Attachment #297202 - Flags: review?(wtc)
Attachment #297200 - Flags: review?(wtc)
(Assignee)

Comment 36

10 years ago
Created attachment 297205 [details] [diff] [review]
getfileinfo patch v.3

One more time, removed a tab in the first patch and fixed up the formatting in the init call as well.
Attachment #297202 - Attachment is obsolete: true
Attachment #297205 - Flags: review?(wtc)
Attachment #297202 - Flags: review?(wtc)

Comment 37

10 years ago
Comment on attachment 297205 [details] [diff] [review]
getfileinfo patch v.3

Jim, I can't apply this patch to my source tree.  I don't know
why.  Could you attach a whitespace-ignored version of this
patch?  I suspect that your changes to ntio.c and w95io.c are
much simpler than they look in this patch.  I think you indented
a large block of code inside the 'else' block, right?

I'd like to suggest some changes such as wrapping lines longer
than 80 characters and moving the code inside the 'else' block
into a new function.  Rather than going back and forth in
code reviews, I'd like to just make those changes myself and
ask you to test a new patch.  So the whitespace-ignored version
of this patch will be very helpful.  Thanks.
(Assignee)

Comment 38

10 years ago
Created attachment 298285 [details] [diff] [review]
getfileinfo patch v.4

Ok, here you go. I'd be happy to make the changes as well if you find you don't have the time.
Attachment #297205 - Attachment is obsolete: true
Attachment #297205 - Flags: review?(wtc)

Comment 39

10 years ago
Created attachment 299516 [details] [diff] [review]
getfileinfo patch v.5

Jim, please review and test this patch.

Nelson, you don't need to review this patch.  I only need your
confirmation that Sun's NSPR-based products don't need to support
Windows NT 4.0.  In this patch I use a function that only exists
on Windows 2000 or later in the "WINNT" build configuration.
(Mozilla clients use the "WIN95" build configuration.)

I've checked in this patch on the NSPR trunk for NSPR 4.7.

Checking in pr/src/md/windows/ntio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntio.c,v  <--  ntio.c
new revision: 3.45; previous revision: 3.44
done
Checking in pr/src/md/windows/w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.33; previous revision: 3.32
done
Attachment #298285 - Attachment is obsolete: true
Attachment #299516 - Flags: superreview?(nelson)
Attachment #299516 - Flags: review?(jmathies)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
(Assignee)

Comment 40

10 years ago
Comment on attachment 299516 [details] [diff] [review]
getfileinfo patch v.5

I can't test the older platforms but xp/vista are fine.
Attachment #299516 - Flags: review?(jmathies) → review+
Comment on attachment 299516 [details] [diff] [review]
getfileinfo patch v.5

I am giving this review request to Christophe.  
He can answer concerning Sun's build platform requirements better than I can.
Attachment #299516 - Flags: superreview?(nelson) → superreview?(christophe.ravel.bugs)

Updated

10 years ago
Attachment #299516 - Flags: superreview?(christophe.ravel.bugs) → superreview+
In retrospect, this patch introduced bug 504527, which is deemed a 
vulnerability.
(Assignee)

Comment 43

8 years ago
(In reply to comment #42)
> In retrospect, this patch introduced bug 504527, which is deemed a 
> vulnerability.

Can you cc me in on that Nelson? (jmathies@mozilla.com)
You need to log in before you can comment on or make changes to this bug.