Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6)

Issue 2512753002: Move const PortName data out of header file (Closed)

Created:
4 years, 1 month ago by brucedawson
Modified:
4 years ago
Reviewers:
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move const PortName data out of header file When non-integral const data is defined in a header file it often ends up being instantiated in multiple translation units. It also ends up in the read/write data segment which means it isn't shared between processes. This change has the following affect on the size of sections in a 32-bit release Windows build: chrome.dll .text: -128 bytes change .rdata: 64 bytes change .data: -800 bytes change .reloc: -12 bytes change Total change: -876 bytes chrome_child.dll .text: 64 bytes change .rdata: 48 bytes change .data: -800 bytes change .reloc: -68 bytes change Total change: -756 bytes Note that the sections that increase in size are shareable. The technique of using static constexpr in a struct is the only way to guarantee both zero duplication of data and no run-time construction of data (until C++17 and inline variables). BUG=630755

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -26 lines) Patch
M mojo/edk/system/core.cc View 1 chunk +1 line, -1 line 0 comments Download
M mojo/edk/system/node_controller.cc View 7 chunks +10 lines, -7 lines 0 comments Download
M mojo/edk/system/ports/name.h View 2 chunks +9 lines, -8 lines 0 comments Download
M mojo/edk/system/ports/name.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M mojo/edk/system/ports/node.cc View 5 chunks +9 lines, -9 lines 0 comments Download
M mojo/edk/system/ports_message.h View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5 (4 generated)
brucedawson
4 years ago (2016-11-21 23:12:40 UTC) #5
On 2016/11/17 20:29:13, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

crrev.com/2515483002 is a simpler fix for this issue. It is theoretically not as
good but in practice seems to work as well or better. Closing this CL.

Powered by Google App Engine
This is Rietveld 408576698