Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Created:
2 years, 6 months ago by brucedawson
Modified:
2 years, 5 months ago
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
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: 144 bytes change .rdata: -32 bytes change .data: -736 bytes change .reloc: 140 bytes change Total change: -484 bytes chrome_child.dll .text: 32 bytes change .rdata: -112 bytes change .data: -736 bytes change .reloc: -84 bytes change Total change: -900 bytes Note that the sections that increase in size are either shareable (.text) or discardable (.reloc). I wish I could use "extern constexpr" to ensure that no constructors are run, but C++ does not allow that. inline constexpr is not yet supported, and making the variables a static constexpr member of a class (see crrev.com/2512753002) seems like overkill and doesn't seem to help. BUG=630755 Committed: https://crrev.com/facf44181190ded72c43ce0813ccd0aae56d78f2 Cr-Commit-Position: refs/heads/master@{#433727}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -2 lines) Patch
M mojo/edk/system/ports/name.h View 1 chunk +2 lines, -2 lines 0 comments Download
M mojo/edk/system/ports/name.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (10 generated)
brucedawson
Minor optimization - PTAL
2 years, 6 months ago (2016-11-17 18:14:31 UTC) #4
Ken Rockot(use gerrit already)
LGTM, thanks!
2 years, 6 months ago (2016-11-17 18:15:32 UTC) #5
brucedawson
On 2016/11/17 18:15:32, Ken Rockot wrote: > LGTM, thanks! crrev.com/2512753002 is a theoretically better solution, ...
2 years, 5 months ago (2016-11-21 23:13:40 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2515483002/1
2 years, 5 months ago (2016-11-21 23:14:58 UTC) #11
commit-bot: I haz the power
Committed patchset #1 (id:1)
2 years, 5 months ago (2016-11-22 01:15:03 UTC) #14
commit-bot: I haz the power
2 years, 5 months ago (2016-11-22 01:21:17 UTC) #16
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/facf44181190ded72c43ce0813ccd0aae56d78f2
Cr-Commit-Position: refs/heads/master@{#433727}

Powered by Google App Engine
This is Rietveld 408576698