286916 - constify gHTMLElements

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Mats Palmgren (inactive)]

constify gHTMLElements.

This saves us from a ~9k "new" allocation and initialization at startup.
Possibly also allows the compiler to optimize uses of this table better?

Before:
   text    data     bss     dec     hex filename
 372775   19224    1320  393319   60067 parser/htmlparser/src/libhtmlpars.so
After:
   text    data     bss     dec     hex filename
 368527   28020    1320  397867   6122b parser/htmlparser/src/libhtmlpars.so

Configured with:
ac_add_options --disable-debug
ac_add_options --enable-optimize="-O2 -gstabs+"
ac_add_options --enable-perf-metrics

(I also removed some TAB and ^M characters in nsElementTable.cpp)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

Comment 3

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

Why have the extra indirection of Initialize as a macro when you could just
write things as arrays?

Comment 4

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

Comment on attachment 178014 [details] [diff] [review]
Patch rev. 1 (diff -w)

>-    gHTMLElements=new nsHTMLElement[eHTMLTag_userdefined+5];

>+    /* 5 dummy entries */
>+    Initialize(eHTMLTag_unknown, eHTMLTag_unknown, eHTMLTag_unknown, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, eHTMLTag_unknown, 0),
>+    Initialize(eHTMLTag_unknown, eHTMLTag_unknown, eHTMLTag_unknown, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, eHTMLTag_unknown, 0),
>+    Initialize(eHTMLTag_unknown, eHTMLTag_unknown, eHTMLTag_unknown, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, eHTMLTag_unknown, 0),
>+    Initialize(eHTMLTag_unknown, eHTMLTag_unknown, eHTMLTag_unknown, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, eHTMLTag_unknown, 0),
>+    Initialize(eHTMLTag_unknown, eHTMLTag_unknown, eHTMLTag_unknown, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, eHTMLTag_unknown, 0)

It looks like what the old code did was equivalent to four dummy entries.  That
said, is there any reason for them?  In the old code, it looks like the memory
was uninitialized.

Also, you should have an #ifdef DEBUG loop somewhere that asserts for each
index in gHTMLElements that gHTMLElements[index].mTagID == index, since that's
no longer enforced any other way.

Comment 5

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

(In reply to comment #4)
> Also, you should have an #ifdef DEBUG loop somewhere that asserts for each
> index in gHTMLElements that gHTMLElements[index].mTagID == index, since that's
> no longer enforced any other way.

Found it.  Never mind.

But you could also remove DeleteElementTable and rename InitializeElementTable
to CheckElementTable and make it entirely #ifdef DEBUG...

Comment 6

[Mats Palmgren (inactive)]

(In reply to comment #3)
> Why have the extra indirection of Initialize as a macro when you could just
> write things as arrays?

Right, I will do that instead.

(In reply to comment #4)
> That said, is there any reason for them? 

I can't think of any good reasons. A bad reason would be to account
for buggy code that had out-of-bounds accesses. But, as you say, those
entries were unitialized anyway so I think we should just remove them...

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

Removed the macro and the dummy entries.
Replaced InitializeElementTable with NS_DEBUG-only CheckElementTable.
Removed DeleteElementTable.

Comment 9

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

Comment on attachment 178018 [details] [diff] [review]
Patch rev. 2 (diff -w)

Note that, at least on Linux, this doesn't get all the benefits of const data
(since it contains pointers that depend on the address of the library), but
it's a good change.  (And if you want to try to fix that, do it after you check
this in.  Don't revise this patch for it.)  sr=dbaron

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 178018 [details] [diff] [review]
Patch rev. 2 (diff -w)

r=mrbkap.

Comment 11

[Mats Palmgren (inactive)]

Filed bug 286986 on the issue in comment 9.

Checked in 2005-03-20 16:45 PDT

->  FIXED