New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 624274



Sign in to add a comment
link

Issue 630755: chrome.dll has unnecessary global variables and code

Reported by brucedaw...@chromium.org, Jul 22 2016 Project Member

Issue description

When comparing gn's chrome.dll to gyp's chrome.dll there are many global arrays that exist in the gn version but not the gyp version. Identifying these and fixing them (by careful changes of source_set targets to static_library targets) can save the space used by these globals and their associated code.

Most of these have been addressed already for  crbug.com/624274  but there are still some smaller ones remaining. At some point it would be good to investigate these to see if they can also be removed, in order to save space.

The space wasted by the variables themselves is not particularly important, especially since many are zero initialized and therefore consume no space on disk. However there is typically code associated with these and that code goes away at the same time, sometimes leading to significant savings.

The list (including sizes in bytes) is archived here because soon gyp builds will not be possible and such a comparison will no longer be available. The set of globals that are gn only and larger than 500 bytes is:

 4624: WebRtcIsac_kQKltLevelsShape
 3136: WebRtcIsac_kQKltLevelsGain
 2600: webrtc::acm2::ACMCodecDB::database_
 2592: WebRtcIsac_kKltT1Shape
 2048: fixed_divide
 1800: webrtc::acm2::ACMCodecDB::codec_settings_
 1032: nacl_global_rng
 1024: kf_low_motion_minq_8
 1024: rtc_minq_8
 1024: inter_minq_8
 1024: arfgf_low_motion_minq_8
 1024: sad_per_bit4lut_8
 1024: kf_high_motion_minq_8
 1024: fixed_invtbl8
 1024: sad_per_bit16lut_8
 1024: arfgf_high_motion_minq_8
  880: vpx_rv
  864: WebRtcIsac_kLpcMeansShape
  768: vp8_six_tap_mmx
  720: WebRtcIsac_kCos
  536: WebRtcIsac_kQMeanLag2Hi
  528: g_NaCl_log_gio
  512: webrtc::RandomVector::kRandomTable
  510: WebRtcIsac_kQPitchGainCdf

Note that this was measured in a 32-bit official build.
 

Comment 1 by brucedaw...@chromium.org, Sep 22 2016

Initial changes to webrtc targeted at removing WebRtcIsac_kQKltLevelsShape caused by following global variables to no longer be pulled in:

WebRtcIsac_kQKltLevelsShape
WebRtcIsac_kQKltLevelsGain
webrtc::acm2::ACMCodecDB::database_
WebRtcIsac_kKltT1Shape
webrtc::acm2::ACMCodecDB::codec_settings_
WebRtcIsac_kLpcMeansShape
WebRtcIsac_kCos
WebRtcIsac_kQMeanLag2Hi
webrtc::RandomVector::kRandomTable
WebRtcIsac_kQPitchGainCdf

That means that the top four entries are gone, along with many others, and the table now looks like this:

...
 2048: fixed_divide
...
 1032: nacl_global_rng
 1024: kf_low_motion_minq_8
 1024: rtc_minq_8
 1024: inter_minq_8
 1024: arfgf_low_motion_minq_8
 1024: sad_per_bit4lut_8
 1024: kf_high_motion_minq_8
 1024: fixed_invtbl8
 1024: sad_per_bit16lut_8
 1024: arfgf_high_motion_minq_8
  880: vpx_rv
...
  768: vp8_six_tap_mmx
...
  528: g_NaCl_log_gio
...

However for some reason virtually no binary size reduction was observed. Wot?

The .text (code) section was reduced by 272 bytes, the .rdata section by 16 bytes, and the .data section was reduced not at all. I cannot explain how I could remove 17 KB of initialized data and have the binary and section sizes basically unchanged, in official and non-official non-component 32-bit chrome-branded release builds.

Many of the other symbols are from vp9, such as rtc_minq_8 and friends which are defined in vp9_ratectrl.c. These are pulled in because vp9_frame_scale_ssse3.obj is in a source_set and then pulls in vp9_encoder.obj. Fixing that moves the trigger to vp9_impl.obj, which takes us back to rtc_source_set.

Comment 2 by bugdroid1@chromium.org, Sep 23 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54

commit b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54
Author: kjellander <kjellander@webrtc.org>
Date: Fri Sep 23 07:38:52 2016

GN: Change rtc_source_set targets --> rtc_static_library

