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

Issue 796207 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

16kb regression in resource_sizes (MonochromePublic.apk) at 524927:524927

Project Member Reported by agrieve@chromium.org, Dec 19 2017

Issue description

Caused by “[Ribbon][Reupload] Remove CSSProperty groups”

Commit: 505ec4645e87ce2bfc7875b053d3f48967aabbc5

Link to size graph: https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev=524927

Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase

It looks like this increase was probably unexpected or might be avoidable. Please take a look.

 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Dec 19 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=796207

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


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

Android Builder
From the CL description, sounded like this change was meant to remove things rather than add, so worth taking a look i think.

From the supersize diff, a few things jump out at me.

1. Increase in .data.rel.ro size:
  Look to be coming from vtables:
+ 0)        100 (1.0%)  R@0x2d798f4  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/AlignContent.cpp
               blink::CSSLonghand::AlignContent [vtable]
- 1)          0 (0.0%)  R@0x0        -100 (100->0)      third_party/WebKit/Source/core/css/properties/longhands/AlignOrJustifyContent.cpp
               blink::CSSLonghand::AlignOrJustifyContent [vtable]
+ 2)        100 (1.0%)  R@0x2d53900  100 (0->100)       blink/core/css/properties/CSSUnresolvedProperty.cpp
               blink::CSSLonghand::AlignmentBaseline [vtable]
+ 3)        200 (1.9%)  R@0x2d79a20  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/AnimationDelay.cpp
               blink::CSSLonghand::AnimationDelay [vtable]
+ 4)        300 (2.9%)  R@0x2d79ae8  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/AnimationDuration.cpp
               blink::CSSLonghand::AnimationDuration [vtable]
+ 5)        400 (3.8%)  R@0x2d79cdc  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/AnimationTimingFunction.cpp
               blink::CSSLonghand::AnimationTimingFunction [vtable]
- 6)        300 (2.9%)  R@0x0        -100 (100->0)      third_party/WebKit/Source/core/css/properties/longhands/AutoOrString.cpp
               blink::CSSLonghand::AutoOrString [vtable]
- 7)        200 (1.9%)  R@0x0        -100 (100->0)      third_party/WebKit/Source/core/css/properties/longhands/BackgroundBox.cpp
               blink::CSSLonghand::BackgroundBox [vtable]
+ 8)        300 (2.9%)  R@0x2d79e6c  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundClip.cpp
               blink::CSSLonghand::BackgroundClip [vtable]
+ 9)        400 (3.8%)  R@0x2d79f34  100 (0->100)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundImage.cpp


2. Removal of manual string pool -> linker string pool reduced .rodata size:
Section Legend: r=.rodata
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
- 0)      -8928 (109.5%) r@0x0        -8928 (8928->0)    blink/core/CSSPropertyNames.cpp
               blink::propertyNameStringsPool
- 1)     -10831 (132.9%) r@0x0        -1903 (1902->0)    blink/core/CSSPropertyNames.cpp
               blink::propertyNameStringsOffsets
~ 2)     -10583 (129.8%) r@Group      248 (6868->7116)   {no path}
               ** merge constants (count=16)
~ 3)     -10469 (128.4%) r@0x28750cb  114 (31->145)      third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
               string literal
~ 4)     -10567 (129.6%) r@0x28752bc  -98 (145->47)      third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
               string literal
~ 5)     -10632 (130.4%) r@0x287479b  -65 (66->0.8)      third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
               string literal (num_aliases=1->10)
~ 6)     -10569 (129.7%) r@0x2874fda  63 (0.8->64)       third_party/WebKit/Source/core/css/PropertyRegistration.cpp
               string literal (num_aliases=10->1)
~ 7)     -10624 (130.3%) r@0x2775f1a  -55 (56->0.6)      third_party/WebKit/Source/core/css/parser/CSSSupportsParser.cpp
               string literal (num_aliases=1->7)
~ 8)     -10679 (131.0%) r@0x276dc79  -54 (56->1.2)      third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
               string literal (num_aliases=1->8)
~ 9)     -10728 (131.6%) r@0x26a1d9b  -49 (50->0.2)      third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
               string literal (num_aliases=1->15)
