David Hyatt Reporter
	Description • 23 years ago

This bug covers fastloading a XUL prototype document and its associated
prototype elements.

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 1 • 23 years ago

-> me

	Jaime Rodriguez, Jr.
	Comment 2 • 23 years ago

what are the chances this will make 098?

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 3 • 23 years ago

This is still a work in progress, and not likely to land before .9.8 closes. -> .9.9

	Peter Trudelle
	Comment 4 • 23 years ago

nsbeta1+, since this is scheduled perf work.

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 5 • 23 years ago

this will (obviously) land in 1.0

	John Morrison
	Comment 6 • 23 years ago

Ben. When ready post a work-in-progress patch for testing. From past 
experience, it took a few iterations before this was ready to be turned
on by default in the trunk.

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 7 • 23 years ago

John, Sure thing. I hope to have something over the next few days. 

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 21 • 23 years ago

I ran some MOZ_TIMELINE tests with these patches and am seeing 9% speed gains
for warm starts, and 17-19% speed gains for cold starts over trunk builds.

	Vadim Berezniker
	Comment 22 • 23 years ago

shouldn't OS be All?

	David Baron :dbaron:
	Comment 23 • 23 years ago

Comment on attachment 76980 [details] [diff] [review]
newest patch to content that fixes preferences window sizing bug

>+    static nsIAtom* GetStyleAtom()
>+    {
>+        if (!sStyleAtom) 
>+            sStyleAtom = NS_NewAtom("style");
>+        return sStyleAtom;
>+    }
>+    static nsIAtom* sStyleAtom;

How about just using |nsXULAtoms::style| instead of this static and function?

	David Hyatt Reporter
	Comment 26 • 23 years ago

Beautiful.

sr=hyatt

	Jaime Rodriguez, Jr.
	Comment 28 • 23 years ago

Changing to ADT2. Pls land after Mozilla 1.0 branches. We will re-evaluate this
one for MachV, after it has baked on the trunk for a few days.

	Brendan Eich [:brendan]
	Comment 30 • 23 years ago

Comment on attachment 77925 [details] [diff] [review]
jst's patch to nsJSEnvironment (fixes shutdown crash) 

sr=brendan@mozilla.org, Ben should r= as jst wrote it (jband could r= or sr=
too).

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 31 • 23 years ago

I'll let jband r= the nsJSEnvironment patch :-)

	Brendan Eich [:brendan]
	Comment 32 • 23 years ago

Comment on attachment 78291 [details] [diff] [review]
newest patch to content/ that fixes file invalidations (due to forgetting to update fastload list)

>@@ -152,6 +153,9 @@
> class nsIWebShell;
> 
> // Global object maintenance
>+nsINodeInfoManager* nsXULPrototypeElement::sNodeInfoManager = nsnull;
>+nsICSSParser* nsXULPrototypeElement::sCSSParser = nsnull;
>+nsIXULPrototypeCache* nsXULPrototypeScript::sXULPrototypeCache = nsnull;
> nsIXBLService * nsXULElement::gXBLService = nsnull;

When in Rome (hail Watersonus Maximus!), use gXULPrototypeCache, etc., not
s....

>@@ -4927,37 +4912,180 @@
> {
>     nsresult rv;
> 
>-#if 0
>-    // XXXbe partial deserializer is not ready for this yet
>+    // Write basic prototype data
>+    aStream->Write32(mType);
>+    aStream->Write32(mLineNo);
>+
>+    // Write Node Info
>+    nsAutoString namespaceURI;
>+    mNodeInfo->GetNamespaceURI(namespaceURI);
>+    aStream->WriteWStringZ(namespaceURI.get());
>+
>+    nsAutoString qualifiedName;
>+    mNodeInfo->GetQualifiedName(qualifiedName);
>+    aStream->WriteWStringZ(qualifiedName.get());
>+
>+    // Write Attributes
>+    aStream->Write32(mNumAttributes);    
>+
>+    nsAutoString attributeValue, attributeNamespaceURI, attributeName;
>+    PRInt32 i;
>+    for (i = 0; i < mNumAttributes; ++i) {
>+        mAttributes[i].mNodeInfo->GetNamespaceURI(attributeNamespaceURI);
>+        aStream->WriteWStringZ(attributeNamespaceURI.get());
>+
>+        mAttributes[i].mNodeInfo->GetQualifiedName(attributeName);
>+        aStream->WriteWStringZ(attributeName.get());
>+
>+        mAttributes[i].mValue.GetValue(attributeValue);
>+        aStream->WriteWStringZ(attributeValue.get());
>+    }
>+
>+    // Now write children
>     rv = aStream->Write32(PRUint32(mNumChildren));
>     if (NS_FAILED(rv)) return rv;

How about checking the earlier Write* calls?  Here's a cheap way:

+    // Write basic prototype data
+    rv = aStream->Write32(mType);
+    rv |= aStream->Write32(mLineNo);
+
+    // Write Node Info
+    nsAutoString namespaceURI;
+    mNodeInfo->GetNamespaceURI(namespaceURI);
+    rv |= aStream->WriteWStringZ(namespaceURI.get());
. . .
-    rv = aStream->Write32(PRUint32(mNumChildren));
+    rv |= aStream->Write32(PRUint32(mNumChildren));
     if (NS_FAILED(rv)) return rv;

Fast and not too ugly (not a bunch of bogus "if (NS_FAILED(rv)) return rv;"
lines interspersed).

>-#endif
> 
>-    // XXXbe check for failure once all elements have been taught to serialize
>-    for (PRInt32 i = 0; i < mNumChildren; i++) {
>-        rv = mChildren[i]->Serialize(aStream, aContext);
>-        NS_ASSERTION(NS_SUCCEEDED(rv) || rv == NS_ERROR_NOT_IMPLEMENTED,
>-                     "can't serialize!");
>+    for (i = 0; i < mNumChildren; i++) {
>+        nsXULPrototypeNode* child = mChildren[i];
>+        switch (child->mType) {
>+        case eType_Element:
>+        case eType_Text:
>+            child->Serialize(aStream, aContext);

rv |= ... here and onward, I won't comment on the remaining places that need
it.

>+            break;
>+        case eType_Script:
>+            aStream->Write32(child->mType);
>+            nsXULPrototypeScript* script = NS_STATIC_CAST(nsXULPrototypeScript*, child);
>+
>+            if (script) {
>+                NS_WriteOptionalCompoundObject(aStream, script->mSrcURI, NS_GET_IID(nsIURI),
>+                                               PR_TRUE);
>+                aStream->WriteBoolean(script->mOutOfLine);
>+                if (! script->mOutOfLine)
>+                    script->Serialize(aStream, aContext);

Hey, you're wasting space (and time) with two booleans, one for the optional
mSrcURI, and one for mOutOfLine, which is the same value.  Just do mOutOfLine
yourself, and WriteCompoundObject in the blocked then-part of the if.

>+            }
>+            break;
>+        }         
>     }
>+
>     return NS_OK;
> }
> 
>+            if (script) {
>+                rv = NS_ReadOptionalObject(aStream, PR_TRUE, getter_AddRefs(script->mSrcURI));
>     if (NS_FAILED(rv)) return rv;
> 
>-    // XXXbe temporary until we stop parsing tags when deserializing
>-    NS_ASSERTION(PRInt32(numChildren) == mNumChildren, "XUL/FastLoad mismatch");
>+                aStream->ReadBoolean(&script->mOutOfLine);
>+            
>+                if (! script->mOutOfLine)
>+                    script->Deserialize(aStream, aContext, aDocumentURI);
>+                else
>+                    script->DeserializeOutOfLineScript(aStream, aContext);
>+            }

Matching change to avoid NS_ReadOptionalObject in favor of ReadObject here, of
course (and rv |= all over, 'cept for the first assignment to rv :-).

>@@ -5067,21 +5191,19 @@
> 
> nsresult
> nsXULPrototypeScript::Deserialize(nsIObjectInputStream* aStream,
>-                                  nsIScriptContext* aContext)
>+                                  nsIScriptContext* aContext, nsIURI* aDocumentURI)

Nit: wouldn't this be better (more readable, prettier) if the third param got
its own line, as the second does (and indented the same)?

>+            if (NS_SUCCEEDED(rv2)) {
>+                nsCOMPtr<nsIURI> tempURI;
>+                rv2 = fastLoadService->SelectMuxedDocument(oldURI, getter_AddRefs(tempURI));

Assert that tempURI is mSrcURI?

>@@ -5232,18 +5448,55 @@
>         if (!cx)
>             return NS_ERROR_UNEXPECTED;
> 
>-        // XXXbe temporary, until we serialize/deserialize everything from the
>-        //       nsXULPrototypeDocument on down...
>+        
>+        if (mOutOfLine) {
>         nsCOMPtr<nsIFastLoadService> fastLoadService(do_GetFastLoadService());

You have cache here, is it better to call cache->GetFastLoadService?  Probably,
saves inlining costs.

>+nsresult
>+nsXULPrototypeText::Deserialize(nsIObjectInputStream* aStream,
>+                                nsIScriptContext* aContext, nsIURI* aDocumentURI)

Ditto previous nit about each param on its own line.

>+{
>+    nsresult rv;
>+
>+    // Write basic prototype data
>+    PRUint32 number;
>+    aStream->Read32(&number);
>+    mLineNo = (PRInt32)number;
>+    
>+    nsXPIDLString str;
>+    rv = aStream->ReadWStringZ(getter_Copies(str));
>+    mValue.Assign(str); 

Can you hand off the malloc'd buffer from str to mValue, with some kind of
Adopt method?  Seems a shame to malloc and copy again.

Here we're in nsXULContentSink.cpp:

>@@ -1504,6 +1500,7 @@
>           }
>       }
> 
>+#if 0
>       // XXXbe temporary, until we serialize/deserialize everything from
>       //       the nsXULPrototypeDocument on down...
>       nsCOMPtr<nsIFastLoadService> fastLoadService;
>@@ -1606,6 +1603,7 @@
>               }
>           }
>       }
>+#endif

Go ahead and remove the code, don't #if 0 it -- cvs will remember.  But if you
want to credit this code, put my name on the file(s) to which this code moved,
alongside yours :-).

