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

This reverts commit 733c2818.

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 ef6f0fb3.
>
> 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}

base/BUILD.gn

@@ -1787,10 +1787,14 @@ buildflag_header("synchronization_flags") {
 buildflag_header("anchor_functions_flags") {
   header = "anchor_functions_flags.h"
   header_dir = "base/android/library_loader"
+  _supports_code_ordering = current_cpu == "arm"
 
   # buildflag entries added to this header must also must be manually added to
   # tools/gn/bootstrap/bootstrap.py
-  flags = [ "USE_LLD=$use_lld" ]
+  flags = [
+    "USE_LLD=$use_lld",
+    "SUPPORTS_CODE_ORDERING=$_supports_code_ordering",
+  ]
 }
 
 # This is the subset of files from base that should not be used with a dynamic

base/android/library_loader/anchor_functions.cc

@@ -4,13 +4,11 @@
 
 #include "base/android/library_loader/anchor_functions.h"
 
-#include "base/android/library_loader/anchor_functions_flags.h"
 #include "base/logging.h"
 #include "build/build_config.h"
 
-// asm() macros below don't compile on x86, and haven't been validated outside
-// ARM.
-#if defined(ARCH_CPU_ARMEL)
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
+
 // These functions are here to, respectively:
 // 1. Check that functions are ordered
 // 2. Delimit the start of .text
@@ -68,20 +66,18 @@ const size_t kStartOfText =
 const size_t kEndOfText =
     reinterpret_cast<size_t>(dummy_function_at_the_end_of_text);
 
-void CheckOrderingSanity() {
+bool IsOrderingSane() {
+  size_t dummy = reinterpret_cast<size_t>(&dummy_function_to_check_ordering);
+  size_t here = reinterpret_cast<size_t>(&IsOrderingSane);
   // The linker usually keeps the input file ordering for symbols.
   // dummy_function_to_anchor_text() should then be after
   // dummy_function_to_check_ordering() without ordering.
   // This check is thus intended to catch the lack of ordering.
-  CHECK_LT(kStartOfText,
-           reinterpret_cast<size_t>(&dummy_function_to_check_ordering));
-  CHECK_LT(kStartOfText, kEndOfText);
-  CHECK_LT(kStartOfText,
-           reinterpret_cast<size_t>(&dummy_function_to_check_ordering));
-  CHECK_LT(kStartOfText, reinterpret_cast<size_t>(&CheckOrderingSanity));
-  CHECK_GT(kEndOfText, reinterpret_cast<size_t>(&CheckOrderingSanity));
+  return kStartOfText < dummy && dummy < kEndOfText && kStartOfText < here &&
+         here < kEndOfText;
 }
 
 }  // namespace android
 }  // namespace base
-#endif  // defined(ARCH_CPU_ARMEL)
+
+#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)

base/android/library_loader/anchor_functions.h

@@ -6,11 +6,12 @@
 #define BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_
 
 #include <cstdint>
+#include "base/android/library_loader/anchor_functions_flags.h"
 
 #include "base/base_export.h"
-#include "build/build_config.h"
 
-#if defined(ARCH_CPU_ARMEL)
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
+
 namespace base {
 namespace android {
 
@@ -18,11 +19,11 @@ namespace android {
 BASE_EXPORT extern const size_t kStartOfText;
 BASE_EXPORT extern const size_t kEndOfText;
 
-// Basic CHECK()s ensuring that the symbols above are correctly set.
-BASE_EXPORT void CheckOrderingSanity();
+// Returns true if the ordering looks sane.
+BASE_EXPORT bool IsOrderingSane();
 
 }  // namespace android
 }  // namespace base
-#endif  // defined(ARCH_CPU_ARMEL)
+#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)
 
 #endif  // BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_

base/android/library_loader/library_loader_hooks.cc

@@ -5,6 +5,7 @@
 #include "base/android/library_loader/library_loader_hooks.h"
 
 #include "base/android/jni_string.h"
+#include "base/android/library_loader/anchor_functions_flags.h"
 #include "base/android/library_loader/library_load_from_apk_status_codes.h"
 #include "base/android/library_loader/library_prefetcher.h"
 #include "base/at_exit.h"
@@ -169,7 +170,12 @@ static jboolean JNI_LibraryLoader_LibraryLoaded(
     const JavaParamRef<jobject>& jcaller) {
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kMadviseRandomExecutableCode)) {
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
     NativeLibraryPrefetcher::MadviseRandomText();
+#else
+    LOG(WARNING) << switches::kMadviseRandomExecutableCode
+                 << " is not supported on this architecture.";
+#endif
   }
 
   if (g_native_initialization_hook && !g_native_initialization_hook())
