106153 - XBL footprint: don't build XML elements for XBL

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Cathleen]

Comment 1

[David Hyatt]

Ok, I'm well along on this bug, and have a content sink up and limping.  

Comment 2

[David Hyatt]

Adding harish to cc, since this involves making a new content sink that derives
from nsXMLContentSink.

Comment 3

[Johnny Stenback (:jst)]

Hyatt, FYI, my removal of mNameSpace from nsXMLElement and nsXULElement slots
has landed...

Comment 4

[David Hyatt]

Attachment: Patch for review. Don't build a content model for <handlers>. Leave everything else alone for now.

Comment 5

[Chris Waterson]

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

Comment 6

[David Hyatt]

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

Comment 7

[David Hyatt]

Attachment: Patch with waterson's suggestions incorporated.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 56174 [details] [diff] [review]
Patch with waterson's suggestions incorporated.

r=bryner

Comment 9

[David Hyatt]

Attachment: Patch #3, make sure not to regress 45989.

Comment 10

[Chris Waterson]

sr=waterson

Comment 11

[Johnny Stenback (:jst)]

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.

Comment 12

[David Hyatt]

Attachment: Patch that reduces the footprint of <resources> (no more content model).

Comment 13

[Johnny Stenback (:jst)]

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

Comment 14

[Brian Ryner (not reading)]

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 15

[Brian Ryner (not reading)]

Comment on attachment 57350 [details] [diff] [review]
Patch that reduces the footprint of <resources> (no more content model).

r=bryner

Comment 16

[David Hyatt]

Ok, <resources> patch has landed.  That just leaves the big one:
<implementation/>.  My big fat baby's diet continues!

Comment 17

[David Hyatt]

Attachment: Patch that stops building a content model for <implementation>.

Comment 18

[David Hyatt]

Attachment: Required patch to XBL files to convert misuses of <property> to <field>.

Comment 19

[Joe Hewitt (gone)]

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

sr=hewitt

Comment 20

[David Hyatt]

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.

Comment 21

[David Hyatt]

Now seeking r/sr for the C++ patch, the only non-obsolete patch in the list!

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 23

[David Hyatt]

Fixed.  I just moved the code into the method/property .cpp files.

Comment 24

[David Hyatt]

Attachment: <implementation> patch, with even more footprint reduction.

Comment 25

[Brian Ryner (not reading)]

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.

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 57520 [details] [diff] [review]
<implementation> patch, with even more footprint reduction.

sr=[e-mail]

Comment 27

[David Hyatt]

Hallelujah, this bug is now fixed.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that ought to reduce the number of allocations

Comment 30

[David Hyatt]

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

Comment 31

[David Hyatt]

Ok, jag explained this all to me.  

sr=hyatt.  Removing AddLeaf and doing everything in FlushText is an awesome
simplification.

Comment 32

[jag (Peter Annema)]

Comment on attachment 57572 [details] [diff] [review]
patch that ought to reduce the number of allocations

r=jag

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[David Hyatt]

Looks good.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Unfortunately, that patch doesn't seem to fix the allocations problem.

Comment 37

[David Hyatt]

String stuff moved to different bug.  Closing this one out.

Comment 38

[Eugene Savitsky]

To what bug number has it moved?

Comment 39

[John Morrison]

bug 109963

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the allocation churn was noise on the tinderbox, since I saw the same
exact *increase* when I checked in the patches. :-)