>+++ xul/document/src/nsXULDocument.cpp	9 Apr 2002 03:45:29 -0000
>@@ -48,7 +48,7 @@
>   Notes
>   -----
> 
>-  1. We do some monkey business in the document observer methods to
>+  1. We do some monkey business in the document observer methods to`

Oops -- glitch character added inadventently to end of line?

In nsXULDocument.cpp now:

>@@ -531,20 +524,17 @@
>         }
> 
>         if (gXULCache) {
>+            gXULCache->RemoveFromFastLoadList(mDocumentURL);
>+

Comment here that this RemoveFromFastLoadList is just in case the document
didn't make it past StartLayout from ResumeWalk.

>+    // Same comment as nsChromeProtocolHandler::NewChannel & nsXULDocument::ResumeWalk

Sacred 80th column (waterson respects ~75, AFAICT), please respect it.

>@@ -1625,6 +1613,20 @@
>     PRBool useXULCache;
>     gXULCache->GetEnabled(&useXULCache);
> 
>+    // If the current prototype is an overlay document (non-master prototype) and we're
>+    // filling the FastLoad disk cache, tell the cache we're done loading it, and write

Way over column 80, wahhhh.

>@@ -1699,20 +1696,10 @@
> }
> 
> 
>+// XXXben dump this
> NS_IMETHODIMP
> nsXULDocument::OnResumeContentSink()
> {
>-    if (mIsFastLoad) {
>-        nsresult rv;
>-        nsCOMPtr<nsIURI> uri;
>-
>-        rv = mCurrentPrototype->GetURI(getter_AddRefs(uri));
>-        if (NS_FAILED(rv)) return rv;
>-
>-        rv = gFastLoadService->SelectMuxedDocument(uri);
>-        if (NS_FAILED(rv))
>-            AbortFastLoads();
>-    }
>     return NS_OK;
>

	Brendan Eich [:brendan]
	Comment 33 • 23 years ago

Comment on attachment 78291 [details] [diff] [review]
newest patch to content/ that fixes file invalidations (due to forgetting to update fastload list)

>@@ -152,6 +153,9 @@
> class nsIWebShell;
> 
> // Global object maintenance
>+nsINodeInfoManager* nsXULPrototypeElement::sNodeInfoManager = nsnull;
>+nsICSSParser* nsXULPrototypeElement::sCSSParser = nsnull;
>+nsIXULPrototypeCache* nsXULPrototypeScript::sXULPrototypeCache = nsnull;
> nsIXBLService * nsXULElement::gXBLService = nsnull;

When in Rome (hail Watersonus Maximus!), use gXULPrototypeCache, etc., not
s....

>@@ -4927,37 +4912,180 @@
> {
>     nsresult rv;
> 
>-#if 0
>-    // XXXbe partial deserializer is not ready for this yet
>+    // Write basic prototype data
>+    aStream->Write32(mType);
>+    aStream->Write32(mLineNo);
>+
>+    // Write Node Info
>+    nsAutoString namespaceURI;
>+    mNodeInfo->GetNamespaceURI(namespaceURI);
>+    aStream->WriteWStringZ(namespaceURI.get());
>+
>+    nsAutoString qualifiedName;
>+    mNodeInfo->GetQualifiedName(qualifiedName);
>+    aStream->WriteWStringZ(qualifiedName.get());
>+
>+    // Write Attributes
>+    aStream->Write32(mNumAttributes);    
>+
>+    nsAutoString attributeValue, attributeNamespaceURI, attributeName;
>+    PRInt32 i;
>+    for (i = 0; i < mNumAttributes; ++i) {
>+        mAttributes[i].mNodeInfo->GetNamespaceURI(attributeNamespaceURI);
>+        aStream->WriteWStringZ(attributeNamespaceURI.get());
>+
>+        mAttributes[i].mNodeInfo->GetQualifiedName(attributeName);
>+        aStream->WriteWStringZ(attributeName.get());
>+
>+        mAttributes[i].mValue.GetValue(attributeValue);
>+        aStream->WriteWStringZ(attributeValue.get());
>+    }
>+
>+    // Now write children
>     rv = aStream->Write32(PRUint32(mNumChildren));
>     if (NS_FAILED(rv)) return rv;

How about checking the earlier Write* calls?  Here's a cheap way:

+    // Write basic prototype data
+    rv = aStream->Write32(mType);
+    rv |= aStream->Write32(mLineNo);
+
+    // Write Node Info
+    nsAutoString namespaceURI;
+    mNodeInfo->GetNamespaceURI(namespaceURI);
+    rv |= aStream->WriteWStringZ(namespaceURI.get());
. . .
-    rv = aStream->Write32(PRUint32(mNumChildren));
+    rv |= aStream->Write32(PRUint32(mNumChildren));
     if (NS_FAILED(rv)) return rv;

Fast and not too ugly (not a bunch of bogus "if (NS_FAILED(rv)) return rv;"
lines interspersed).

>-#endif
> 
>-    // XXXbe check for failure once all elements have been taught to serialize
>-    for (PRInt32 i = 0; i < mNumChildren; i++) {
>-        rv = mChildren[i]->Serialize(aStream, aContext);
>-        NS_ASSERTION(NS_SUCCEEDED(rv) || rv == NS_ERROR_NOT_IMPLEMENTED,
>-                     "can't serialize!");
>+    for (i = 0; i < mNumChildren; i++) {
>+        nsXULPrototypeNode* child = mChildren[i];
>+        switch (child->mType) {
>+        case eType_Element:
>+        case eType_Text:
>+            child->Serialize(aStream, aContext);

rv |= ... here and onward, I won't comment on the remaining places that need
it.

>+            break;
>+        case eType_Script:
>+            aStream->Write32(child->mType);
>+            nsXULPrototypeScript* script = NS_STATIC_CAST(nsXULPrototypeScript*, child);
>+
>+            if (script) {
>+                NS_WriteOptionalCompoundObject(aStream, script->mSrcURI, NS_GET_IID(nsIURI),
>+                                               PR_TRUE);
>+                aStream->WriteBoolean(script->mOutOfLine);
>+                if (! script->mOutOfLine)
>+                    script->Serialize(aStream, aContext);

Hey, you're wasting space (and time) with two booleans, one for the optional
mSrcURI, and one for mOutOfLine, which is the same value.  Just do mOutOfLine
yourself, and WriteCompoundObject in the blocked then-part of the if.

>+            }
>+            break;
>+        }         
>     }
>+
>     return NS_OK;
> }
> 
>+            if (script) {
>+                rv = NS_ReadOptionalObject(aStream, PR_TRUE, getter_AddRefs(script->mSrcURI));
>     if (NS_FAILED(rv)) return rv;
> 
>-    // XXXbe temporary until we stop parsing tags when deserializing
>-    NS_ASSERTION(PRInt32(numChildren) == mNumChildren, "XUL/FastLoad mismatch");
>+                aStream->ReadBoolean(&script->mOutOfLine);
>+            
>+                if (! script->mOutOfLine)
>+                    script->Deserialize(aStream, aContext, aDocumentURI);
>+                else
>+                    script->DeserializeOutOfLineScript(aStream, aContext);
>+            }

Matching change to avoid NS_ReadOptionalObject in favor of ReadObject here, of
course (and rv |= all over, 'cept for the first assignment to rv :-).

>@@ -5067,21 +5191,19 @@
> 
> nsresult
> nsXULPrototypeScript::Deserialize(nsIObjectInputStream* aStream,
>-                                  nsIScriptContext* aContext)
>+                                  nsIScriptContext* aContext, nsIURI* aDocumentURI)

Nit: wouldn't this be better (more readable, prettier) if the third param got
its own line, as the second does (and indented the same)?

>+            if (NS_SUCCEEDED(rv2)) {
>+                nsCOMPtr<nsIURI> tempURI;
>+                rv2 = fastLoadService->SelectMuxedDocument(oldURI, getter_AddRefs(tempURI));

Assert that tempURI is mSrcURI?

>@@ -5232,18 +5448,55 @@
>         if (!cx)
>             return NS_ERROR_UNEXPECTED;
> 
>-        // XXXbe temporary, until we serialize/deserialize everything from the
>-        //       nsXULPrototypeDocument on down...
>+        
>+        if (mOutOfLine) {
>         nsCOMPtr<nsIFastLoadService> fastLoadService(do_GetFastLoadService());

You have cache here, is it better to call cache->GetFastLoadService?  Probably,
saves inlining costs.

>+nsresult
>+nsXULPrototypeText::Deserialize(nsIObjectInputStream* aStream,
>+                                nsIScriptContext* aContext, nsIURI* aDocumentURI)

Ditto previous nit about each param on its own line.

>+{
>+    nsresult rv;
>+
>+    // Write basic prototype data
>+    PRUint32 number;
>+    aStream->Read32(&number);
>+    mLineNo = (PRInt32)number;
>+    
>+    nsXPIDLString str;
>+    rv = aStream->ReadWStringZ(getter_Copies(str));
>+    mValue.Assign(str); 

Can you hand off the malloc'd buffer from str to mValue, with some kind of
Adopt method?  Seems a shame to malloc and copy again.

Here we're in nsXULContentSink.cpp:

>@@ -1504,6 +1500,7 @@
>           }
>       }
> 
>+#if 0
>       // XXXbe temporary, until we serialize/deserialize everything from
>       //       the nsXULPrototypeDocument on down...
>       nsCOMPtr<nsIFastLoadService> fastLoadService;
>@@ -1606,6 +1603,7 @@
>               }
>           }
>       }
>+#endif

Go ahead and remove the code, don't #if 0 it -- cvs will remember.  But if you
want to credit this code, put my name on the file(s) to which this code moved,
alongside yours :-).

>+++ xul/document/src/nsXULDocument.cpp	9 Apr 2002 03:45:29 -0000
>@@ -48,7 +48,7 @@
>   Notes
>   -----
> 
>-  1. We do some monkey business in the document observer methods to
>+  1. We do some monkey business in the document observer methods to`

