New issue
Advanced search Search tips

Issue 821113 link

Starred by 1 user

Issue metadata

Status: Duplicate
Owner:
Cc:
EstimatedDays: ----
NextAction: 2018-03-27
OS: ----
Pri: 3
Type: Bug

Blocking:
issue v8:6666



Sign in to add a comment

319kb regression in resource_sizes (MonochromePublic.apk) at 542396:542396

Project Member Reported by mheikal@chromium.org, Mar 12 2018

Issue description

Caused by “Reland "[builtins] Execute binary-embedded builtin code"”

Commit: 5025e41545b9a5528d164bce50b324d7e08b15ae

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

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 graph: 320k native code

The huge increase is from including the generated file v8/embedded.cc

It's not clear to me whether or not this increase was expected.
Please have a look and either:

Close as “Won't Fix” with a short justification, or
Land a revert / fix-up.
 
Section Sizes (Total=320kb (327682 bytes)):
    .bss: 0 bytes (0 bytes) (not included in totals)
    .data: 0 bytes (0 bytes) (0.0%)
    .data.rel.ro: 0 bytes (0 bytes) (0.0%)
    .other: 1006 bytes (1006 bytes) (0.3%)
    .pak.nontranslated: 0 bytes (0 bytes) (0.0%)
    .pak.translations: 0 bytes (0 bytes) (0.0%)
    .rel.dyn: 0 bytes (0 bytes) (0.0%)
    .rodata: 0 bytes (0 bytes) (0.0%)
    .text: 319kb (326672 bytes) (99.7%)

13 symbols added (+), 9 changed (~), 11 removed (-), 852151 unchanged (not shown)
Of changed symbols, 16 grew, 17 shrank
Number of unique symbols 501006 -> 501011 (+5)
1 paths added, 0 removed, 4 changed

Showing 33 symbols (21 -> 23 unique) with total pss: 326670 bytes
Histogram of symbols based on PSS:
    (-256,-128]: 2   (-32,-16]: 1   (-2,-1]: 3     [4,8): 2      [64,128): 2   [262144,524288): 1
     (-128,-64]: 1    (-16,-8]: 1    (-1,0): 3    [8,16): 5     [256,512): 1
      (-64,-32]: 4     (-8,-4]: 2     (0,1): 1   [32,64): 3   [4096,8192): 1
.text=319kb      .rodata=0 bytes    .data.rel.ro=0 bytes    .data=0 bytes    .bss=0 bytes    .pak.translations=0 bytes    .pak.nontranslated=0 bytes    .other=-2 bytes   total=319kb
Number of unique paths: 6

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)     319548 (97.8%) t@0x15fe660  319548 (0->319520) v8/embedded.cc
~ 1)     326700 (100.0%) t@Group      7152 (351068->358220) {{no path}}
               ** lld merge strings (count=2)
+ 2)     326956 (100.1%) t@0x140e14c  256 (0->256)       v8/src/isolate.cc
               v8::internal::CreateOnHeapTrampolines
- 3)     326732 (100.0%) t@0x0        -224 (224->0)      v8/src/isolate.cc
               v8::internal::MoveBuiltinsOffHeap
- 4)     326588 (100.0%) t@0x0        -144 (144->0)      v8/src/instruction-stream.cc
               v8::internal::InstructionStream::InstructionStream
+ 5)     326706 (100.0%) r@0x0        118 (0->0)         {{no path}}
               ** aggregate padding of diff'ed symbols
~ 6)     326774 (100.0%) t@0x13f07b8  68 (72->140)       v8/src/instruction-stream.cc
               v8::internal::InstructionStream::TryLookupCode
- 7)     326708 (100.0%) r@0x0        -66 (66->0)        v8/src/instruction-stream.cc
               string literal
- 8)     326656 (100.0%) t@0x0        -52 (52->0)        v8/src/instruction-stream.cc
               v8::internal::InstructionStream::~InstructionStream
- 9)     326608 (100.0%) t@0x0        -48 (48->0)        v8/src/instruction-stream.cc
               v8::internal::InstructionStream::TryLookupInstructionStream
~ 10)    326560 (100.0%) t@0x140bf5c  -48 (556->508)     v8/src/isolate.cc
               v8::internal::Isolate::Deinit
~ 11)    326606 (100.0%) t@0x145290e  46 (28->74)        v8/src/objects.cc
               v8::internal::Code::OffHeapInstructionEnd
