Move memory management code into separate class for future reuse.

base/debug/activity_tracker.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Activity tracking provides a low-overhead method of collecting information
 // about the state of the application for analysis both while it is running
 // and after it has terminated unexpectedly. Its primary purpose is to help
 // locate reasons the browser becomes unresponsive by providing insight into
 // what all the various threads and processes are (or were) doing.
 
@@ skipping 107 matching lines @@
   static ActivityData ForProcess(const int64_t id) {
     ActivityData data;
     data.process.process_id = id;
     return data;
   }
 };
 
 // A "null" activity-data that can be passed to indicate "do not change".
 extern const ActivityData kNullActivityData;
 
+
+// A helper class that is used for managing memory allocations within a
+// persistent memory allocator. Instances of this class are NOT thread-safe.
+// Use from a single thread or protect access with a lock.
+class ActivityTrackerMemoryAllocator {
+ public:
+  using Reference = PersistentMemoryAllocator::Reference;
+
+  // Creates a instance for allocating objects of a fixed |object_type| and
+  // |object_size|. An internal cache of the last |cache_size| released
+  // references will be kept for quick future fetches.
+  ActivityTrackerMemoryAllocator(PersistentMemoryAllocator* allocator,
+                                 uint32_t object_type,
+                                 size_t object_size,
+                                 size_t cache_size);
+  ~ActivityTrackerMemoryAllocator();
+
+  // Gets a reference to an object of the configured type. This can return
+  // a null reference if it was not possible to allocate the memory.
+  Reference GetObjectReference();
+
+  // Returns an object to the "free" pool.
+  void ReleaseObjectReference(Reference ref);
+
+  // The current "used size" of the internal cache, visible for testing.
+  size_t cache_used() { return cache_used_; }
+
+ private:
+  PersistentMemoryAllocator* const allocator_;
+  const uint32_t object_type_;
+  const size_t object_size_;
+  const size_t cache_size_;
+
+  // An iterator for going through persistent memory looking for free'd objects.
+  PersistentMemoryAllocator::Iterator iterator_;
+
+  // The cache of released object memories.
+  std::unique_ptr<Reference[]> cache_values_;

[manzagop (departed), 2016/10/04 21:02:43]

How does a fixed array compare to a vector? Vector gives a variable size, which
enables holding more (if needed) to possibly avoid the slow iteration, at the
price of potential a reallocation. I have no intuition so I thought I'd ask! ;)

[bcwhite, 2016/10/05 16:34:11]

The implementation of std::vector is not strictly defined and so can allocate
memory at undefined times.  Allocating memory can involve locks.  I could use a
vector and initially reserve the number of slots I need but this ends up being
simpler.

+  size_t cache_used_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActivityTrackerMemoryAllocator);
+};
+
+
 // This structure is the full contents recorded for every activity pushed
 // onto the stack. The |activity_type| indicates what is actually stored in
 // the |data| field. All fields must be explicitly sized types to ensure no
 // interoperability problems between 32-bit and 64-bit systems.
 struct Activity {
   // The type of an activity on the stack. Activities are broken into
   // categories with the category ID taking the top 4 bits and the lower
   // bits representing an action within that category. This combination
   // makes it easy to "switch" based on the type during analysis.
   enum Type : uint8_t {
@@ skipping 199 matching lines @@
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(ThreadActivityTracker);
 };
 
 
 // The global tracker manages all the individual thread trackers. Memory for
 // the thread trackers is taken from a PersistentMemoryAllocator which allows
 // for the data to be analyzed by a parallel process or even post-mortem.
 class BASE_EXPORT GlobalActivityTracker {
-  template <typename T>
-  class ThreadSafeStack {
-   public:
-    ThreadSafeStack(size_t size)
-        : size_(size), values_(new T[size]), used_(0) {}
-    ~ThreadSafeStack() {}
-
-    size_t size() { return size_; }
-    size_t used() {
-      base::AutoLock autolock(lock_);
-      return used_;
-    }
-
-    bool push(T value) {
-      base::AutoLock autolock(lock_);
-      if (used_ == size_)
-        return false;
-      values_[used_++] = value;
-      return true;
-    }
-
-    bool pop(T* out_value) {
-      base::AutoLock autolock(lock_);
-      if (used_ == 0)
-        return false;
-      *out_value = values_[--used_];
-      return true;
-    }
-
-   private:
-    const size_t size_;
-
-    std::unique_ptr<T[]> values_;
-    size_t used_;
-    base::Lock lock_;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(ThreadSafeStack);
-  };
-
  public:
   // Type identifiers used when storing in persistent memory so they can be
   // identified during extraction; the first 4 bytes of the SHA1 of the name
   // is used as a unique integer. A "version number" is added to the base
   // so that, if the structure of that object changes, stored older versions
   // will be safely ignored. These are public so that an external process
   // can recognize records of this type within an allocator.
   enum : uint32_t {
     kTypeIdActivityTracker     = 0x5D7381AF + 1,  // SHA1(ActivityTracker) v1
-    kTypeIdActivityTrackerFree = 0x3F0272FB + 1,  // SHA1(ActivityTrackerFree)
   };
 
   // This is a thin wrapper around the thread-tracker's ScopedActivity that
   // accesses the global tracker to provide some of the information, notably
   // which thread-tracker to use. It is safe to create even if activity
   // tracking is not enabled.
   class BASE_EXPORT ScopedThreadActivity
       : public ThreadActivityTracker::ScopedActivity {
    public:
     ScopedThreadActivity(const void* origin,
@@ skipping 85 matching lines @@
   // Releases the activity-tracker for the current thread (for testing only).
   void ReleaseTrackerForCurrentThreadForTesting();
 
  private:
   friend class ActivityTrackerTest;
 
   enum : int {
     // The maximum number of threads that can be tracked within a process. If
     // more than this number run concurrently, tracking of new ones may cease.
     kMaxThreadCount = 100,
+    kCachedThreadMemories = 10,

[manzagop (departed), 2016/10/04 21:02:43]

Can you comment on why 10? IIUC caching is cheap compared to having a cache
that's too small? 10 is what we expect, plus a buffer?

[bcwhite, 2016/10/05 16:34:11]

It's arbitrary.  If it's too small (many resources created followed by many
resources released followed by many resources created), then the "many" beyond
this number will cause a search via iteration for free memory blocks.

Generally the resources being tracked are fairly permanent so I don't imagine
there to be very much churn.  I could create metrics for this but that has its
own set of complications that I'm choosing to avoid for the moment.

   };
 
   // A thin wrapper around the main thread-tracker that keeps additional
   // information that the global tracker needs to handle joined threads.
   class ManagedActivityTracker : public ThreadActivityTracker {
    public:
     ManagedActivityTracker(PersistentMemoryAllocator::Reference mem_reference,
                            void* base,
                            size_t size);
     ~ManagedActivityTracker() override;
@@ skipping 31 matching lines @@
   // The size (in bytes) of memory required by a ThreadActivityTracker to
   // provide the stack-depth requested during construction.
   const size_t stack_memory_size_;
 
   // The activity tracker for the currently executing thread.
   base::ThreadLocalStorage::Slot this_thread_tracker_;
 
   // The number of thread trackers currently active.
   std::atomic<int> thread_tracker_count_;
 
-  // A cache of thread-tracker memories that have been previously freed and
-  // thus can be re-used instead of allocating new ones.
-  ThreadSafeStack<PersistentMemoryAllocator::Reference> available_memories_;
+  // A caching memory allocator for thread-tracker objects.
+  ActivityTrackerMemoryAllocator thread_tracker_allocator_;
+  base::Lock thread_tracker_allocator_lock_;
 
   // The active global activity tracker.
   static GlobalActivityTracker* g_tracker_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalActivityTracker);
 };
 
 
 // Record entry in to and out of an arbitrary block of code.
 class BASE_EXPORT ScopedActivity
@@ skipping 83 matching lines @@
   explicit ScopedProcessWaitActivity(const base::Process* process);
  private:
   DISALLOW_COPY_AND_ASSIGN(ScopedProcessWaitActivity);
 };
 #endif
 
 }  // namespace debug
 }  // namespace base
 
 #endif  // BASE_DEBUG_ACTIVITY_TRACKER_H_