Need to move more logically-const data from .data to .rdata segments |
|||
Issue descriptionAnalysis of chrome.dll.pdb and chrome_child.dll.pdb with ShowGlobals shows a number of large arrays that are in the .data segment (read/write) even though they are logically const and should therefore be in the .rdata segment. There are a few different reasons: 1) Some arrays simply aren't marked as const. Easy fix. 2) Some arrays contain structs that have const members, and VC++ doesn't like that (https://connect.microsoft.com/VisualStudio/feedback/details/3117602) 3) Some arrays are initialized at run-time. Moving data to the read-only segment can reduce bugs, but its main advantage is that it increases the amount of data that can be shared. If arrays are initialized at run-time in chrome_child.dll then every renderer process may end up with a copy of them. Even arrays that are not written to may end up in copy-on-write pages if they share pages with modified data - this will typically hit a couple of KB per array. In some cases (the VC++ bug) there appears to be actual code associated with placing the bytes in the read/write segment and the savings can be quite significant. The VC++ bug is presumably Windows specific but the other issues are portable.
,
Dec 29 2016
,
Dec 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60cd9d9826b9611587c218b5bfb59f5a75767bb2 commit 60cd9d9826b9611587c218b5bfb59f5a75767bb2 Author: brucedawson <brucedawson@chromium.org> Date: Thu Dec 29 19:41:12 2016 Move ev_root_ca_metadata 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 ~6,800 bytes to the .rdata (read-only) data segment. 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 Review-Url: https://codereview.chromium.org/2607183002 Cr-Commit-Position: refs/heads/master@{#440977} [modify] https://crrev.com/60cd9d9826b9611587c218b5bfb59f5a75767bb2/net/cert/ev_root_ca_metadata.cc
,
Dec 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40b53de7f19c627cca389d0904916ba81af58924 commit 40b53de7f19c627cca389d0904916ba81af58924 Author: brucedawson <brucedawson@chromium.org> Date: Thu Dec 29 23:53:37 2016 Move some sniffer 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 some should-be-harmless 'const' keywords to work around this quirk and move ~2,500 bytes to the .rdata (read-only) data segment across chrome.dll and chrome_child.dll. The arrays affected include kMagicNumbers, kExtraMagicNumbers, kSniffableTags, kOfficeExtensionTypes, and a few others. 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 Review-Url: https://codereview.chromium.org/2607993002 Cr-Commit-Position: refs/heads/master@{#441007} [modify] https://crrev.com/40b53de7f19c627cca389d0904916ba81af58924/net/base/mime_sniffer.cc
,
Jan 3 2017
The following revision refers to this bug: https://skia.googlesource.com/skia.git/+/34194690328b612557f06ecec99b232d41888d4b commit 34194690328b612557f06ecec99b232d41888d4b Author: Bruce Dawson <brucedawson@google.com> Date: Thu Dec 29 22:05:39 2016 Move two skia arrays to read-only data segment The BCP47FromLanguageID and UnicodeFromMacRoman arrays are logically const but were not marked as such. Marking them as const lets the compiler/linker store them in the read-only data segment, which is strictly better than being in read/write memory. This change moves about 3,000 bytes from the .data to .rdata segment in both chrome.dll and chrome_child.dll. BUG= 677351 Change-Id: I85ff44d61aa232cf29178833fd2bb2e004032b9e Reviewed-on: https://skia-review.googlesource.com/6424 Reviewed-by: Brian Salomon <bsalomon@google.com> Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Commit-Queue: Mike Klein <mtklein@chromium.org> [modify] https://crrev.com/34194690328b612557f06ecec99b232d41888d4b/src/sfnt/SkOTTable_name.cpp
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f92666f75f56d656bf0c771ed79c3c7b33d6de28 commit f92666f75f56d656bf0c771ed79c3c7b33d6de28 Author: skia-deps-roller <skia-deps-roller@chromium.org> Date: Tue Jan 03 01:06:03 2017 Roll src/third_party/skia/ c0b4d21e1..341946903 (1 commit). https://skia.googlesource.com/skia.git/+log/c0b4d21e13b3..34194690328b $ git log c0b4d21e1..341946903 --date=short --no-merges --format='%ad %ae %s' 2016-12-29 brucedawson Move two skia arrays to read-only data segment BUG= 677351 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, see: http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=csmartdalton@google.com Review-Url: https://codereview.chromium.org/2612473002 Cr-Commit-Position: refs/heads/master@{#441081} [modify] https://crrev.com/f92666f75f56d656bf0c771ed79c3c7b33d6de28/DEPS
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eafbd71d2e7eda209965d66a540d596038edd68c commit eafbd71d2e7eda209965d66a540d596038edd68c Author: brucedawson <brucedawson@chromium.org> Date: Tue Jan 03 01:15:25 2017 Move blink::serializedCharacterData array to read-only data segment This array is logically const but was not marked as such. Marking it as const lets the compiler/linker store it in the read-only data segment, which is strictly better than being in read/write memory. This change moves 11,844 bytes from the .data to .rdata segment. Note that "const" implies static which is why "extern const" is needed. BUG= 677351 Review-Url: https://codereview.chromium.org/2608823002 Cr-Commit-Position: refs/heads/master@{#441083} [modify] https://crrev.com/eafbd71d2e7eda209965d66a540d596038edd68c/third_party/WebKit/Source/platform/text/Character.cpp [modify] https://crrev.com/eafbd71d2e7eda209965d66a540d596038edd68c/third_party/WebKit/Source/platform/text/CharacterPropertyDataGenerator.cpp
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7a0168cc2fae634b15e720527bfcd2d75ff21def commit 7a0168cc2fae634b15e720527bfcd2d75ff21def Author: brucedawson <brucedawson@chromium.org> Date: Tue Jan 03 21:26:55 2017 Save 166 KB of per-process private data by deleting 'const' Also saves a total of 224 KB of code. VC++ has an odd quirk where if you have a const array of structs where the structs have some const members then the compiler stores the array in the .data (read/write section) instead of the .rdata (read-only section). Worse yet, in this case the compiler generates code to do the initialization, even in full official builds. This wastes code/reloc space and also means that the arrays all turn into per-process private data. That is, every Chrome process was ending up with a private copy of the many global arrays which this change fixes. This anomaly was discovered while investigating why a couple of large const arrays were in the wrong section. The section size improvements for full official build are shown below: chrome.dll .text: -111744 bytes change .rdata: 165824 bytes change .data: -166208 bytes change .reloc: -23864 bytes change Total change: -135992 bytes chrome_child.dll .text: -111904 bytes change .rdata: 166000 bytes change .data: -166240 bytes change .reloc: -23832 bytes change Total change: -135976 bytes The on-disk size of each binary shrinks by 88,576 bytes. I do not understand any of the math behind how section sizes map to binary sizes. The VC++ tracking bug is: https://connect.microsoft.com/VisualStudio/feedback/details/3117602 BUG= 677351 Review-Url: https://codereview.chromium.org/2607893002 Cr-Commit-Position: refs/heads/master@{#441211} [modify] https://crrev.com/7a0168cc2fae634b15e720527bfcd2d75ff21def/device/usb/usb_ids.h
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f647c9f14b76450131f88d99c186010d960aeccf commit f647c9f14b76450131f88d99c186010d960aeccf Author: brucedawson <brucedawson@chromium.org> Date: Tue Jan 03 23:26:59 2017 Move syncer::kModelTypeInfoMap array 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 some should-be-harmless 'const' keywords to work around this quirk and move ~960 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 Review-Url: https://codereview.chromium.org/2605393002 Cr-Commit-Position: refs/heads/master@{#441250} [modify] https://crrev.com/f647c9f14b76450131f88d99c186010d960aeccf/components/sync/syncable/model_type.cc
,
Jan 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c62b45ea72fa4a9f57bf25ead85db4c397e43e5d commit c62b45ea72fa4a9f57bf25ead85db4c397e43e5d Author: brucedawson <brucedawson@chromium.org> Date: Wed Jan 04 18:38:54 2017 Roll src\third_party\ced\src/ e57cdc44b..368a9cc09 (1 commit). https://chromium.googlesource.com/external/github.com/google/compact_enc_det.git/+log/e57cdc44bd54..368a9cc09ad8 $ git log e57cdc44b..368a9cc09 --date=short --no-merges --format='%ad %ae %s' 2016-12-29 brucedawson Save 908,288 bytes by deleting 'const' three times TBR=jinsukkim@chromium.org BUG= 677351 , 629332 Review-Url: https://codereview.chromium.org/2617463003 Cr-Commit-Position: refs/heads/master@{#441416} [modify] https://crrev.com/c62b45ea72fa4a9f57bf25ead85db4c397e43e5d/DEPS
,
Jan 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90aef9d95d98c323811bc8ac8ae86570954c6d8c commit 90aef9d95d98c323811bc8ac8ae86570954c6d8c Author: rsleevi <rsleevi@chromium.org> Date: Fri Jan 06 01:47:10 2017 Move the known CT logs to the 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 ~256 bytes to the .rdata (read-only) data segment. VC++ bug is https://connect.microsoft.com/VisualStudio/feedback/details/3117602 BUG= 677351 Review-Url: https://codereview.chromium.org/2613723005 Cr-Commit-Position: refs/heads/master@{#441804} [modify] https://crrev.com/90aef9d95d98c323811bc8ac8ae86570954c6d8c/net/cert/ct_known_logs_static-inc.h
,
Jan 9 2017
These issues were found using binary-size investigation tools that are documented here: https://www.chromium.org/developers/windows-binary-sizes
,
Jan 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aef5c1f5d4a23b24300e2bc3e2099c14e48ce21e commit aef5c1f5d4a23b24300e2bc3e2099c14e48ce21e Author: brucedawson <brucedawson@chromium.org> Date: Mon Jan 09 20:37:15 2017 Move seven small arrays to read-only data segment Global arrays should be tagged as const when possible. This lets most compilers put them in the read-only data segment which is always shared between processes. Two of the relevant structs also fell afoul of a VC++ bug that prevents variables from being put in the read-only data segment. Thus this change works by *adding* const eight times and *removing* it twice. The net result is just 556 bytes being moved, but it's the thought that counts. Compilers are awesome. VC++ bug is filed here: https://connect.microsoft.com/VisualStudio/feedback/details/3117602 BUG= 677351 Review-Url: https://codereview.chromium.org/2618873002 Cr-Commit-Position: refs/heads/master@{#442329} [modify] https://crrev.com/aef5c1f5d4a23b24300e2bc3e2099c14e48ce21e/chrome/browser/themes/browser_theme_pack.cc
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbbcb90c9c4388226eadba1632bc85040e66c8cc commit cbbcb90c9c4388226eadba1632bc85040e66c8cc Author: brucedawson <brucedawson@chromium.org> Date: Fri Jan 13 18:24:37 2017 Move ~200 Feature objects to read-only memory in Windows builds A VC++ bug was preventing base::Feature objects from going into the read-only section despite being tagged as const. Adding a constexpr constructor solves this problem. Some of the variables (kTimerThrottlingForHiddenFrames for instance) get optimized away entirely which is why the size changes are not symmetrical. The VC++ bug is: https://connect.microsoft.com/VisualStudio/feedback/details/3117602 Previous instances of this bug were worked around by deleting const but this seemed undesirable in this case. Luckily the constexpr fix seems to work just as well. Approximate deltas are: chrome.dll .text: -48 bytes change .rdata: 1216 bytes change .data: -1376 bytes change .reloc: -32 bytes change Total change: -240 bytes chrome_child.dll .text: -16 bytes change .rdata: 704 bytes change .data: -800 bytes change .reloc: 32 bytes change Total change: -80 bytes Modest, but it's good to move a few hundred variables to read-only memory. This should not affect other platforms. BUG= 677351 Review-Url: https://codereview.chromium.org/2623373004 Cr-Commit-Position: refs/heads/master@{#443616} [modify] https://crrev.com/cbbcb90c9c4388226eadba1632bc85040e66c8cc/base/feature_list.h
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4419391c9bd15c97e01fcacbf40871344476b5ca commit 4419391c9bd15c97e01fcacbf40871344476b5ca Author: brucedawson <brucedawson@chromium.org> Date: Fri Jan 27 01:31:01 2017 Update static_initializers tool to build easier, find more static_initializers failed to find the initializer/destructor code for g_fallback_crash_handler_launcher because it was of this form: `dynamic atexit destructor for ' So, this pattern is now watched for. This change also adds a build.bat file and .gitignore file. If we add this to the bots we'll need to make it part of the .gn build. R=scottmg@chromium.org BUG= 677351 Review-Url: https://codereview.chromium.org/2656223002 Cr-Commit-Position: refs/heads/master@{#446524} [add] https://crrev.com/4419391c9bd15c97e01fcacbf40871344476b5ca/tools/win/static_initializers/.gitignore [add] https://crrev.com/4419391c9bd15c97e01fcacbf40871344476b5ca/tools/win/static_initializers/build.bat [modify] https://crrev.com/4419391c9bd15c97e01fcacbf40871344476b5ca/tools/win/static_initializers/static_initializers.cc
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b7c41be9c2235fa51c1e6e0abd3963d277cc1df2 commit b7c41be9c2235fa51c1e6e0abd3963d277cc1df2 Author: sabbakumov <sabbakumov@yandex-team.ru> Date: Mon Feb 27 04:31:30 2017 Make GRIT not to generate const struct members chrome.dll: .text: 16 bytes change .rdata: 25376 bytes change .data: -25536 bytes change .reloc: 128 bytes change Total change: -16 bytes BUG= 677351 Review-Url: https://codereview.chromium.org/2685073002 Cr-Commit-Position: refs/heads/master@{#453145} [modify] https://crrev.com/b7c41be9c2235fa51c1e6e0abd3963d277cc1df2/tools/grit/grit/format/resource_map.py [modify] https://crrev.com/b7c41be9c2235fa51c1e6e0abd3963d277cc1df2/tools/grit/grit/format/resource_map_unittest.py
,
May 16 2017
The latest update from Microsoft is that the const fixes that I requested (allowing extern const on constexpr variables, and fixing the bug where structs with const members don't go into .rdata (https://connect.microsoft.com/VisualStudio/feedback/details/3117602)) will not be in VS 2017 Update 3. Maybe in the next compiler release, but no promises.
,
Oct 2
I redid this analysis on 64-bit Chrome canary M71. This revealed many objects that were reported as being in section 0, which is impossible. This is believed to be due to an lld-link quirk/bug, filed here: https://bugs.llvm.org/show_bug.cgi?id=39147 The only objects of interest, then, are those in section 3, the read/write section. I looked at a bunch of these and couldn't find any that could be easily moved to the const data section. They are all initialized at runtime and it would take some work to move that to compile-time initialization. I've attached the current list but I am going to close this as fixed since there is no clear path forward. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Dec 29 2016