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

Issue 2509403002: Move const double data out of header file (Closed)

Created:
4 years, 1 month ago by brucedawson
Modified:
4 years, 1 month ago
Reviewers:
danakj, Nico, jbroman, dcheng, fgorski
CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move const double 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: -704 bytes change .rdata: 80 bytes change .data: -128 bytes change .reloc: 204 bytes change Total change: -548 bytes Note that the sections that increase in size are either shareable (.rdata) or discardable (.reloc). 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 Committed: https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc Cr-Commit-Position: refs/heads/master@{#433015}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -19 lines) Patch
M components/offline_pages/offline_page_storage_manager.h View 1 chunk +13 lines, -8 lines 0 comments Download
M components/offline_pages/offline_page_storage_manager.cc View 4 chunks +11 lines, -4 lines 2 comments Download
M components/offline_pages/offline_page_storage_manager_unittest.cc View 5 chunks +11 lines, -7 lines 0 comments Download

Messages

Total messages: 26 (9 generated)
brucedawson
This is the theoretically optimal solution, and it does indeed shrink things a bit more. ...
4 years, 1 month ago (2016-11-17 18:41:44 UTC) #4
fgorski
If chromium does not use anything like that yet, I'd run that through Nico or ...
4 years, 1 month ago (2016-11-17 18:46:42 UTC) #5
brucedawson
On 2016/11/17 18:46:42, fgorski wrote: > If chromium does not use anything like that yet, ...
4 years, 1 month ago (2016-11-17 18:51:17 UTC) #6
brucedawson
thakis@/dcheng@, can we get your thoughts on this CL? It is the canonical C++ 11/14 ...
4 years, 1 month ago (2016-11-17 18:55:57 UTC) #8
dcheng
It seems reasonable to me, but I think the C++11 styleguide OWNERS should be the ...
4 years, 1 month ago (2016-11-17 19:01:59 UTC) #10
danakj
static constexpr stuff can still have the same ODR problems that static const stuff can, ...
4 years, 1 month ago (2016-11-17 19:09:03 UTC) #11
dcheng
On 2016/11/17 19:09:03, danakj wrote: > static constexpr stuff can still have the same ODR ...
4 years, 1 month ago (2016-11-17 19:10:23 UTC) #12
danakj
Oh.. why the struct then, is this invoking some C++ magic?
4 years, 1 month ago (2016-11-17 19:12:01 UTC) #13
brucedawson
On 2016/11/17 19:12:01, danakj wrote: > Oh.. why the struct then, is this invoking some ...
4 years, 1 month ago (2016-11-17 19:16:27 UTC) #14
danakj
I found the c-users thread, thanks. And I read http://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-header-file-and-odr again and it specifically says ...
4 years, 1 month ago (2016-11-17 19:21:15 UTC) #15
jbroman
I don't mind too much either way between this and an inline constexpr function in ...
4 years, 1 month ago (2016-11-17 20:03:14 UTC) #16
brucedawson
On 2016/11/17 20:03:14, jbroman wrote: > I don't mind too much either way between this ...
4 years, 1 month ago (2016-11-17 20:15:10 UTC) #17
fgorski
lgtm with a comment https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/offline_page_storage_manager.cc File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/offline_page_storage_manager.cc#newcode21 components/offline_pages/offline_page_storage_manager.cc:21: constexpr double constants::kOfflinePageStorageLimit; just to ...
4 years, 1 month ago (2016-11-17 22:10:16 UTC) #20
brucedawson
https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/offline_page_storage_manager.cc File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/offline_page_storage_manager.cc#newcode21 components/offline_pages/offline_page_storage_manager.cc:21: constexpr double constants::kOfflinePageStorageLimit; On 2016/11/17 22:10:15, fgorski wrote: > ...
4 years, 1 month ago (2016-11-17 22:19:39 UTC) #21
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/2509403002/1
4 years, 1 month ago (2016-11-17 22:20:59 UTC) #23
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 1 month ago (2016-11-17 23:20:05 UTC) #24
commit-bot: I haz the power
4 years, 1 month ago (2016-11-17 23:21:55 UTC) #26
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc
Cr-Commit-Position: refs/heads/master@{#433015}

Powered by Google App Engine
This is Rietveld 408576698