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

Issue 677450 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

0.4% regression in sizes at 440470:440472

Project Member Reported by alexclarke@chromium.org, Dec 29 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=677450

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg7-3j7AkM


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

win
Labels: OS-Windows
Owner: brucedaw...@chromium.org
Status: Assigned (was: Untriaged)
I'm going to test my sizes regressions tools and see how well they work on this.

Size of chrome.dll went from 57,522,200 to 57,727,000, between bb2dd59 and 6ccb8b8.
Cc: -alexclarke@chromium.org msarett@chromium.org
Actual change appears to be from 06ef945..96de5d5 - this is visually obvious on the graph. I only noticed this after building between the two reported hashes gave precisely zero differences, because third_party\libdrm is for chromiumos only.

Growth is primarily in the code (.text) segment, partially in the read-only data segment:

chrome.dll
       .text:  181744 bytes change
      .rdata:    1440 bytes change
      .reloc:    1992 bytes change
Total change:  185176 bytes

New globals in the .rdata segment include:

  20  2       kToLinear
1024  2       sk_linear_from_2dot2
  20  2       kFromLinear

I'm suspecting:

https://skia-review.googlesource.com/c/6396/

I used the -diff function in https://github.com/adrianstone55/SymbolSort/ to compare before/after versions of chrome.pdb and I have attached the report. The most crucial items are the two reports that show size increases by file path and by template function (with template arguments summarized as 'T'). These are ~150 color_xform_RGBA<T> functions generating most of the code:

File Contributions
--------------------------------------
Increases in Size
        Size   Count  Source Path
      179401     390  c:\src
      178960     793  c:\src\chromium3\src\third_party
      178944     918  c:\src\chromium3\src\third_party\skia\src
      178914     886  c:\src\chromium3\src\third_party\skia
      178807     941  c:\src\chromium3\src\third_party\skia\src\core
      178785     162  c:\src\chromium3\src
      174491     390  c:\src\chromium3\src\third_party\skia\src\core\skcolorspacexform.cpp

Increases in Total Size
  Total Size  Total Count  Name
      146496          300  color_xform_RGBA<T>
       12612           20  protected: virtual bool __thiscall SkColorSpaceXform_XYZ<T>::onApply(enum SkColorSpaceXform::ColorFormat,void *,enum SkColorSpaceXform::ColorFormat,void const *,int,enum SkAlphaType)const 
        6120           20  private: __thiscall SkColorSpaceXform_XYZ<T>::SkColorSpaceXform_XYZ<T>(class SkColorSpace_XYZ *,class SkMatrix44 const &,class SkColorSpace_XYZ *)

Cc: -msarett@chromium.org brucedaw...@chromium.org alexclarke@chromium.org
Owner: msarett@chromium.org
I missed up the owner and CC - fixing.

Comment 5 by msar...@google.com, Jan 13 2017

This class is large and I'm working on improving that.

I am surprised that this change affected anything though.  Chrome was already compiling/using this class.  Why did an additional use affect code size?
Status: Fixed (was: Assigned)
https://skia-review.googlesource.com/c/8848/
The skia change made it to Chromium in r452228 (crrev.com/2709623005). This dropped the chrome.dll size from 50,699,300 to 50,621,400, a savings of 77,900 bytes.

The original regression was from 57,522,200 to 57,727,000 bytes, an increase of 204,800 bytes. So, there is still some net increase, but it may be from other sources or may be reasonable and appropriate for this change.

I re-ran SymbolSort and it said:

68184          216  color_xform_RGBA<T>

Previously it said:

146496          300  color_xform_RGBA<T>

So, there are slightly fewer of these functions and the total size is significantly lower. Seem reasonable?

Thanks for the analysis.

The CL in #6 trims some non-critical code from the fast path.  There is still ongoing work into improving the performance-size tradeoff, but the net increase now seems more appropriate.

All seems reasonable to me.
Labels: Binary-Size
Labels: -binary-size Performance-Size

Sign in to add a comment