122523 - Avoid stat call when opening file.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

We should try to avoid the stat() call when we are opening a file. Instead gain
stat info via the file descriptor.

If we change this pattern:

    PR_GetFileInfo64(INPUT_FILE, &info);
    fd = PR_Open(INPUT_FILE, PR_RDWR | PR_CREATE_FILE, 00666);

to this:

    fd = PR_Open(INPUT_FILE, PR_RDWR | PR_CREATE_FILE, 00666);
    PR_GetOpenFileInfo64(fd, &info);

we should see a improvement of 50%.  How this plays out in the mozilla file
transport, I am not sure yet.

Comment 1

[Doug Turner (:dougt)]

Attachment: changes v.1

Comment 2

[Doug Turner (:dougt)]

Attachment: nspr stat test.

Comment 3

[Doug Turner (:dougt)]

Attachment: nspr stat test (now in plain text)

Comment 4

[Doug Turner (:dougt)]

Attachment: and now for something that works

Comment 5

[Doug Turner (:dougt)]

Attachment: how about this time... v.3

Comment 6

[Cathleen]

Here some test results on a 
win98, 200MHz 64MB machine

startup (warm)
------------------
original:  8850, 8790, 8490, 8490, 8920, 8810 --> avg 8725 ms
w/ patch:  8400, 8980, 8510, 8000, 8120, 8670 --> avg 8446 ms
                                             -----------------
                                                 diff  279 ms   ( -3.19%)


page-load
------------------
original:  2646, 2645, 2647 --> avg 2646 ms
w/ patch:  2641, 2641, 2624 --> avg 2635 ms
                            ------------------
                               diff   11 ms   ( -0.4 %)
                              


memory after page-load
-------------------------
original:  Data KB  20,124, 20,424
           Code KB   9,596   9,596 

w/ patch:  Data KB  18,252  18,444
           Code KB   9,580   9,600

(appear to have shaved off some Data or VM size)

   

                            

Comment 7

[Doug Turner (:dougt)]

Cathleen, thanks for this data. :-)

Does anyone understand why this patch cut ~2mb of data?  Please, do not tell me
that PR_GetFileInfo64 is that expensive! 

Comment 8

[Suresh Duddi (gone)]

It is baffling this patch can reduce 2MB of data. Maybe app didn't need to
sbrk() to get that extra 2MB because of the change in malloc pattern and things
just aligned !!!

Comment 9

[Doug Turner (:dougt)]

looking at unix and mac, I do not think that a similar optimization is possible
unless we invest alot more time - eg. switching over to nsprs' FileInfo(64) stat
structure. 
 
is there another way to avoid a stat when opening a file? 

Comment 10

[Doug Turner (:dougt)]

dp, can you review these changes?

Comment 11

[Suresh Duddi (gone)]

Comment on attachment 67046 [details] [diff] [review]
how about this time... v.3

 if (isDir) {
+	 PR_Close(mFD);


Are we sure mFD will be set here ?

r=dp otherwise

Comment 12

[Doug Turner (:dougt)]

It is protected above.  If mFD is null Open() will be called.  If open succeeds,
mFD will vaild.  Just to be a bit paranoid, i will add a assertion.

Comment 13

[Darin Fisher]

dougt: nsLocalFileUnix could use fstat in place of stat when it already has a
file descriptor.

Comment 14

[Doug Turner (:dougt)]

right, but files usually get opened with openNSPRFileDesc().  calling fstat() on
a nspr file descriptor probably won't work.

Comment 15

[Darin Fisher]

what about this:

  struct stat buf;
  fstat(PR_FileDesc2NativeHandle(nsprFD), &buf);

isn't this valid?

Comment 16

[Doug Turner (:dougt)]

now that is a routine I have not seen before!

Comment 17

[Darin Fisher]

it's in pprio.h and fwiw we already use it in nsFileStreams.cpp to support file
truncation (nsISeekableStream::SetEOF).

Comment 18

[Doug Turner (:dougt)]

Oh... that is why i didn't see it.  It is both private and obsolete. :-/

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 67046 [details] [diff] [review]
how about this time... v.3

>+    nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mFile, &rv);
>+    if (NS_FAILED(rv)) return rv;
>+    if (mIOFlags == -1)
>+        mIOFlags = PR_RDONLY;
>+    if (mPerm == -1)
>+        mPerm = 0;

What do these magic -1's mean/do? Is there a descriptive #define or const for
it?

>+    if (!mFD) {
>+        // NS_ASSERTION(mFD, "Your suppose to call Open()");

Take out the comment, or at least fix the spelling ("You're supposed...")

> nsFileInputStream::Init(nsIFile* file, PRInt32 ioFlags, PRInt32 perm, PRBool deleteOnClose)
>-{
>-    NS_ASSERTION(mFD == nsnull, "already inited");
>-    if (mFD != nsnull)
>-        return NS_ERROR_FAILURE;
>-    nsresult rv;
>+{   
>+    nsresult rv = NS_OK;
>     nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(file, &rv);
>     if (NS_FAILED(rv)) return rv;
>     if (ioFlags == -1)
>@@ -469,10 +489,22 @@
>     if (perm == -1)
>         perm = 0;
> 
>-    mLineBuffer = nsnull;
>-    
>-    rv = localFile->OpenNSPRFileDesc(ioFlags, perm, &mFD);
>+    PRFileDesc* fd;
>+    rv = localFile->OpenNSPRFileDesc(ioFlags, perm, &fd);
>     if (NS_FAILED(rv)) return rv;

Where did this fd come from? I assume you compiled this so it must be
there somewhere, but it's hard to tell with so little context.

sr=dveditz

Comment 20

[Doug Turner (:dougt)]

What do these magic -1's mean/do? Is there a descriptive #define or const for
it?

no defines or const.  They are well known values in the stream code.


>+    if (!mFD) {
>+        // NS_ASSERTION(mFD, "Your suppose to call Open()");

Take out the comment, or at least fix the spelling ("You're supposed...")

I did.  thanks.

>+    PRFileDesc* fd;  <--------------------------------------
>+    rv = localFile->OpenNSPRFileDesc(ioFlags, perm, &fd);
> if (NS_FAILED(rv)) return rv;

Where did this fd come from? I assume you compiled this so it must be
there somewhere, but it's hard to tell with so little context.
 

Comment 21

[Darin Fisher]

cc'ing wtc... can you comment on comment #15.  thx!

Comment 22

[Wan-Teh Chang]

Darin,

Why don't you replace the fstat() call by a PR_GetOpenFileInfo64()
or PR_GetOpenFileInfo() call?  PR_FileDesc2NativeHandle() is a
semi-private function that should only be used when there is no
NSPR alternative.

Comment 23

[Doug Turner (:dougt)]

Checking in nsFileStreams.cpp;
/cvsroot/mozilla/netwerk/base/src/nsFileStreams.cpp,v  <--  nsFileStreams.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in nsFileStreams.h;
/cvsroot/mozilla/netwerk/base/src/nsFileStreams.h,v  <--  nsFileStreams.h
new revision: 1.16; previous revision: 1.15

Checking in nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.78; previous revision: 1.77
done

We can file a seperate bug on converting the Unix version of nsLocalFile over to
using the nspr file info structure if you like.  I am not sure if the current
owners of this agree with doing this.  brendan, shaver, ?

Comment 24

[tenthumbs]

May I suggest that open+fstat is a big win on unix because it avoids a 
potential race condition. It's also slightly faster on Linux.

Comment 25

[Doug Turner (:dougt)]

I checked in the windows version nsLocalFile changes without any problems, but
the nsFileStream stuff just doesn't work on linux clobber.  Even after I fixed
the mLineBuffer crash, the build stays yellow.

I don;t think that this matters all that much anymore since the cache is going
to be moving to a blocked file format.  I am futuring this until I can find time
to look at the crash.

Cathleen, DP, If you want to figure out why |brad| went yellow, please reassign. :-)

Comment 26

[Doug Turner (:dougt)]

I moved this to future out of frustration... resetting milestone...  sorry about
that. 

Comment 27

[Doug Turner (:dougt)]

Attachment: fixes up mLineBuffer

Comment 28

[Doug Turner (:dougt)]

chris and I spun seperate clobber builds with the last patch and both were
successful.  I have sent mail to the build owner of the "Brad" tinerbox so that
we can work out a way where this orangeness can be tracked down.

Comment 29

[Doug Turner (:dougt)]

Attachment: fixes infinite loop problem.

Fixes a infinite loop cause by the mimeservice wanting to open a file.

Comment 30

[Bradley Baetz (:bbaetz)]

Comment on attachment 70370 [details] [diff] [review]
fixes infinite loop problem.

r=bbaetz

Comment 31

[Darin Fisher]

Comment on attachment 70370 [details] [diff] [review]
fixes infinite loop problem.

sr=darin

some nits:

1-

if(contentLength)

should be

if (contentLength)


2- 

nsCRT::strdup("literal-string")

can be replaced with:

strdup("literal-string")

there's no fear of invoking the wrong allocator here.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz for 0.9.9

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> 2- 
> nsCRT::strdup("literal-string")
> can be replaced with:
> strdup("literal-string")
> there's no fear of invoking the wrong allocator here.

But there should be fear of invoking the wrath of the Mac!

Comment 34

[Doug Turner (:dougt)]

yup... the mac will go red, but will return.  

Fixes checked in to 0.99

Comment 35

[Darin Fisher]

dbaron: LXR tells me that strdup is used all over the place.  am i missing
something?

Comment 36

[Doug Turner (:dougt)]

in any case, paranoia got me and I changed my strdup's to nsCRT::strdup's to
avoid bustage.  

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe it requires a different header?

Comment 38

[David Bradley]

Doug, I think you may have accidentally got part of patch from bug 122892
checked in when you checked in this one, that was previously backed out. This
caused a memory leak. You can see bug 124497 for more details. Just trying to
get this all tied together.

Comment 39

[benc]

dougt: are there any steps I can use to reproduce/verify this problem?

This is on my verification list.