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/886321Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531858}

base/BUILD.gn

@@ -1787,14 +1787,10 @@ 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",
-    "SUPPORTS_CODE_ORDERING=$_supports_code_ordering",
-  ]
+  flags = [ "USE_LLD=$use_lld" ]
 }
 
 # This is the subset of files from base that should not be used with a dynamic

base/android/library_loader/anchor_functions.cc

@@ -4,11 +4,13 @@
 
 #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"
 
-#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
-
+// asm() macros below don't compile on x86, and haven't been validated outside
+// ARM.
+#if defined(ARCH_CPU_ARMEL)
 // These functions are here to, respectively:
 // 1. Check that functions are ordered
 // 2. Delimit the start of .text
@@ -66,18 +68,20 @@ const size_t kStartOfText =
 const size_t kEndOfText =
     reinterpret_cast<size_t>(dummy_function_at_the_end_of_text);
 
-bool IsOrderingSane() {
-  size_t dummy = reinterpret_cast<size_t>(&dummy_function_to_check_ordering);
-  size_t here = reinterpret_cast<size_t>(&IsOrderingSane);
+void CheckOrderingSanity() {
   // 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.
-  return kStartOfText < dummy && dummy < kEndOfText && kStartOfText < here &&
-         here < kEndOfText;
+  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));
 }
 
 }  // namespace android
 }  // namespace base
-
-#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)
+#endif  // defined(ARCH_CPU_ARMEL)

base/android/library_loader/anchor_functions.h

@@ -6,12 +6,11 @@
 #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 BUILDFLAG(SUPPORTS_CODE_ORDERING)
-
+#if defined(ARCH_CPU_ARMEL)
 namespace base {
 namespace android {
 
@@ -19,11 +18,11 @@ namespace android {
 BASE_EXPORT extern const size_t kStartOfText;
 BASE_EXPORT extern const size_t kEndOfText;
 
-// Returns true if the ordering looks sane.
-BASE_EXPORT bool IsOrderingSane();
+// Basic CHECK()s ensuring that the symbols above are correctly set.
+BASE_EXPORT void CheckOrderingSanity();
 
 }  // namespace android
 }  // namespace base
-#endif  // BUILDFLAG(SUPPORTS_CODE_ORDERING)
+#endif  // defined(ARCH_CPU_ARMEL)
 
 #endif  // BASE_ANDROID_LIBRARY_LOADER_ANCHOR_FUNCTIONS_H_

base/android/library_loader/library_loader_hooks.cc

@@ -5,7 +5,6 @@
 #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"
@@ -170,12 +169,7 @@ 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())
@@ -195,31 +189,19 @@ 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) {
-#if BUILDFLAG(SUPPORTS_CODE_ORDERING)
-  NativeLibraryPrefetcher::PercentageOfResidentNativeLibraryCode();
-#else
-  LOG(WARNING) << "Collecting residency is not supported.";
-#endif
+  return NativeLibraryPrefetcher::PeriodicallyCollectResidency();
 }
 
 void SetVersionNumber(const char* version_number) {

base/android/library_loader/library_prefetcher.h

@@ -8,32 +8,33 @@
 #include <jni.h>
 
 #include <stdint.h>
+#include <string>
+#include <utility>
+#include <vector>
 
-#include "base/android/library_loader/anchor_functions_flags.h"
-#include "base/base_export.h"
+#include "base/debug/proc_maps_linux.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 getting the address range, only
-//   crashing the forked process in case of mistake.
+// - Isolating the main process from mistakes in the parsing. If the parsing
+//   returns an incorrect address, only the forked process will crash.
 // - 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:
-  // Finds the executable code range, forks a low priority process pre-fetching
-  // it wait()s for the process to exit or die.
+  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.
   // 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();
@@ -46,12 +47,38 @@ class BASE_EXPORT NativeLibraryPrefetcher {
   static void MadviseRandomText();
 
  private:
-  // Returns the percentage of [start, end] currently resident in
+  // 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
   // memory, or -1 in case of error.
-  static int PercentageOfResidentCode(size_t start, size_t end);
+  static int PercentageOfResidentCode(const std::vector<AddressRange>& ranges);
 
+  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);
 };
@@ -59,6 +86,4 @@ 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,15 +7,96 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <sys/mman.h>
-#include "base/android/library_loader/anchor_functions_flags.h"
+#include <string>
+#include <vector>
+#include "base/debug/proc_maps_linux.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 {
@@ -27,20 +108,55 @@ TEST(NativeLibraryPrefetcherTest, TestPercentageOfResidentCode) {
   base::SharedMemory shared_mem;
   ASSERT_TRUE(shared_mem.CreateAndMapAnonymous(length));
   void* address = shared_mem.memory();
-  size_t start = reinterpret_cast<size_t>(address);
-  size_t end = start + length;
+
+  std::vector<NativeLibraryPrefetcher::AddressRange> ranges = {
+      {reinterpret_cast<uintptr_t>(address),
+       reinterpret_cast<uintptr_t>(address) + length}};
 
   // Remove everything.
   ASSERT_EQ(0, madvise(address, length, MADV_DONTNEED));
-  EXPECT_EQ(0, NativeLibraryPrefetcher::PercentageOfResidentCode(start, end));
+  // TODO(lizeb): If flaky, mock mincore().
+  EXPECT_EQ(0, NativeLibraryPrefetcher::PercentageOfResidentCode(ranges));
 
   // Get everything back.
   ASSERT_EQ(0, mlock(address, length));
-  EXPECT_EQ(100, NativeLibraryPrefetcher::PercentageOfResidentCode(start, end));
+  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));
   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);
-  CHECK(base::android::IsOrderingSane());
+  base::android::CheckOrderingSanity();
 }
 
 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', 'SUPPORTS_CODE_ORDERING': 'false'})
+      {'USE_LLD': 'false'})
 
   write_gn_ninja(os.path.join(tempdir, 'build.ninja'),
                  root_gen_dir, options)