Issue metadata
Sign in to add a comment
|
57kb regression in resource_sizes (MonochromePublic.apk) at 500277:500277 |
||||||||||||||||||||
Issue descriptionCaused by “[bootstrapper] Reliably set SFI::lazy_deserialization_builtin_id” V8 Commit: 258e42cda68bcef8c9bc46ab4fbc6b44a492ffe6 Link to size graph: https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev= 500277 Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase Looks like growth was entirely from native code, and enitrely from bootstrapper.cc: Showing 15 symbols (aliases not grouped for diffs) with total pss: 61095 bytes .text=59.7kb .rodata=0 bytes .data.rel.ro=0 bytes .data=0 bytes .bss=0 bytes total=59.7kb Number of unique paths: 2 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) 57628 (94.3%) t@0xf4a718 57628 (48468->106096) v8/src/bootstrapper.cc v8::internal::Genesis::InitializeGlobal ~ 1) 58920 (96.4%) t@0xf482b0 1292 (1948->3240) v8/src/bootstrapper.cc v8::internal::Genesis::CreateAsyncIteratorMaps ~ 2) 59724 (97.8%) t@0xf4777c 804 (2064->2868) v8/src/bootstrapper.cc v8::internal::Genesis::CreateIteratorMaps ~ 3) 60264 (98.6%) t@0xf69e7c 540 (6736->7276) v8/src/bootstrapper.cc v8::internal::Genesis::InstallNatives ~ 4) 60648 (99.3%) t@0xf6476c 384 (1540->1924) v8/src/bootstrapper.cc v8::internal::InstallError ~ 5) 60932 (99.7%) t@0xf64fbc 284 (416->700) v8/src/bootstrapper.cc v8::internal::Genesis::CreateArrayBuffer ~ 6) 61188 (100.2%) t@0xf66e38 256 (11588->11844) v8/src/bootstrapper.cc v8::internal::Bootstrapper::ExportFromRuntime ~ 7) 61404 (100.5%) t@0xf65734 216 (1012->1228) v8/src/bootstrapper.cc v8::internal::Genesis::InitializeGlobal_harmony_promise_finally ~ 8) 61244 (100.2%) t@0xf64588 -160 (412->252) v8/src/bootstrapper.cc v8::internal::SimpleInstallGetterSetter ~ 9) 61152 (100.1%) t@0xf6d0d8 -92 (244->152) v8/src/bootstrapper.cc v8::internal::SimpleInstallFunction ~ 10) 61068 (100.0%) t@0xf6d32c -84 (264->180) v8/src/bootstrapper.cc v8::internal::SimpleInstallGetter ~ 11) 61098 (100.0%) t@0xf476c4 30 (152->182) v8/src/bootstrapper.cc v8::internal::CreateFunction ~ 12) 61122 (100.0%) t@0xf65c00 24 (372->396) v8/src/bootstrapper.cc v8::internal::Genesis::InitializeGlobal_harmony_number_format_to_parts 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.
,
Sep 15 2017
An increase here is not expected. The only way I can explain this is that the compiler decides to inline CreateFunction [0], where I added
if (Builtins::IsLazy(call)) {
result->shared()->set_lazy_deserialization_builtin_id(call);
}
See e.g. the growth in Genesis::CreateAsyncIteratorMaps, which was not touched directly by this CL, but which does call into CreateFunction multiple times.
I'll try marking these as V8_NOINLINE. And FYI we also have plans to unship some of bootstrapper.cc that's not required by snapshot builds.
[0] https://chromium-review.googlesource.com/c/v8/v8/+/652477/5/src/bootstrapper.cc#368
,
Sep 15 2017
I'm a bit surprised though that this doesn't show up on V8 binary size graphs at all: https://chromeperf.appspot.com/report?sid=047bc17f652b59e286699cd18945a39b0a417fa88e9a79f989cd636c1c5db60a&start_rev=47845&end_rev=48024 (Look for point ID 47867.)
,
Sep 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/01dcb1ef26b71c6b14c1a9e28e8f85e1da941f5f commit 01dcb1ef26b71c6b14c1a9e28e8f85e1da941f5f Author: Jakob Gruber <jgruber@chromium.org> Date: Fri Sep 15 09:43:16 2017 [bootstrapper] Mark helper functions V8_NOINLINE Don't inline these functions to avoid regressions in APK size. Bug: chromium:763185 Change-Id: I0a1ca16661a460728e56b67a7109be943397cbf5 Reviewed-on: https://chromium-review.googlesource.com/667109 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#48027} [modify] https://crrev.com/01dcb1ef26b71c6b14c1a9e28e8f85e1da941f5f/src/bootstrapper.cc
,
Sep 15 2017
V8_NOINLINE landed, setting next action to verify effect on bots.
,
Sep 19 2017
The NextAction date has arrived: 2017-09-19
,
Sep 20 2017
'MonochromePublic.apk_Specifics / normalized apk size' decreased by 223K during this roll, closing.
,
Sep 20 2017
Ran to confirm: tools/binary_size/diagnose_bloat.py --cloud 49a21bf75fa559f77bf5584130630af1dbda971a First symbols shows the reduction: ~ 0) -69288 (117.5%) t@0xf6f9c8 -69288 (107092->37804) v8/src/bootstrapper.cc v8::internal::Genesis::InitializeGlobal Thanks for the fix!
,
Sep 20 2017
Had a quick look where the remaining improvements came from: -167,897 bytes V8 Snapshots size // CSA write barriers & co. -61,376 bytes Native code size // NOINLINE bootstrapper.cc Nice :) |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Sep 8 2017