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

Issue 799646 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Regression



Sign in to add a comment

0.5%-0.9% regression in sizes at 526748:526752

Project Member Reported by benjhayden@chromium.org, Jan 5 2018

Issue description

rnk: Rolling clang regressed win sizes by about 0.7%
https://chromium.googlesource.com/chromium/src/+/0d72bb1494c94523ae7bd944476a495e8b281bcc

+grt, owner of win sizes benchmark.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=799646

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=b3ed90c95cb1a9aeccc1471d439ba4eda679cfae9f514d5e0f3ffac7751bf33a


Bot(s) for this bug's original alert(s):

win
win-32

Comment 2 by r...@chromium.org, Jan 6 2018

We can look, but I'm not sure when we will find time to bisect the 2800 revisions in the LLVM revision range 318667:321529.

The last size regression we had was worse (2.8%) and we didn't get around to it in a reasonable amount of time:  http://crbug.com/668225 

Comment 3 by grt@chromium.org, Jan 8 2018

Cc: brucedaw...@chromium.org
Labels: OS-Windows
Looks like mini_installer grew by only 26.6KB.

Bruce: would any of your tools help to shine a light on what type of data in the dll grew as a result of the roll?
Do you have the exact build settings use by the win/win-32 bots that reported the regression?

I can take a look but it's likely that this will be death by a thousand code-gen cuts so ???

Comment 5 by grt@chromium.org, Jan 9 2018

Before: https://ci.chromium.org/buildbot/chromium.perf/Win%20Builder/131692
After: https://ci.chromium.org/buildbot/chromium.perf/Win%20Builder/131693

Both used the following gn.args:

goma_dir = "C:\\b\\c\\goma_client"
is_chrome_branded = true
is_debug = false
is_official_build = true
strip_absolute_paths_from_debug_symbols = true
target_cpu = "x86"
use_goma = true

The binaries produced can be downloaded via the "download" link in each build's "package build" step (using your @google.com credentials).
I did an initial pair of builds with similar-but-not-quite-identical build settings. It showed a fairly small size change, about 100 KB (about .14% to .16%). Running tools\win\pe_summarize.py showed that the increase was entirely from the .text section, so increased code size.

I'm trying with more correct build settings but I'm not sure that is_chrome_branded and is_official_build are really going to help.
I did a test build at 89122ef0bd79bc79174e9ee50f622dd9d24200de with the clang version number toggled between old and new. That is I applied this diff:


diff --git a/tools/clang/scripts/update.py b/tools/clang/scripts/update.py
index b6c0a3cfcb0c..b0357e48cf95 100755
--- a/tools/clang/scripts/update.py
+++ b/tools/clang/scripts/update.py
@@ -27,7 +27,7 @@ import zipfile
 # Do NOT CHANGE this if you don't know what you're doing -- see
 # https://chromium.googlesource.com/chromium/src/+/master/docs/updating_clang.md
 # Reverting problematic clang rolls is safe, though.
-CLANG_REVISION = '321529'
+CLANG_REVISION = '318667'

 use_head_revision = bool(os.environ.get('LLVM_FORCE_HEAD_REVISION', '0')
                          in ('1', 'YES'))


I used these settings:

is_chrome_branded = true
is_debug = false
is_official_build = true
target_cpu = "x86"
remove_webcore_debug_symbols = true
symbol_level = 2
use_goma = true
use_lld = true
is_clang = true