Oops -- glitch character added inadventently to end of line?

In nsXULDocument.cpp now:

>@@ -531,20 +524,17 @@
>         }
> 
>         if (gXULCache) {
>+            gXULCache->RemoveFromFastLoadList(mDocumentURL);
>+

Comment here that this RemoveFromFastLoadList is just in case the document
didn't make it past StartLayout from ResumeWalk.

>+    // Same comment as nsChromeProtocolHandler::NewChannel & nsXULDocument::ResumeWalk

Sacred 80th column (waterson respects ~75, AFAICT), please respect it.

>@@ -1625,6 +1613,20 @@
>     PRBool useXULCache;
>     gXULCache->GetEnabled(&useXULCache);
> 
>+    // If the current prototype is an overlay document (non-master prototype) and we're
>+    // filling the FastLoad disk cache, tell the cache we're done loading it, and write

Way over column 80, wahhhh.

>@@ -1699,20 +1696,10 @@
> }
> 
> 
>+// XXXben dump this
> NS_IMETHODIMP
> nsXULDocument::OnResumeContentSink()
> {
>-    if (mIsFastLoad) {
>-        nsresult rv;
>-        nsCOMPtr<nsIURI> uri;
>-
>-        rv = mCurrentPrototype->GetURI(getter_AddRefs(uri));
>-        if (NS_FAILED(rv)) return rv;
>-
>-        rv = gFastLoadService->SelectMuxedDocument(uri);
>-        if (NS_FAILED(rv))
>-            AbortFastLoads();
>-    }
>     return NS_OK;
>

	Brendan Eich [:brendan]
	Comment 34 • 23 years ago

Bugzilla sucks for not enforcing that limit more gracefully.  Complete
attachment coming up, I hope.

/be

	Brendan Eich [:brendan]
	Comment 36 • 23 years ago

Read to the end, no truncation in my saved buffer (I copied to a message window
and saved the draft, also sent it to Ben).

/be

	Brendan Eich [:brendan]
	Comment 37 • 23 years ago

Ben, why don't you r= my attachment 78311 [details] or get dbaron to r= it if you're not
comfortable doing the deed?  Then maybe waterson will sr=.

/be

	Brendan Eich [:brendan]
	Comment 38 • 23 years ago

This fixes what seems to me to be an ancient bug: if you read from a FastLoad
file (say navigator stuff, including the URL object for
chrome:://navigator/content/navigator.xul), then make an updater because you
open a new window, say prefs, whose data is not yet in the FastLoad file,
you'll write out decremented (possibly to 0) mStrongRefCnt and mWeakRefCnt
fields in the footer's object map.

Easily fixed by saving a copy of the tiny record from the reader's footer, and
reinitializing the updater's idea of the refcnts to be written from it, no
matter how far toward zero the in-memory copies have counted down during the
read episode.

/be

	Brendan Eich [:brendan]
	Comment 39 • 23 years ago

Adds only two uint16 members to each nsFastLoadReader::nsObjectMapEntry -- the
last version was a rush-job, it lazily saved the whole
nsFastLoadSharpObjectInfo record read in for each object map entry in the
footer, but that wastes a uint32 mCIDOffset copy in addition to the needed
back-up copies of mStrongRefCnt and mWeakRefCnt.  Whew!

/be

	Brendan Eich [:brendan]
	Comment 40 • 23 years ago

Comment on attachment 78291 [details] [diff] [review]
newest patch to content/ that fixes file invalidations (due to forgetting to update fastload list)

Ben, I think you want this:

+	     if (NS_SUCCEEDED(rv2) && mSrcURI)
+		 rv2 = fastLoadService->EndMuxedDocument(mSrcURI);
+
+	     if (NS_SUCCEEDED(rv2)) {
+		 nsCOMPtr<nsIURI> tempURI;
+		 rv2 = fastLoadService->SelectMuxedDocument(oldURI,
getter_AddRefs(tempURI));
+	     }

to look like this, instead:

+	     if (NS_SUCCEEDED(rv2) && mSrcURI) {
+		 rv2 = fastLoadService->EndMuxedDocument(mSrcURI);
+
+		 if (NS_SUCCEEDED(rv2)) {
+		     nsCOMPtr<nsIURI> tempURI;
+		     rv2 = fastLoadService->SelectMuxedDocument(oldURI,
getter_AddRefs(tempURI));
+		     NS_ASSERTION(NS_SUCCEEDED(rv2) && tempURI == mSrcURI);
+		 }
+	     }

No need to select back if you didn't select mSrcURI (because it was null, an
inline script).

Hope I have this right, I'm typing in a tiny textarea!

/be

	David Baron :dbaron:
	Comment 41 • 23 years ago

Comment on attachment 78463 [details] [diff] [review]
Brendan's better latest xpcom/io patch

>+++ nsFastLoadFile.cpp	10 Apr 2002 00:28:07 -0000
>@@ -305,7 +305,8 @@
> }
> 
> struct nsStringMapEntry : public PLDHashEntryHdr {
>-    const char* mString;                // key, must come first
>+    const char*             mString;    // key, must come first
>+    nsCOMPtr<nsISupports>   mURI;       // for SelectMuxedDocument return value

I wonder whether it's misleading to use nsCOMPtr for a struct whose
constructor and destructor are not called.  (It's also inconsistent
with some of the other hashtable entry structs in fastload code, I
think.)

>@@ -1468,15 +1488,23 @@
> nsFastLoadFileWriter::EndMuxedDocument(nsISupports* aURI)
> {
>     nsCOMPtr<nsISupports> key(do_QueryInterface(aURI));
>-#ifdef NS_DEBUG
>     nsURIMapWriteEntry* uriMapEntry =
>         NS_STATIC_CAST(nsURIMapWriteEntry*,
>                        PL_DHashTableOperate(&mURIMap, key, PL_DHASH_LOOKUP));
>-    NS_ASSERTION(uriMapEntry && uriMapEntry->mDocMapEntry,
>+    NS_ASSERTION(uriMapEntry->mDocMapEntry,
>                  "unknown aURI passed to EndMuxedDocument!");
>-#endif
> 
>-    PL_DHashTableOperate(&mURIMap, key, PL_DHASH_REMOVE);
>+    // Drop our ref to the URI object that was passed to StartMuxedDocument.
>+    if (uriMapEntry->mDocMapEntry)
>+        uriMapEntry->mDocMapEntry->mURI = nsnull;
>+
>+    // Shrink the table if half the entries are removed sentinels.
>+    PRUint32 size = PL_DHASH_TABLE_SIZE(&mURIMap);
>+    if (mURIMap.removedCount >= (size >> 2))
>+        PL_DHashTableOperate(&mURIMap, key, PL_DHASH_REMOVE);
>+    else
>+        PL_DHashTableRawRemove(&mURIMap, uriMapEntry);
>+
>     TRACE_MUX(('w', "end %p (%p)\n", aURI, key.get()));
>     return NS_OK;
> }

Why do you need this?  Doesn't the clearEntry callback take care of it?

>Index: nsIFastLoadFileControl.idl
>+    void        startMuxedDocument(in nsISupports aURI, in string aURISpec);
>+    nsISupports selectMuxedDocument(in nsISupports aURI);
>+    void        endMuxedDocument(in nsISupports aURI);
>Index: nsIFastLoadService.idl
>+    void        startMuxedDocument(in nsISupports aURI,
>+                                   in string aURISpec,
>+                                   in PRInt32 aDirectionFlags);
>+    nsISupports selectMuxedDocument(in nsISupports aURI);
>+    void        endMuxedDocument(in nsISupports aURI);

Some comments on these methods (in at least one of the headers) would
be nice, especilly given the nsISupports return value.

I don't follow all the control flow relating to the refcount changes,
but it seems to make sense, so r=dbaron with the comments above.

	Brendan Eich [:brendan]
	Comment 44 • 23 years ago

dbaron: EndMuxedDocument for both the reader and the writer wants to drop
uriMapEntry->mDocMapEntry->mURI right then and there, rather than let it get
dropped from strmap_ClearEntry, which may run much later.  Otherwise, URI bloat
due to unnecessary lifetime extension could result.  I commented this better in
both places.

I added comments to nsIFastLoadService.idl and nsIFastLoadFileControl.idl
discussing select's new return value.

Thanks for pointing out my casual use of nsCOMPtr where a raw pointer is
probably best.	I could have added strmap_InitEntry and constructed and
destructed the nsCOMPtr<nsISupports> mURI member explicitly, but I decided to
match other raw member pointers as you suggested.  Please give this one more
look and stamp r=dbaron if you like it.  Thanks,

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 45 • 23 years ago

Make sure we tidy up on an Abort (clear the FastLoad set, close streams). The
lack of cleanup was causing a startup assert when an attempt was made to load
an absent css file.

	Brendan Eich [:brendan]
	Comment 46 • 23 years ago

Ben found a case where nsFastLoadFileWriter::AddDependency reall shouldn't add
(or rather, should remove) a dependency, if it seems the file does not exist.

Oh, and I found two places (EndMuxedDocument, the clause I commented on last
time in reply to dbaron) where I failed to use NS_RELEASE rather than setting
to null, now that nsStringMapEntry::mURI is no longer an nsCOMPtr.

/be

	Chris Waterson
	Comment 47 • 23 years ago

Comment on attachment 78643 [details] [diff] [review]
last xpcom/io patch, i hope

sr=waterson

	John Morrison
	Comment 48 • 23 years ago

I'm confused, but I'll post some measurements that I have first, and maybe 
my confusion will clear later.

I tested warm and cold startup performance, on Linux/500MHz/128MB, using the
FastLoadXUL_20020410_BRANCH & FastLoadXUL_20020410_BASE, pulled Friday
afternoon (4/12) as the two comparative builds. (built with 
--enable-optimize=-O
--disable-debug --enable-jprof --enable-timeline).

COLD start times
   BASE times               BRANCH times       
1)   00018.400: ...main1      00016.863: ...main1
2)   00018.459: ...main1      00016.887: ...main1
3)   00018.317: ...main1      00016.847: ...main1

