181383 - initialize layout atoms in a loop

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Alec Flett]

So this whole scheme we have with nsHTMLAtoms is cool, but its slightly bloaty.
The bloaty part is nsHTMLAtoms::AddRefAtoms, nsHTMLAtoms::ReleaseAtoms, which
seperately call AddRef and Release on each atom - when you add up the total size
of all the ns*Atoms::AddRef/ReleaseAtoms() methods, the code itself adds up to
somewhere around 50k... since this is in the "shared" static lib, this means
we're duplicating it, so we're talking somewhere just under 100k of total footprint.

We need to come up with some way of initializing this in a loop.. here's my
idea..  we create some dummy first atom, and declare the initialization strings
in a data structure like:

static const char* AtomValues[] = {

#define HTML_ATOM(_name, _value) _value,
#include "nsHTMLAtomList.h"
#undef HTML_ATOM
}

then we have a loop, something like:

nsIAtom** firstAtom = &nsHTMLAtoms::dummyFirstAtom;
firstAtom++; // advance past dummy atom
for (i=0; i<(sizeof(AtomValues)/sizeof(AtomValues[0])); i++) {
  *firstAtom++ = NS_NewAtom(AtomValues[i]);
} 

and then we can do something similar with ReleaseAtoms();

Comment 1

[Alec Flett]

I should also add that the worst offender is nsHTMLAtoms::ReleaseAtoms, which
alone compiles to about 30k.. after that we have nsXULAtoms::ReleaseAtoms, and
so forth. The other ugly thing about these giant unrolled loop is that we lose
all benefits that we would have seen from the cache.

Comment 2

[Brendan Eich [:brendan]]

Good thinking, C/C-pre-processor techniques rule!  Nit: maybe use *++atom in the
loop, not firstAtom++ (misnomer after the first ++).

/be

Comment 3

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

Maybe put the initialization / destruction functions in shared code, and perhaps
even have simple macros for implementing atom lists using those shared
functions?  (Note also that the nsCSS* atom lists have a membership testing
method as well, which is simply linear search, since the lists are short enough
that I think linear search is good enough.  After all, the list length is a
constant, so it's constant time. :-)

Comment 4

[Alec Flett]

so I ran into a little issue with this... I whipped up a patch like the example,
which pointed the first atom at nsHTMLAtoms::_baseHref, and initialized
everything in a loop.. the only problem was that in memory, they don't seem to
be lined up in the same order they are declared. This means that all the
nsHTMLAtoms::XXX get random actual atoms assigned to them, and this leads to
total chaos, of course.

So my next thought was to do something like:

nsIAtom* AtomArray[ATOM_COUNT];
for (i=0; i<AtomArray[ATOM_COUNT]; i++) {
    AtomArray[i] = NS_NewPermenantAtom(AtomValues[i]);
}

#define HTML_ATOM(_name, _value) nsHTMLAtoms::_name = AtomArray[i];
#include "nsHTMLAtomList.h"
#undef HTML_ATOM
}

Unfortunately, what this means is that we're still accessing each nsHTMLAtoms
thing individually. That's pretty annoying... and it probably means that the
code will still be fairly large.

So one thought I had to fix this was to make a global macro first:

#define NS_HTML_ATOM(_name) nsHTMLAtoms::_name

and do a giant search-and-replace in the tree

then, we change this so that the atoms are just stored in an array:

nsIAtom* nsHTMLAtoms[i];

and make a set of enums:

enum eHTMLAtom {
#define HTML_ATOM(_name, _value) eHTMLAtom_#_name,
};

and change the macro to:

#define NS_HTML_ATOM(_name) nsHTMLAtoms[eHTMLAtom_#_name];

that way NS_HTMLAtom(id) will still map to the ID attribute, and we still get
exact addressing for individual atoms. (because the compiler should be able to
calculate the value of nsHTMLAtoms[eHTMLAtom_id] at compile time)

that is quite a task though, so I'm pushing it off to mozilla1.3beta

Comment 5

[Alec Flett]

nominating some of my footprint/perf bugs for topembed

Comment 6

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

I just thought of a way to do this that should work, using pointer-to-member
values in a const table along with the strings.  I'm going to see if it works...

Comment 7

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

er, not pointer-to-member, just pointers, since they'er static members...

Comment 8

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

Attachment: patch

This patch reduces the size of libgklayout.so in my optimized Linux build (not
stripped) from 7403449 to 7382745.

Comment 9

[Alec Flett]

wow, this is great!
I just looked over the patch - I like the idea. I'll do a full review today, if
you think its ready.
One question I have is how the pointer-to-member table is generated once
compiled - I mean do you think it is a static table and the linker adjusts
everything at runtime? I guess I'm wondering if there's any trick to throwing
out the table once we're done using it. I'm guessing not..

Comment 10

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

Taking bug.

We're never done using the table (although really, with permanent atoms, we
could get rid of the ReleaseAtoms).  But any way that would allow us to get rid
of it would mean generating it at runtime, which would require a loop.

Comment 11

[Alec Flett]

Comment on attachment 113103 [details] [diff] [review]
patch

yeah, this looks great. Is it possible/helpful to have the sort of
double-constness that simon suggested for the i18n stuff?
(I'm guessing its not possible, since we're dealing with structs and non-const
nsIAtom*'s and so forth ..?)

Comment 12

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

Comment on attachment 113103 [details] [diff] [review]
patch

I don't think there are any more places where |const| is appropriate, since
it's not a |const nsIAtom|, and we need to modify the pointer, so it's not an
|nsIAtom *const|, and the nsAtomListInfo being const gives us the equivalent of
|nsIAtom **const| already.

Comment 13

[Simon Fraser [no longer active]]

This begs the question of why we refcount atoms at all, since, by definition,
they are singletons.
Looking at the patch.

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 113103 [details] [diff] [review]
patch

I don't really like the various methods that call nsAtomListUtils::IsMember().
nsCSSAnonBoxes::IsAnonBox() is never called.
nsCSSPseudoClasses::IsPseudoClass() is called from the nsCSSParser, but it
seems like their should be a better way to get this info than by a linear
search through the atoms. Same with nsCSSPseudoElements::IsPseudoElement().

nsAtomListUtils.cpp looks like it uses 4-space indentation despite what the
modeline says.

Otherwise, a good set of changes.

Comment 15

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

I could've sworn I made the modeline say 4, but I think I then deleted the file
and started over.  I'll fix that.

I'm likely to want to use those functions using IsMember at some point soon
(which is why I added them), and I don't think it's worth worrying about a
linear scan when the list is of known length.  After all, a linear scan through
a list that length probably takes less time than a hashtable lookup (and if
more, not much more).  I'll wait until I see it on a profile to worry about it.

Comment 16

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

Attachment: patch

With corrected modeline and copyright year.

Comment 17

[Michael Buckland]

Discussed in edt bug triage.  Plussing.

Comment 18

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

Fix checked in to trunk, 2003-02-22 07:57/58 PST, plus bustage fix (missing
|const|s) in nsAtomListUtils.cpp later.