There are about 190 chrome urls and they cause - 190 clones - 190 jar urls - 190 resource urls of 6 unique jar files - 190 file urls Thats the bulk of the url creations. I have patch eliminating the 190 clones. I think the jar urls are going to be hard to eliminate. The resource urls can be probably cached. The file urls can be eliminated along with their channel being created.

this patch eliminates creation of the file:// URL and channel. it also eliminates stat'ing the file on the UI thread, which is never ever a good idea.

On my system (733Mhz 128Mb RAM redhat 7.0 linux), this patch saved about 0.9% on startup, which amounts to about only 1/10th of a second. also, it turns out that this patch does not eliminate creation of the file:// urls, so that could explain why the benefit of the patch is not so great.

i'm working on a patch which would make nsResProtocolHandler::NewURI return a resolved URI, so as to alleviate the need for nsResChannel.

the latest patch does the following: 1) adds: string nsIResProtocolHandler::resolveURI(in nsURI resourceURI) 2) nsResProtocolHandler::NewChannel calls resolveURI, and returns a channel for the resolved uri. 3) nsJARChannel checks if the uri is resource and calls resolveURI to get the file uri and then a nsIFile -- to be replaced with nsIFileURL::getFile, which implies a separate nsResURL implementation (probably aggregating nsStdURL). as a result, a file:// url and channel are never created, and the jar channel gets a nsIFile without instantiating a downloader. adding nsResURL is probably the last step to wrapping up this patch.

r=hyatt on dp's patch of 8/29/01 17:15.

darin: you can do a cvs diff -uN after cvs adding any new files, to create an all-in-one patch. FYI, and review from me in a bit. /be

Comment on attachment 47570 [details] [diff] [review] Eliminating the chrome url Clone() Hyatt reviewed this r=hyatt

dp, some unsolicited comments on the latest patch (which is very cool, btw): -SplitURL(nsIURI* aChromeURI, nsCString& aPackage, nsCString& aProvider, nsCString& aFile) +SplitURL(nsIURI *aChromeURI, nsCString& aPackage, nsCString& aProvider, nsCString& aFile, + PRBool *modified = nsnull) aModified, please follow the formal argument naming convention. - if (! str) + if (! (const char *)str) return NS_ERROR_INVALID_ARG; Is str.get() better here than (const char *)str? Just askin'. Also, why not NS_ERROR_OUT_OF_MEMORY (pre-existing code, but fix it anyway)? + PRBool nofile = PR_FALSE; if (aFile.Length() == 0) { + nofile = PR_TRUE; Nit: PRBool noFile = (aFile.Length() == 0); if (noFile) {...} seems clearer and as, if not more, efficient in compiled code. /be

Thanks brendan. All review comments accepted and changes made. Will attach patch. > Is str.get() better here than (const char *)str? Just askin'. nsXPIDLString.h sez (const char *) is deprecated. So I guess .get() is better.

Comment on attachment 48205 [details] [diff] [review] v1.1 "cvs diff -uN" version + minor fix for msvc++ looks good. be sure to remove the nsResChannel from the mac project.

r=dp - Put back the changes to nsCStringKey creation - take care of new files being added on mac

Comment on attachment 48228 [details] [diff] [review] v1.2 new patch following code review with dp forgot to use cvs -uN

Comment on attachment 48229 [details] [diff] [review] v1.2 new patch following code review with dp + nsResURL.{h,cpp} r=dp

Comment on attachment 48342 [details] [diff] [review] Eliminate 200 url creations and 400 url Parse() on startup with blank page. Hyatt reviewed this.

Comment on attachment 48342 [details] [diff] [review] Eliminate 200 url creations and 400 url Parse() on startup with blank page. sr=waterson

My patch checked in.

http://bugzilla.mozilla.org/attachment.cgi?id=48229 checked in on trunk. resolving bug FIXED.

This patch might cause Bug 98838. Please take a look.

backed out my patch due to bug 98838.

Darin, sorry I didn't sr= -- should I look for a new patch? Who super-reviewed the one that got checked in, anyway? /be

brendan: i used dougt's review as sr= ... i'm working on a new patch to resolve the regression (over the weekend) resulting from my checkin.

the problem with the previous patch was in the way i was attempting to reuse the nsStdURL implementation. in particular, nsStdURL::Equals requires its argument to also be a nsStdURL if the two are to be considered equal (ie. they do not just have to have similar uri strings). so, i've redone nsResURL to make it simply inherit from nsStdURL, overriding nsIFileURL::GetFile only. this seems to do the trick, and is IMO a much simpler and more efficient solution. dp, brendan: can you guys please review this patch? thanks!

Comment on attachment 49064 [details] [diff] [review] v1.3 revised patch to avoid bug 98838 obsolete per review with dp

Comment on attachment 49163 [details] [diff] [review] v1.4 revised per comments from dp wrong patch

Comment on attachment 49164 [details] [diff] [review] v1.4 revised patch per comments from dp has r=dp, waiting for sr=

Comment on attachment 49164 [details] [diff] [review] v1.4 revised patch per comments from dp - Use do_GetIOService() instead of do_GetService(kIOServiceCID, &rv) with yet another static CID for the I/O service. - Why is mSubstitutions a mapping from "host" to an nsISupportsArray when we use only the first element of the array? Can you file a bug to simplify to a scalar instead of a vector mapped type here? - Move nsJarChannel::EnsureJARFileAvailable's fileURL decl down to its first assignment, and initialize the decl. Oh wait, you can't because of the two goto error; statements -- much as I like a well-used goto (break across multiple switch/loop levels, typically), these two are gratuitous. Can you eliminate them without hideously overindenting the code after each? Nice cleanup in general, sr=brendan@mozilla.org if you fix the above fluff. We need more of this kind of bogo-thread-safety and false generality policing. /be

brendan: thanks for the comments. 1. does using do_GetIOService really reduce the number of statics? afterall, it is an inline function. maybe i'm mistaken, but i thought that the compiler would simply inline the code preventing the linker from knowing that the static CID is replicated. do_GetIOService is/was the only reason nsNetUtil.h is included, which really slows down the build each time it is included/modified. 2. see bug 99410 3. i should be able to clean up EnsureJARFileAvailable.

1. How often is nsNetUtil.h modified? I'd rather consolidate declarations, if not compiler-propagated nsCID constants (which are coalesced by the GNU toolchain, at least). Build time seems like the wrong deciding factor here. 2&3. Thanks! /be

brendan: knowing that some compilers will do the right thing is enough for me. thanks!

fixed-on-trunk