[Sampling profiler] Move Windows module cache into ModuleCache

Moves the parallel module caching implementation for Windows into
ModuleCache. This implementation will be merged with the existing module
caching implementation over the course of later refactoring CLs.

Bug: 931418
Change-Id: I65309bca1edc2675e7e5d74e0305bf5cfe017fd1
Reviewed-on: https://chromium-review.googlesource.com/c/1478035
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634493}

base/profiler/native_stack_sampler_win.cc

@@ -19,7 +19,6 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/no_destructor.h"
 #include "base/profiler/win32_stack_frame_unwinder.h"
 #include "base/sampling_heap_profiler/module_cache.h"
 #include "base/stl_util.h"
@@ -428,24 +427,22 @@ class NativeStackSamplerWin : public NativeStackSampler {
 
   win::ScopedHandle thread_handle_;
 
+  ModuleCache* module_cache_;
+
   NativeStackSamplerTestDelegate* const test_delegate_;
 
   // The stack base address corresponding to |thread_handle_|.
   const void* const thread_stack_base_address_;
 
-  // The module objects, indexed by the module handle.
-  std::map<HMODULE, std::unique_ptr<ModuleCache::Module>> module_cache_;
-
   DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerWin);
 };
 
 NativeStackSamplerWin::NativeStackSamplerWin(
     win::ScopedHandle thread_handle,
-    // Ignored for now, until we can switch the internal module cache to use
-    // this one.
     ModuleCache* module_cache,
     NativeStackSamplerTestDelegate* test_delegate)
     : thread_handle_(thread_handle.Take()),
+      module_cache_(module_cache),
       test_delegate_(test_delegate),
       thread_stack_base_address_(
           GetThreadEnvironmentBlock(thread_handle_.Get())->Tib.StackBase) {}
