Script make_names.pl should generate lazily created arrays for attributes and tags.

• Component: DOM
• Status: RESOLVED WONTFIX

[Vivek Galatage]

Currently make_names.pl generate local static arrays for the tags and the attributes. 

These arrays hold pointers to the global data and these arrays are declared as 
static.

As the pointer data itself is global, the array need not be declared as static. We could create it lazily.

Also even though these functions, get{{namespace}}Tags/Attrs(), may not be called upon, the binary would still be loaded with 
allocation of that much amount of static data. 

Blink review URL: https://codereview.chromium.org/137783012/

[Vivek Galatage]

Created attachment 221455 [details]
Patch

[Darin Adler]

Comment on attachment 221455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221455&action=review

> Source/WebCore/ChangeLog:17
> +        As the pointer data itself is global, the array need not be declared as static. 
> +        We could create it lazily.
> +
> +        Also even though these functions, get{{namespace}}Tags/Attrs(), may not be called upon,
> +        the binary would still be loaded with allocation of that much amount of static data. 

Doesn’t seem like a good tradeoff to me. In most systems, there is no significant cost to having static data that is never accessed, due to virtual memory and the OS. The code to build this structure seems like it will be significantly bigger than the data object.

Was there some specific case where this savings proved worthwhile? Could you give some numbers for the improvement?

> Source/WebCore/dom/make_names.pl:704
> +    printHeaderHead($F, "DOM", $parameters{namespace}, "#include \"QualifiedName.h\"\n#include <wtf/PassOwnPtr.h>\n");

This is wrong. Code doesn’t use PassOwnPtr.

[Vivek Galatage]

(In reply to comment #2)
> (From update of attachment 221455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221455&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        As the pointer data itself is global, the array need not be declared as static. 
> > +        We could create it lazily.
> > +
> > +        Also even though these functions, get{{namespace}}Tags/Attrs(), may not be called upon,
> > +        the binary would still be loaded with allocation of that much amount of static data. 
> 
> Doesn’t seem like a good tradeoff to me. In most systems, there is no significant cost to having static data that is never accessed, due to virtual memory and the OS. The code to build this structure seems like it will be significantly bigger than the data object.
> 
> Was there some specific case where this savings proved worthwhile? Could you give some numbers for the improvement?

I haven't calculated it specifically on Mac but did some measurements on linux and android using blink. Here is the link to the data saving table [1]. It saved us about 4.2kb from .data.rel.ro and .rel.dyn sections. Also saved about 360 relocations. The patch url also has array based approach [2]

[1] https://code.google.com/p/chromium/issues/detail?id=249746#c21
[2] https://codereview.chromium.org/137783012/#ps1

> 
> > Source/WebCore/dom/make_names.pl:704
> > +    printHeaderHead($F, "DOM", $parameters{namespace}, "#include \"QualifiedName.h\"\n#include <wtf/PassOwnPtr.h>\n");
> 
> This is wrong. Code doesn’t use PassOwnPtr.

Yeah this is not required.

[Darin Adler]

(In reply to comment #3)
> Here is the link to the data saving table [1]. It saved us about 4.2kb from .data.rel.ro and .rel.dyn sections. Also saved about 360 relocations.

How much did it add to the text section?

[Vivek Galatage]

(In reply to comment #4)
> (In reply to comment #3)
> > Here is the link to the data saving table [1]. It saved us about 4.2kb from .data.rel.ro and .rel.dyn sections. Also saved about 360 relocations.
> 
> How much did it add to the text section?

These arethe breakups

Section         Before      After        Diff

.text           24008156    24012876     +4720 (increased)
.data.rel.ro      995480      994040     -1440 (decreased)
.rel.dyn         1991888     1989008     -2880 (decreased)
                                         ------
                                          +400

So around 400 bytes were increased in the text section.

[Vivek Galatage]

+--------------------+--------------------------+
|                    |    Relocation Details    |
+   Detail           +------------+-------------+
|                    |    Before  |      After  |
+--------------------+------------+-------------+
| relocations        |     248986 |      248626 |
| relative (99%)     |     248935 |      248575 |
| PLT entries        |        338 |         338 |
| for local syms     |        338 |         338 |
+--------------------+------------+-------------+

[Darin Adler]

My thinking would be to leave this as-is. Replacing read-only data with code of the same size doesn’t seem like a win to me. Replacing process-load relocation time with “lazy creation” code at runtime might be a win, but a small one. If we’re really concerned about the cost of these relocations, maybe we can change the data structure so there are fewer pointers involved?

[Darin Adler]

But mine is not the only opinion that matters. I’d love to hear from other WebKit experts on this.

[Darin Adler]

(In reply to comment #7)
> Replacing read-only data with code of the same size doesn’t seem like a win to me.

I should have said “replacing read-only data with code that is slightly larger than that data”.

[Ahmad Saleem]

[e-mail] - Is it something needed anymore?

Webkit Source - https://github.com/WebKit/WebKit/blob/5ddb4127ac4e4a104338422e546a1cdc7b698769/Source/WebCore/dom/make_names.pl

[Ryosuke Niwa]

This is won't fix at this point.