122892 - nsLocalFile::Clone should preserve stat info

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

A C++ copy construction of "this" is all that really is needed.  This will avoid
extra stat()'s when they are clearly not needed.

Comment 1

[Doug Turner (:dougt)]

Attachment: Fixes up windows and unix

Apparently the mac already does this.

Comment 2

[Doug Turner (:dougt)]

this should avoid a double stat in (at least) xpcom registration.

Comment 3

[Conrad Carlen (not reading bugmail)]

The compiler-generated copy constructor looks fine on the Windows impl -
everything should just be copied.

On Unix, you might save some cycles by assigning the rest of the fields and then
doing:
if (mHaveCachedStat)
    mCachedStat = src.mCachedStat;
In other words, save an assignment of a sizable struct if it's not valid.

Unless struct stat is massive, r=ccarlen

Comment 4

[Conrad Carlen (not reading bugmail)]

Comment on attachment 67354 [details] [diff] [review]
Fixes up windows and unix

r=ccarlen - what I suggested would have little to no benefit.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 67354 [details] [diff] [review]
Fixes up windows and unix

sr=dveditz

Comment 6

[Doug Turner (:dougt)]

Thanks

Checking in nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.84; previous revision: 1.83
done
Checking in nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 7

[Doug Turner (:dougt)]

backed out.  

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

Comment 8

[Bradley Baetz (:bbaetz)]

You can't use the default copy constructor, becasue that does a bitwise copy. It
copies the char* in teh xpidlcstring, not what the char* points to.

See bug 113733, though - don't rely (yet) on xpidlcstring's assignment operator,
if you do write your own copy constructor.

Comment 9

[Bradley Baetz (:bbaetz)]

Actually, I take that back. I think. Its not bitwise, its memberwise copy by
default, and nsLocalFileUnix doesn't contain any non-POD types w/o copy
constructors.

Comment 10

[John Morrison]

Attachment: listing of contents of profile dir on Linux, running with and without this patch to nsLocalFileUnix.cpp

Sorry, to say, but in addition to any compiler issues, there were some serious
problems with the functionality of this patch, at least on Linux. 

The Linux tinderboxes that were able to compile and run tests were showing some

very odd numbers (sharp jump in startup time, page laod time, and a sharp drop
in window open time). 

The problem is that this was trashing the profile. See the attached set of
listings in a brand new file, after creating, running, quitting, running,
quitting, on Linux, without and then with this patch (and darin's bustage
tweak).

Most notably, with the patch, the Cache folder becomes something called
"Cache.Trash" and eventually is deleted, the chrome/ directory gets nuked, 
XUL.mfasl is always corrupt, and various other files are wrong. Here's the 
ending state of a profile after running with the patch (this doesn't include 
damage to mailnews files/dirs that are also not treated well). 

with the patch (after create/run/quit/run/quit) ...
				 ... and the same, without the patch

	     ???		       4096 00:58 Cache/	    
 15918 01:03 XUL.mfasl		     553519 00:58 XUL.mfasl	   
  5225 01:04 bookmarks.html	       5225 00:58 bookmarks.html   
	     ???		       4096 00:57 chrome/	    
   639 01:04 history.dat		639 00:58 history.dat	   
  5666 01:04 localstore.rdf	       5666 00:58 localstore.rdf   
	     ???			287 00:57 mimeTypes.rdf    
  2169 01:03 panels.rdf 	       2169 00:57 panels.rdf	   
   206 01:04 prefs.js			206 00:58 prefs.js	   
	     ???		       2335 00:57 search.rdf

Comment 11

[John Morrison]

> in a brand new file

with a brand new profile

Comment 12

[Doug Turner (:dougt)]

not going to happen yet.  so that I don't have to write a copy constructor, I am
going to wait until 113733 is fixed.

Comment 13

[Doug Turner (:dougt)]

Gagan, I do not think that this is nsbeta1+.

Comment 14

[Gagan]

I thought you convinced me that this needed a plus! :) alright taking it out... 

Comment 15

[[e-mail]]

When this is fixed, we can clean up the code & comments at
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStandardURL.cpp#2178.

Comment 16

[[e-mail]]

Actually - we can fix this now (irrelevant of the XPIDLString copy constructor),
since darin changed the nsLocalFile* impls to use nsCStrings instead of
XPIDLStrings quite a while ago.

In addition, I think we _do_ need to write our own copy constructor, rather than
letting the compiler synthesize it - we do _not_ want to copy the mRefCnt
member. (We could use the synthesized copy ctor and then manually reset mRefCnt
to zero, but I think that looks hackish, and the end result (in compiled code)
will be worse).

