New issue
Advanced search Search tips

Issue 805883 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

MessageLoop start time regression due to code ordering on public bots.

Project Member Reported by lizeb@chromium.org, Jan 25 2018

Issue description

See
https://chromeperf.appspot.com/report?sid=9df0ec76b0fffc74931cc7bdcf44cd4d540f81bb34ccde3571c2211ec8360ee6&start_rev=531281&end_rev=531574

The culprit is this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/874470

Which was reverted in:
https://chromium-review.googlesource.com/c/chromium/src/+/886321

The regression is in the ~80ms range for cold start.


But... the CL above is actually an improvement.
The reason is that it introduces a new way to prefetch the native library on startup. This change makes the prefetch more selective (only .text instead of .text, .rodata and other things), but it also requires code to be ordered in order to kick in.
This can be checked by seeing that the prefetch task is <1ms on these bots (and from logcat).

The regression we see on the bots are only for the ones running chrome_public_apk, which doesn't use ordering. As a consequence, prefetch doesn't happen. This means that the benefit of prefetching is ~80ms with an unordered native library.
Internal bots with code ordered showed a ~40-60ms improvement that disappeared with the revert, see https://chromeperf.appspot.com/report?sid=15c2861109c546f72304a81d85a3692578ce2256e4b012df32602fa8f7b72089&start_rev=1516160166&end_rev=1516881481

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff4e4bf57b472519927fd4695973e49a12c6081b

commit ff4e4bf57b472519927fd4695973e49a12c6081b
Author: Benoit L <lizeb@chromium.org>
Date: Thu Jan 25 13:32:40 2018

Reland "android: Don't parse /proc/self/maps to prefetch the library."

This reverts commit 733c281814fea37fb2d1e3d98c3597e9a9f7fde3.

Reason for reland: The initial change is actually an improvement, see
the linked bug.

Original change's description:
> Revert "android: Don't parse /proc/self/maps to prefetch the library."
>
> This reverts commit ef6f0fb35c08f1fbe5d44a183f470d1bb7a84af7.
>
> Reason for revert: Regressed startup performance.
> See https://chromeperf.appspot.com/report?sid=9df0ec76b0fffc74931cc7bdcf44cd4d540f81bb34ccde3571c2211ec8360ee6&start_rev=531281&end_rev=531574
>
> Original change's description:
> > android: Don't parse /proc/self/maps to prefetch the library.
> >
> > This commit changes the behavior of the native library prefetcher to
> > rely on sentinel symbols rather than parsing /proc/self/maps.
> >
> > Behavior changes:
> > 1. This will make the code no longer prefetch .data (because we don't
> >    look for it in the mappings), neither the sections that are part of
> >    the same mapping as .text (for instance, .rodata when using
> >    ld.gold). This is intended.
> > 2. As a consequence, the UMA metric
> >    LibraryLoader.PercentageOfResidentCodeBeforePrefetch will move, as
> >    both sides of the ratio will change, and the population will change
> >    slightly as well (since code is currently not correctly ordered on
> >    ARM64 and x86_64)
> >
> > - Removes the reliance on a somewhat brittle parsing of a file
> > - Parsing /proc/self/maps costs ~50ms of CPU on a Nexus 5X (on a little
> >   core, as this is triggered from an AsyncTask)
> > - Allows to only fetch part of the native library (in a forthcoming CL)
> >
> > Rationale:
> > Change-Id: I0bb7b28af3c3bd4af5745e2ebcc1fbf283bcc0c1
> > Reviewed-on: https://chromium-review.googlesource.com/874470
> > Commit-Queue: Benoit L <lizeb@chromium.org>
> > Reviewed-by: Egor Pasko <pasko@chromium.org>
> > Reviewed-by: agrieve <agrieve@chromium.org>
> > Reviewed-by: Matthew Cary <mattcary@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#531515}
>
> TBR=pasko@chromium.org,agrieve@chromium.org,lizeb@chromium.org,mattcary@chromium.org
>
> Change-Id: I11d7cd7898d5f8d8fbebf391eba5fdaca154c08a
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/886321
> Reviewed-by: Benoit L <lizeb@chromium.org>
> Commit-Queue: Benoit L <lizeb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#531858}

TBR=pasko@chromium.org,agrieve@chromium.org,lizeb@chromium.org,mattcary@chromium.org
Bug:  805883 

Change-Id: Ida889088ec69c09f4dd95dd976174b5532811a53
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/886621
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531880}
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/BUILD.gn
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/anchor_functions.cc
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/anchor_functions.h
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/library_loader_hooks.cc
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/library_prefetcher.cc
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/library_prefetcher.h
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/base/android/library_loader/library_prefetcher_unittest.cc
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/tools/cygprofile/lightweight_cygprofile.cc
[modify] https://crrev.com/ff4e4bf57b472519927fd4695973e49a12c6081b/tools/gn/bootstrap/bootstrap.py

Comment 2 by lizeb@chromium.org, Jan 25 2018

Status: Fixed (was: Assigned)

Sign in to add a comment