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

Issue 734631 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

0.7%-0.8% regression in sizes at 480386:480386

Project Member Reported by chiniforooshan@chromium.org, Jun 19 2017

Issue description

See the link to graphs below.
 
Owner: sakal@chromium.org
sakal@ could you please take a look? https://codereview.chromium.org/2944823002 has increased chrome.dll size. Is that expected?

Comment 3 by grt@chromium.org, Jun 20 2017

Status: Assigned (was: Untriaged)

Comment 4 by sakal@chromium.org, Jun 20 2017

Cc: sakal@chromium.org
Owner: zhihuang@chromium.org
This is caused by: https://codereview.webrtc.org/2854123003
Confirmed by rolling just that CL here: https://codereview.chromium.org/2944273002

Zhihuang, can you please fix this ASAP or we may have to revert the change?
I'll take a look.

Comment 6 by grt@chromium.org, Jun 20 2017

Cc: brucedaw...@chromium.org
+brucedawson: do you have some handy tooling to find out why the culprit CL adds 600K to chrome.dll?
I would build before/after binaries with similar or identical settings (must have full debug information and no fastlink) and then use SymbolSort as described here: https://www.chromium.org/developers/windows-binary-sizes

> SymbolSort -in new\chrome.dll.pdb -diff old\chrome.dll.pdb -out change.txt
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/9ed16097375fb8d9b45623c58d9086d33e503760

commit 9ed16097375fb8d9b45623c58d9086d33e503760
Author: zhihuang <zhihuang@webrtc.org>
Date: Wed Jun 21 06:58:36 2017

Try to fix the binary size increase issue on Chromium.

The target common_video used to depend on rtc_media_base which introduces
the dependency on p2p. This probably causes the binary size increase on Win
Chromium. Some chromium targets like src/media/gpu:gpu depends on common_video directly.

BUG= chromium:734631 

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

[modify] https://crrev.com/9ed16097375fb8d9b45623c58d9086d33e503760/webrtc/common_video/BUILD.gn
[modify] https://crrev.com/9ed16097375fb8d9b45623c58d9086d33e503760/webrtc/media/BUILD.gn
[modify] https://crrev.com/9ed16097375fb8d9b45623c58d9086d33e503760/webrtc/pc/BUILD.gn

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 21 2017

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

commit c2e208a9249452590fa282ef5aba43e480bc5794
Author: zhihuang <zhihuang@webrtc.org>
Date: Wed Jun 21 07:30:49 2017