which is ~8.3% reduction in cold start time.

In a series of warm startup test, the BRANCH build takes 3.1 seconds, and
the BASE build takes 3.2 seconds, consistently, which is about 3% faster.

Notes: I put in some timeline stuff to time the verification of the
checksum. On cold boot, this is about 0.26 seconds (in the BRANCH build).  [In
comparison, it is only about 0.025 seconds in a warm start of the same build
on the same platform].

There were also timeline markers for js and xul deserialization (brendan,ben)

In the warm start of the BRANCH build, the cumulative times for these are:
  'chrome js deserialize total:' -> 0.140 seconds  [same in BASE build]
  'XUL PD deserialize total:'    -> 0.345 seconds  [absent in BASE build]

In the cold start of the BRANCH build, the cumulative times for these are:
  'chrome js deserialize total:' -> 0.140 seconds  [same in BASE build]
  'XUL PD deserialize total:'    -> 0.460 seconds  [absent in BASE build]


Okay, so that's all well and good. But my confusion is that, in looking at
the timeline logs, I can see that the BRANCH build is moving faster than the
BASE build from the very beginning, long before XUL/JS deserialization kicks
in. That doesn't make sense, given the two source trees that these are based
on. Or does it? (I haven't looked at the patch(es)).

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 49 • 23 years ago

Comment on attachment 78643 [details] [diff] [review]
last xpcom/io patch, i hope

Oi!

How about this, in nsFastLoadFileReader::ReadFooter: (loop at end)

	PRInt64 currentMtime;
	rv = file->GetLastModifiedTime(&currentMtime);
	if (NS_FAILED(rv))
	    return rv;

of course fails messily with my include of imaginary file "goat.css" and causes
the fastload process to fail and attempt fasl file regeneration on every
execute.

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 50 • 23 years ago

Comment on attachment 78643 [details] [diff] [review]
last xpcom/io patch, i hope

my bad, this patch wasn't completely applied to my branch.

	Brendan Eich [:brendan]
	Comment 51 • 23 years ago

Ben, I'm trying to catch up after a trip -- can you blurb me with the latest
patches that need my review still?  Also, any remaining glitches or concerns.
Thanks.

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 52 • 23 years ago

OK, after much pain, flailing, and gnashing of teeth I have built debug on mac,
isolated and fixed the overlay loading problem. turns out mac-carbon is the only
platform that returns null when one foolishly tries to allocate a 0-length array.

updated patch as soon as cvs responds to me...

if it doesn't, the changes make it so that the blocks in
nsXULPrototypeElement::Deserialize that alloc an attribute array and inspect
attributes for things like inline style/classes is only executed if
|mNumAttributes| > 0, and likewise only walks the children if |mNumChildren| > 0.

	Brendan Eich [:brendan]
	Comment 54 • 23 years ago

Comment on attachment 76727 [details] [diff] [review]
patch to caps/

mstoltz can probably r= in a rubber-stamp sense.  Or perhaps dbaron or waterson
(who might claim to know what's going on here :-) can do the deed.

sr=brendan@mozilla.org.

/be

	Brendan Eich [:brendan]
	Comment 55 • 23 years ago

Comment on attachment 76923 [details] [diff] [review]
patch to netwerk/

Looks ok to me, but SOP is to NS_NOTREACHED("goat!") or better (kidding about
the "goat!" -- "nsStandardURL::GetInterfaces" would do in this case) before
returning NS_ERROR_NOT_IMPLEMENTED.

Fix that and sr=brendan@mozilla.org -- darin or dougt can prolly r= in a hurry.

/be

	Brendan Eich [:brendan]
	Comment 56 • 23 years ago

Comment on attachment 76927 [details] [diff] [review]
fix bogus include in chrome reg. 

rs=brendan@mozilla.org -- is this the only rdf/chrome change?

waterson or hyatt can r= in a trice, I'm sure.

/be

	Brendan Eich [:brendan]
	Comment 57 • 23 years ago

Comment on attachment 81135 [details] [diff] [review]
and here tis, hopefully the last patch to content/

>+                if (! script->mOutOfLine)
>+                    rv |= script->Serialize(aStream, aContext);
>+                else
>+                    rv |= aStream->WriteCompoundObject(script->mSrcURI, 
>+                                                       NS_GET_IID(nsIURI), PR_TRUE);

Nit: looks cleaner to put each arg on its own line, justified, once you have to
break the call across two or more lines.

>+    // Read Attributes
>+    rv |= aStream->Read32(&number);
>+    mNumAttributes = PRInt32(number);
>+	
>+	if (mNumAttributes > 0) {

Eek, a tab.  Please fix.

Otherwise, r=brendan@mozilla.org since I'm FastLoad owner.  Great work.  Hyatt
sr='d cavalierly early on, he should rs= again :-).

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 58 • 23 years ago

Comment on attachment 77925 [details] [diff] [review]
jst's patch to nsJSEnvironment (fixes shutdown crash) 

r=ben@netscape.com

	Brendan Eich [:brendan]
	Comment 59 • 23 years ago

Comment on attachment 77925 [details] [diff] [review]
jst's patch to nsJSEnvironment (fixes shutdown crash) 

I think this should go into the trunk ASAP.  Ben, can you check it in?	With a
little bake time, we should also propose it for the 1.0 branch.

/be

	Mitchell Stoltz (not reading bugmail)
	Comment 60 • 23 years ago

Comment on attachment 76727 [details] [diff] [review]
patch to caps/

r=mstoltz on the caps patch.

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 62 • 23 years ago

tell you what, let's not use NS_NOTREACHED. Looks like other pieces of code
were QI'ing URLs to nsIClassInfo and were always failing before, now that they
succeed, they try and call some of the methods (GetHelperForLanguage,
GetInterfaces), and spraying me with assertions on startup.

	Doug Turner (:dougt)
	Comment 63 • 23 years ago

Comment on attachment 81558 [details] [diff] [review]
newer patch to netwerk/

r=dougt

	Brendan Eich [:brendan]
	Comment 64 • 23 years ago

Still has r= and sr= -- no changes other than update and hand-resolve one silly
cvs conflict.

/be

	Brendan Eich [:brendan]
	Comment 65 • 23 years ago

Comment on attachment 81584 [details] [diff] [review]
xpcom/io patch refreshed to apply to top of trunk

Carrying forward review stamps from last xpcom/io patch.

Ben, on the netwerk patch: if NS_NOTREACHED is "spraying" at you, do something:
file a bug, get on the horn, but don't just avoid assertions because they might
botch.	Tracking down the failure rv return in the "if (...) return rv"
haystack truly sucks.

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 66 • 23 years ago

Brendan, I'm unsure as to what the correct way to resolve that would be. It
doesn't seem that the calling code (see stack below) really requires anything
from nsStandardURL, so either returning not implemented or returning valid null
may be correct. 

cc'ing jband

Example stack: 

nsStandardURL::GetHelperForLanguage(nsStandardURL * const 0x0327b54c, unsigned
int 2, nsISupports * * 0x0012a080) line 2427 + 21 bytes
XPCWrappedNative::GatherProtoScriptableCreateInfo(nsIClassInfo * 0x0327b54c,
XPCNativeScriptableCreateInfo * 0x0012a1fc) line 537 + 38 bytes
XPCWrappedNative::GatherScriptableCreateInfo(nsISupports * 0x0327b544,
nsIClassInfo * 0x0327b54c, XPCNativeScriptableCreateInfo * 0x0012a1fc,
XPCNativeScriptableCreateInfo * 0x0012a1f0) line 569 + 13 bytes
XPCWrappedNative::GetNewOrUsed(XPCCallContext & {...}, nsISupports * 0x0327b540,
XPCWrappedNativeScope * 0x013aab30, XPCNativeInterface * 0x032840a0,
XPCWrappedNative * * 0x0012a234) line 265 + 61 bytes
XPCConvert::NativeInterface2JSObject(XPCCallContext & {...},
nsIXPConnectJSObjectHolder * * 0x0012a2d4, nsISupports * 0x0327b540, const nsID
* 0x0012a5e0, JSObject * 0x032706d8, unsigned int * 0x0012a694) line 1059 + 30 bytes
XPCConvert::NativeData2JS(XPCCallContext & {...}, long * 0x0012a4b8, const void
* 0x0012a55c, const nsXPTType & {...}, const nsID * 0x0012a5e0, JSObject *
0x032706d8, unsigned int * 0x0012a694) line 461 + 58 bytes
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_GETTER) line 2114 + 50 bytes
XPCWrappedNative::GetAttribute(XPCCallContext & {...}) line 1823 + 14 bytes
XPC_WN_GetterSetter(JSContext * 0x013aa598, JSObject * 0x032706d8, unsigned int
0, long * 0x0325d478, long * 0x0012a84c) line 1298 + 12 bytes
js_Invoke(JSContext * 0x013aa598, unsigned int 0, unsigned int 2) line 788 + 23
bytes
js_InternalInvoke(JSContext * 0x013aa598, JSObject * 0x032706d8, long 52889440,
unsigned int 0, unsigned int 0, long * 0x00000000, long * 0x0012b64c) line 880 +
20 bytes
js_GetProperty(JSContext * 0x013aa598, JSObject * 0x032706d8, long 46088000,
long * 0x0012b64c) line 2502 + 45 bytes
js_Interpret(JSContext * 0x013aa598, long * 0x0012b7fc) line 2576 + 2032 bytes
js_Invoke(JSContext * 0x013aa598, unsigned int 4, unsigned int 2) line 805 + 13
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x032760d8,
nsXPCWrappedJS * 0x031eb338, unsigned short 3, const nsXPTMethodInfo *
0x03158dd8, nsXPTCMiniVariant * 0x0012bd44) line 1193 + 22 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x031eb338, unsigned short 3,
const nsXPTMethodInfo * 0x03158dd8, nsXPTCMiniVariant * 0x0012bd44) line 430
PrepareAndDispatch(nsXPTCStubBase * 0x031eb338, unsigned int 3, unsigned int *
0x0012bdf4, unsigned int * 0x0012bde4) line 115 + 31 bytes
SharedStub() line 139
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x031388f4, nsIRequest *
0x0327b7d8, int 458753, unsigned int 0) line 1105

	John Bandhauer
	Comment 67 • 23 years ago