(We could also use this for the ::InitWithFile(nsILocalFile*) impl, i.e. have an
operator=(const nsLocalFile&), but that would rely on being able to cast an
nsILocalFile to an nsLocalFile{Unix,Win,..}. I'm not sure that's a good thing to
do, so I'll leave that the way it is for now, but comments are welcome. Note
that nsLocalFileMac does this at the moment, using dynamic_cast.)

So, with that said... patch coming up to convert all the nsLocalFile*::Clone()
methods to use a copy constructor.

Comment 17

[[e-mail]]

Attachment: use copy constructors v1

and here it is. dougt, mind taking a look if you have some time? thx!

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 127437 [details] [diff] [review]
use copy constructors v1

this looks fine, but i remember some problems in using a copy constructor on
certian platforms -- that is why these copy constructors are not implemented on
all platforms.

Before you land this, please verify that everything continues to function as
expected. :-)

Comment 19

[[e-mail]]

Thanks - will do. (Your r= is sufficient for xpcom, right? No need to get sr?)

darin, dougt: do you have any opinion on comment 16 paragraph 3? If we can make
the assumption that an nsILocalFile can be cast to the platform-specific
nsLocalFile we're dealing with, then we can optimize that a little bit too.

I'd also like to evaluate using nsSharableStrings for the members of
nsLocalFile*, instead of plain old CStrings, since ::Clone() is probably a
fairly common operation (e.g. when an interface has an nsIFile attribute, with a
getter and setter for it). This might save on allocations... (nsFileSpec does this.)

Comment 20

[Doug Turner (:dougt)]

I would not advise assuming nsILocalFile's are implemented by some standard
concrete class.  

Comment 21

[[e-mail]]

Sounds good to me. :)

Comment 22

[Darin Fisher]

Comment on attachment 127437 [details] [diff] [review]
use copy constructors v1

>Index: xpcom/io/nsLocalFileMac.cpp

> nsLocalFile::Clone(nsIFile **file)
> {
>+    NS_ENSURE_ARG_POINTER(file);

NS_ASSERTION should be sufficient, since it would be really bogus
to call this with a null pointer.  same goes for other Clone impls.


>Index: xpcom/io/nsLocalFileOS2.cpp

>+nsLocalFile::nsLocalFile(const nsLocalFile& other)

w.r.t. dougt's comment about copy ctor.. may be good to check with
mkaply to make sure OS/2 is going to be happy with this.

sr=darin fwiw ;)

Comment 23

[Darin Fisher]

>I'd also like to evaluate using nsSharableStrings for the members of
>nsLocalFile*, instead of plain old CStrings, since ::Clone() is probably a
>fairly common operation (e.g. when an interface has an nsIFile attribute, with a
>getter and setter for it). This might save on allocations... (nsFileSpec does
>this.)

i'm not sure that you can do this since a clone of a nsIFile instance might be
accessed on a different thread (indeed this does happen in necko).  deep copying
is probably correct since nsLocalFile implements threadsafe addref/release.

Comment 24

[Brendan Eich [:brendan]]

> >+    NS_ENSURE_ARG_POINTER(file);
>
> NS_ASSERTION should be sufficient, since it would be really bogus
> to call this with a null pointer.  same goes for other Clone impls.

Don't even bother -- a null dereference is as good as an assertbotch (unless you
are of the evil school who wants assertions that don't abort, which means you
want lots of bogus assertions, which is what you will get in any large project
where assertions don't abort).

/be

Comment 25

[Darin Fisher]

>Don't even bother

yes! even better :)

Comment 26

[[e-mail]]

>i'm not sure that you can do this since a clone of a nsIFile instance might be
>accessed on a different thread (indeed this does happen in necko).  deep copying
>is probably correct since nsLocalFile implements threadsafe addref/release.

Very good point... I'll leave that part alone, then :)

mkaply: per darin's suggestion, could I ask you to take a look at the copy
constructor patch and see if that'll break stuff for OS/2? If you can test (or
know of someone who could test), that would be really great too...

Comment 27

[[e-mail]]

Attachment: make nsStandardURL use nsIFile::Clone()

this fixes the XXX comment in nsStandardURL.cpp, so we cache the file and clone
it.

I've tested the copy-constructor patch as well as this; it looks okay to me on
linux, no idea about other plats yet. :)

Comment 28

[Darin Fisher]

dan: it might be nice to get some usage stats on how often nsStandardURL::mFile
is reused.  while you're patch makes it so that reuse is faster, the first call
to GetFile is now more expensive.  is that worth it?

Comment 29

[[e-mail]]

Sure thing. On browser startup, nsStandardURL::GetFile() is called ~43 times. On
a stock trunk, all of these 43 times result in the spec being parsed out from
the file (i.e. there are no ::SetFile() calls involved here; that's not a common
codepath it seems).

With my patch, 26 calls result in parses, and for the remaining 17 we already
have the result and clone it. On my P3-550/linux, parsing takes 200-300us and
cloning takes <10us. So we'll save around 4-5ms Ts here. (::GetFile and
::SetFile are not called very often on pageload.)

That said, there's a problem with my patch... we only clone the first time we
parse, and not each time we hand the file out. The old code was inconsistent;
it'd hand out a file given to it by ::SetFile() without cloning it, but then
it'd be paranoid and not cache a parsed result. Since cloning is cheap, I think
we should just clone everything we hand out. Or we could fix the callers that
try to modify it. Personally, I think we should not rely on callers being sane,
and just clone everything coming in/going out (SetFile/GetFile). Note that the
.idl disagrees with me here:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIFileURL.idl#56

Comment 30

[[e-mail]]

Attachment: fix nsStandardURL, v2

now with consistent clonage.

Comment 31

[Darin Fisher]

dan: the point of the IDL comment is to try to optimize GetFile since that is
the common operation.  cloning is not zero cost, so if we can avoid it that'd be
a good thing.  i can understand changing the implementation to always Clone
since that helps you avoid having to fix bugs like bug 161921, but i don't think
it means that the interface comments are bad... maybe they're just idealistic. 
if only we had a way to construct an immutable nsLocalFile ;-)

btw: have you checked to see if bug 161921 still exists if you always just hand
back mFile?  if it does, then i think this patch should include a XXX comment
stating that we are still trying to workaround problems in the browser that
prevent us from implementing the interface contract.

Comment 32

[[e-mail]]

>dan: the point of the IDL comment is to try to optimize GetFile since that is
>the common operation.

yeah... it would be best if we could fix 161921 and actually have consumers that
obey the idl :)

>btw: have you checked to see if bug 161921 still exists if you always just hand
>back mFile?

sure... it's a little more risky, but i'll go check that out.

Comment 33

[[e-mail]]

Attachment: fix nsStandardURL, v2.01

so I talked with darin, and he's okay with just putting a comment in there
about consumers, and looking at checking them all later... I filed bug 212724
about that, and added a comment about it. (I think it'll take a bit of time to
check all the consumers by hand, and it's not just as simple as seeing if bug
161921 still exists if we don't clone, so...)

Comment 34

[[e-mail]]

ergh... and before you nit that patch, just assume I've shifted that |nsresult
rv;| inside the |if| block where it's actually used. :)

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

I tested this patch on windows 2000, and it seems to work fine

Comment 36

[Mike Kaply [:mkaply]]

This isn't working on OS/2.

Opening the pref file is failing because mResolvedPath is empty in the call to
PR_Open in OpenNSPRFileDesc.

mWorkingPath is valid, so the nsLocalFile is not bad.

Comment 37

[Mike Kaply [:mkaply]]

I was wrong on the OS/2 stuff. I needed to grab some trunk stuff. Better results
tomorrow.

Comment 38

[Darin Fisher]

Comment on attachment 127782 [details] [diff] [review]
fix nsStandardURL, v2.01

looks good, thanks!  sr=darin

Comment 39

[[e-mail]]

Comment on attachment 127782 [details] [diff] [review]
fix nsStandardURL, v2.01

Andreas, would you mind taking a look at this patch? thx!

Comment 40

[Andreas Otte]

Comment on attachment 127782 [details] [diff] [review]
fix nsStandardURL, v2.01

looks good to me too

Comment 41

[[e-mail]]

thanks for the reviews/testing!... marking fixed (both the localfile patch and
the standardurl patch have been checked in)

Comment 42

[[e-mail]]

Ts dropped on luna by 6ms with the standardurl patch - a whopping 0.2% ;)

Comment 43

[Darin Fisher]

I now believe that this patch was a mistake.  See discussion in bug 307815 and
bug 307429.  Caching stat buffers is a real big hack that results in a variety
of workaround hacks.  The ability to dump the cache by cloning a nsIFile is
nice.  The alternative is to create a new nsIFile and initialize it via
initWithFile, but that is more work for consumers.  It would seem to be exactly
what clone was designed to provide, so we should make it work consistently with
initWithFile.