Size reduction from moving const char arrays out of header

Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd
Cr-Commit-Position: refs/heads/master@{#436499}

[brucedawson, 2016-11-22 22:26:34]

Description was changed from

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

BUG=630755
==========

to

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brucedawson, 2016-11-22 22:33:07]

brucedawson@chromium.org changed reviewers:
+ thakis@chromium.org

[brucedawson, 2016-11-22 22:33:07]

I'm curious about your take on this. I noticed that some of these character
arrays (kCategoryFrame in particular) were being included in chrome.dll and
chrome_child.dll multiple times, thus wasting space. So I applied the simplest
fix. No problem.

However the actual gains are extremely fragile and depend on what source file I
add the definitions to. A little bit of code-size increase seems possible,
depending on where the arrays end up relative to the code (maybe?) but it still
seems odd.

My best guess is that when the arrays are declared in the header file the
compiler places some of them adjacent to the code which allows for more
efficient addressing modes. When the arrays are declared in the .cc file they go
in the .rdata segment which makes for less efficient code, in some cases. Or
something like that.

Anyway, not really asking for a code review, just any thoughts.

[Nico, 2016-12-02 01:30:19]

On 2016/11/22 22:33:07, brucedawson wrote:
> I'm curious about your take on this. I noticed that some of these character
> arrays (kCategoryFrame in particular) were being included in chrome.dll and
> chrome_child.dll multiple times, thus wasting space. So I applied the simplest
> fix. No problem.
> 
> However the actual gains are extremely fragile and depend on what source file
I
> add the definitions to. A little bit of code-size increase seems possible,
> depending on where the arrays end up relative to the code (maybe?) but it
still
> seems odd.
> 
> My best guess is that when the arrays are declared in the header file the
> compiler places some of them adjacent to the code which allows for more
> efficient addressing modes. When the arrays are declared in the .cc file they
go
> in the .rdata segment which makes for less efficient code, in some cases. Or
> something like that.

That's what I'd hand-wave too :-) If the const is closer, you can use a small
lea relative to rip or something. I guess you could check if some of the strings
show up amidst code to verify if this happens?

In any case, this seems like a good change for cleanliness, so lgtm if you want
to land it.

[brucedawson, 2016-12-05 01:07:21]

Description was changed from

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brucedawson, 2016-12-05 01:08:39]

brucedawson@chromium.org changed reviewers:
+ aelias@chromium.org

[brucedawson, 2016-12-05 01:08:39]

aelias, WDYT?

The definitions could be put elsewhere if we want to avoid the cost of adding
yet-another-source-file.

[aelias_OOO_until_Jul13, 2016-12-05 22:42:08]

lgtm

[brucedawson, 2016-12-05 22:51:59]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2016-12-05 22:52:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 01:14:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 01:14:01]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[brucedawson, 2016-12-06 01:30:06]

Broken test - it failed with and without the patch. Retrying.

[brucedawson, 2016-12-06 01:30:09]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2016-12-06 01:30:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 02:48:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480987809660150,
"parent_rev": "0fa651c95b5836f3b5a3953f4017dd471ad3e175", "commit_rev":
"6abb3f5e04d0dd941eaaa21c244cd94921f867e2"}

[commit-bot: I haz the power, 2016-12-06 02:49:19]

Description was changed from

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2016-12-06 02:49:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-06 02:53:58]

Description was changed from

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG=630755
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd
Cr-Commit-Position: refs/heads/master@{#436499}
==========

[commit-bot: I haz the power, 2016-12-06 02:53:59]

Patchset 2 (id:??) landed as
https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd
Cr-Commit-Position: refs/heads/master@{#436499}