This changes most non-test related rtc_source_set targets to be
rtc_static_library instead. Targets without any .cc files are excluded.
This should bring back the build behavior we used to have with GYP
(i.e. same symbols exported in the libjingle_peerconnection.a file, which
are used by some downstream projects).

After doing an Android build with these changes:
$ nm --defined-only -g -C out/Release/lib.unstripped/libjingle_peerconnection_so.so | grep -i createpeerconnectionf
00077c51 T Java_org_webrtc_PeerConnectionFactory_nativeCreatePeerConnectionFactory
$ nm --defined-only -g -C out/Release/obj/webrtc/api/libjingle_peerconnection.a | grep -i createpeerconnectionf
00000001 T webrtc::CreatePeerConnectionFactory(rtc::Thread*, rtc::Thread*, rtc::Thread*, webrtc::AudioDeviceModule*, cricket::WebRtcVideoEncoderFactory*, cricket::WebRtcVideoDecoderFactory*)
00000001 T webrtc::CreatePeerConnectionFactory()

See https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookbook.md#Note-on-static-libraries
for more details on this.

NOTICE: This should be further cleaned up in the future, to reduce
binary bloat and unnecessary linking time. Right now it's more
important to restore the desired build output though.

BUG= webrtc:6410 ,  chromium:630755 

