[Sampling profiler] Use vector for module cache storage

std::map isn't providing a benefit for the ModuleCache internal
representation:

* it doesn't support range-based lookup so its find() function cannot
  be used for module lookup by address

* it doesn't have random access iterators so lookup via std::upper_bound
  is O(n)

* given the linear performance, it's more complicated to use than vector

So switch to vector for the internal representation.

R=charliea@chromium.org

Bug: 931418
Change-Id: Ica6b47815d3e496f14f6b67de80fb596cc7dc793
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1478939
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#639497}

base/sampling_heap_profiler/module_cache.cc

@@ -12,28 +12,32 @@ ModuleCache::ModuleCache() = default;
 ModuleCache::~ModuleCache() = default;
 
 const ModuleCache::Module* ModuleCache::GetModuleForAddress(uintptr_t address) {
-  auto it = modules_cache_map_.upper_bound(address);
-  if (it != modules_cache_map_.begin()) {
-    DCHECK(!modules_cache_map_.empty());
-    --it;
-    const Module* module = it->second.get();
-    if (address < module->GetBaseAddress() + module->GetSize())
-      return module;
-  }
+  auto it = std::find_if(modules_.begin(), modules_.end(),
+                         [address](const std::unique_ptr<Module>& module) {
+                           return address >= module->GetBaseAddress() &&
+                                  address < module->GetBaseAddress() +
+                                                module->GetSize();
+                         });
+  if (it != modules_.end())
+    return it->get();
 
   std::unique_ptr<Module> module = CreateModuleForAddress(address);
   if (!module)
     return nullptr;
-  return modules_cache_map_.emplace(module->GetBaseAddress(), std::move(module))
-      .first->second.get();
+  modules_.push_back(std::move(module));
+  return modules_.back().get();
 }
 
 std::vector<const ModuleCache::Module*> ModuleCache::GetModules() const {
   std::vector<const Module*> result;
-  result.reserve(modules_cache_map_.size());
-  for (const auto& it : modules_cache_map_)
-    result.push_back(it.second.get());
+  result.reserve(modules_.size());
+  for (const std::unique_ptr<Module>& module : modules_)
+    result.push_back(module.get());
   return result;
 }
 
+void ModuleCache::InjectModuleForTesting(std::unique_ptr<Module> module) {
+  modules_.push_back(std::move(module));
+}
+
 }  // namespace base

base/sampling_heap_profiler/module_cache.h

@@ -5,7 +5,6 @@
 #ifndef BASE_SAMPLING_HEAP_PROFILER_MODULE_CACHE_H_
 #define BASE_SAMPLING_HEAP_PROFILER_MODULE_CACHE_H_
 
-#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -71,12 +70,18 @@ class BASE_EXPORT ModuleCache {
   const Module* GetModuleForAddress(uintptr_t address);
   std::vector<const Module*> GetModules() const;
 
+  void InjectModuleForTesting(std::unique_ptr<Module> module);
+
  private:
   // Creates a Module object for the specified memory address. Returns null if
   // the address does not belong to a module.
   static std::unique_ptr<Module> CreateModuleForAddress(uintptr_t address);
 
-  std::map<uintptr_t, std::unique_ptr<Module>> modules_cache_map_;
+  // Unsorted vector of cached modules. The number of loaded modules is
+  // generally much less than 100, and more frequently seen modules will tend to
+  // be added earlier and thus be closer to the front to the vector. So linear
+  // search to find modules should be acceptable.
+  std::vector<std::unique_ptr<Module>> modules_;
 };
 
 }  // namespace base

base/sampling_heap_profiler/module_cache_unittest.cc

@@ -2,29 +2,51 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
+
 #include "base/sampling_heap_profiler/module_cache.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 namespace {
 
-class ModuleCacheTest : public ::testing::Test {};
-
 int AFunctionForTest() {
   return 42;
 }
 
-// Checks that ModuleCache returns the same module instance for
-// addresses within the module.
+// Provides a module that is guaranteed to be isolated from (and non-contiguous
+// with) any other module, by placing the module in the middle of a block of
+// heap memory.
+class IsolatedModule : public ModuleCache::Module {
+ public:
+  IsolatedModule() : memory_region_(new char[kRegionSize]) {}
+
+  // ModuleCache::Module
+  uintptr_t GetBaseAddress() const override {
+    // Place the module in the middle of the region.
+    return reinterpret_cast<uintptr_t>(&memory_region_[kRegionSize / 4]);
+  }
+
+  std::string GetId() const override { return ""; }
+  FilePath GetDebugBasename() const override { return FilePath(); }
+  size_t GetSize() const override { return kRegionSize / 2; }
+
+ private:
+  static const int kRegionSize = 100;
+
+  std::unique_ptr<char[]> memory_region_;
+};
+
 #if defined(OS_MACOSX) && !defined(OS_IOS) || defined(OS_WIN)
-#define MAYBE_ModuleCache ModuleCache
-#define MAYBE_ModulesList ModulesList
+#define MAYBE_TEST(TestSuite, TestName) TEST(TestSuite, TestName)
 #else
-#define MAYBE_ModuleCache DISABLED_ModuleCache
-#define MAYBE_ModulesList DISABLED_ModulesList
+#define MAYBE_TEST(TestSuite, TestName) TEST(TestSuite, DISABLED_##TestName)
 #endif
-TEST_F(ModuleCacheTest, MAYBE_ModuleCache) {
+
+// Checks that ModuleCache returns the same module instance for
+// addresses within the module.
+MAYBE_TEST(ModuleCacheTest, LookupCodeAddresses) {
   uintptr_t ptr1 = reinterpret_cast<uintptr_t>(&AFunctionForTest);
   uintptr_t ptr2 = ptr1 + 1;
   ModuleCache cache;
@@ -37,7 +59,21 @@ TEST_F(ModuleCacheTest, MAYBE_ModuleCache) {
   EXPECT_GT(module1->GetBaseAddress() + module1->GetSize(), ptr2);
 }
 
-TEST_F(ModuleCacheTest, MAYBE_ModulesList) {
+MAYBE_TEST(ModuleCacheTest, LookupRange) {
+  ModuleCache cache;
+  auto to_inject = std::make_unique<IsolatedModule>();
+  const ModuleCache::Module* module = to_inject.get();
+  cache.InjectModuleForTesting(std::move(to_inject));
+
+  EXPECT_EQ(nullptr, cache.GetModuleForAddress(module->GetBaseAddress() - 1));
+  EXPECT_EQ(module, cache.GetModuleForAddress(module->GetBaseAddress()));
+  EXPECT_EQ(module, cache.GetModuleForAddress(module->GetBaseAddress() +
+                                              module->GetSize() - 1));
+  EXPECT_EQ(nullptr, cache.GetModuleForAddress(module->GetBaseAddress() +
+                                               module->GetSize()));
+}
+
+MAYBE_TEST(ModuleCacheTest, ModulesList) {
   ModuleCache cache;
   uintptr_t ptr = reinterpret_cast<uintptr_t>(&AFunctionForTest);
   const ModuleCache::Module* module = cache.GetModuleForAddress(ptr);
@@ -46,7 +82,7 @@ TEST_F(ModuleCacheTest, MAYBE_ModulesList) {
   EXPECT_EQ(module, cache.GetModules().front());
 }
 
-TEST_F(ModuleCacheTest, InvalidModule) {
+MAYBE_TEST(ModuleCacheTest, InvalidModule) {
   ModuleCache cache;
   EXPECT_EQ(nullptr, cache.GetModuleForAddress(1));
 }