92575 - nsVoidArray -> nsAutoVoidArray patch for content/*

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

From bug 90545.  I'd like to see these committed ASAP.  This is just the
content/* part of the second patch from that bug.  It assumes that the main
patch from 90545 has been applied.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I'll take this.  Why isn't there a "content" category???

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: content/* patches from 90545

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comments from 90545 about member vars:

nsContentSubtreeIterator:
        nsAutoVoidArray mStartNodes;
        nsAutoVoidArray mStartOffsets;
        nsAutoVoidArray mEndNodes;
        nsAutoVoidArray mEndOffsets;
    These all normally get entries added in ::Init

nsBaseContentList:
        nsAutoVoidArray mElements;
    These seemed to always end up with elements in them in practice.

nsDocument: (a giant object if ever there was one)
        nsAutoVoidArray mStyleSheets;
        nsAutoVoidArray mObservers; // basically always has at least 1 entry
    Note I only did these two; other nsVoidArrays in nsDocument
    (mCharSetObservers, mPresShells, mSubDocuments) I left alone, because I was 
    less sure if they should be converted (perhaps mPresShells and 
    mCharSetObservers).

nsDocumentEncoder:
        nsAutoVoidArray   mCommonAncestors;
        nsAutoVoidArray   mStartNodes;
        nsAutoVoidArray   mStartOffsets;
        nsAutoVoidArray   mEndNodes;
        nsAutoVoidArray   mEndOffsets;
    Again, when DocumentEncoder is created and used, these seem to always end up 
    with elements in them.

nsFormControlList:
        nsAutoVoidArray mElements;  // Holds WEAK references - bug 36639
    Again, this seems to always have at least one item in it when it exists.

HTMLContentSink: (Another huge object)
        nsAutoVoidArray mContextStack;
    This gets used a lot.

nsCSSLoader:
        // mParsingData is (almost?) always needed, so create with storage
        nsAutoVoidArray   mParsingData; // array of data for sheets currently
parsing
    As stated.  There are two other nsVoidArrays I didn't touch.

CSSStyleSheetInner:
        nsAutoVoidArray           mSheets;
    This does an AppendElement in it's constructor.

nsXBLStreamListener:
        nsAutoVoidArray mBindingRequests;
    These seem to get requests associated with them almost always.

nsXULElement:
        nsAutoVoidArray                     mChildren;           // [OWNER]
    Many (most) XUL elements seem to have children

nsXULContentSinkImpl:
        nsAutoVoidArray mNameSpaceStack;
    Again, almost always used - it's called from ::OpenContainer

nsXULDocument: (another BIG object)
        // This always has at least one observer
        nsAutoVoidArray            mObservers;
        // This is set in nsPresContext::Init, which calls SetShell.
        // Since I think this is almost always done, take the 32-byte hit for
        // an nsAutoVoidArray instead of having it be a separate allocation.
        nsAutoVoidArray            mCharSetObservers;
        // is we're attached to a DocumentViewImpl, we have a presshell
        nsAutoVoidArray            mPresShells;
   The comments speak for themselves.

Comment 4

[Pierre Saslawsky]

r=pierre for changes in the style system - thanks Randell!

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Note: I don't have any figures on memory usage for these changes.  I'm trying to
figure out the bloat-capture stuff now that I have a linux box.  However, in
general I was careful to only expand arrays that seemed to always or often get
entries added.  Pierre has r='d the style system stuff, which was what I was
most worried about bloat in.

Comment 6

[David Hyatt]

r=hyatt on xbl changes.

Comment 7

[Johnny Stenback (:jst)]

I'd really like to see numbers on the savings/bloat introduced by making
mChildren and mAttributes in nsGenericElement and nsXULElement before those
changes are checked in, other than those, these changes seem fine with me. A few
exceptions tho:

nsXULDocument::mPresShells (same goes for nsDocument::mPresShells), in todays
code there is never ever more than one presshell in a document (yes, it's silly
to have an array to hold the presshell pointer, but that's how it is for future
possible extensions of this system), so wouldn't it make sense to pass in 1 to
the mPresShells' constructor in stead and pre-allocate the space to hold that
one pointer, in stead of making mPresShells a nsAutoVoidArray?
Other than that, sr=jst (i.e. leave out the mChildren and mAttrbites changes for
now).

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

In nsGenericElement.cpp, mChildren and mAttributes (from a quick check and
memory) are allocated as AutoVoidArrays only when we would otherwise need to
allocate an mImpl structure anyways.  If there's any overhead in the allocator,
we may save space for small numbers of items in the array.  If the arrays have
>8 items, then the original 8 items will not be used, so there is a possibility
for a small amount of bloat.

As for nsXULElement/etc: mPresShells is an autovoidarray because you can't
specify a (variable) size for a member element.  I'd have to modify voidarray to
allow an external array to be specified, etc.  No fun.   Especially if it only
has 1 element, autovoidarray is probably better than voidarray.   Voidarray will
require a second allocation with space for 8 items if it needs to store one.   I
could new the voidarray in the CTOR with a size, but that would require 2
allocations, one for nsVoidArray, 1 for the mImpl (plus space for 1 entry).

So, I think these (above) are ok.  However,  I'm willing to leave some or all of
these out if needed to get r/sr/a= for tomorrow.

mChildren in nsXULElement.h - I think we should get more details on savings/cost
for that.  Even if the cost is high, it may makes sense to use CheapVoidArray or
some such (or simply make it an nsVoidArray* and allocate it when needed.  (it
would require a fair amount of error-checking code).

Comment 9

[Johnny Stenback (:jst)]

sr=jst, but leave out the changes to mChildren for now, leave the bug open to
track what to do about mChildren.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

requested a= for 0.9.4

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Checking in minus mChildren changes in nsXULContentSink and nsGenericElement.
Leaving bug open for those changes; new (small) patch to be attached soon.
Reducing severity.

Comment 12

[Johnny Stenback (:jst)]

I hacked up some statistics gathering code that spits out how many elements are
in memory, how many of those have no children, 1 child, more than 8 children and
similar data for attributes. Here's the data from having 2 windows open, one
window contains www.mozilla.org, the other contains the lxr view of
nsGlobalWindow.cpp.

Documents in memory                   : 36
Elements in memory                    : 13447
Child array sizes                     : 258156
Attr array sizes                      : 308864
Elements with no attrs                : 2295
Elements with no children             : 807
Elements with no elemets or attrs     : 388
Elements with 1 child node            : 12289
Elements with more than 8 child nodes : 31
Elements with more than 8 attrs       : 0
Elements with more than 8 attrs+child : 38
Average attr+child count              : 3.9

This shows us that making mChildren and mAttributes nsAutoVoidArrays would not
cost us much at all in terms of unused buffers in the nsAutoVoidArray. Note, the
above data does not contain XUL elements, only HTML and XML (i.e. XBL).

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Note about jst's figures that child arrays don't actually allocate a buffer
(nsVoidArray or nsAutoVoidArray) unless they have >1 entry, due to
nsCheapVoidArray.  That's why he reaches his conclusion.

Attrs don't use nsCheapVoidArray I believe, so they're using 8 entries of space
as soon as you add the first entry, and most seem to have 1 to 8 entries for
attrs (11000 out of 13000).  CheapVoidArray for attrs (if 1 and 0 entries are
quite common) would be a win bloatwise over nsAutoVoidArray, and/or a smaller
AutoVoidArray.

Still, in any case, it looks like we should commit the rest of the original
patch that we held out before.  r/sr?

Comment 14

[Johnny Stenback (:jst)]

sr=jst

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: mChildren-only patch

Comment 16

[Chris Waterson]

Comment on attachment 51098 [details] [diff] [review]
mChildren-only patch

r=waterson

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 51098 [details] [diff] [review]
mChildren-only patch

sr=jst

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in