As describe in http://www.mozilla.org/performance/footprint-reduction-techniques.html - there is a giant switch statement which creates content nodes based on a tag. At the very least we can provide a table mapping from tag -> constructor and run a tight loop that looks through the table and creates the right object.

adding footprint, accepting, and adding priority

No need for a loop here, just access directly into the array, you're passed the index (the HTML tag enum). Even better...

nice, I will try that approach.

These bugs missed the 1.2 train. Moving these out to 1.3alpha

A while back I looked at doing what jst said. Ended up with this... it works. However, I wasnt happy with the kind of changes that I ended up doing in the parser. Never really found the time to figure out a good way to keep this maintainable while not adding unrelated stuff in nsHTMLTagList.h (Thought I should just dump it here since I am about to nuke my tree.)

that's really not bad - it would be nice to get a little more cleanup to fix up the edge cases, but I like it.

I'm surprised that you like it :-) I personally did not think that it was clean to have a parser header know about the |tag -> content class| mapping. Thats really content's own business. Right? But if you do like it ... > it would be nice to get a little more cleanup to fix up the edge cases Umm ... yes there is some cleanup that I could do (like make MakeContentObject not take the useless docshell parameter etc.) , but I think I got all the corner cases. Did you see anything specific functionality wise that I missed? In any case, at this stage, this is 1.5 material right? I'll post a cleaned up version at that time.

So, I finally found some time to refresh this patch. Here is an updated version. It also gets rid of an unused nsIDocShell* parameter in MakeContentObject(); The diff -u doesn't make it easy to see what MakeContentObject() ends up looking like, so pasting it here for easier reading. nsresult MakeContentObject(nsHTMLTag aNodeType, nsINodeInfo *aNodeInfo, nsIDOMHTMLFormElement* aForm, nsIHTMLContent** aResult, PRBool aInsideNoXXXTag, PRBool aFromParser) { if (aNodeType == eHTMLTag_form) { // the form was already created if (aForm) { return CallQueryInterface(aForm, aResult); } else { return NS_NewHTMLFormElement(aResult, aNodeInfo); } } nsresult rv; // The "creator" functions for input and select elements don't have // the usual prototype and hence are not in the creator callback // table if (aNodeType == eHTMLTag_input) { rv = NS_NewHTMLInputElement(aResult, aNodeInfo, aFromParser); if (!aInsideNoXXXTag) { SetForm(*aResult, aForm); } return rv; } if (aNodeType == eHTMLTag_select) { rv = NS_NewHTMLSelectElement(aResult, aNodeInfo, aFromParser); if (!aInsideNoXXXTag) { SetForm(*aResult, aForm); } return rv; } rv = sContentCreatorCallbacks[aNodeType](aResult, aNodeInfo); switch (aNodeType) { case eHTMLTag_button: case eHTMLTag_fieldset: case eHTMLTag_label: case eHTMLTag_legend: case eHTMLTag_object: case eHTMLTag_textarea: if (!aInsideNoXXXTag) { SetForm(*aResult, aForm); } } return rv; }

Comment on attachment 130909 [details] [diff] [review] Patch against trunk Dunno if alecf is doing reviews nowadays. peterv, could you please have a look at this?

Comment on attachment 130909 [details] [diff] [review] Patch against trunk Nice. >Index: htmlparser/public/nsHTMLTags.h >=================================================================== >+#define HTML_TAG(_tag, _classname) eHTMLTag_##_tag, > enum nsHTMLTag { > /* this enum must be first and must be zero */ > eHTMLTag_unknown = 0, > #include "nsHTMLTagList.h" > > /* The remaining enums are not for tags */ > eHTMLTag_text, eHTMLTag_whitespace, eHTMLTag_newline, > eHTMLTag_comment, eHTMLTag_entity, eHTMLTag_doctypeDecl, > eHTMLTag_markupDecl, eHTMLTag_instruction, > eHTMLTag_userdefined I think you should use an additional macro (HTML_OTHER?) for these. You can define it to nothing when not needed. >Index: content/html/document/src/nsHTMLContentSink.cpp >=================================================================== >+#define HTML_TAG(_tag, _classname) NS_NewHTML##_classname##Element, >+static const contentCreatorCallback sContentCreatorCallbacks[] = { >+ NS_NewHTMLUnknownElement, >+#include "nsHTMLTagList.h" >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLSpanElement, >+ NS_NewHTMLUnknownElement >+}; >+#undef HTML_TAG And then use HTML_OTHER to define these. > nsresult > MakeContentObject(nsHTMLTag aNodeType, nsINodeInfo *aNodeInfo, >- nsIDOMHTMLFormElement* aForm, nsIDocShell* aDocShell, >- nsIHTMLContent** aResult, PRBool aInsideNoXXXTag, >- PRBool aFromParser) >+ nsIDOMHTMLFormElement* aForm, nsIHTMLContent** aResult, >+ PRBool aInsideNoXXXTag, PRBool aFromParser) > { >+ if (aNodeType == eHTMLTag_form) { > // the form was already created > if (aForm) { >- rv = CallQueryInterface(aForm, aResult); >+ return CallQueryInterface(aForm, aResult); > } else { else-after-return

