[heap profiler] Move observer notifications out of the base::Lock

The stack traversal call seems to be long enough causing blocking other
threads from execution.

BUG=1002285

Change-Id: I24980c72d5fcddd395eaaaab4e7fba32563c250c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797322Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695791}

base/sampling_heap_profiler/poisson_allocation_sampler.cc

@@ -457,17 +457,21 @@ void PoissonAllocationSampler::DoRecordAlloc(intptr_t accumulated_bytes,
     return;
 
   ScopedMuteThreadSamples no_reentrancy_scope;
-  AutoLock lock(mutex_);
+  std::vector<SamplesObserver*> observers_copy;
+  {
+    AutoLock lock(mutex_);
 
-  // TODO(alph): Sometimes RecordAlloc is called twice in a row without
-  // a RecordFree in between. Investigate it.
-  if (sampled_addresses_set().Contains(address))
-    return;
-  sampled_addresses_set().Insert(address);
-  BalanceAddressesHashSet();
+    // TODO(alph): Sometimes RecordAlloc is called twice in a row without
+    // a RecordFree in between. Investigate it.
+    if (sampled_addresses_set().Contains(address))
+      return;
+    sampled_addresses_set().Insert(address);
+    BalanceAddressesHashSet();
+    observers_copy = observers_;
+  }
 
   size_t total_allocated = mean_interval * samples;
-  for (auto* observer : observers_)
+  for (auto* observer : observers_copy)
     observer->SampleAdded(address, size, total_allocated, type, context);
 }
 
@@ -479,10 +483,14 @@ void PoissonAllocationSampler::DoRecordFree(void* address) {
   // thus reenter DoRecordAlloc. However the call chain won't build up further
   // as RecordAlloc accesses are guarded with pthread TLS-based ReentryGuard.
   ScopedMuteThreadSamples no_reentrancy_scope;
-  AutoLock lock(mutex_);
-  for (auto* observer : observers_)
+  std::vector<SamplesObserver*> observers_copy;
+  {
+    AutoLock lock(mutex_);
+    observers_copy = observers_;
+    sampled_addresses_set().Remove(address);
+  }
+  for (auto* observer : observers_copy)
     observer->SampleRemoved(address);
-  sampled_addresses_set().Remove(address);
 }
 
 void PoissonAllocationSampler::BalanceAddressesHashSet() {

base/sampling_heap_profiler/poisson_allocation_sampler.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/sampling_heap_profiler/lock_free_address_hash_set.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 
 namespace base {
 
@@ -77,6 +78,13 @@ class BASE_EXPORT PoissonAllocationSampler {
   static void SetHooksInstallCallback(void (*hooks_install_callback)());
 
   void AddSamplesObserver(SamplesObserver*);
+
+  // Note: After an observer is removed it is still possible to receive
+  // a notification to that observer. This is not a problem currently as
+  // the only client of this interface is the base::SamplingHeapProfiler,
+  // which is a singleton.
+  // If there's a need for this functionality in the future, one might
+  // want to put observers notification loop under a reader-writer lock.
   void RemoveSamplesObserver(SamplesObserver*);
 
   void SetSamplingInterval(size_t sampling_interval);
@@ -109,7 +117,12 @@ class BASE_EXPORT PoissonAllocationSampler {
   void BalanceAddressesHashSet();
 
   Lock mutex_;
-  std::vector<SamplesObserver*> observers_;
+  // The |observers_| list is guarded by |mutex_|, however a copy of it
+  // is made before invoking the observers (to avoid performing expensive
+  // operations under the lock) as such the SamplesObservers themselves need
+  // to be thread-safe and support being invoked racily after
+  // RemoveSamplesObserver().
+  std::vector<SamplesObserver*> observers_ GUARDED_BY(mutex_);
 
   static PoissonAllocationSampler* instance_;
 

base/sampling_heap_profiler/sampling_heap_profiler.cc

@@ -179,7 +179,6 @@ void SamplingHeapProfiler::SampleAdded(
   if (UNLIKELY(base::ThreadLocalStorage::HasBeenDestroyed()))
     return;
   DCHECK(PoissonAllocationSampler::ScopedMuteThreadSamples::IsMuted());
-  AutoLock lock(mutex_);
   Sample sample(size, total, ++last_sample_ordinal_);
   sample.allocator = type;
   using CaptureMode = trace_event::AllocationContextTracker::CaptureMode;
@@ -191,6 +190,7 @@ void SamplingHeapProfiler::SampleAdded(
   } else {
     CaptureNativeStack(context, &sample);
   }
+  AutoLock lock(mutex_);
   RecordString(sample.context);
   samples_.emplace(address, std::move(sample));
 }
@@ -210,6 +210,8 @@ void SamplingHeapProfiler::CaptureMixedStack(const char* context,
   CHECK_LE(backtrace.frame_count, kMaxStackEntries);
   std::vector<void*> stack;
   stack.reserve(backtrace.frame_count);
+
+  AutoLock lock(mutex_);  // Needed for RecordString call.
   for (int i = base::checked_cast<int>(backtrace.frame_count) - 1; i >= 0;
        --i) {
     const base::trace_event::StackFrame& frame = backtrace.frames[i];

base/sampling_heap_profiler/sampling_heap_profiler.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/sampling_heap_profiler/poisson_allocation_sampler.h"
 #include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 #include "base/threading/thread_id_name_manager.h"
 
 namespace base {
@@ -114,13 +115,13 @@ class BASE_EXPORT SamplingHeapProfiler
 
   void CaptureMixedStack(const char* context, Sample* sample);
   void CaptureNativeStack(const char* context, Sample* sample);
-  const char* RecordString(const char* string);
+  const char* RecordString(const char* string) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
 
   // Mutex to access |samples_| and |strings_|.
   Lock mutex_;
 
   // Samples of the currently live allocations.
-  std::unordered_map<void*, Sample> samples_;
+  std::unordered_map<void*, Sample> samples_ GUARDED_BY(mutex_);
 
   // When CaptureMode::PSEUDO_STACK or CaptureMode::MIXED_STACK is enabled
   // the call stack contents of samples may contain strings besides
@@ -128,7 +129,7 @@ class BASE_EXPORT SamplingHeapProfiler
   // In this case each string pointer is also added to the |strings_| set.
   // The set does only contain pointers to static strings that are never
   // deleted.
-  std::unordered_set<const char*> strings_;
+  std::unordered_set<const char*> strings_ GUARDED_BY(mutex_);
 
   // Mutex to make |running_sessions_| and Add/Remove samples observer access
   // atomic.