@@ -453,7 +450,6 @@ NativeStackSamplerWin::NativeStackSamplerWin(
 NativeStackSamplerWin::~NativeStackSamplerWin() {}
 
 void NativeStackSamplerWin::ProfileRecordingStarting() {
-  module_cache_.clear();
 }
 
 std::vector<Frame> NativeStackSamplerWin::RecordStackFrames(
@@ -486,32 +482,12 @@ std::vector<Frame> NativeStackSamplerWin::CreateFrames(
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cpu_profiler.debug"),
                "NativeStackSamplerWin::CreateFrames");
 
-  static NoDestructor<ModuleCache::Module> invalid_module;
-
   std::vector<Frame> frames;
   frames.reserve(stack.size());
 
   for (const auto& frame : stack) {
-    auto frame_ip = reinterpret_cast<uintptr_t>(frame.instruction_pointer);
-
-    HMODULE module_handle = frame.module.Get();
-    if (!module_handle) {
-      frames.emplace_back(frame_ip, invalid_module.get());
-      continue;
-    }
-
-    auto loc = module_cache_.find(module_handle);
-    if (loc != module_cache_.end()) {
-      frames.emplace_back(frame_ip, loc->second.get());
-      continue;
-    }
-
-    std::unique_ptr<ModuleCache::Module> module =
-        ModuleCache::CreateModuleForHandle(module_handle);
-    frames.emplace_back(frame_ip,
-                        module->is_valid ? module.get() : invalid_module.get());
-    if (module->is_valid)
-      module_cache_.insert(std::make_pair(module_handle, std::move(module)));
+    frames.emplace_back(reinterpret_cast<uintptr_t>(frame.instruction_pointer),
+                        module_cache_->GetModuleForHandle(frame.module.Get()));
   }
 
   return frames;

base/sampling_heap_profiler/module_cache.h

@@ -64,6 +64,9 @@ class BASE_EXPORT ModuleCache {
   ModuleCache();
   ~ModuleCache();
 
+  // Gets the module containing |address| or an invalid module if |address| is
+  // not within a module. The returned module remains owned by and has the same
+  // lifetime as the ModuleCache object.
   const Module* GetModuleForAddress(uintptr_t address);
   std::vector<const Module*> GetModules() const;
 
@@ -84,8 +87,13 @@ class BASE_EXPORT ModuleCache {
 #endif
 
 #if defined(OS_WIN)
+  const Module* GetModuleForHandle(HMODULE module_handle);
   static std::unique_ptr<Module> CreateModuleForHandle(HMODULE module_handle);
   friend class NativeStackSamplerWin;
+
+  // The module objects, indexed by the module handle.
+  // TODO(wittman): Merge this state into modules_cache_map_ and remove
+  std::map<HMODULE, std::unique_ptr<Module>> win_module_cache_;
 #endif
 
   std::map<uintptr_t, std::unique_ptr<Module>> modules_cache_map_;

base/sampling_heap_profiler/module_cache_win.cc

@@ -7,6 +7,7 @@
 #include <objbase.h>
 #include <psapi.h>
 
+#include "base/no_destructor.h"
 #include "base/process/process_handle.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -80,6 +81,27 @@ std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForAddress(
   return module;
 }
 
+const ModuleCache::Module* ModuleCache::GetModuleForHandle(
+    HMODULE module_handle) {
+  static NoDestructor<ModuleCache::Module> invalid_module;
+
+  if (!module_handle)
+    return invalid_module.get();
+
+  auto loc = win_module_cache_.find(module_handle);
+  if (loc != win_module_cache_.end())
+    return loc->second.get();
+
+  std::unique_ptr<ModuleCache::Module> module =
+      ModuleCache::CreateModuleForHandle(module_handle);
+  if (module->is_valid) {
+    const auto result = win_module_cache_.insert(
+        std::make_pair(module_handle, std::move(module)));
+    return result.first->second.get();
+  }
+  return invalid_module.get();
+}
+
 // static
 std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForHandle(
     HMODULE module_handle) {

components/metrics/call_stack_profile_builder.cc

@@ -95,18 +95,17 @@ void CallStackProfileBuilder::OnSampleCompleted(
       continue;
 
     // Dedup modules.
-    const base::ModuleCache::Module* module = frame.module;
-    auto module_loc = module_index_.find(module->base_address);
+    auto module_loc = module_index_.find(frame.module);
     if (module_loc == module_index_.end()) {
-      modules_.push_back(module);
+      modules_.push_back(frame.module);
       size_t index = modules_.size() - 1;
-      module_loc = module_index_.emplace(module->base_address, index).first;
+      module_loc = module_index_.emplace(frame.module, index).first;
     }
 
     // Write CallStackProfile::Location protobuf message.
     ptrdiff_t module_offset =
         reinterpret_cast<const char*>(frame.instruction_pointer) -
-        reinterpret_cast<const char*>(module->base_address);
+        reinterpret_cast<const char*>(frame.module->base_address);
     DCHECK_GE(module_offset, 0);
     location->set_address(static_cast<uint64_t>(module_offset));
     location->set_module_id_index(module_loc->second);

components/metrics/call_stack_profile_builder.h

@@ -120,8 +120,8 @@ class CallStackProfileBuilder
   // The indexes of stacks, indexed by stack's address.
   std::map<const CallStackProfile::Stack*, int, StackComparer> stack_index_;
 
-  // The indexes of modules, indexed by module's base_address.
-  std::unordered_map<uintptr_t, size_t> module_index_;
+  // The indexes of modules in the modules_ vector below..
+  std::unordered_map<const base::ModuleCache::Module*, size_t> module_index_;
 
   // The distinct modules in the current profile.
   std::vector<const base::ModuleCache::Module*> modules_;

components/metrics/call_stack_profile_builder_unittest.cc

@@ -311,11 +311,9 @@ TEST(CallStackProfileBuilderTest, DedupModules) {
   base::FilePath module_path("/some/path/to/chrome");
 #endif
 
-  Module module1 = {module_base_address, "1", module_path};
-  Frame frame1 = {module_base_address + 0x10, &module1};
-
-  Module module2 = {module_base_address, "1", module_path};
-  Frame frame2 = {module_base_address + 0x20, &module2};
+  Module module = {module_base_address, "1", module_path};
+  Frame frame1 = {module_base_address + 0x10, &module};
+  Frame frame2 = {module_base_address + 0x20, &module};
 
   std::vector<Frame> frames = {frame1, frame2};
 
@@ -334,8 +332,8 @@ TEST(CallStackProfileBuilderTest, DedupModules) {
   ASSERT_EQ(1, profile.stack_size());
   ASSERT_EQ(2, profile.stack(0).frame_size());
 
-  // Since module1 and module2 have the same base address, they are considered
-  // the same module and therefore deduped.
+  // The two frames share the same module, which should be deduped in the
+  // output.
   ASSERT_TRUE(profile.stack(0).frame(0).has_module_id_index());
   EXPECT_EQ(0, profile.stack(0).frame(0).module_id_index());
   ASSERT_TRUE(profile.stack(0).frame(0).has_address());