Taking.

Addresses peterv's comments

Comment on attachment 131901 [details] [diff] [review] v3 Peter, could you please have a look at the HTML_OTHER bits to make sure I correctly understood what you wanted.

Comment on attachment 131901 [details] [diff] [review] v3 >Index: htmlparser/public/nsHTMLTagList.h >=================================================================== >+/* XXX: The second parameters in some of the following entries look >+ like they are just wrong. They should really be NOTUSED. For now, >+ I'm just emulating nsHTMLContentSink has done all along. emulating what nsHTML...

Comment on attachment 131901 [details] [diff] [review] v3 Sweet! sr=jst

This busted a couple of tinderboxes. This is the first fix. A few of the functions that I was trying to use function pointers to were inlined. gcc2.91 cant deal with this.

gcc3.4 doesn't like extra commas at all.

Its best to get rid of these inlines since they are not used at all anymore.

Comment on attachment 132046 [details] [diff] [review] Additional patch that I think we should take Fine by me, let's make sure jst agrees.

so on the platforms that didn't bust, did we see any perf or mZ improvement?

Comment on attachment 132046 [details] [diff] [review] Additional patch that I think we should take Cool with me. sr=jst

alecf : No noticable change in Tp. mZ did drop a little bit (<1K).

Yeah, but also moved some stuff from code to data: Overall Change in Size Total: -588 (+1140/-1728) Code: -1132 (+576/-1708) Data: +544 (+564/-20)

Actually, now that I look at it more closely, this was not that straightforward. The numbers were quite interesting to me at least. For the initial checkin (that I think Peter quoted numbers for) codesighs on comet said the following: Overall Change in Size Total: -588 (+1140/-1728) Code: -1132 (+576/-1708) Data: +544 (+564/-20) libgklayout.so Total: -588 (+1140/-1728) Code: -1132 (+576/-1708) Data: +544 (+564/-20) +544 (+564/-20) D (DATA) +544 (+564/-20) UNDEF:libgklayout.so:D +512 sContentCreatorCallbacks +32 kWhitespace.4506 +12 kWhitespace.2812 +8 map.3547 -20 kWhitespace.4503 -1132 (+576/-1708) T (CODE) -1132 (+576/-1708) UNDEF:libgklayout.so:T +256 MakeContentObject(nsHTMLTag, nsINodeInfo *, nsIDOMHTMLFormElement *, nsIHTMLContent **, int, int) +44 NS_NewHTMLBaseElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLEmbedElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLIsIndexElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLParamElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLSpacerElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLTableColGroupElement(nsIHTMLContent **, nsINodeInfo *) +44 NS_NewHTMLWBRElement(nsIHTMLContent **, nsINodeInfo *) +12 NS_NewHTMLNOTUSEDElement(nsIHTMLContent **, nsINodeInfo *) -4 SinkContext::~SinkContext(void) -4 SinkContext::UpdateChildCounts(void) -4 SinkContext::IsAncestorContainer(nsHTMLTag) -4 SinkContext::End(void) -4 SinkContext::AddText(nsAString const &) -4 HTMLContentSink::CreateContentObject(nsIParserNode const &, nsHTMLTag, nsIDOMHTMLFormElement *, nsIDocShell *, nsIHTMLContent **) -8 NS_CreateHTMLElement(nsIHTMLContent **, nsINodeInfo *, int) -16 SetForm(nsIHTMLContent *, nsIDOMHTMLFormElement *) -1660 MakeContentObject(nsHTMLTag, nsINodeInfo *, nsIDOMHTMLFormElement *, nsIDocShell *, nsIHTMLContent **, int, int) Notice that the compiler started generating non-inlined versions of some of the the functions which were earlier inlined. (Which makes sense since this particular machine didn't go red.) My subsequent checkin to fix btek, had the side effect of causing this extra code to go away on comet. Overall Change in Size Total: -308 (+0/-308) Code: -308 (+0/-308) Data: +0 (+0/+0) libgklayout.so Total: -308 (+0/-308) Code: -308 (+0/-308) Data: +0 (+0/+0) -308 (+0/-308) T (CODE) -308 (+0/-308) UNDEF:libgklayout.so:T -44 NS_NewHTMLWBRElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLTableColGroupElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLSpacerElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLParamElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLIsIndexElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLEmbedElement(nsIHTMLContent **, nsINodeInfo *) -44 NS_NewHTMLBaseElement(nsIHTMLContent **, nsINodeInfo *) So, at the end of the day, on comet at least, it cumulatively ended up being: Total: -896 Code: -1440 Data: +544

additional patch checked in. fixed.