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

Apparently the mac already does this.

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

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 on attachment 67354 [details] [diff] [review] Fixes up windows and unix r=ccarlen - what I suggested would have little to no benefit.

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

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

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

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.

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.

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

> in a brand new file with a brand new profile

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.

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

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

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

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.

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

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

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

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

Sounds good to me. :)

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

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

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

>Don't even bother yes! even better :)

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

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

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?

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

now with consistent clonage.

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.

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

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

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

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

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.

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

Comment on attachment 127782 [details] [diff] [review] fix nsStandardURL, v2.01 looks good, thanks! sr=darin

Comment on attachment 127782 [details] [diff] [review] fix nsStandardURL, v2.01 Andreas, would you mind taking a look at this patch? thx!

Comment on attachment 127782 [details] [diff] [review] fix nsStandardURL, v2.01 looks good to me too

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

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

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.