Stop using static arrays for *Tags / *Attrs

Stop using static arrays for *Tags / *Attrs

Stop using static arrays for *Tags / *Attrs and construct a Vector dynamically
instead. We don't need these static arrays because the callers of get*Attrs() /
get*Tags() already make sure those functions are called only once.

The objective is to decrease memory usage and enable faster loading of the
library on Android by getting rid of relocations. This reduces the number of
relocations by 834 according to relinfo.

BUG=249746

[Inactive, 2013-10-30 18:35:36]

For reference, here are the diffs for the generated HTMLNames.*:
http://pastebin.com/1C6HtTVS
http://pastebin.com/d3rKg253

https://codereview.chromium.org/51983003/diff/40001/Source/build/scripts/temp...
File Source/build/scripts/templates/MakeQualifiedNames.h.tmpl (right):

https://codereview.chromium.org/51983003/diff/40001/Source/build/scripts/temp...
Source/build/scripts/templates/MakeQualifiedNames.h.tmpl:30: void
get{{namespace}}Tags(Vector<const WebCore::QualifiedName*>& tags);
iirc, we decided on the mailing list to not rely on NRVO until we actually
switch to C++11, which is why I am passing the vector in argument.

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/HTMLObje...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/HTMLObje...
Source/core/html/HTMLObjectElement.cpp:393: static bool
isRecognizedTagName(const QualifiedName& tagName)
I am getting rid of this method in:
https://codereview.chromium.org/50453004/

Once this is gone, HTMLNames::getHTMLTags() will only be called in one place
(html/parser/HTMLIdentifier.cpp).

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLIdentifier.cpp (left):

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/parser/H...
Source/core/html/parser/HTMLIdentifier.cpp:79: const unsigned
kHTMLNamesIndexOffset = 0;
These constants were passed to HTMLIdentifier::addNames() but not used by that
method.

[abarth-chromium, 2013-10-30 19:26:51]

On 2013/10/30 18:35:36, Chris Dumez wrote:
> For reference, here are the diffs for the generated HTMLNames.*:
> http://pastebin.com/1C6HtTVS
> http://pastebin.com/d3rKg253

Doesn't this just move the data into the text segment?  Won't the references in
the text segment need to be relocated just the same?  /me doesn't really
understand this stuff is mostly guessing.

[abarth-chromium, 2013-10-30 19:30:11]

This CL looks good assuming this change is actually a savings.  Can you look at
the change in binary size?

If @digit is convinced this is a savings, I'm happy to approve this CL.  I'm
mostly ignorant here and would like to learn.

Thanks!

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/HTMLObje...
File Source/core/html/HTMLObjectElement.cpp (right):

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/HTMLObje...
Source/core/html/HTMLObjectElement.cpp:393: static bool
isRecognizedTagName(const QualifiedName& tagName)
On 2013/10/30 18:35:36, Chris Dumez wrote:
> I am getting rid of this method in:
> https://codereview.chromium.org/50453004/
> 
> Once this is gone, HTMLNames::getHTMLTags() will only be called in one place
> (html/parser/HTMLIdentifier.cpp).

Great.

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLIdentifier.cpp (left):

https://codereview.chromium.org/51983003/diff/40001/Source/core/html/parser/H...
Source/core/html/parser/HTMLIdentifier.cpp:79: const unsigned
kHTMLNamesIndexOffset = 0;
On 2013/10/30 18:35:36, Chris Dumez wrote:
> These constants were passed to HTMLIdentifier::addNames() but not used by that
> method.

Yeah, this was old tech before we had thread-safe strings.  Thanks for the
cleanup.

[Inactive, 2013-10-30 19:31:16]

On 2013/10/30 19:26:51, abarth wrote:
> On 2013/10/30 18:35:36, Chris Dumez wrote:
> > For reference, here are the diffs for the generated HTMLNames.*:
> > http://pastebin.com/1C6HtTVS
> > http://pastebin.com/d3rKg253
> 
> Doesn't this just move the data into the text segment?  Won't the references
in
> the text segment need to be relocated just the same?  /me doesn't really
> understand this stuff is mostly guessing.

I don't see it ending up in the .text segment.

