Closed Bug 286916 Opened 20 years ago Closed 20 years ago

constify gHTMLElements

Categories

(Core :: DOM: HTML Parser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

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)
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
Why have the extra indirection of Initialize as a macro when you could just
write things as arrays?
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.
(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...
(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...
Attached patch Patch rev. 2Splinter Review
Attachment #178013 - Attachment is obsolete: true
Attachment #178014 - Attachment is obsolete: true
Removed the macro and the dummy entries.
Replaced InitializeElementTable with NS_DEBUG-only CheckElementTable.
Removed DeleteElementTable.
Attachment #178018 - Flags: superreview?(dbaron)
Attachment #178018 - Flags: review?(mrbkap)
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
Attachment #178018 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 178018 [details] [diff] [review]
Patch rev. 2 (diff -w)

r=mrbkap.
Attachment #178018 - Flags: review?(mrbkap) → review+
Filed bug 286986 on the issue in comment 9.

Checked in 2005-03-20 16:45 PDT

->  FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: