|
|
Created:
4 years, 1 month ago by brucedawson Modified:
4 years, 1 month ago 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. |
DescriptionMove 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
Messages
Total messages: 26 (9 generated)
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
brucedawson@chromium.org changed reviewers: + fgorski@chromium.org
This is the theoretically optimal solution, and it does indeed shrink things a bit more. But, it is more disruptive. Your call as to whether you prefer crrev.com/2505163004 or this one.
If chromium does not use anything like that yet, I'd run that through Nico or dcheng@ before making a call. The other code seems more canonical to our code base. WDYT?
On 2016/11/17 18:46:42, fgorski wrote: > If chromium does not use anything like that yet, I'd run that through Nico or > dcheng@ before making a call. > > The other code seems more canonical to our code base. > > WDYT? Agreed,
brucedawson@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org
thakis@/dcheng@, can we get your thoughts on this CL? It is the canonical C++ 11/14 way to declare constants in a header file without risking runtime initialization or duplication. But it's a bit ugly. The tidier (but less optimal) solution is in crrev.com/2505163004. The size benefits are listed in the description, but the main advantage of this CL is the guarantee of no runtime initialization. WDYT?
dcheng@chromium.org changed reviewers: + danakj@chromium.org, jbroman@chromium.org
It seems reasonable to me, but I think the C++11 styleguide OWNERS should be the ones to decide. (+danakj, +jbroman)
static constexpr stuff can still have the same ODR problems that static const stuff can, right? The way we have specified to do this in chromium is to use enums, one value per enum, like enum { kMyThing = 2 }; enum { kMyOtherThing = 3 }; See "[chromium-dev] Style question: static const members vs enums" for the discussion thread.
On 2016/11/17 19:09:03, danakj wrote: > static constexpr stuff can still have the same ODR problems that static const > stuff can, right? The way we have specified to do this in chromium is to use > enums, one value per enum, like > > enum { kMyThing = 2 }; > enum { kMyOtherThing = 3 }; > > See "[chromium-dev] Style question: static const members vs enums" for the > discussion thread. That only helps for integral constants though: the constants here are changing to base::TimeDelta (which I would argue is a strict improvement).
Oh.. why the struct then, is this invoking some C++ magic?
On 2016/11/17 19:12:01, danakj wrote: > Oh.. why the struct then, is this invoking some C++ magic? I asked this on the c-users@ mailing list. Until inline variables (C++ 17) become available this is the only way to have constexpr (guaranteed compile-time initialization) *and* no data duplication. This alternate solution (https://codereview.chromium.org/2505163004/) avoids the data duplication, but apparently still has some run-time initialization (notice the 272 bytes of extra code savings on this CL). The sizes involved are tiny, but run-time initialization is best avoided.
I found the c-users thread, thanks. And I read http://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-h... again and it specifically says "at namespace scope" which is I guess what this is avoiding. LGTM and we should update that chromium-dev thread to say that this is a possible way around things too.
I don't mind too much either way between this and an inline constexpr function in the header (which has its own syntactic quirk, but should have the effect of inlining the constant everywhere in a reasonable compiler at compile time).
On 2016/11/17 20:03:14, jbroman wrote: > I don't mind too much either way between this and an inline constexpr function > in the header (which has its own syntactic quirk, but should have the effect of > inlining the constant everywhere in a reasonable compiler at compile time). I have a slight preference for the static constexpr technique because it is efficient even in debug builds.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a comment https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_storage_manager.cc:21: constexpr double constants::kOfflinePageStorageLimit; just to double-check: are these necessary?
https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/2509403002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_storage_manager.cc:21: constexpr double constants::kOfflinePageStorageLimit; On 2016/11/17 22:10:15, fgorski wrote: > just to double-check: are these necessary? Yeah, they are needed in order to reserve storage space for the constants. I think that they often can be omitted without consequence, but doing so is technically incorrect.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc Cr-Commit-Position: refs/heads/master@{#433015} |