New issue
Advanced search Search tips

Issue 784924 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

69kb regression in resource_sizes (MonochromePublic.apk) at 516183:516183

Project Member Reported by agrieve@chromium.org, Nov 14 2017

Issue description

Caused by “[parser] Greatly simplify Array spread rewriting code”

V8 Commit: 18cac20c50f2fd9d3238f12c89f1e7be56539b1b

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

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

Based on the CL description, I'd guess that this increase was probably unexpected or might be avoidable.
Please have a look and either:

Close as “Won't Fix” with a short justification, or
Land a revert / fix-up.

 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Nov 14 2017

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

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


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

Android Builder
Supersize output (via: tools/diagnose_bloat.py --subrepo v8 18cac20c5)

1 symbols added (+), 33 changed (~), 66 removed (-), 836417 unchanged (not shown)
Of changed symbols, 34 grew, 66 shrank
Number of unique symbols 512399 -> 512366 (-33)
0 paths added, 1 removed, 2 changed

Showing 100 symbols (aliases not grouped for diffs) with total pss: 70014 bytes
Histogram of symbols based on PSS:
    (-2048,-1024]: 1   (-128,-64]: 11   (-16,-8]: 1         [8,16): 1    [4096,8192): 3
      (-512,-256]: 3    (-64,-32]: 11    (-4,-2]: 1    [1024,2048): 21
      (-256,-128]: 7    (-32,-16]: 16     (-1,0): 15   [2048,4096): 9
.text=68.6kb     .rodata=0 bytes    .data.rel.ro=-256 bytes .data=0 bytes    .bss=0 bytes    total=68.4kb
Number of unique paths: 3

Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
~ 0)       7328 (10.5%) t@0x11fa044  7328 (2764->10092) v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseLeftHandSideExpression
~ 1)      13036 (18.6%) t@0x11fe894  5708 (1484->7192)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseMemberExpressionContinuation
~ 2)      17484 (25.0%) t@0x120eb98  4448 (1240->5688)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseConditionalExpression
~ 3)      20568 (29.4%) t@0x1203a64  3084 (2152->5236)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseForAwaitStatement
~ 4)      23620 (33.7%) t@0x11f5860  3052 (2616->5668)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseClassLiteral
~ 5)      26644 (38.1%) t@0x1207dfc  3024 (1628->4652)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseSwitchStatement
~ 6)      29668 (42.4%) t@0x12113ac  3024 (1440->4464)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseUnaryExpression
~ 7)      32620 (46.6%) t@0x1210380  2952 (1188->4140)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseBinaryExpression
~ 8)      35568 (50.8%) t@0x120d0e4  2948 (1004->3952)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseStandardForLoop
~ 9)      38504 (55.0%) t@0x120c0ec  2936 (1152->4088)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseForEachStatementWithoutDeclarations
~ 10)     41400 (59.1%) t@0x120ada0  2896 (1692->4588)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseForEachStatementWithDeclarations
~ 11)     44268 (63.2%) t@0x11fd4b8  2868 (1552->4420)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseArguments
- 12)     42636 (60.9%) t@0x0        -1632 (1632->0)    v8/src/parsing/parser.cc
               v8::internal::NonPatternRewriter::RewriteExpression
~ 13)     44224 (63.2%) t@0x11f7f8c  1588 (2428->4016)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseObjectPropertyDefinition
~ 14)     45764 (65.4%) t@0x1201484  1540 (844->2384)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseSingleExpressionFunctionBody
~ 15)     47296 (67.6%) t@0x120e258  1532 (836->2368)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseYieldExpression
~ 16)     48820 (69.7%) t@0x11fcc18  1524 (684->2208)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseMemberWithNewPrefixesExpression
~ 17)     50336 (71.9%) t@0x11eb360  1516 (4128->5644)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseAssignmentExpression
~ 18)     51844 (74.1%) t@0x11e7c1c  1508 (1356->2864)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseFormalParameter
~ 19)     53348 (76.2%) t@0x12008e8  1504 (776->2280)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseClassFieldInitializer
~ 20)     54852 (78.4%) t@0x11f6e84  1504 (1344->2848)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseTemplateLiteral
~ 21)     56348 (80.5%) t@0x1209e94  1496 (2012->3508)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseVariableDeclarations
~ 22)     57832 (82.6%) t@0x1204ee0  1484 (1844->3328)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseForStatement
~ 23)     59316 (84.7%) t@0x12068e4  1484 (600->2084)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseThrowStatement
~ 24)     60800 (86.8%) t@0x12075d4  1484 (604->2088)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseWithStatement
~ 25)     62276 (89.0%) t@0x1209028  1476 (1152->2628)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseExpressionOrLabelledStatement
~ 26)     63744 (91.1%) t@0x11ea6a8  1468 (1020->2488)  v8/src/parsing/parser.cc
               v8::internal::Parser::ParseExportDefault
~ 27)     65212 (93.1%) t@0x1203260  1468 (584->2052)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseWhileStatement
~ 28)     66676 (95.2%) t@0x12021a4  1464 (692->2156)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseIfStatement
~ 29)     68128 (97.3%) t@0x1202a10  1452 (676->2128)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseDoWhileStatement
~ 30)     69580 (99.4%) t@0x11f8f3c  1452 (2204->3656)  v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParsePropertyName
~ 31)     71016 (101.4%) t@0x121251c  1436 (496->1932)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParsePostfixExpression
~ 32)     72436 (103.5%) t@0x120604c  1420 (780->2200)   v8/src/parsing/parser.cc
               v8::internal::ParserBase::ParseReturnStatement
Labels: OS-Android
Based on all of these parser symbols getting bigger, I'd guess there's some over-inlining happening?

Comment 4 by adamk@chromium.org, Nov 14 2017

Thanks for the data, this does look like over-inlining. We were already asking for forced inlining here, but my cleanup seems to have enabled a bunch more than was happening before.

This should be a relatively easy matter of removing some always-inline attributes, but I'm not sure the best way to experiment here. Will diagnose_bloat help me bisect, or is there a better tool?
diagnose_bloat is what I use. It's just a wrapper building + running supersize, but does at least do incremental builds (leaves not room for manual error).

You can also just build libchrome manually with target_os="android" is_official_build=true and readelf -S (or "size") to see the new section sizes.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1abeb0caa21301f5ace7177711c4f09f2d6447d9

commit 1abeb0caa21301f5ace7177711c4f09f2d6447d9
Author: Adam Klein <adamk@chromium.org>
Date: Tue Nov 14 23:06:25 2017

[parser] Remove inlining of Parser::RewriteSpreads

Despite the V8_INLINE annotation, it was never actually inlined until
18cac20c50f2fd9d3238f12c89f1e7be56539b1b removed the NonPatternRewriter,
causing all calls to Parser::RewriteNonPattern() to inline RewriteSpreads.

This patch should recover the binary bloat in the attached bug while
retaining the inlining of the rest of RewriteNonPattern, which in
the common case does very little work (and doesn't call out to RewriteSpreads).

Bug:  chromium:784924 
Change-Id: I1c2062b41ceb51a9522d49bdb9353e1840393ca1
Reviewed-on: https://chromium-review.googlesource.com/769442
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49370}
[modify] https://crrev.com/1abeb0caa21301f5ace7177711c4f09f2d6447d9/src/parsing/parser.h

Status: Fixed (was: Assigned)
Saw a 73kb reduction when this rolled. Thanks!

https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev=516581

Sign in to add a comment