New issue
Advanced search Search tips

Issue 647223 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

chrome.exe size regression

Project Member Reported by grt@chromium.org, Sep 15 2016

Issue description

Comment 1 by grt@chromium.org, Sep 15 2016

As discussed in other venues, it looks to me like something in the skia roll in r411308. Trying to understand if and why skia is getting pulled into chrome.exe, and what changed in that roll to make it a problem.
Project Member

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

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

commit 1504ef7199454f8096cf1ecdece90a8476aa4a31
Author: grt <grt@chromium.org>
Date: Thu Sep 15 21:34:53 2016

chrome_initial does not need //ui/gfx on Windows.

chrome.exe appears to have grown by 150% when skia rolled in r411308.
chrome.exe shouldn't need skia or gfx, so let's try not depending on
them!

BUG= 647223 
R=brucedawson@chromium.org

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

[modify] https://crrev.com/1504ef7199454f8096cf1ecdece90a8476aa4a31/chrome/BUILD.gn

I double checked and I am seeing that if is_chrome_branded = false then the regression remains:

C:\src\chromium3\src>type out\release\args.gn
is_chrome_branded = true
is_component_build = false
is_debug = false
target_cpu = "x86"

C:\src\chromium3\src>dir out\release\chrome.exe
09/15/2016  02:17 PM           921,600 chrome.exe

C:\src\chromium3\src>type out\release_no_branding\args.gn
is_component_build = false
is_debug = false
target_cpu = "x86"

C:\src\chromium3\src>dir out\release_no_branding\chrome.exe
09/15/2016  02:57 PM         3,036,160 chrome.exe

C:\src\chromium3\src>git log -2 --oneline
5a8bc1a chrome_initial does not need //ui/gfx on Windows.
dce171b CrOS MD - use vectorized icons for ethernet and VPN.


This is unexpected.

This means that while the official release binary will *probably* be the right size, there is clearly still some fragility in the setup, so probably best to leave this bug open for future investigation.

Comment 4 by grt@chromium.org, Sep 16 2016

I found the difference between Chrome and Chromium branded builds: r355292 added a dependency on //ui/base to //components/version_info:version_info. This is a problem because the latter is needed for //components/startup_metric_utils/browser:lib. This might be more tricky to fix, so I propose that we merge r418986 back to M54 while we work on the rest of this. What do you think?
That sounds reasonable. Fixing the immediate regression with a simple change, and fixing the longer-term risk with a more complex change.

Did you find out why the initial regression wasn't noticed? It seems like the ideal test-case for sizes monitoring.

Comment 6 by grt@chromium.org, Sep 19 2016

Labels: Merge-Request-54
Yes. While chrome.exe's size was shown on the dashboard, it wasn't being monitored for changes. It is now.

Requesting merge of r418986 to M54 as per comments 4 and 5.

Comment 7 by dimu@chromium.org, Sep 19 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 19 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d9efb8c742f159c782a6574be6299feb3cf403d

commit 9d9efb8c742f159c782a6574be6299feb3cf403d
Author: Greg Thompson <grt@chromium.org>
Date: Mon Sep 19 09:51:52 2016

chrome_initial does not need //ui/gfx on Windows.

chrome.exe appears to have grown by 150% when skia rolled in r411308.
chrome.exe shouldn't need skia or gfx, so let's try not depending on
them!

BUG= 647223 
R=brucedawson@chromium.org

Review-Url: https://codereview.chromium.org/2345503006
Cr-Commit-Position: refs/heads/master@{#418986}
(cherry picked from commit 1504ef7199454f8096cf1ecdece90a8476aa4a31)
TBR=grt@chromium.org

Review URL: https://codereview.chromium.org/2348223002 .

Cr-Commit-Position: refs/branch-heads/2840@{#408}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/9d9efb8c742f159c782a6574be6299feb3cf403d/chrome/BUILD.gn

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 20 2016

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

commit a237cc46f9274d80df60813c10b69ec3067d7c2c
Author: grt <grt@chromium.org>
Date: Tue Sep 20 22:56:19 2016

Break chrome_initial's dependence on //components/startup_metric_utils/browser:lib

We can't have chrome.exe depending on browser code since doing so can
unexpectedly cause the size of chrome.exe to bloat. This change
accomplishes the breakage by passing the main exe entry TimeTicks
through to chrome.dll directly by way of ChromeMain and
ChromeMainDelegate rather than via an environment variable.

BUG= 647223 
R=brucedawson@chromium.org

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

[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/BUILD.gn
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/chrome_exe_main_win.cc
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/chrome_main.cc
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/chrome_main_delegate.h
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/main_dll_loader_win.cc
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/chrome/app/main_dll_loader_win.h
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/components/startup_metric_utils/browser/startup_metric_utils.cc
[modify] https://crrev.com/a237cc46f9274d80df60813c10b69ec3067d7c2c/components/startup_metric_utils/browser/startup_metric_utils.h

Comment 10 by grt@chromium.org, Sep 21 2016

Status: Fixed (was: Started)
chrome.exe is smallish again, and monitoring is now on. Calling this fixed.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 27 2016

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

commit 9d9efb8c742f159c782a6574be6299feb3cf403d
Author: Greg Thompson <grt@chromium.org>
Date: Mon Sep 19 09:51:52 2016

chrome_initial does not need //ui/gfx on Windows.

chrome.exe appears to have grown by 150% when skia rolled in r411308.
chrome.exe shouldn't need skia or gfx, so let's try not depending on
them!

BUG= 647223 
R=brucedawson@chromium.org

Review-Url: https://codereview.chromium.org/2345503006
Cr-Commit-Position: refs/heads/master@{#418986}
(cherry picked from commit 1504ef7199454f8096cf1ecdece90a8476aa4a31)
TBR=grt@chromium.org

Review URL: https://codereview.chromium.org/2348223002 .

Cr-Commit-Position: refs/branch-heads/2840@{#408}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/9d9efb8c742f159c782a6574be6299feb3cf403d/chrome/BUILD.gn

Sign in to add a comment