Of the nsIClassInfo methods, (I believe) XPConnect calls only
GetHelperForLanguage,  GetInterfaces, GetFlags, and GetContractID (in debug
builds only). XPConnect is OK with error codes for any or all of those. No NOT
return NS_OK unless you return valid data (valid data includes nsnull and 0).
But I think returning not implemented is still best if you really are not going
to implement them. This may be a special case where the NOT_REACHED check is not
appropriate. But still, why not just add:

+NS_IMETHODIMP 
+nsStandardURL::GetInterfaces(PRUint32 *count, nsIID * **array)
+{
+    *count = 0;
+    *array = nsnull;
+    return NS_OK;
+}
+
+NS_IMETHODIMP 
+nsStandardURL::GetHelperForLanguage(PRUint32 language, nsISupports **_retval)
+{
+    *_retval = nsnull; 
+    return NS_OK;
+}
+
+NS_IMETHODIMP 
+nsStandardURL::GetContractID(char * *aContractID)
+{
+    *aContractID = nsnull;
+    return NS_OK;
+}
+
+NS_IMETHODIMP 
+nsStandardURL::GetClassDescription(char * *aClassDescription)
+{
+    *aClassDescription = nsnull;
+    return NS_OK;
+}
+
+NS_IMETHODIMP 
+nsStandardURL::GetClassID(nsCID * *aClassID)
+{
+    *aClassID = (nsCID*) nsMemory::Alloc(sizeof(nsCID));
+    if (!*aClassID)
+        return NS_ERROR_OUT_OF_MEMORY;
+    return GetClassIDNoAlloc(*aClassID);
+}

	Doug Turner (:dougt)
	Comment 70 • 23 years ago

Comment on attachment 81933 [details] [diff] [review]
MAIN_THREAD_ONLY patch for netwerk/

looks good! r=dougt

	Darin Fisher
	Comment 71 • 23 years ago

Comment on attachment 81933 [details] [diff] [review]
MAIN_THREAD_ONLY patch for netwerk/

r/sr=darin

do you need to do the same for nsSimpleURI ??

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 72 • 23 years ago

darin: None of the code that *needs* nsIClassInfo implementation uses
nsSimpleURI afaik, only nsStandardURL. 

	Brendan Eich [:brendan]
	Comment 73 • 23 years ago

Comment on attachment 81135 [details] [diff] [review]
and here tis, hopefully the last patch to content/

Re-recording my r=, hyatt's sr= (from irc).

/be

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 74 • 23 years ago

All of these patches have now been checked in. For issues relating to this
checkin, file new bugs and mark them as blocking bug 134576.

	John Morrison
	Comment 75 • 23 years ago

see followup bug http://bugzilla.mozilla.org/show_bug.cgi?id=142165
"Don't serialize 'http://host/file.xul' and 'file:///c:/file.xul'"
which is leading to crashes after loading remote or local, non-chrome, XUL

	John Morrison
	Comment 76 • 23 years ago

Well, after botching something and me thinking that this was now slower on
windows, I re-ran several tests with different builds on the same machine. The
real answer is that this is about 5% faster on "warm" start on win2k, and up
to 20% faster on a "cold" (post reboot) start.

            I              II             III

