286986 - Improve gHTMLElements further

Status: RESOLVED WONTFIX

(Core :: DOM: HTML Parser, enhancement)

Description

[Mats Palmgren (inactive)]

Followup from bug 286916 comment 9:
> 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)

Comment 1

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

This isn't necessarily something we'd even want to fix, although maybe it is.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -uw)

Overview of changes:
  1. changed the "tag list" pointers in the element table to an enum
     that is used to index an array of tag sets. (I changed the type
     name from 'TagList' to 'nsTagSet'). The element table is now completely
     literal and is placed in the text segment.
  2. Removed unused tag sets: 'AParents', 'InAddress', 'InTR', 'BodyParents',
     'LegendParents' and 'CaptionExcludableParents'.
  3. Removed 'mCanBeContained' field of nsHTMLElement - there was only one
     use ('nsHTMLElement::CanBeContained()') and only one element using it
(<LI>)
     and we can explicitly check for 'eHTMLTag_li' instead.
     (BTW, 'nsHTMLElement::CanBeContained()' does not seem to be used and
      should be removed - I'll file that separately).
  4. Moved 'CheckElementTable()' (added in bug 286916) from global
     namespace into 'nsHTMLElement::CheckElementTable()' instead.

and a few other minor cleanups as well...

BEFORE:

# size parser/htmlparser/src/libhtmlpars.so
   text    data     bss     dec     hex filename
 368463   27668    1320  397451   6108b parser/htmlparser/src/libhtmlpars.so

# objdump -x parser/htmlparser/src/libhtmlpars.so |grep HTMLElements
0005a180 l     O .data	000020e8	      gHTMLElements

AFTER:

# size parser/htmlparser/src/libhtmlpars.so
   text    data     bss     dec     hex filename
 374135   19100    1320  394555   6053b parser/htmlparser/src/libhtmlpars.so

# objdump -x parser/htmlparser/src/libhtmlpars.so |grep HTMLElements
00051a40 l     O .rodata	00001f14	     
_ZN13nsHTMLElement13sHTMLElementsE

The performance seems to be the same as before, using TestParser with
ac_add_options --disable-debug
ac_add_options --enable-optimize="-O2 -gstabs+"
ac_add_options --enable-perf-metrics

Comment 4

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

FWIW, regarding comment 0 -- what this does is move the extra work for being not
completely |const| from library load time to data use time (since you're now
getting two global variables (in a position-independent way) instead of one). 
I'm not sure whether that's a good thing or not.

Comment 5

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

Also, you didn't include the new file in the patch -- do a "cvs add" of the file
and then add "-N" to the options you use for diff.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

> Also, you didn't include the new file in the patch

Sorry, here's a complete patch.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -uw)

Comment 8

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

So I guess (given comment 4) I'm actually unsure whether I'd consider this an
improvement at all, since I think this is slower at runtime.

Comment 9

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

Is the amount of static data now smaller, i.e., were there many lists that were
common that are now condensed?

Comment 10

[Mats Palmgren (inactive)]

This old parser enhancement is obsolete now.