~ 10)    -10679 (131.0%) r@0x2874f9f  49 (9.5->59)       third_party/WebKit/Source/core/css/PropertyRegistration.cpp
               string literal (num_aliases=2->1)
~ 11)    -10630 (130.4%) r@0x2875702  48 (1.2->50)       third_party/WebKit/Source/core/css/cssom/CSSNumericValue.cpp
               string literal (num_aliases=5->1)
~ 12)    -10678 (131.0%) r@0x293af68  -48 (64->16)       third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
               string literal


3. Many new symbols of size 120 bytes (maybe they used to be inlined?). This is most easily seen from the main diff output:
tools/binary_size/diagnose_bloat.py 505ec4645e87ce2bfc7875b053d3f48967aabbc5 --cloud:

Section Sizes (Total=17.9kb (18310 bytes)):
    .bss: 1792 bytes (1792 bytes) (not included in totals)
    .data: 0 bytes (0 bytes) (0.0%)
    .data.rel.ro: 10.2kb (10496 bytes) (57.3%)
    .other: 4 bytes (4 bytes) (0.0%)
    .pak.nontranslated: 0 bytes (0 bytes) (0.0%)
    .pak.translations: 0 bytes (0 bytes) (0.0%)
    .rel.dyn: -1208 bytes (-1208 bytes) (-6.6%)
    .rodata: -7.98kb (-8176 bytes) (-44.7%)
    .text: 16.8kb (17202 bytes) (93.9%)

Showing 2,495 symbols (aliases not grouped for diffs) with total pss: 19525 bytes
Histogram of symbols based on PSS:
    (-16384,-8192]: 1   (-256,-128]: 3     (-16,-8]: 77   (-1,0): 42      [2,4): 84       [32,64): 96
     (-4096,-2048]: 1    (-128,-64]: 50     (-8,-4]: 32      {{0}}: 2       [4,8): 564     [64,128): 305
     (-2048,-1024]: 1     (-64,-32]: 115    (-4,-2]: 23    (0,1): 75     [8,16): 417    [128,256): 2
      (-1024,-512]: 1     (-32,-16]: 166    (-2,-1]: 40    [1,2): 136   [16,32): 261   [512,1024): 1
.text=16.8kb     .rodata=-7.98kb    .data.rel.ro=10.2kb     .data=0 bytes    .bss=1808 bytes .pak.translations=0 bytes    .pak.nontranslated=0 bytes    .other=4 bytes    total=19.1kb
Number of unique paths: 247

Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, p=.pak.translations, P=.pak.nontranslated, o=.other
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
- 0)      -8928 (-43.0%) r@0x0        -8928 (8928->0)    blink/core/CSSPropertyNames.cpp
               blink::propertyNameStringsPool
~ 1)     -12376 (-59.6%) t@0x19bb8a0  -3448 (9004->5556) blink/core/StyleBuilder.cpp
               blink::StyleBuilder::ApplyProperty
- 2)     -14279 (-68.8%) r@0x0        -1903 (1902->0)    blink/core/CSSPropertyNames.cpp
               blink::propertyNameStringsOffsets
+ 3)     -13561 (-65.3%) t@0x1b58b8c  718 (0->716)       third_party/WebKit/Source/core/css/properties/CSSParsingUtils.cpp
               blink::CSSParsingUtils::ParseBackgroundOrMask
~ 4)     -14261 (-68.7%) t@0x1b6c930  -700 (712->12)     third_party/WebKit/Source/core/css/properties/shorthands/Background.cpp
               blink::CSSShorthand::Background::ParseShorthand const (num_aliases=1->2)
~ 5)     -14013 (-67.5%) r@Group      248 (6868->7116)   {{no path}}
               ** merge constants (count=16)
~ 6)     -14223 (-68.5%) t@Group      -210 (0->0)        {{no path}}
               ** symbol gaps (count=9)
- 7)     -14367 (-69.2%) t@0x0        -144 (144->0)      third_party/WebKit/Source/core/css/properties/longhands/PaintStroke.cpp
               blink::CSSLonghand::PaintStroke::ParseSingleValue const
