285157 - PR_GetFileInfo much slower on Windows than native system call

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Christian Hudon]

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

[Wan-Teh Chang]

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.)

Comment 2

[Christian Hudon]

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

[Wan-Teh Chang]

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().

Comment 4

[Christian Hudon]

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

[Wan-Teh Chang]

Shanmu, could you take a stab at this bug?  Thanks.

Comment 6

[Shanmu]

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

[Shanmu]

Attachment: 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

[Wan-Teh Chang]

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

[Wan-Teh Chang]

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.

Comment 10

[Shanmu]

Attachment: 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

[Wan-Teh Chang]

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).

Comment 12

[Shanmu]

Attachment: patch using cvs diff -u.

I generated the unified patch. Please review this.

Comment 13

[Jungshik Shin]

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

[Darin Fisher]

-> 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.

Comment 15

[Darin Fisher]

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

Comment 16

[Jungshik Shin]

(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

[Darin Fisher]

-> reassign to default owner

Comment 18

[Jim Mathies [:jimm]]

Mike S. asked me to pick this one up, so I'm snagging it.

Comment 19

[Mike Schroepfer]

Moving to blocking list - if comment 16 means this doesn't have much of an impact that we should remove it...

Comment 20

[Jim Mathies [:jimm]]

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

[Wan-Teh Chang]

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.

Comment 22

[Jim Mathies [:jimm]]

> 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.  

Comment 23

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 24

[Jim Mathies [:jimm]]

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

[Wan-Teh Chang]

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.

Comment 26

[Jim Mathies [:jimm]]

Cool, thanks. I'll take a look and roll another patch.

Comment 27

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 28

[Mike Schroepfer]

(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?

Comment 29

[Jim Mathies [:jimm]]

> 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

[Mike Schroepfer]

(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..

Comment 31

[Jim Mathies [:jimm]]

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

Done! Bug 411579. 

Comment 32

[Wan-Teh Chang]

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?

Comment 33

[Jim Mathies [:jimm]]

>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.

Comment 34

[Jim Mathies [:jimm]]

Attachment: getfileinfo patch v.3

Ok, here's a new rev with some clean up and better commenting.

Comment 35

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 36

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 37

[Wan-Teh Chang]

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.

Comment 38

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 39

[Wan-Teh Chang]

Attachment: 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

Comment 40

[Jim Mathies [:jimm]]

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

I can't test the older platforms but xp/vista are fine.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

In retrospect, this patch introduced bug 504527, which is deemed a 
vulnerability.

Comment 43

[Jim Mathies [:jimm]]

(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? ([e-mail])