Review-Url: https://codereview.webrtc.org/2361623004
Cr-Commit-Position: refs/heads/master@{#14364}

[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/api/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/audio/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/call/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/common_audio/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/common_video/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/examples/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/libjingle/xmllite/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/libjingle/xmpp/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/media/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/audio_coding/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/audio_conference_mixer/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/audio_device/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/audio_mixer/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/audio_processing/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/bitrate_controller/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/congestion_controller/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/desktop_capture/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/media_file/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/pacing/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/remote_bitrate_estimator/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/rtp_rtcp/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/utility/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/video_capture/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/video_coding/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/modules/video_processing/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/p2p/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/pc/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/sdk/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/stats/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/system_wrappers/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/test/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/test/fuzzers/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/tools/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/video/BUILD.gn
[modify] https://crrev.com/b62dbbe985c643cf4ee28e4c73c75bb3ef5e4d54/webrtc/voice_engine/BUILD.gn

Comment 3 by brucedaw...@chromium.org, Oct 20 2016

Cc: kerz@chromium.org grt@chromium.org brucedaw...@chromium.org
 Issue 641017  has been merged into this issue.

Comment 4 by bugdroid1@chromium.org, Nov 17 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bd1c0eec7d99b2cd485ef833887370ce547febc

commit 2bd1c0eec7d99b2cd485ef833887370ce547febc
Author: brucedawson <brucedawson@chromium.org>
Date: Thu Nov 17 23:19:36 2016

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 

Review-Url: https://codereview.chromium.org/2509403002
Cr-Commit-Position: refs/heads/master@{#433015}

[modify] https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc/components/offline_pages/offline_page_storage_manager.cc
[modify] https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc/components/offline_pages/offline_page_storage_manager.h
[modify] https://crrev.com/2bd1c0eec7d99b2cd485ef833887370ce547febc/components/offline_pages/offline_page_storage_manager_unittest.cc

Comment 5 by bugdroid1@chromium.org, Nov 21 2016

Project Member
The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/759ae5639b0dfe5a0d5491aa0e0b9855aa42ab73

commit 759ae5639b0dfe5a0d5491aa0e0b9855aa42ab73
Author: Bruce Dawson <brucedawson@google.com>
Date: Fri Nov 18 20:08:45 2016

Avoid runtime initialization of FLT_EPSILON_SQRT

FLT_EPSILON_SQRT is initialized with a call to sqrt which, in release
builds on Windows, results in an initializer call (on official release
builds this is optimized away). On Windows release (all flavors)
builds this also triggers duplicate instantiations of the variable
(always a risk with non-integral const variables defined in header
files) with 32 copies ending up in both chrome.dll and chrome_child.dll.

This change avoids the run-time initializer and as a side effect it
also avoids most or all of the duplication. Section size savings in a
Windows 32-bit release official build are:

chrome.dll
     .text:   -64 bytes change
    .rdata:   -16 bytes change
     .data:  -256 bytes change
    .reloc:  -116 bytes change
Total change:  -452 bytes

chrome_child.dll
     .text:   160 bytes change
    .rdata:  -144 bytes change
     .data:  -256 bytes change
    .reloc:   -60 bytes change
Total change:  -300 bytes

A more complete fix would include using extern const to declare these
constants in the header file but define them once in a .cc file, but
it's not clear that this is necessary.

The size savings on non-official builds are greater. The increase in
the .text segment is odd, but harmless since those bytes are shared.

BUG= 630755 

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=5061

Change-Id: I53d0cdc38e022039646df491d824a1aaf11def80
Reviewed-on: https://skia-review.googlesource.com/5061
Reviewed-by: Cary Clark <caryclark@google.com>
Reviewed-by: Bruce Dawson <brucedawson@google.com>
Commit-Queue: Cary Clark <caryclark@google.com>

[modify] https://crrev.com/759ae5639b0dfe5a0d5491aa0e0b9855aa42ab73/src/pathops/SkPathOpsTypes.h

Comment 6 by bugdroid1@chromium.org, Nov 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/facf44181190ded72c43ce0813ccd0aae56d78f2

commit facf44181190ded72c43ce0813ccd0aae56d78f2
Author: brucedawson <brucedawson@chromium.org>
Date: Tue Nov 22 01:14:35 2016

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 

Review-Url: https://codereview.chromium.org/2515483002
Cr-Commit-Position: refs/heads/master@{#433727}

[modify] https://crrev.com/facf44181190ded72c43ce0813ccd0aae56d78f2/mojo/edk/system/ports/name.cc
[modify] https://crrev.com/facf44181190ded72c43ce0813ccd0aae56d78f2/mojo/edk/system/ports/name.h

Comment 7 by bugdroid1@chromium.org, Nov 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3e725b562cdfa78546201688cf0a6d1889b3760

commit f3e725b562cdfa78546201688cf0a6d1889b3760
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Tue Nov 22 01:39:56 2016

Roll src/third_party/skia/ 71de072f9..759ae5639 (2 commits).

https://skia.googlesource.com/skia.git/+log/71de072f99a0..759ae5639b0d

$ git log 71de072f9..759ae5639 --date=short --no-merges --format='%ad %ae %s'
2016-11-18 brucedawson Avoid runtime initialization of FLT_EPSILON_SQRT
2016-11-17 jvanverth Add shadowrrect geometry processor

BUG= 630755 

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=egdaniel@google.com

Review-Url: https://codereview.chromium.org/2521063003
Cr-Commit-Position: refs/heads/master@{#433733}

[modify] https://crrev.com/f3e725b562cdfa78546201688cf0a6d1889b3760/DEPS

Comment 8 by bugdroid1@chromium.org, Nov 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c920c0bb4aa978130a2037fc1d77b010f03d68c

commit 7c920c0bb4aa978130a2037fc1d77b010f03d68c
Author: nacl-deps-roller <nacl-deps-roller@chromium.org>
Date: Tue Nov 22 23:23:31 2016

Roll src/native_client/ 0846b7b2d..e4a9254d3 (2 commits).

https://chromium.googlesource.com/native_client/src/native_client.git/+log/0846b7b2d308..e4a9254d3505

$ git log 0846b7b2d..e4a9254d3 --date=short --no-merges --format='%ad %ae %s'
2016-11-22 brucedawson Revert of Change some source_sets to static_library to shrink binary (patchset #1 id:1 of https://codereview.chromium.org/2519103003/ )
2016-11-22 brucedawson Change some source_sets to static_library to shrink binary

BUG= 630755 

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.chromium.linux:linux_chromium_dbg_32_ng,linux_nacl_sdk_build
TBR=mseaborn@chromium.org

Review-Url: https://codereview.chromium.org/2523863002
Cr-Commit-Position: refs/heads/master@{#434019}

[modify] https://crrev.com/7c920c0bb4aa978130a2037fc1d77b010f03d68c/DEPS

Comment 9 by brucedaw...@chromium.org, Nov 24 2016

Note that the ~500 KB of ff_cos_* and ff_sin_* variables that was supposed to be removed months ago is in fact still present in PGO builds, but not in official chrome-branded builds. Either the full-LTCG or the PGO settings are causing the extra data (and presumably extra code) to still be brought in.

Comment 10 by brucedaw...@chromium.org, Dec 5 2016

I've confirmed that the 786,064 bytes of ff_cos_* and ff_sin_* global variables that were removed months ago still show up in full_wpo_on_official builds. No WPO required. The settings I used were:

is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"
is_official_build = true
full_wpo_on_official = true

Finding what is pulling in those variables will save both the space that they use and presumably the space used by the associated code. However the investigations could be challenging since linking chrome.dll with these settings takes about 50 minutes on a Z620, making for long iterations.

Comment 11 by bugdroid1@chromium.org, Dec 6 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c534450c50f9b906c1c47c5987d04db7a3b129dd

commit c534450c50f9b906c1c47c5987d04db7a3b129dd
Author: brucedawson <brucedawson@chromium.org>
Date: Tue Dec 06 02:48:49 2016

Size reduction from moving const char arrays out of header

Change from initial\chrome.dll to out\release\chrome.dll
     .text:   192 bytes change
    .rdata:  -352 bytes change
    .reloc:    28 bytes change
Total change:  -132 bytes

Change from initial\chrome_child.dll to out\release\chrome_child.dll
     .text:  -512 bytes change
    .rdata:  -384 bytes change
    .reloc:  -320 bytes change
Total change: -1216 bytes

This isn't particularly worthwhile for the size savings, but it seems
worthwhile just because const non-integral variable definitions in
header files are not ideal.

BUG= 630755 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2523063002
Cr-Commit-Position: refs/heads/master@{#436499}

[modify] https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd/cc/BUILD.gn
[add] https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd/cc/debug/devtools_instrumentation.cc
[modify] https://crrev.com/c534450c50f9b906c1c47c5987d04db7a3b129dd/cc/debug/devtools_instrumentation.h

Comment 12 by bugdroid1@chromium.org, Dec 8 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/036e2ceb51ebaed01c6ad552fffc227be20c655e

commit 036e2ceb51ebaed01c6ad552fffc227be20c655e
Author: brucedawson <brucedawson@chromium.org>
Date: Thu Dec 08 04:49:42 2016

Don't define log2 and log2f in blink

The consequences are surprising.

A long time ago VC++'s CRT didn't supply log2 and log2f so WebKit/blink
supplied them as inline functions in MathExtras.h. In some cases
(including the full official builds that we ship to customers) this
meant that when Skia called log2f they ended up pulling in blink's
version. This pulled in PeriodicWave.obj which then pulled in various
other object files, and somehow the unreferenced-code logic in the
linker was unable to get rid of all of the rubbish. I noticed this
because during the gyp->gn transition I noticed that ff_cos_131072
and friends were in gn builds of chrome.dll but not gyp builds. I
fixed this but then they came back in full LTCG official builds
(full_wpo_on_official = true). Analysis of a verbose link gave this
chain of events for why rdft.obj (the definer of ff_*) was pulled in:

rdft.obj referenced by
        avfft.obj for symbol ff_rdft_init

avfft.obj referenced by
        FFTFrameFFMPEG.obj for symbol av_rdft_calc

FFTFrameFFMPEG.obj referenced by
        PeriodicWave.obj for symbol public: void __thiscall blink::FFTFrame::doInverseFFT(float *)

PeriodicWave.obj referenced by
        SkGeometry.obj for symbol log2f

Usually the trick to resolving these unwanted symbols is changing a
source_set to static_library, but this time it was recognizing that
SkGeometry has no business pulling in PeriodicWave, and removing the
VC++ definition of log2f that is the culprit. The effect on the size of
chrome.dll is:

chrome.dll
     .text:     -6656 bytes change
    .rdata:       240 bytes change
     .data:   -786624 bytes change
   .rodata:      -592 bytes change
    .reloc:      -872 bytes change
Total change: -794504 bytes

BUG= 630755 

Review-Url: https://codereview.chromium.org/2559063002
Cr-Commit-Position: refs/heads/master@{#437181}

[modify] https://crrev.com/036e2ceb51ebaed01c6ad552fffc227be20c655e/third_party/WebKit/Source/wtf/MathExtras.h

Comment 13 by dalecur...@chromium.org, Dec 8 2016

Great writeup! Is it correct to say that Android shouldn't suffer from this issue because it's standard library doesn't provide log2, log2f? Certainly the binary size change when we enabled ffmpeg there was not 800kb :)

Comment 14 by brucedaw...@chromium.org, Dec 8 2016

It looks like Android's standard library now does provide log2 and log2f, so I'm going to land a change (crrev.com/2556383002) to remove log2 and log2f completely, just in case.

Much of the size effect on Windows was zero initialized arrays, which don't affect binary size. Also, linkers appear to be chaotic so it is impossible to predict whether Android would have been affected - if the linker happened to find the standard library versions of these functions first (as it does in other Windows build variants) then the issue would not have occurred. If the linker did a perfect job of discarding unreferenced code and data then the issue could also have been avoided.

Comment 15 by brucedaw...@chromium.org, Dec 9 2016

Status: Fixed (was: Assigned)
crrev.com/2556383002 landed. At this point I am not aware of any significant symbols that are in gn builds that weren't in gyp builds. There are probably still some unnecessary ones, but I don't know how to find them anymore so I'm closing this as fixed.

For reference I've attached the dumps of the interesting global variables (from my custom dia2dump -i) from the gyp and gn versions of chrome.dll and chrome_child.dll from the transition time, in case they are needed for future investigations. These were created from local builds. Generating them from canary builds around that time is left as an exercise for the reader.

The tools to investigate where global variables come from are being checked in to src\tools\win separately for use in investigating future regressions.
gn_chrome.txt
30.3 KB View Download
gn_chrome_child.txt
57.3 KB View Download
gyp_chrome.txt
28.1 KB View Download
gyp_chrome_child.txt
57.0 KB View Download

Comment 16 by bugdroid1@chromium.org, Dec 10 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de77b2055e40cab9321479335ffd3b8416c3e34c

commit de77b2055e40cab9321479335ffd3b8416c3e34c
Author: brucedawson <brucedawson@chromium.org>
Date: Sat Dec 10 02:36:11 2016

Script for parsing of verbose linker output

When trying to understand why particular symbols are pulled in to a
binary it can be helpful to examine the verbose linker output.
However it quickly becomes unwieldy. This parses it and allows
querying of it to explain why a particular object file is pulled
in. This was used in the investigation of the linked bug, and others.

BUG= 630755 

Review-Url: https://codereview.chromium.org/2564543003
Cr-Commit-Position: refs/heads/master@{#437730}

[add] https://crrev.com/de77b2055e40cab9321479335ffd3b8416c3e34c/tools/win/linker_verbose_tracking.py

Comment 17 by bugdroid1@chromium.org, Dec 14 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd1071cd2d2b9b79a849e3c82857c552fb31e7bb

commit bd1071cd2d2b9b79a849e3c82857c552fb31e7bb
Author: brucedawson <brucedawson@chromium.org>
Date: Wed Dec 14 00:41:13 2016

Script for summarizing Windows binary section sizes

When trying to do build-size optimizations it is handy to be able to
easily measure progress. This script summarizes the memory and file
sizes of each PE (Portable Executable) section and can print the
differences between two versions of the same binary.

This tool is mostly used to measure progress, but can also be used to
help understand where a regression has come from - what section has
grown larger.

BUG= 630755 

Review-Url: https://codereview.chromium.org/2568973003
Cr-Commit-Position: refs/heads/master@{#438366}

[add] https://crrev.com/bd1071cd2d2b9b79a849e3c82857c552fb31e7bb/tools/win/pe_summarize.py

Comment 18 by bugdroid1@chromium.org, Dec 21 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8

commit 06d14dce6d5cb0b4ba2df631c5247c5c85047dd8
Author: brucedawson <brucedawson@chromium.org>
Date: Wed Dec 21 02:19:08 2016

ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG= 630755 

Review-Url: https://codereview.chromium.org/2580833003
Cr-Commit-Position: refs/heads/master@{#439981}

[add] https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8/tools/win/ShowGlobals/ShowGlobals.cc
[add] https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8/tools/win/ShowGlobals/ShowGlobals.sln
[add] https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8/tools/win/ShowGlobals/ShowGlobals.vcxproj

Comment 19 by bugdroid1@chromium.org, Dec 27 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc618746e94bb7503d926317abe6d9c37531d35e

commit cc618746e94bb7503d926317abe6d9c37531d35e
Author: brucedawson <brucedawson@chromium.org>
Date: Tue Dec 27 18:12:59 2016

Script to compare global variables between two PDBs

This script relies on having the recently added ShowGlobals.exe built
and in your path.

R=stanisc@chromium.org
BUG= 630755 

Review-Url: https://codereview.chromium.org/2592323002
Cr-Commit-Position: refs/heads/master@{#440764}

[add] https://crrev.com/cc618746e94bb7503d926317abe6d9c37531d35e/tools/win/pdb_compare_globals.py

Sign in to add a comment