warm       2453           2505            2404    
warm       2360           2384            2259    
--------------------------------------------------
gain          4%             5%              6%    

cold       7877           7734            6864    
cold       6444           6846            5518    
--------------------------------------------------
gain         18%            11%             20%    


Sweet! I'll try to get Linux (and hopefully Mac numbers later) [although the 
tinderbox tests show this is about 3% faster for warm start on Linux].

system tested: win2k/500MHz/128MB

Build Type I: 
ac_add_options --disable-debug  ac_add_options --enable-optimize=-O1
ac_add_options --enable-extensions=default,p3p  ac_add_options --enable-crypto
MOZ_PROFILE=1 (i.e., with symbols)
with sources pulled at 19:30PDT and 20:50PDT May 02 

Build Type II: 
ac_add_options --disable-debug ac_add_options --enable-optimize=-O1
ac_add_options --enable-extensions=default,p3p ac_add_options --enable-crypto
(no symbols; i.e., "set MOZ_PROFILE=")
with sources pulled at 19:30PDT and 20:50PDT May 02 

Build Type III: 
The official mozilla bits, from the installer stub: 
- 2002-05-02-08-trunk
- 2002-05-03-08-trunk

[Although, I'm more than a bit puzzled though by the 'cold-start' times. While
these vary a great deal between the "build types", they vary by less than 2%
in the measurements for any given build (i.e., this is reproducible). I'm
using a script which controls the entire reboot testing, so I'm very confident
that the test conditions were the same for each.]

	Jan Varga [:janv]
	Comment 77 • 23 years ago

I think, there's still something wrong on Linux, like it was on Mac.
I see XUL.mfasl rewritten after opening each new window.

	Peter Trudelle
	Comment 78 • 22 years ago

Jan, could you file a separate bug report on that please?

	Ben Goodger (use ben at mozilla dot org for email) Assignee
	Comment 79 • 22 years ago

For people's information: IIRC, Jan was building using a newer version of gcc
and higher optimization flags? (Can't recall exact details, have deleted the
email from several weeks ago), but don't think it was the default/supported
configuration. 

	Jan Varga [:janv]
	Comment 80 • 22 years ago

There is already one filled - bug 142869.
Brendan thinks it might be a bug in compiler or an uninitialized variable in new
fastload code.

	John Morrison
	Comment 81 • 21 years ago

It's been a long, long, long time running...