+ 12)    326646 (100.0%) t@0x13f0790  40 (0->40)         v8/src/instruction-stream.cc
               v8::internal::InstructionStream::PcIsOffHeap
~ 13)    326682 (100.0%) t@0x14528d2  36 (24->60)        v8/src/objects.cc
               v8::internal::Code::OffHeapInstructionStart
- 14)    326650 (100.0%) r@0x0        -32 (32->0)        v8/src/instruction-stream.cc
               string literal
- 15)    326630 (100.0%) r@0x0        -20 (20->0)        v8/src/instruction-stream.cc
               string literal
+ 16)    326644 (100.0%) t@0x152752c  14 (0->14)         v8/src/snapshot/snapshot-common.cc
               v8::internal::EmbeddedData::InstructionStartOfBuiltin const
+ 17)    326656 (100.0%) t@0x164c680  12 (0->12)         v8/embedded.cc
               v8::internal::DefaultEmbeddedBlob
+ 18)    326668 (100.0%) t@0x152753a  12 (0->12)         v8/src/snapshot/snapshot-common.cc
               v8::internal::EmbeddedData::InstructionSizeOfBuiltin const
~ 19)    326656 (100.0%) t@0x140c2c4  -12 (738->726)     v8/src/isolate.cc
               v8::internal::Isolate::~Isolate
+ 20)    326666 (100.0%) t@0x164c68c  +10 (0->10)        v8/embedded.cc
               v8::internal::DefaultEmbeddedBlobSize
+ 21)    326674 (100.0%) t@0x13f0788  +8 (0->8)          v8/src/instruction-stream.cc
               v8::internal::InstructionStream::InstructionStream
+ 22)    326668 (100.0%) t@0x0        -6 (0->0)          {{no path}}
               ** aggregate padding of diff'ed symbols
+ 23)    326672 (100.0%) t@0x1408dec  +4 (0->4)          v8/src/isolate.cc
               v8::internal::Isolate::embedded_blob const
+ 24)    326676 (100.0%) t@0x1408df0  +4 (0->4)          v8/src/isolate.cc
               v8::internal::Isolate::embedded_blob_size const
~ 25)    326672 (100.0%) t@0x143cd8c  -4 (6064->6060)    v8/src/objects.cc
               v8::internal::JSObject::MigrateToMap
- 26)    326670 (100.0%) t@0x0        -1.2 (1.2->0)      v8/src/isolate.cc
               std::__ndk1::vector<>::__emplace_back_slow_path<> (num_aliases=125)
~ 27)    326669 (100.0%) o@0x0        -1 (1010->1009)    {{no path}}
               META-INF/CHROMIUM.RSA
~ 28)    326668 (100.0%) o@0x0        -1 (73816->73815)  {{no path}}

I 824943 Creating: Resource Sizes Diff

******************************Resource Sizes Diff******************************
MonochromePublic.apk_Breakdown (+327,682 bytes)
  +327,684 bytes Native code size
        -2 bytes Package metadata size
MonochromePublic.apk_Specifics
  +327,682 bytes normalized apk size
  +327,684 bytes main lib size

Cc: agrieve@chromium.org
Blocking: v8:6666
NextAction: 2018-03-27
Status: Assigned (was: Untriaged)
Thanks for triaging - the increase is expected. What's going on is that generated code for builtins is currently being moved from the snapshot (snapshot_blob.bin) to the binary itself. 

We're currently in an intermediate state in which the code is in both snapshot and binary, hence the size increase. An upcoming CL will remove (now duplicate) code from the snapshot. This will happen before the next branch point. 

Leaving this open for now.
Status: Duplicate (was: Assigned)
This has since been fixed. Dupe of  https://crbug.com/822287 .
The NextAction date has arrived: 2018-03-27
Here's the v8 roll with the fix:
https://chromeperf.appspot.com/report?sid=108cc26f8fd662831bad1550f602fd114b618dede22636424ae8fb94a83188b7&rev=544004

So, increased by 320, shrank by 290. Seems fine, but let me know if it's not what you expected.
Seems about right. The net difference can be explained by 1. slight increase in code size due to indirections when loading constants / external references, 2. data in snapshot-serialization format is usually a bit smaller than its on-heap form, and 3. the snapshot now contains tiny trampolines for each embedded builtin.

FYI I assume a few follow-up CLs reducing indirections for external references will also slightly decrease the snapshot & binary size.

Thanks for checking :)

Sign in to add a comment