Revert of Try to fix the binary size increase issue on Chromium. (patchset #1 id:1 of https://codereview.webrtc.org/2945233002/ )

Reason for revert:
The Android 32 (more config) bot is broken.

Original issue's description:
> Try to fix the binary size increase issue on Chromium.
>
> The target common_video used to depend on rtc_media_base which introduces
> the dependency on p2p. This probably causes the binary size increase on Win
> Chromium. Some chromium targets like src/media/gpu:gpu depends on common_video directly.
>
> BUG= chromium:734631 
>
> Review-Url: https://codereview.webrtc.org/2945233002
> Cr-Commit-Position: refs/heads/master@{#18693}
> Committed: https://chromium.googlesource.com/external/webrtc/+/9ed16097375fb8d9b45623c58d9086d33e503760

TBR=kjellander@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:734631 

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

[modify] https://crrev.com/c2e208a9249452590fa282ef5aba43e480bc5794/webrtc/common_video/BUILD.gn
[modify] https://crrev.com/c2e208a9249452590fa282ef5aba43e480bc5794/webrtc/media/BUILD.gn
[modify] https://crrev.com/c2e208a9249452590fa282ef5aba43e480bc5794/webrtc/pc/BUILD.gn

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/130ca7e783d123ed04758056b5fceb3dbbf5c1fa

commit 130ca7e783d123ed04758056b5fceb3dbbf5c1fa
Author: zhihuang <zhihuang@webrtc.org>
Date: Wed Jun 21 08:02:59 2017

Reland of Try to fix the binary size increase issue on Chromium. (patchset #1 id:1 of https://codereview.webrtc.org/2949953003/ )

Reason for revert:
Relanding the orginal CL. The breakage would be a flakey build.

Original issue's description:
> Revert of Try to fix the binary size increase issue on Chromium. (patchset #1 id:1 of https://codereview.webrtc.org/2945233002/ )
>
> Reason for revert:
> The Android 32 (more config) bot is broken.
>
> Original issue's description:
> > Try to fix the binary size increase issue on Chromium.
> >
> > The target common_video used to depend on rtc_media_base which introduces
> > the dependency on p2p. This probably causes the binary size increase on Win
> > Chromium. Some chromium targets like src/media/gpu:gpu depends on common_video directly.
> >
> > BUG= chromium:734631 
> >
> > Review-Url: https://codereview.webrtc.org/2945233002
> > Cr-Commit-Position: refs/heads/master@{#18693}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/9ed16097375fb8d9b45623c58d9086d33e503760
>
> TBR=kjellander@webrtc.org,deadbeef@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= chromium:734631 
>
> Review-Url: https://codereview.webrtc.org/2949953003
> Cr-Commit-Position: refs/heads/master@{#18694}
> Committed: https://chromium.googlesource.com/external/webrtc/+/c2e208a9249452590fa282ef5aba43e480bc5794

TBR=kjellander@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:734631 

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

[modify] https://crrev.com/130ca7e783d123ed04758056b5fceb3dbbf5c1fa/webrtc/common_video/BUILD.gn
[modify] https://crrev.com/130ca7e783d123ed04758056b5fceb3dbbf5c1fa/webrtc/media/BUILD.gn
[modify] https://crrev.com/130ca7e783d123ed04758056b5fceb3dbbf5c1fa/webrtc/pc/BUILD.gn

Comment 11 by grt@chromium.org, Jun 21 2017

Here's a diff from SymbolSort over an official build. I hope this helps.
change.txt
674 KB View Download
The WebRTC change is being submitted shortly in https://codereview.chromium.org/2950933004/
sorry it took so long. Hopefully the sizes graph will recover.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02493a9858bd5505aa82f4d4491278c909a15282

commit 02493a9858bd5505aa82f4d4491278c909a15282
Author: sakal <sakal@chromium.org>
Date: Wed Jun 21 13:11:30 2017

Roll WebRTC 18665:18695 (17 commits)

Changes: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+log/e6fddec..0f3c15e

$ git log e6fddec..0f3c15e --date=short --no-merges --format=%ad %ae %s
2017-06-21 zhihuang@webrtc.org Reland of Try to fix the binary size increase issue on Chromium. (patchset #1 id:1 of https://codereview.webrtc.org/2949953003/ )
2017-06-21 zhihuang@webrtc.org Revert of Try to fix the binary size increase issue on Chromium. (patchset #1 id:1 of https://codereview.webrtc.org/2945233002/ )
2017-06-20 zhihuang@webrtc.org Try to fix the binary size increase issue on Chromium.
2017-06-20 tschumim@webrtc.org Test and fix for huge bwe drop after alr state.
2017-06-20 andersc@webrtc.org Support more formats in RTCVideoFrame
2017-06-20 mellem@webrtc.org Create VideoDecoderFactory interface and implementation.
2017-06-20 mellem@webrtc.org Create a hardware VideoDecoder implementation using Android MediaCodec.
2017-06-20 brucedawson@chromium.org Use constexpr to avoid a static initializer
2017-06-20 henrik.lundin@webrtc.org Improve the simulation stats aggregation in neteq_rtpplay
2017-06-20 ilnik@webrtc.org Hotfix for psnr regresion with fec tests caused by timing frames.
2017-06-20 henrik.lundin@webrtc.org Add Matlab plotting script generator to neteq_rtpplay
2017-06-20 aleloi@webrtc.org Added new AudioProcessing fuzzer
2017-06-20 henrik.lundin@webrtc.org Add henrik.lundin to fuzzers OWNERS
2017-06-20 tommi@webrtc.org Minor updates to VideoReceiveStream: * Change decoder thread to use new thread function type. * Reduce the time of when video_receiver_ receives callbacks on the process thread to match with Start/Stop of the decoder. * Not triggering shutdown unless the thread is running.
2017-06-19 denicija@google.com Bugfix:setting capture framerate always defaults to 30fps.
2017-06-19 emadomara@google.com Fix test break by the recent changes in IcerServer
2017-06-19 zijiehe@chromium.org Add reference counter of DxgiDuplicatorController to unload DXGI components

TBR=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
BUG= 734631 

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

[modify] https://crrev.com/02493a9858bd5505aa82f4d4491278c909a15282/DEPS

Some data, steps to follow, and a fix, all from a release 32-bit build:

Ths script shows that the increase is mostly in the .text (code) segment:
C:\src\chromium3\src>python tools\win\pe_summarize.py without_patch\chrome.dll with_patch\chrome.dll
Size of without_patch\chrome.dll is 35.828736 MB
      name:   mem size  ,  disk size
     .text: 28.394326 MB
    .rdata:  6.091540 MB
     .data:  0.835236 MB,  0.097792 MB
      .tls:  0.000029 MB
   .rodata:  0.005520 MB
    .gfids:  0.003324 MB
    _RDATA:  0.000288 MB
  CPADinfo:  0.000036 MB
     .rsrc:  0.130504 MB
    .reloc:  1.102024 MB

Size of with_patch\chrome.dll is 36.129792 MB
      name:   mem size  ,  disk size
     .text: 28.619254 MB
    .rdata:  6.161412 MB
     .data:  0.836228 MB,  0.098304 MB
      .tls:  0.000029 MB
   .rodata:  0.005520 MB
    .gfids:  0.003324 MB
    _RDATA:  0.000288 MB
  CPADinfo:  0.000036 MB
     .rsrc:  0.130504 MB
    .reloc:  1.107848 MB

Memory size change from without_patch\chrome.dll to with_patch\chrome.dll
       .text:  224928 bytes change
      .rdata:   69872 bytes change
       .data:     992 bytes change
      .reloc:    5824 bytes change
Total change:  301616 bytes


SymbolSort (SymbolSort.exe -in with_patch\chrome.dll.pdb -diff without_patch\chrome.dll.pdb -out change.txt) says that these are the biggest causes of increases in chrome.dll:
Increases in Total Size
  Total Size  Total Count  Name
        7872            1  _opus_encode_native
        7200            1  mdct_twiddles960
        6744            1  _celt_encode_with_ec
        5609            1  WebRtcIsac_Fftradix
        5182            1  tonality_analysis
        5088            1  CELT_PVQ_U_DATA
        4624            1  WebRtcIsac_kQKltLevelsShape
        3840            1  fft_twiddles48000_960
        3555            1  _WebRtcIsac_InitializePitch
        3237            1  _silk_pitch_analysis_core_FLP
        3172            1  _silk_Encode
        3136            1  WebRtcIsac_kQKltLevelsGain
        2592            1  WebRtcIsac_kKltT1Shape


_opus_encode_native is defined in opus_encoder.c so I did a /verbose link of chrome.dll and then analyze the output with "python tools\win\linker_verbose_tracking.py verbose.txt opus_encoder" which gave these results:
Database loaded - 10009 xrefs found
opus_encoder.obj pulled in for symbol "_opus_encoder_create" by
        Command-line obj file: opus_interface.obj


So, opus_interface.obj is specified on the command-line (presumably in a source_set) and it referenced _opus_encoder_create which pulled in opus_encoder. Ah, source_sets.

Ideally opus_encoder wouldn't be pulled in to the chrome.dll build so I ran gn path to find out why it is:

C:\src\chromium3\src>gn path out\release //chrome:main_dll //third_party/webrtc/modules/audio_coding:webrtc_opus_c
//chrome:main_dll --[private]-->
//headless:headless_shell_browser_lib --[private]-->
//content/public/browser:browser --[public]-->
//content/public/browser:browser_sources --[private]-->
//content/browser:browser --[private]-->
//third_party/webrtc/media:rtc_media_base --[private]-->
//third_party/webrtc/api/audio_codecs:builtin_audio_encoder_factory --[private]-->
//third_party/webrtc/modules/audio_coding:builtin_audio_encoder_factory_internal --[private]-->
//third_party/webrtc/modules/audio_coding:webrtc_opus --[public]-->
//third_party/webrtc/modules/audio_coding:webrtc_opus_c


Fixing that is what we really want to do, but that path has been there for a while. So, a more practical fix for the moment is to change a couple of source sets to static_libraries in order to break the chain. This change was found to fix the regression on my local builds:

C:\src\chromium3\src\third_party\webrtc>git diff
diff --git a/media/BUILD.gn b/media/BUILD.gn
index 249ceb8..90fe508 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -32,7 +32,7 @@ config("rtc_media_warnings_config") {
   }
 }

-rtc_source_set("rtc_media_base") {
+rtc_static_library("rtc_media_base") {
   # TODO(kjellander): Remove (bugs.webrtc.org/6828)
   # Enabling GN check triggers cyclic dependency error:
   #   //webrtc/media:rtc_media_base ->
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 2720a1e..395544a 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -184,7 +184,7 @@ rtc_static_library("peerconnection") {
 # need should use CreateModularCreatePeerConnectionFactory instead, using the
 # "peerconnection" build target and other targets specific to their
 # requrements. See comment in peerconnectionfactoryinterface.h.
-rtc_source_set("create_pc_factory") {
+rtc_static_library("create_pc_factory") {
   sources = [
     "createpeerconnectionfactory.cc",
   ]

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 22 2017

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

commit ab97e18fa97a73d8ad0ac51c11d7111dd397984d
Author: zhihuang <zhihuang@webrtc.org>
Date: Thu Jun 22 08:28:59 2017

Fix the binary size regression on Chromium Windows.

There is a dependency chain from Chromium windows main_dll to Opus
which should never exist. We used to rely on rtc_static_library
to break this chain. So this CL replaced some rtc_source_set
with rtc_static_library.

libvpx fix (https://chromium-review.googlesource.com/c/544107/) for
ios-simulator linking issue is landed and this CL can be sumbitted once the new
Chromium is rolled into WebRTC.

BUG= chromium:734631 

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

[modify] https://crrev.com/ab97e18fa97a73d8ad0ac51c11d7111dd397984d/webrtc/media/BUILD.gn
[modify] https://crrev.com/ab97e18fa97a73d8ad0ac51c11d7111dd397984d/webrtc/pc/BUILD.gn

Cc: lanwei@chromium.org
 Issue 735689  has been merged into this issue.

Sign in to add a comment