@@ -189,19 +195,31 @@ void LibraryLoaderExitHook() {
 static jboolean JNI_LibraryLoader_ForkAndPrefetchNativeLibrary(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz) {
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
   return NativeLibraryPrefetcher::ForkAndPrefetchNativeLibrary();
+#else
+  return false;
+#endif
 }
 
 static jint JNI_LibraryLoader_PercentageOfResidentNativeLibraryCode(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz) {
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
   return NativeLibraryPrefetcher::PercentageOfResidentNativeLibraryCode();
+#else
+  return -1;
+#endif
 }
 
 static void JNI_LibraryLoader_PeriodicallyCollectResidency(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz) {
-  return NativeLibraryPrefetcher::PeriodicallyCollectResidency();
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
+  NativeLibraryPrefetcher::PercentageOfResidentNativeLibraryCode();
+#else
+  LOG(WARNING) << "Collecting residency is not supported.";
+#endif
 }
 
 void SetVersionNumber(const char* version_number) {

base/android/library_loader/library_prefetcher.h

@@ -8,33 +8,32 @@
 #include <jni.h>
 
 #include <stdint.h>
-#include <string>
-#include <utility>
-#include <vector>
 
-#include "base/debug/proc_maps_linux.h"
+#include "base/android/library_loader/anchor_functions_flags.h"
+#include "base/base_export.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
+
 namespace base {
 namespace android {
 
 // Forks and waits for a process prefetching the native library. This is done in
 // a forked process for the following reasons:
-// - Isolating the main process from mistakes in the parsing. If the parsing
-//   returns an incorrect address, only the forked process will crash.
+// - Isolating the main process from mistakes in getting the address range, only
+//   crashing the forked process in case of mistake.
 // - Not inflating the memory used by the main process uselessly, which could
 //   increase its likelihood to be killed.
 // The forked process has background priority and, since it is not declared to
 // the Android runtime, can be killed at any time, which is not an issue here.
 class BASE_EXPORT NativeLibraryPrefetcher {
  public:
-  using AddressRange = std::pair<uintptr_t, uintptr_t>;
-
-  // Finds the ranges matching the native library, forks a low priority
-  // process pre-fetching these ranges and wait()s for it.
+  // Finds the executable code range, forks a low priority process pre-fetching
+  // it wait()s for the process to exit or die.
   // Returns true for success.
   static bool ForkAndPrefetchNativeLibrary();
+
   // Returns the percentage of the native library code currently resident in
   // memory, or -1 in case of error.
   static int PercentageOfResidentNativeLibraryCode();
@@ -47,38 +46,12 @@ class BASE_EXPORT NativeLibraryPrefetcher {
   static void MadviseRandomText();
 
  private:
-  // Returns true if the region matches native code or data.
-  static bool IsGoodToPrefetch(const base::debug::MappedMemoryRegion& region);
-  // Filters the regions to keep only libchrome ranges if possible.
-  static void FilterLibchromeRangesOnlyIfPossible(
-      const std::vector<base::debug::MappedMemoryRegion>& regions,
-      std::vector<AddressRange>* ranges);
-  // Finds the ranges matching the native library in /proc/self/maps.
-  // Returns true for success.
-  static bool FindRanges(std::vector<AddressRange>* ranges);
-
-  // Returns the percentage of the given address ranges currently resident in
+  // Returns the percentage of [start, end] currently resident in
   // memory, or -1 in case of error.
-  static int PercentageOfResidentCode(const std::vector<AddressRange>& ranges);
+  static int PercentageOfResidentCode(size_t start, size_t end);
 
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestIsGoodToPrefetchNoRange);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestIsGoodToPrefetchUnreadableRange);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestIsGoodToPrefetchSkipSharedRange);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestIsGoodToPrefetchLibchromeRange);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestIsGoodToPrefetchBaseApkRange);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestFilterLibchromeRangesOnlyIfPossibleNoLibchrome);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestFilterLibchromeRangesOnlyIfPossibleHasLibchrome);
   FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
                            TestPercentageOfResidentCode);
-  FRIEND_TEST_ALL_PREFIXES(NativeLibraryPrefetcherTest,
-                           TestPercentageOfResidentCodeTwoRegions);
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(NativeLibraryPrefetcher);
 };
@@ -86,4 +59,6 @@ class BASE_EXPORT NativeLibraryPrefetcher {
 }  // namespace android
 }  // namespace base
 
+#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)
+
 #endif  // BASE_ANDROID_LIBRARY_LOADER_LIBRARY_PREFETCHER_H_

base/android/library_loader/library_prefetcher_unittest.cc