+ 8)     -14223 (-68.5%) t@0x1b5b82c  144 (0->144)       third_party/WebKit/Source/core/css/properties/CSSParsingUtils.cpp
               blink::CSSParsingUtils::ParsePaintStroke
- 9)     -14363 (-69.2%) t@0x0        -140 (140->0)      blink/core/CSSPropertyNames.cpp
               blink::getPropertyNameAtomicString
+ 10)    -14243 (-68.6%) t@0x1b5bf38  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/AlignContent.cpp
               blink::CSSLonghand::AlignContent::GetPropertyNameAtomicString const
+ 11)    -14123 (-68.0%) t@0x19af1f8  120 (0->120)       blink/core/css/properties/CSSUnresolvedProperty.cpp
               blink::CSSLonghand::AlignmentBaseline::GetPropertyNameAtomicString const
+ 12)    -14003 (-67.5%) t@0x1b5c23c  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/AnimationDelay.cpp
               blink::CSSLonghand::AnimationDelay::GetPropertyNameAtomicString const
+ 13)    -13883 (-66.9%) t@0x1b5c3e4  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/AnimationDuration.cpp
               blink::CSSLonghand::AnimationDuration::GetPropertyNameAtomicString const
+ 14)    -13763 (-66.3%) t@0x1b5c7d0  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/AnimationTimingFunction.cpp
               blink::CSSLonghand::AnimationTimingFunction::GetPropertyNameAtomicString const
+ 15)    -13643 (-65.7%) t@0x1b5cad0  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundClip.cpp
               blink::CSSLonghand::BackgroundClip::GetPropertyNameAtomicString const
+ 16)    -13523 (-65.2%) t@0x1b5cc74  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundImage.cpp
               blink::CSSLonghand::BackgroundImage::GetPropertyNameAtomicString const
+ 17)    -13403 (-64.6%) t@0x1b5ccf8  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundOrigin.cpp
               blink::CSSLonghand::BackgroundOrigin::GetPropertyNameAtomicString const
+ 18)    -13283 (-64.0%) t@0x1b5cdf4  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundPositionX.cpp
               blink::CSSLonghand::BackgroundPositionX::GetPropertyNameAtomicString const
+ 19)    -13163 (-63.4%) t@0x1b5cf10  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundPositionY.cpp
               blink::CSSLonghand::BackgroundPositionY::GetPropertyNameAtomicString const
+ 20)    -13043 (-62.8%) t@0x19af58c  120 (0->120)       blink/core/css/properties/CSSUnresolvedProperty.cpp
               blink::CSSLonghand::BackgroundRepeatX::GetPropertyNameAtomicString const
+ 21)    -12923 (-62.3%) t@0x19af610  120 (0->120)       blink/core/css/properties/CSSUnresolvedProperty.cpp
               blink::CSSLonghand::BackgroundRepeatY::GetPropertyNameAtomicString const
+ 22)    -12803 (-61.7%) t@0x1b5cfa0  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BackgroundSize.cpp
               blink::CSSLonghand::BackgroundSize::GetPropertyNameAtomicString const
+ 23)    -12683 (-61.1%) t@0x1b5d108  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BlockSize.cpp
               blink::CSSLonghand::BlockSize::GetPropertyNameAtomicString const
+ 24)    -12563 (-60.5%) t@0x1b5d2d0  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BorderBottomLeftRadius.cpp
               blink::CSSLonghand::BorderBottomLeftRadius::GetPropertyNameAtomicString const
+ 25)    -12443 (-60.0%) t@0x1b5d3d0  120 (0->120)       third_party/WebKit/Source/core/css/properties/longhands/BorderBottomRightRadius.cpp
               blink::CSSLonghand::BorderBottomRightRadius::GetPropertyNameAtomicString const
+ 26)    -12323 (-59.4%) t@0x19af694  120 (0->120)       blink/core/css/properties/CSSUnresolvedProperty.cpp
               blink::CSSLonghand::BorderBottomStyle::GetPropertyNameAtomicString const

Comment 3 by rjwright@google.com, Dec 20 2017

It was supposed to remove some code complexity, but it was always going to add more classes & more code.
Sounds good. So long as the symbol diffs look as expected, feel free to close as Won't Fix.
Status: WontFix (was: Assigned)

Sign in to add a comment