I used use_lld=true because then I could use goma and symbol_level=2 without having to use fastlink which means that I can still run SymbolSort (https://github.com/adrianstone55/SymbolSort) on the output.

For chrome.dll the file size went from 43,788,288 to 43,930,112 which is a .32% increase - not very big actually.

I then ran this:

> symbolsort -in out\clang_new\chrome.dll -diff out\clang_old\chrome.dll -out diff.txt

I separately generated reports for old and new, cleverly titled old.txt and new.txt. They are all attached as a .zip file.

Here are two highlight areas from diff.txt. If I am interpreting this right then the incases in total count are cases where the new compiler generates more instances of some functions than before. Increase in total size is showing where functions grew the most. It's not clear how to interpret Total Size == 0 and Total Count == 0 but I think those are cases where there used to be zero instances of the function and now there are non-zero. So, it looks like hair_path<> used to be fully inlined and now it isn't. This may mean that the enclosing functions got smaller with the new compiler - such fun. The interpretation can be clarified by looking at old.txt and new.txt.

Increases in Total Count
  Total Size  Total Count  Name
         160            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
         160            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
         160            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
         154            2  Microsoft::WRL::Details::Make<Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundatio
         154            2  Microsoft::WRL::Details::Make<Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundatio
         127            2  start_iMCU_row
         106            2  Microsoft::WRL::Details::Make<Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundatio
         106            2  Microsoft::WRL::Details::Make<Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundatio
         106            2  Microsoft::WRL::Details::Make<Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundatio
          77            2  google::protobuf::internal::MapEntryImpl<google::protobuf::internal::MapEntryLite<std::basi
          77            2  google::protobuf::internal::MapEntryImpl<google::protobuf::internal::MapEntryLite<std::basi
          77            2  google::protobuf::internal::MapEntryImpl<google::protobuf::internal::MapEntryLite<std::basi
          77            2  public: class google::protobuf::Map<class std::basic_string<char,struct std::char_traits<ch
          52            2  google::protobuf::internal::MapEntryImpl<google::protobuf::internal::MapEntryLite<std::basi
          52            2  google::protobuf::internal::MapEntryImpl<google::protobuf::internal::MapEntryLite<std::basi
          48            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
          35            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
          24            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
          24            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  Microsoft::WRL::Details::InvokeHelper<ABI::Windows::Foundation::ITypedEventHandler<ABI::Win
           0            2  public: class google::protobuf::Map<class std::basic_string<char,struct std::char_traits<ch
           0            2  public: class google::protobuf::Map<class std::basic_string<char,struct std::char_traits<ch
           0            2  std::_Tree<std::_Tmap_traits<syncer::ModelType,syncer::EnumSet<syncer::ModelType,syncer::BO
        4601            1  hair_path<SkPaint::kRound_Cap>
        4601            1  hair_path<SkPaint::kSquare_Cap>
        4257            1  hair_path<SkPaint::kButt_Cap>
         769            1  extend_pts<SkPaint::kRound_Cap>
         769            1  extend_pts<SkPaint::kSquare_Cap>
         735            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namespace)::ProcessOneGlyphBounds
         699            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../third_party/skia/src/gpu/te
         660            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namespace)::ProcessOneGlyphBounds
         596            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namespace)::ProcessOneGlyphBounds
         596            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../third_party/skia/src/gpu/te
         589            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namespace)::ProcessOneGlyphBounds



Increases in Total Size
  Total Size  Total Count  Name
        4601            1  hair_path<SkPaint::kRound_Cap>
        4601            1  hair_path<SkPaint::kSquare_Cap>
        4257            1  hair_path<SkPaint::kButt_Cap>
        1660            0  std::_Buffered_merge_unchecked<std::pair<std::basic_string<char,
         769            1  extend_pts<SkPaint::kRound_Cap>
         769            1  extend_pts<SkPaint::kSquare_Cap>
         735            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         723            0  std::_Buffered_merge_sort_unchecked<std::pair<std::basic_string<
         710            0  private: unsigned __int64 __thiscall viz::SurfaceAggregator::Rem
         699            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         660            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         596            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         596            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         589            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         588            0  private: void __thiscall SkAmbientShadowTessellator::addEdge(str
         585            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         582            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<(anonymous namesp
         558            1  void __cdecl std::_Chunked_merge_unchecked<class url::Origin *,c
         533            1  google::protobuf::internal::MapEntryImpl<google::protobuf::inter
         533            1  google::protobuf::internal::MapEntryImpl<google::protobuf::inter
         529            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         526            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         522            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         515            1  SkFindAndPlaceGlyph::GlyphFindAndPlaceSubpixel<`lambda at ../../
         512            1  get_scale_factor<kBoth_MinMaxOrBoth>


I hope this is helpful.
symboldiffs.zip
863 KB Download

Comment 8 by p...@chromium.org, Jan 23 2018

Cc: p...@chromium.org briander...@chromium.org
 Issue 797081  has been merged into this issue.

Comment 9 by r...@chromium.org, Jun 20 2018

Status: WontFix (was: Assigned)
I never found time to work on this. At this point, if we want chrome to be smaller, it's probably more effective to attack the problem directly rather than attempting to revert whatever change caused the 0.32% size increase last January.

Sign in to add a comment