@@ -7,96 +7,15 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <sys/mman.h>
-#include <string>
-#include <vector>
-#include "base/debug/proc_maps_linux.h"
+#include "base/android/library_loader/anchor_functions_flags.h"
 #include "base/memory/shared_memory.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
 namespace base {
 namespace android {
 
-namespace {
-const uint8_t kRead = base::debug::MappedMemoryRegion::READ;
-const uint8_t kReadPrivate = base::debug::MappedMemoryRegion::READ |
-                             base::debug::MappedMemoryRegion::PRIVATE;
-const uint8_t kExecutePrivate = base::debug::MappedMemoryRegion::EXECUTE |
-                                base::debug::MappedMemoryRegion::PRIVATE;
-}  // namespace
-
-TEST(NativeLibraryPrefetcherTest, TestIsGoodToPrefetchNoRange) {
-  const base::debug::MappedMemoryRegion regions[4] = {
-      base::debug::MappedMemoryRegion{0x4000, 0x5000, 10, 0, kReadPrivate, ""},
-      base::debug::MappedMemoryRegion{0x4000, 0x5000, 10, 0, kReadPrivate,
-                                      "foo"},
-      base::debug::MappedMemoryRegion{0x4000, 0x5000, 10, 0, kReadPrivate,
-                                      "foobar.apk"},
-      base::debug::MappedMemoryRegion{0x4000, 0x5000, 10, 0, kReadPrivate,
-                                      "libchromium.so"}};
-  for (int i = 0; i < 4; ++i) {
-    ASSERT_FALSE(NativeLibraryPrefetcher::IsGoodToPrefetch(regions[i]));
-  }
-}
-
-TEST(NativeLibraryPrefetcherTest, TestIsGoodToPrefetchUnreadableRange) {
-  const base::debug::MappedMemoryRegion region = {
-      0x4000, 0x5000, 10, 0, kExecutePrivate, "base.apk"};
-  ASSERT_FALSE(NativeLibraryPrefetcher::IsGoodToPrefetch(region));
-}
-
-TEST(NativeLibraryPrefetcherTest, TestIsGoodToPrefetchSkipSharedRange) {
-  const base::debug::MappedMemoryRegion region = {
-      0x4000, 0x5000, 10, 0, kRead, "base.apk"};
-  ASSERT_FALSE(NativeLibraryPrefetcher::IsGoodToPrefetch(region));
-}
-
-TEST(NativeLibraryPrefetcherTest, TestIsGoodToPrefetchLibchromeRange) {
-  const base::debug::MappedMemoryRegion region = {
-      0x4000, 0x5000, 10, 0, kReadPrivate, "libchrome.so"};
-  ASSERT_TRUE(NativeLibraryPrefetcher::IsGoodToPrefetch(region));
-}
-
-TEST(NativeLibraryPrefetcherTest, TestIsGoodToPrefetchBaseApkRange) {
-  const base::debug::MappedMemoryRegion region = {
-      0x4000, 0x5000, 10, 0, kReadPrivate, "base.apk"};
-  ASSERT_TRUE(NativeLibraryPrefetcher::IsGoodToPrefetch(region));
-}
-
-TEST(NativeLibraryPrefetcherTest,
-     TestFilterLibchromeRangesOnlyIfPossibleNoLibchrome) {
-  std::vector<base::debug::MappedMemoryRegion> regions;
-  regions.push_back(base::debug::MappedMemoryRegion{0x1, 0x2, 0, 0,
-                                                    kReadPrivate, "base.apk"});
-  regions.push_back(base::debug::MappedMemoryRegion{0x3, 0x4, 0, 0,
-                                                    kReadPrivate, "base.apk"});
-  std::vector<NativeLibraryPrefetcher::AddressRange> ranges;
-  NativeLibraryPrefetcher::FilterLibchromeRangesOnlyIfPossible(regions,
-                                                               &ranges);
-  EXPECT_EQ(ranges.size(), 2U);
-  EXPECT_EQ(ranges[0].first, 0x1U);
-  EXPECT_EQ(ranges[0].second, 0x2U);
-  EXPECT_EQ(ranges[1].first, 0x3U);
-  EXPECT_EQ(ranges[1].second, 0x4U);
-}
-
-TEST(NativeLibraryPrefetcherTest,
-     TestFilterLibchromeRangesOnlyIfPossibleHasLibchrome) {
-  std::vector<base::debug::MappedMemoryRegion> regions;
-  regions.push_back(base::debug::MappedMemoryRegion{0x1, 0x2, 0, 0,
-                                                    kReadPrivate, "base.apk"});
-  regions.push_back(base::debug::MappedMemoryRegion{
-      0x6, 0x7, 0, 0, kReadPrivate, "libchrome.so"});
-  regions.push_back(base::debug::MappedMemoryRegion{0x3, 0x4, 0, 0,
-                                                    kReadPrivate, "base.apk"});
-  std::vector<NativeLibraryPrefetcher::AddressRange> ranges;
-  NativeLibraryPrefetcher::FilterLibchromeRangesOnlyIfPossible(regions,
-                                                               &ranges);
-  EXPECT_EQ(ranges.size(), 1U);
-  EXPECT_EQ(ranges[0].first, 0x6U);
-  EXPECT_EQ(ranges[0].second, 0x7U);
-}
-
 // Fails with ASAN, crbug.com/570423.
 #if !defined(ADDRESS_SANITIZER)
 namespace {
@@ -108,55 +27,20 @@ TEST(NativeLibraryPrefetcherTest, TestPercentageOfResidentCode) {
   base::SharedMemory shared_mem;
   ASSERT_TRUE(shared_mem.CreateAndMapAnonymous(length));
   void* address = shared_mem.memory();
-
-  std::vector<NativeLibraryPrefetcher::AddressRange> ranges = {
-      {reinterpret_cast<uintptr_t>(address),
-       reinterpret_cast<uintptr_t>(address) + length}};
+  size_t start = reinterpret_cast<size_t>(address);
+  size_t end = start + length;
 
   // Remove everything.
   ASSERT_EQ(0, madvise(address, length, MADV_DONTNEED));
-  // TODO(lizeb): If flaky, mock mincore().
-  EXPECT_EQ(0, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
+  EXPECT_EQ(0, NativeLibraryPrefetcher::PercentageOfResidentCode(start, end));
 
   // Get everything back.
   ASSERT_EQ(0, mlock(address, length));
-  EXPECT_EQ(100, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
-  munlock(address, length);
-}
-
-TEST(NativeLibraryPrefetcherTest, TestPercentageOfResidentCodeTwoRegions) {
-  size_t length = 4 * kPageSize;
-  base::SharedMemory shared_mem;
-  ASSERT_TRUE(shared_mem.CreateAndMapAnonymous(length));
-  void* address = shared_mem.memory();
-
-  size_t length2 = 8 * kPageSize;
-  base::SharedMemory shared_mem2;
-  ASSERT_TRUE(shared_mem2.CreateAndMapAnonymous(length2));
-  void* address2 = shared_mem2.memory();
-
-  std::vector<NativeLibraryPrefetcher::AddressRange> ranges = {
-      {reinterpret_cast<uintptr_t>(address),
-       reinterpret_cast<uintptr_t>(address) + length},
-      {reinterpret_cast<uintptr_t>(address2),
-       reinterpret_cast<uintptr_t>(address2) + length2}};
-
-  // Remove everything.
-  ASSERT_EQ(0, madvise(address, length, MADV_DONTNEED));
-  ASSERT_EQ(0, madvise(address2, length, MADV_DONTNEED));
-  // TODO(lizeb): If flaky, mock mincore().
-  EXPECT_EQ(0, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
-
-  // Get back the first range.
-  ASSERT_EQ(0, mlock(address, length));
-  EXPECT_EQ(33, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
-  // The second one.
-  ASSERT_EQ(0, mlock(address2, length2));
-  EXPECT_EQ(100, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
+  EXPECT_EQ(100, NativeLibraryPrefetcher::PercentageOfResidentCode(start, end));
   munlock(address, length);
-  munlock(address2, length);
 }
 #endif  // !defined(ADDRESS_SANITIZER)
 
 }  // namespace android
 }  // namespace base
+#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)

tools/cygprofile/lightweight_cygprofile.cc

@@ -161,7 +161,7 @@ void Disable() {
 void SanityChecks() {
   CHECK_LT(base::android::kEndOfText - base::android::kStartOfText,
            kMaxTextSizeInBytes);
-  base::android::CheckOrderingSanity();
+  CHECK(base::android::IsOrderingSane());
 }
 
 bool SwitchToNextPhaseOrDump(int pid, uint64_t start_ns_since_epoch) {

tools/gn/bootstrap/bootstrap.py

@@ -227,7 +227,7 @@ def build_gn_with_ninja_manually(tempdir, options):
 
   write_buildflag_header_manually(
       root_gen_dir, 'base/android/library_loader.h',
-      {'USE_LLD': 'false'})
+      {'USE_LLD': 'false', 'SUPPORTS_CODE_ORDERING': 'false'})
 
   write_gn_ninja(os.path.join(tempdir, 'build.ninja'),
                  root_gen_dir, options)