MakeQualifiedNames should generate lazily created arrays for attributes and tags.

MakeQualifiedNames should generate lazily created arrays for attributes and tags.

The templates MakeQualifiedNames.cpp.tmpl and MakeQualifiedNames.h.tmpl currently generate
local static arrays for the tags and the attributes.

Currently these arrays hold pointers to the global data and these arrays are declared as
static. Due to this static declaration, the sections .rel.dyn and .data.rel.ro will
allocate the bytes equal to the size of the array.

Even though these functions may not be called upon, the binary would still be loaded with
allocation of that much amount of data.

Making these arrays as non-static and allocating them lazily, we are able to
save about 4.2KB size in these sections.

* With the patch, we save - 
    = 2880 bytes in .rel.dyn 
    = 1440 bytes in .data.rel.ro

* Total size increases by -
    = 384 bytes

Using the tool relinfo.pl from [1] we get:
* About 360 relocations are avoided by the patch.
* About 360 relatives are avoided by the patch.

[1] http://www.akkadia.org/drepper/relinfo.pl

BUG=249746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165316

[vivekg, 2014-01-15 12:29:01]

The size data per section is shown in [1]. This data was taken for
ChromiumTestShellTestApk target with the size taken for libchromiumtestshell.so.

In the gain/loss columns, the rel.dyn and data.rel.ro constitues around ~4.2kb
of data together.

Please take a look, thank you.

[1] https://gist.github.com/vivekgalatage/8435324

[Inactive, 2014-01-15 15:19:54]

Very similar to my patch at:
https://codereview.chromium.org/51983003/

Could you please measure the impact on the number of relocations using relinfo?
(http://www.akkadia.org/drepper/relinfo.pl)

It would be good to provide this information in the Changelog.

[Inactive, 2014-01-15 15:27:14]

On 2014/01/15 15:19:54, Chris Dumez wrote:
> Very similar to my patch at:
> https://codereview.chromium.org/51983003/
> 
> Could you please measure the impact on the number of relocations using
relinfo?
> (http://www.akkadia.org/drepper/relinfo.pl)
> 
> It would be good to provide this information in the Changelog.

Also, please update the Changelog to indicate the impact on the overall binary
size since this was an issue in my original patch.

[vivekg, 2014-01-15 15:30:35]

On 2014/01/15 15:19:54, Chris Dumez wrote:
> Very similar to my patch at:
> https://codereview.chromium.org/51983003/
>

Wow, thats a sweet coincidence. Similar minds may be! :)

> Could you please measure the impact on the number of relocations using
relinfo?
> (http://www.akkadia.org/drepper/relinfo.pl)
> 
> It would be good to provide this information in the Changelog.

Would provide these details. Thanks!

[Inactive, 2014-01-15 19:27:02]

https://codereview.chromium.org/137783012/diff/1/Source/build/scripts/templat...
File Source/build/scripts/templates/MakeQualifiedNames.h.tmpl (right):

https://codereview.chromium.org/137783012/diff/1/Source/build/scripts/templat...
Source/build/scripts/templates/MakeQualifiedNames.h.tmpl:30: void
get{{namespace}}Tags(WebCore::QualifiedName* tags[], unsigned length);
Passing the length here is a bit odd. Maybe, we can keep
"get{{namespace}}Tags()" as prototype and return a PassOwnPtr<const
QualifiedName*[]> ?

https://codereview.chromium.org/137783012/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/137783012/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLTreeBuilder.cpp:589: WebCore::QualifiedName*
attrs[length];
You probably don't need the WebCore:: here

https://codereview.chromium.org/137783012/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLTreeBuilder.cpp:629: WebCore::QualifiedName*
attrs[XLinkNames::XLinkAttrsCount];
Ditto.

https://codereview.chromium.org/137783012/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLTreeBuilder.cpp:633: WebCore::QualifiedName*
xmlAttrs[XMLNames::XMLAttrsCount];
Ditto.

[vivekg_samsung, 2014-01-16 10:27:21]

Updated the patch.

I am getting around 360 relocations saved with this patch. Using either array
based approach in #1 or #2 with using PassOwnPtr we are getting the same number
of relocations saved.

Another reason previously for you getting ~800 relocations saved could be due to
the existence of HTMLIdentifier class. This class has been removed off on the
latest ToT and hence we may be getting somewhere around ~360 relocations.

[Inactive, 2014-01-16 14:37:46]

Also, please update the Changelog to indicate the overall impact on binary size.

There is also a s/stil/still typo in there.

https://codereview.chromium.org/137783012/diff/180001/Source/build/scripts/te...
File Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl (right):

https://codereview.chromium.org/137783012/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:33: const
QualifiedName** tags = new const QualifiedName*[{{namespace}}TagsCount];
We usually adopt as soon as we allocate.

https://codereview.chromium.org/137783012/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:37: return
adoptArrayPtr(tags);
Then release() when returning.

https://codereview.chromium.org/137783012/diff/180001/Source/build/scripts/te...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:49: const
QualifiedName** attrs = new const QualifiedName*[{{namespace}}AttrsCount];
Ditto.

[vivekg, 2014-01-16 22:52:17]

Thanks Chris for the review. Incorporated all the changes and updated the patch.
PTAL.

[Inactive, 2014-01-17 13:57:39]

lgtm:
- The impact on the overall binary size is minimal
- It moves data out of .data.rel.ro segment
- It reduces the number of relocations quite a bit
- We don't really need static arrays here as these methods are called only once
on startup.

[commit-bot: I haz the power, 2014-01-17 14:46:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/137783012/250001

[commit-bot: I haz the power, 2014-01-17 15:52:43]

Change committed as 165316