Before:
$ objdump -x out/Release/lib/libchromeview_prebuilt.so.bak | grep HTMLAttrs |
c++filt
022ef9f4 l     O .data	00000548 WebCore::HTMLNames::getHTMLAttrs()::HTMLAttr
01176f21 l     F .text	0000000c .hidden WebCore::HTMLNames::getHTMLAttrs()

After:
$ objdump -x out/Release/lib/libchromeview_prebuilt.so | grep HTMLAttrs |
c++filt
01179f29 l     F .text	000014e8 .hidden
WebCore::HTMLNames::getHTMLAttrs(WTF::Vector<WebCore::QualifiedName*, 0u>&)

Also, as explained in the CL description, I confirmed with relinfo that 834
relocations are gone.

[abarth-chromium, 2013-10-30 19:34:13]

Ok.  LGTM, but I'm not super confident here.  Ideally we'd get digit to comment.

[Inactive, 2013-10-30 19:38:36]

On 2013/10/30 19:34:13, abarth wrote:
> Ok.  LGTM, but I'm not super confident here.  Ideally we'd get digit to
comment.

Yes, I would also like digit to confirm. I had this idea when he said we should
avoid using static variables in Blink as much as possible.

I checked the .so size as you suggested and it is actually ~75k bigger :/

[Inactive, 2013-10-30 20:16:45]

On 2013/10/30 19:38:36, Chris Dumez wrote:
> On 2013/10/30 19:34:13, abarth wrote:
> > Ok.  LGTM, but I'm not super confident here.  Ideally we'd get digit to
> comment.
> 
> Yes, I would also like digit to confirm. I had this idea when he said we
should
> avoid using static variables in Blink as much as possible.
> 
> I checked the .so size as you suggested and it is actually ~75k bigger :/

Ok, so I confimed using the following command that I moved data from .data to
.text:
size -t -A
out/Release/obj/third_party/WebKit/Source/core/gen/blink/webcore_derived.HTMLNames.o

Diff before / after:
http://pastebin.com/wTQDX5GX

My understanding is that while my change increases a bit the binary size (+75k),
it moves data to .text segment which is readonly and shareable. It also gets rid
of a lot of relocations which helps with library load time.

Let's wait for digit's feedback on this as it seems to be a trade-off situation.

[Inactive, 2013-11-01 15:38:26]

digit@, any opinion on this change?

[digit1, 2013-11-08 16:30:16]

Please let me have a look at the generated binaries.

I think the change is good, but I need confirmation from the compiler to be 100%
sure :) Assuming this is not part of a performance-sensitive loop, of course.

https://codereview.chromium.org/51983003/diff/210001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl (right):

https://codereview.chromium.org/51983003/diff/210001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:31: void
get{{namespace}}Tags(Vector<const WebCore::QualifiedName*>& tags)
The Chromium style guide forbids non-constant references. I don't see anything
related to this topic on http://www.chromium.org/blink/coding-style, so maybe
avoid them here too?

I.e. 'use a Vector<const WebCode::QualifiedName*>* tags' parameter and adjust
callers accordingly.

It may be that the current style is completely ok within Blink, I don't know
better :)

[abarth-chromium, 2013-11-08 18:07:57]

On 2013/11/08 16:30:16, digit1 wrote:
> I think the change is good, but I need confirmation from the compiler to be
100%
> sure :) Assuming this is not part of a performance-sensitive loop, of course.

It's just executed once at startup.

>
https://codereview.chromium.org/51983003/diff/210001/Source/build/scripts/tem...
> File Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/51983003/diff/210001/Source/build/scripts/tem...
> Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:31: void
> get{{namespace}}Tags(Vector<const WebCore::QualifiedName*>& tags)
> The Chromium style guide forbids non-constant references. I don't see anything
> related to this topic on http://www.chromium.org/blink/coding-style, so maybe
> avoid them here too?
> 
> I.e. 'use a Vector<const WebCode::QualifiedName*>* tags' parameter and adjust
> callers accordingly.
> 
> It may be that the current style is completely ok within Blink, I don't know
> better :)

We tend to use non-const references in Blink.  We might want to change that
sometime, but that seems sort of out-of-scope for this CL.