Fastload XUL documents and elements.

VERIFIED FIXED in mozilla1.0

Status

()

P1
normal
VERIFIED FIXED
18 years ago
5 years ago

People

(Reporter: hyatt, Assigned: bugs)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [05/02/02][adt2])

Attachments

(8 attachments, 24 obsolete attachments)

1.34 KB, patch
security-bugs
: review+
brendan
: superreview+
Details | Diff | Splinter Review
1.51 KB, patch
brendan
: superreview+
Details | Diff | Splinter Review
3.19 KB, patch
bugs
: review+
brendan
: superreview+
Details | Diff | Splinter Review
16.83 KB, text/plain
Details
27.74 KB, text/html
Details
94.04 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
13.76 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
3.21 KB, patch
dougt
: review+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
This bug covers fastloading a XUL prototype document and its associated
prototype elements.
(Reporter)

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
(Reporter)

Updated

18 years ago
Summary: Fastload XUL prototype documents and elements. → Fastload XUL documents and elements.
(Reporter)

Updated

18 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
-> me
Assignee: hyatt → ben
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Priority: -- → P1

Updated

18 years ago
Blocks: 94199, 102472
what are the chances this will make 098?
This is still a work in progress, and not likely to land before .9.8 closes. -> .9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Severity: normal → enhancement
Keywords: nsbeta1

Comment 4

17 years ago
nsbeta1+, since this is scheduled perf work.
Keywords: nsbeta1 → nsbeta1+
this will (obviously) land in 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Whiteboard: 03/07/02

Comment 6

17 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.
Whiteboard: 03/07/02 → 03/13/02

Updated

17 years ago
Whiteboard: 03/13/02 → 03/13/02 [adt1]
Whiteboard: 03/13/02 [adt1] → 03/22/02 [adt1]
John, Sure thing. I hope to have something over the next few days. 
Whiteboard: 03/22/02 [adt1] → 03/31/02 [adt1]
Posted patch remove some unused functions (obsolete) — Splinter Review
Attachment #76711 - Attachment is obsolete: true
Attachment #76713 - Attachment is obsolete: true
Posted patch brendan's patch to xpcom/ (obsolete) — Splinter Review
Attachment #76659 - Attachment is obsolete: true
Posted patch patch to chrome registry (obsolete) — Splinter Review
Attachment #76726 - Attachment is obsolete: true
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.

No longer blocks: 134576

Comment 22

17 years ago
shouldn't OS be All?

Updated

17 years ago
OS: Windows 2000 → All
Hardware: PC → All
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?
Posted patch oops. use nsXULAtoms::style (obsolete) — Splinter Review
Attachment #76980 - Attachment is obsolete: true

Updated

17 years ago
Severity: enhancement → normal
(Reporter)

Comment 26

17 years ago
Beautiful.

sr=hyatt
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.
Whiteboard: 03/31/02 [adt1] → [Need new ETA] [adt2]
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
Attachment #77925 - Flags: superreview+
I'll let jband r= the nsJSEnvironment patch :-)
Attachment #78291 - Flags: needs-work+
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;
>
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;
>
Bugzilla sucks for not enforcing that limit more gracefully.  Complete
attachment coming up, I hope.

/be
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
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
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
Attachment #78231 - Attachment is obsolete: true
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
Attachment #78461 - Attachment is obsolete: true
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
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.
Attachment #78463 - Flags: review+
Posted patch newest patch to content/ (obsolete) — Splinter Review
Attachment #78291 - Attachment is obsolete: true
Posted patch newest patch to content/ (obsolete) — Splinter Review
Attachment #78499 - Attachment is obsolete: true
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
Attachment #78463 - Attachment is obsolete: true
Posted patch content/ patch (obsolete) — Splinter Review
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.
Attachment #78503 - Attachment is obsolete: true
Posted patch last xpcom/io patch, i hope (obsolete) — Splinter Review
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
Attachment #78604 - Attachment is obsolete: true

Updated

17 years ago
Blocks: 136702

Comment 47

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

sr=waterson
Attachment #78643 - Flags: superreview+

Comment 48

17 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)).
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.
Attachment #78643 - Flags: needs-work+
Comment on attachment 78643 [details] [diff] [review]
last xpcom/io patch, i hope

my bad, this patch wasn't completely applied to my branch.
Attachment #78643 - Flags: needs-work+
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
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.
Attachment #76922 - Attachment is obsolete: true
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
Attachment #76727 - Flags: superreview+
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
Attachment #76923 - Flags: superreview+
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
Attachment #76927 - Flags: superreview+
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
Attachment #81135 - Flags: superreview+
Comment on attachment 77925 [details] [diff] [review]
jst's patch to nsJSEnvironment (fixes shutdown crash) 

r=ben@netscape.com
Attachment #77925 - Flags: review+
Whiteboard: [Need new ETA] [adt2] → [05/01/02][adt2]
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
Whiteboard: [05/01/02][adt2] → [05/02/02][adt2]
Comment on attachment 76727 [details] [diff] [review]
patch to caps/

r=mstoltz on the caps patch.
Attachment #76727 - Flags: review+
Posted patch newer patch to netwerk/ (obsolete) — Splinter Review
Attachment #76923 - Attachment is obsolete: true
Posted patch newer patch to netwerk/ (obsolete) — Splinter Review
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.
Attachment #81552 - Attachment is obsolete: true
Comment on attachment 81558 [details] [diff] [review]
newer patch to netwerk/

r=dougt
Attachment #81558 - Flags: review+
Still has r= and sr= -- no changes other than update and hand-resolve one silly
cvs conflict.

/be
Attachment #78643 - Attachment is obsolete: true
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
Attachment #81584 - Flags: superreview+
Attachment #81584 - Flags: review+
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

Comment 67

17 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);
+}
Attachment #81558 - Attachment is obsolete: true
Comment on attachment 81933 [details] [diff] [review]
MAIN_THREAD_ONLY patch for netwerk/

looks good! r=dougt
Attachment #81933 - Flags: review+

Comment 71

17 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 ??
darin: None of the code that *needs* nsIClassInfo implementation uses
nsSimpleURI afaik, only nsStandardURL. 
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
Attachment #81135 - Flags: review+
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.

Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 75

17 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

Comment 76

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

Comment 77

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

Comment 78

17 years ago
Jan, could you file a separate bug report on that please?
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. 

Comment 80

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

Comment 81

16 years ago
It's been a long, long, long time running...
Status: RESOLVED → VERIFIED

Updated

11 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.