Removing -mstackrealign grew x86 Cronet by 8.3% |
|||||||
Issue descriptionhttps://chromeperf.appspot.com/report?masters=ChromiumAndroid&bots=android_cronet_x86_builder&tests=sizes%2Flibcronet.so&checked=libcronet.so%2Clibcronet.so_ref%2Cref&rev=499402 crrev.com/499401 reduced libcronet's code size (by avoiding stack realignment prologue code) but this shrinkage was masked entirely by a doubling in size of the .eh_frame section. To build Cronet for x86: 1. gn gen out/Release --args='use_errorprone_java_compiler=true arm_use_neon=false target_os="android" enable_websockets=false disable_file_support=true disable_ftp_support=true disable_brotli_filter=false use_platform_icu_alternatives=true enable_reporting=false is_component_build=false ignore_elf32_limitations=true use_partition_alloc=false include_transport_security_state_preload_list=false is_debug=false is_official_build=true target_cpu="x86"' 2. ninja -C out/Release cronet Then you can find libcronet at out/Release/libcronet.*.so And the unstripped version at out/Release/lib.unstripped/libcronet.*.so I built libcronet with and without -mstackrealign. I compared assembly with "objdump -d" on the unstripped libcronet. I compared section sizes with "objdump -h".
,
Sep 6 2017
I think Cronet uses the unwind tables, see Issue 479283 .
,
Sep 6 2017
rnk, do you know (or know who knows) why _removing_ -mstackrealign would increase .eh_frame section size?
,
Sep 6 2017
My theory is that -mstack-realign effectively implies -fno-omit-frame-pointer, and I would expect the .eh_frame of a function with a frame pointer to be smaller than one without.
,
Sep 7 2017
Adding -fno-omit-frame-pointer didn't change the binary size, but then I realized it was probably getting overridden by the enable_frame_pointers logic in compiler.gni. After forcing on enable_frame_pointers the 8.3% regression turned into a 3.9% improvement (probably because we don't need the "andl" instruction to align the stack pointer in every function, though we still need the instruction to setup the frame pointer).
,
Sep 8 2017
Two interesting notes: 1. This affects all of Chrome, not just Cronet. I think Chrome for Android for x86 got significantly bigger because of this. I cannot find a perfbot tracking this. 2. -fno-exceptions doesn't avoid generation of .eh_frame section. The unwind tables are used for other forms of unwinding too. The .eh_frame section can be avoided via -fno-asynchronous-unwind-tables. I've got a proposed fix: https://chromium-review.googlesource.com/c/chromium/src/+/657566
,
Sep 8 2017
+ Performance-Size
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0c6c3f8dd59d7392d60ad37879ffc4490458704 commit a0c6c3f8dd59d7392d60ad37879ffc4490458704 Author: Paul Jensen <pauljensen@chromium.org> Date: Tue Sep 19 16:03:20 2017 Fix x86 Android size regression from removing -mstackrealign -mstackrealign implied -fno-omit-frame-pointer which greatly shrank the unwind tables (.eh_frame section). Removing the flag (in crrev.com/499401) therefore improved code size but requires emission of full unwind tables, significantly impacting overall binary size. When building for x86 Android and optimizing for size and generating unwind tables, this fix enables frame pointers. Bug: 762629 Change-Id: I43515e6159a2aa1ef06e362c52f6525d2d67bf4f Reviewed-on: https://chromium-review.googlesource.com/657566 Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#502860} [modify] https://crrev.com/a0c6c3f8dd59d7392d60ad37879ffc4490458704/build/config/compiler/BUILD.gn [modify] https://crrev.com/a0c6c3f8dd59d7392d60ad37879ffc4490458704/build/config/compiler/compiler.gni
,
Sep 19 2017
,
Sep 19 2017
Is the regression only recovered, or is cronet a bit smaller now?
,
Sep 19 2017
According to: https://chromeperf.appspot.com/report?masters=ChromiumAndroid&bots=android_cronet_x86_builder&tests=sizes%2Flibcronet.so&checked=libcronet.so%2Clibcronet.so_ref%2Cref&rev=499402 Regression was 446464 Improvement was 634880 So Cronet is smaller now.
,
Oct 11 2017
Can exclude_unwind_tables be moved into declare_args() so it's configurable at build time? This was the previous behavior before it was moved from BUILD.gn to compiler.gni. This is useful for Linux distro builds of Chromium: https://bugs.archlinux.org/task/55914
,
Oct 11 2017
@comment 12: I put up a CL to do this: https://chromium-review.googlesource.com/c/chromium/src/+/712575 There is also a CL up for review to move optimize_for_size back to decare_args: https://chromium-review.googlesource.com/c/chromium/src/+/699026
,
Oct 11 2017
Awesome, thanks!
,
Oct 11 2017
I don't think this should be different per distro. If we want this off in Linux builds, we should turn it off in Linux builds. If there's a reason it's on, we should keep it on. This is not a setting that should be different in different distros.
,
Oct 11 2017
My thoughts from the Arch Linux request: "This seems like a significant size reduction of the main chromium binary (~21 MiB). I can't think of any obvious drawbacks since we strip debug symbols anyway; both versions are useless for getting meaningful backtraces." I don't see why making it configurable again is questionable.
,
Oct 11 2017
To clarify a bit further; the need for it being configurable is that I would want exclude_unwind_tables=false if I was doing a build that kept the debug symbols (either in the same package or a split package). Our current Chromium package in Arch strips the debug symbols, so the size reduction is nice to have (thus the need for exclude_unwind_tables=true). |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by thakis@chromium.org
, Sep 6 2017