Closed Bug 106153 Opened 23 years ago Closed 23 years ago

XBL footprint: don't build XML elements for XBL

Categories

(Core :: XBL, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: cathleennscp, Assigned: hyatt)

References

Details

Attachments

(1 file, 7 obsolete files)

 
Blocks: 92580
Summary: throw away XML elements in XBL → XBL footprint: throw away XML elements in XBL
Target Milestone: --- → mozilla0.9.6
Ok, I'm well along on this bug, and have a content sink up and limping.  
Status: NEW → ASSIGNED
Summary: XBL footprint: throw away XML elements in XBL → XBL footprint: don't build XML elements for XBL
Adding harish to cc, since this involves making a new content sink that derives
from nsXMLContentSink.
Hyatt, FYI, my removal of mNameSpace from nsXMLElement and nsXULElement slots
has landed...
- Why do you even need to create an nsXMLDocument object at all? Can't
  you just create an nsXBLContentSink and hook that directly up
  to a parser, document be damned? RDF does this, e.g.

- Don't screw up the tabs in Makefile.in's (they have hard tabs).

- Don't need to use .get() to compare comptr's to raw ptrs anymore

- Did you want to leave this in? If so, use #if 0 (and explain why you're
  doing it) instead of /**/ comments.

 if (!hasExtends) 
       protoBinding->SetHasBasePrototype(PR_FALSE);
     else {
+      /*// Nuke the display attribute off the content node to reduce our
+      // in-memory footprint.
+      if (hasDisplay)
+        child->UnsetAttr(kNameSpaceID_None, nsXBLAtoms::display, PR_FALSE);
+*/
+
+      // Now slice 'em up to see what we've got.


- I'm not seeing the implementation for nsXBLAtoms. Did you forget to cvs add
  a file?

I'll need to look at the other stuff more carefully.
This is an incremental change, and I still rely on the document for other
unconverted pieces of XBL (this patch only converts <handlers>).  Removing the
nsXMLDocument will be extremely involved (and should probably be covered in a
completely separate bug), but you are right that it will be possible to do so. 
I just can't do it yet. :)


Attachment #56150 - Attachment is obsolete: true
Comment on attachment 56174 [details] [diff] [review]
Patch with waterson's suggestions incorporated.

r=bryner
Attachment #56174 - Flags: review+
Attachment #56174 - Attachment is obsolete: true
sr=waterson
Do we really want a namespace ID (i.e. a PRInt32) in nsXBLAtoms? Seems silly to
me, why not just #define it in nsINameSpaceManager like we do for all other well
known namespaces? It's not like XBL won't always be there any more, and if it's
not we're talking very little code to predefine the namespace.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attachment #56186 - Attachment is obsolete: true
Comment on attachment 57350 [details] [diff] [review]
Patch that reduces the footprint of <resources> (no more content model).

Add a license header to nsXBLPrototypeResources.cpp, add a out of memory check
to
nsXBLResourceLoader::StyleSheetLoaded() and
nsXBLResourceLoader::AddResourceListener()

sr=jst
Attachment #57350 - Flags: superreview+
Looks good.  Tested that it compiles fine on Linux.

You should also fix the license blocks in nsXBLResourceLoader.h and
nsXBLPrototypeResources.h to say 2001 instead of 1998 (and preferably use MPL
instead of NPL).
Comment on attachment 57350 [details] [diff] [review]
Patch that reduces the footprint of <resources> (no more content model).

r=bryner
Attachment #57350 - Flags: review+
Attachment #57350 - Attachment is obsolete: true
Ok, <resources> patch has landed.  That just leaves the big one:
<implementation/>.  My big fat baby's diet continues!

Comment on attachment 57456 [details] [diff] [review]
Required patch to XBL files to convert misuses of <property> to <field>.

sr=hewitt
Attachment #57456 - Flags: superreview+
Comment on attachment 57456 [details] [diff] [review]
Required patch to XBL files to convert misuses of <property> to <field>.

Hixie reviewed.  This patch has landed.
Attachment #57456 - Attachment is obsolete: true
Attachment #57456 - Flags: review+
Now seeking r/sr for the C++ patch, the only non-obsolete patch in the list!
Maybe you should make removeJSGCRoot and AddJSGCRoot non-static, since they're
already used in a bunch of files already?  Putting static functions in a .h file
seems rather evil.
Fixed.  I just moved the code into the method/property .cpp files.
Attachment #57455 - Attachment is obsolete: true
Comment on attachment 57520 [details] [diff] [review]
<implementation> patch, with even more footprint reduction.

r=bryner.  don't forget to remove the #includes of nsIXBLPrototypeProperty.h in
nsXBLBinding.cpp and nsXBLPrototypeBinding.h.
Attachment #57520 - Flags: review+
Comment on attachment 57520 [details] [diff] [review]
<implementation> patch, with even more footprint reduction.

sr=ben@netscape.com
Attachment #57520 - Flags: superreview+
Hallelujah, this bug is now fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
For running the bloat URLs in the page cycler, based on my new leak stats
tinderbox (not all that chrome-intensive):

before:
  Leaks: 865355 bytes, 3911 allocations
  Maximum Heap Size: 8654380 bytes
  65024885 bytes were allocated in 422381 allocation

after:
  Leaks: 864333 bytes, 3873 allocations
  Maximum Heap Size: 8369059 bytes
  67428214 bytes were allocated in 520298 allocations.


That's a 285K decrease in heap size!

Any idea what caused the increased churn in small allocations?
There is some string fu I've never seen before that I want to learn about.

mHandlerText = ToNewUnicode(nsDependentString(temp) + aText);

So the A + B string is a string that doesn't really make a new buffer to hold
A+B, but instead keeps the buffers separate and just lets you iterate over them
as though they are one string?  If so, cool! I didn't know that existed.  What
is the name of the string class that is made when you do A+B?

Second question.  What the heck is this?

const nsASingleFragmentString& text

Hoping to get a bit of string education, so I can write more clever code in the
future... :)

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #57520 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Ok, jag explained this all to me.  

sr=hyatt.  Removing AddLeaf and doing everything in FlushText is an awesome
simplification.
Attachment #57572 - Flags: superreview+
Comment on attachment 57572 [details] [diff] [review]
patch that ought to reduce the number of allocations

r=jag
Attachment #57572 - Flags: review+
Actually, in hindsight, I probably don't want to clobber CDATA sections that
aggressively -- I should fall through to the XML content sink for CDATA in the
same conditions as we fall through for FlushText.
In other words, I changed AddCDATASection to be:

NS_IMETHODIMP
nsXBLContentSink::AddCDATASection(const nsIParserNode& aNode)
{
  if (mState == eXBL_InHandlers || mState == eXBL_InImplementation)
    return AddText(aNode.GetText());
  return nsXMLContentSink::AddCDATASection(aNode);
}
Looks good.
Unfortunately, that patch doesn't seem to fix the allocations problem.
String stuff moved to different bug.  Closing this one out.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
To what bug number has it moved?
bug 109963
Status: RESOLVED → VERIFIED
Actually bug 109963 is pretty much unrelated.  If the problem doesn't go away
when I check in the patch above (which is good even if it doesn't fix the
problem), then I'll file an additional bug.  It could be that the tinderbox I
was looking at does something that I wasn't doing when I tried to test to see if
it helps.
I think the allocation churn was noise on the tinderbox, since I saw the same
exact *increase* when I checked in the patches. :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: