Move GritResourceMap arrays (~12,500 bytes) to read-only data segment

Move GritResourceMap arrays (~12,500 bytes) to read-only data segment

Due to a quirk in VC++ it is easy to accidentally prevent a const global
array from being placed in the read-only data segment. This change
removes a should-be-harmless 'const' keyword to work around this quirk
and move ~12,500 bytes to the .rdata (read-only) data segment in
chrome.dll.

VC++ bug is filed here:
https://connect.microsoft.com/VisualStudio/feedback/details/3117602

Other instances of this quirk have also been worked around.

BUG=677351

[brucedawson, 2016-12-29 19:17:31]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 19:17:47]

Dry run: 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-29 19:23:38]

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

[commit-bot: I haz the power, 2016-12-29 19:23:39]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brucedawson, 2016-12-29 19:37:35]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 19:37:49]

Dry run: 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-29 19:48:20]

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

[commit-bot: I haz the power, 2016-12-29 19:48:20]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2016-12-29 20:00:52]

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

[brucedawson, 2016-12-29 20:00:53]

I thought you might find this interesting or amusing. It is odd that *removing*
const can cause VC++ to move more data to the .rdata (read-only) data segment.
This is one of the smaller gains (no code-size change) but still worthwhile I
think.

[brucedawson, 2016-12-31 21:49:59]

On 2016/12/29 20:00:53, brucedawson wrote:
> I thought you might find this interesting or amusing. It is odd that
*removing*
> const can cause VC++ to move more data to the .rdata (read-only) data segment.
> This is one of the smaller gains (no code-size change) but still worthwhile I
> think.

BTW, a couple of the more interesting gains are in
https://codereview.chromium.org/2607893002/ and
https://github.com/randomascii/compact_enc_det. Pretty decent savings actually.

[Nico, 2017-01-05 00:06:30]

won't that hurt other compilers? it seems super hacky to work around such a
basic compiler bug -- in how many places do we need this?

[brucedawson, 2017-01-05 01:07:48]

On 2017/01/05 00:06:30, Nico (ooo sick) wrote:
> won't that hurt other compilers? it seems super hacky to work around such a
> basic compiler bug -- in how many places do we need this?

It's pretty rare for removing const from a member variable to hurt code-gen. In
general it could hurt guarantees about the code except that every instance of
GritResourceMap is marked 'const' as a whole, so the 'const' markers on an
individual member variable are redundant in this case.

Of the last seven changes I've landed five have been about removing 'const' to
avoid this problem (I think they were all cases where the only usage of the
struct was to declare const objects) - the other two changes, amusingly enough,
were to *add* const to force global arrays into the read-only data segment.

This change is the last one I'm aware of, although I'll probably do a final
audit of the read/write data segment - much easier now that more than 1,500
globals have been removed from it!

I agree that is hacky, but I think it's harmless hacky. I predict Microsoft will
fix this for VC++ 2017 but I don't know if I would even bother restoring the
'const' markers after we switch to 2017.

[brucedawson, 2017-01-05 01:34:53]

Description was changed from

==========
Move GritResourceMap arrays to read-only data segment

Due to a quirk in VC++ it is easy to accidentally prevent a const global
array from being placed in the read-only data segment. This change
removes a should-be-harmless 'const' keyword to work around this quirk
and move ~12,500 bytes to the .rdata (read-only) data segment in
chrome.dll.

VC++ bug is filed here:
https://connect.microsoft.com/VisualStudio/feedback/details/3117602

Other instances of this quirk are also being worked around.

BUG=677351
==========

to

==========
Move GritResourceMap arrays to read-only data segment

Due to a quirk in VC++ it is easy to accidentally prevent a const global
array from being placed in the read-only data segment. This change
removes a should-be-harmless 'const' keyword to work around this quirk
and move ~12,500 bytes to the .rdata (read-only) data segment in
chrome.dll.

VC++ bug is filed here:
https://connect.microsoft.com/VisualStudio/feedback/details/3117602

Other instances of this quirk have also been worked around.

BUG=677351
==========

[brucedawson, 2017-01-05 23:08:21]

Description was changed from

==========
Move GritResourceMap arrays to read-only data segment

Due to a quirk in VC++ it is easy to accidentally prevent a const global
array from being placed in the read-only data segment. This change
removes a should-be-harmless 'const' keyword to work around this quirk
and move ~12,500 bytes to the .rdata (read-only) data segment in
chrome.dll.

VC++ bug is filed here:
https://connect.microsoft.com/VisualStudio/feedback/details/3117602

Other instances of this quirk have also been worked around.

BUG=677351
==========

to

==========
Move GritResourceMap arrays (~12,500 bytes) to read-only data segment

Due to a quirk in VC++ it is easy to accidentally prevent a const global
array from being placed in the read-only data segment. This change
removes a should-be-harmless 'const' keyword to work around this quirk
and move ~12,500 bytes to the .rdata (read-only) data segment in
chrome.dll.

VC++ bug is filed here:
https://connect.microsoft.com/VisualStudio/feedback/details/3117602

Other instances of this quirk have also been worked around.

BUG=677351
==========

[brucedawson, 2017-01-14 00:10:25]

I looked at this again to see if there were any better solutions. A fix that
worked elsewhere was adding a constexpr constructor to the GritResourceMap type,
but that did not help here.

The ideal solution would be to mark all of the arrays as constexpr, but VC++
2015 does not support making constexpr objects visible outside of the
translation unit where they are defined. Pity. This should be fixed in VC++
2017.

So, our options are:
- Land it as is - because the arrays are all const this is safe
- Make the const conditional using an ifdef
- Close the issue - it only affects ~11 arrays totaling ~12.5 KB

The problem goes away anyway when we move off of VC++ 2015 so I'm not too
worried either way.