Added support for storing arbitrary user data.

base/debug/activity_tracker.h

Index: base/debug/activity_tracker.h
diff --git a/base/debug/activity_tracker.h b/base/debug/activity_tracker.h
index 566b682bfeeb8fad744c28ee9ce5e0018f884aed..35e5a5876f9281dbf92a3b7f54f454d35de989c2 100644
--- a/base/debug/activity_tracker.h
+++ b/base/debug/activity_tracker.h
@@ -16,11 +16,13 @@
 // PersistentMemoryAllocator which also uses std::atomic and is written
 // by the same author.
 #include <atomic>
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/base_export.h"
+#include "base/gtest_prod_util.h"
 #include "base/location.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/threading/platform_thread.h"
@@ -136,12 +138,14 @@ class ActivityTrackerMemoryAllocator {
   // Creates a instance for allocating objects of a fixed |object_type|, a
   // corresponding |object_free| type, and the |object_size|. An internal
   // cache of the last |cache_size| released references will be kept for
-  // quick future fetches.
+  // quick future fetches. If |make_iterable| then allocated objects will
+  // be marked "iterable" in the allocator.
   ActivityTrackerMemoryAllocator(PersistentMemoryAllocator* allocator,
                                  uint32_t object_type,
                                  uint32_t object_free_type,
                                  size_t object_size,
-                                 size_t cache_size);
+                                 size_t cache_size,
+                                 bool make_iterable);
   ~ActivityTrackerMemoryAllocator();
 
   // Gets a reference to an object of the configured type. This can return
@@ -160,6 +164,7 @@ class ActivityTrackerMemoryAllocator {
   const uint32_t object_free_type_;
   const size_t object_size_;
   const size_t cache_size_;
+  const bool make_iterable_;
 
   // An iterator for going through persistent memory looking for free'd objects.
   PersistentMemoryAllocator::Iterator iterator_;
@@ -236,6 +241,9 @@ struct Activity {
   // enabled.
   uint64_t call_stack[kActivityCallStackSize];
 
+  // Reference to arbitrary user data within the persistent memory segment.
+  uint32_t user_data;
+
   // The (enumerated) type of the activity. This defines what fields of the
   // |data| record are valid.
   uint8_t activity_type;
@@ -243,7 +251,7 @@ struct Activity {
   // Padding to ensure that the next member begins on a 64-bit boundary
   // even on 32-bit builds which ensures inter-operability between CPU
   // architectures. New fields can be taken from this space.
-  uint8_t padding[7];
+  uint8_t padding[3];
 
   // Information specific to the |activity_type|.
   ActivityData data;
@@ -284,6 +292,103 @@ struct BASE_EXPORT ActivitySnapshot {
 };
 
 
+class BASE_EXPORT ActivityUserData {

[manzagop (departed), 2016/11/11 17:01:42]

nit: mention it's meant to be used on a single thread.

[manzagop (departed), 2016/11/11 17:01:42]

Does this need atomic operations as well (to ensure a consistent view from the
outside)?

[manzagop (departed), 2016/11/11 17:01:42]

nit: class comment, perhaps with a high level mention of the external
distinction.

[bcwhite, 2016/11/14 15:08:42]

Done.

[bcwhite, 2016/11/14 15:08:42]

Done.

[bcwhite, 2016/11/14 15:08:42]

Good point.  I've gotten in to thinking only of the post-mortem analysis.

+  // List of known value type. EXTERNAL types must immediately follow the non-
+  // external types.
+  enum ValueType : uint8_t {
+    EMPTY_VALUE,
+    RAW_VALUE,
+    EXTERNAL_RAW_VALUE,

[manzagop (departed), 2016/11/11 17:01:42]

Would RAW_VALUE_REFERENCE be more descriptive? I wasn't sure what external meant
initially. Maybe it's more confusing. Thinking aloud here. Naming hard.

[bcwhite, 2016/11/14 15:08:42]

I think that's better.

+    STRING_VALUE,
+    EXTERNAL_STRING_VALUE,
+    CHAR_VALUE,
+    SIGNED_VALUE,
+    UNSIGNED_VALUE,
+  };
+
+ public:
+  ActivityUserData(void* memory, size_t size);
+  ~ActivityUserData();
+
+  // Write the value (as part of a key/value pair) that will be included with
+  // the activity in any reports. The same |name| can be written multiple times
+  // with each successive call overwriting the previous value. The maximum size

[manzagop (departed), 2016/11/11 17:01:42]

nit: I think it could be clearer that it's the value as in |value| (so not the
type)

[bcwhite, 2016/11/14 15:08:42]

Done.

+  // (for raw and string values) is limited by the first call, however.

[manzagop (departed), 2016/11/11 17:01:42]

Mention @pre name.length() <= std::numeric_limits<uint8_t>::max()
This might trip some folks who use dynamic strings as names.

[manzagop (departed), 2016/11/11 17:01:42]

Mention that if memory is lacking, the value may be truncated or the insertion
will fail.

[bcwhite, 2016/11/14 15:08:42]

Done.

[bcwhite, 2016/11/14 15:08:42]

Done.

+  void Set(StringPiece name, const void* memory, size_t size) {
+    Set(name, RAW_VALUE, memory, size);
+  }
+  void SetString(StringPiece name, StringPiece value) {
+    Set(name, STRING_VALUE, value.data(), value.length());
+  }
+  void SetChar(StringPiece name, char value) {
+    Set(name, CHAR_VALUE, &value, sizeof(value));
+  }
+  void SetInt(StringPiece name, int64_t value) {
+    Set(name, SIGNED_VALUE, &value, sizeof(value));
+  }
+  void SetUint(StringPiece name, uint64_t value) {
+    Set(name, UNSIGNED_VALUE, &value, sizeof(value));
+  }
+
+  // These function as above but don't actually copy the data into the
+  // persistent memory. The store unaltered pointers along with a size that

[manzagop (departed), 2016/11/11 17:01:42]

typo: The -> They

[bcwhite, 2016/11/14 15:08:43]

Done.

+  // can be used in conjuction with a memory dump to find certain pieces of
+  // information.
+  void SetExternal(StringPiece name, const void* memory, size_t size) {
+    SetExternal(name, EXTERNAL_RAW_VALUE, memory, size);
+  }
+  void SetExternalString(StringPiece name, StringPiece value) {
+    SetExternal(name, EXTERNAL_RAW_VALUE, value.data(), value.length());
+  }
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ActivityTrackerTest, UserDataTest);
+
+  enum : size_t { kMemoryAlignment = sizeof(uint64_t) };
+
+  struct ValueInfo {
+    ValueInfo();
+    ValueInfo(ValueInfo&&);
+    ~ValueInfo();
+
+    StringPiece name;
+    void* memory;
+    size_t size;
+    size_t extent;

[manzagop (departed), 2016/11/11 17:01:42]

I wasn't sure which of size/extent was which without looking at the code. Would
something like consumed/used/filled help?

[bcwhite, 2016/11/14 15:08:43]

Done.

+    ValueType type;
+  };
+
+  struct Header {
+    uint8_t type;
+    uint8_t name_size;
+    uint16_t value_size;

[manzagop (departed), 2016/11/11 17:01:42]

Is this the size or the extent?

[bcwhite, 2016/11/14 15:08:42]

Size.  Comments added.

+    uint16_t record_size;

[manzagop (departed), 2016/11/11 17:01:42]

Comment on whether inclusive of header size?

[bcwhite, 2016/11/14 15:08:42]

Inclusive.

+  };
+
+  struct ExternalRecord {
+    uint64_t address;
+    uint64_t size;
+  };
+
+  void Set(StringPiece name, ValueType type, const void* memory, size_t size);
+  void SetExternal(StringPiece name,
+                   ValueType type,
+                   const void* memory,
+                   size_t size);
+
+  // TODO(bcwhite): Add Get() methods for Analyzer to use.
+
+  std::map<StringPiece, ValueInfo> values_;
+
+  char* memory_;
+  size_t available_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActivityUserData);
+};
+
+
 // This class manages tracking a stack of activities for a single thread in
 // a persistent manner, implementing a bounded-size stack in a fixed-size
 // memory allocation. In order to support an operational mode where another
@@ -295,6 +400,8 @@ struct BASE_EXPORT ActivitySnapshot {
 // objects.
 class BASE_EXPORT ThreadActivityTracker {
  public:
+  using ActivityId = uint32_t;
+
   // This is the base class for having the compiler manage an activity on the
   // tracker's stack. It does nothing but call methods on the passed |tracker|
   // if it is not null, making it safe (and cheap) to create these objects
@@ -304,27 +411,26 @@ class BASE_EXPORT ThreadActivityTracker {
     ScopedActivity(ThreadActivityTracker* tracker,
                    const void* origin,
                    Activity::Type type,
-                   const ActivityData& data)
-        : tracker_(tracker) {
-      if (tracker_)
-        tracker_->PushActivity(origin, type, data);
-    }
+                   const ActivityData& data);
+    ~ScopedActivity();
 
-    ~ScopedActivity() {
-      if (tracker_)
-        tracker_->PopActivity();
-    }
+    // Changes some basic metadata about the activity.
+    void ChangeTypeAndData(Activity::Type type, const ActivityData& data);
 
-    void ChangeTypeAndData(Activity::Type type, const ActivityData& data) {
-      if (tracker_)
-        tracker_->ChangeActivity(type, data);
-    }
+    // Returns an object for manipulating user data.
+    ActivityUserData& user_data();
 
    private:
     // The thread tracker to which this object reports. It can be null if
     // activity tracking is not (yet) enabled.
     ThreadActivityTracker* const tracker_;
 
+    // An identifier that indicates a specific activity on the stack.
+    ActivityId activity_id_;
+
+    // An object that manages additional user data, created only upon request.
+    std::unique_ptr<ActivityUserData> user_data_;
+
     DISALLOW_COPY_AND_ASSIGN(ScopedActivity);
   };
 
@@ -336,10 +442,11 @@ class BASE_EXPORT ThreadActivityTracker {
 
   // Indicates that an activity has started from a given |origin| address in
   // the code, though it can be null if the creator's address is not known.
-  // The |type| and |data| describe the activity.
-  void PushActivity(const void* origin,
-                    Activity::Type type,
-                    const ActivityData& data);
+  // The |type| and |data| describe the activity. Returned is an ID that can
+  // be used to adjust the pushed activity.
+  ActivityId PushActivity(const void* origin,
+                          Activity::Type type,
+                          const ActivityData& data);
 
   // Changes the activity |type| and |data| of the top-most entry on the stack.
   // This is useful if the information has changed and it is desireable to
@@ -348,10 +455,15 @@ class BASE_EXPORT ThreadActivityTracker {
   // unchanged. The type, if changed, must remain in the same category.
   // Changing both is not atomic so a snapshot operation could occur between
   // the update of |type| and |data| or between update of |data| fields.
-  void ChangeActivity(Activity::Type type, const ActivityData& data);
+  void ChangeActivity(ActivityId id,
+                      Activity::Type type,
+                      const ActivityData& data);
 
   // Indicates that an activity has completed.
-  void PopActivity();
+  void PopActivity(ActivityId id);
+
+  // Returns an object capable of storing arbitrary user data.
+  std::unique_ptr<ActivityUserData> GetUserData(ActivityId id);
 
   // Returns whether the current data is valid or not. It is not valid if
   // corruption has been detected in the header or other data structures.
@@ -399,8 +511,12 @@ class BASE_EXPORT GlobalActivityTracker {
   // 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
+    kTypeIdActivityTracker     = 0x5D7381AF + 2,  // SHA1(ActivityTracker) v2
+    kTypeIdUserDataRecord      = 0x615EDDD7 + 1,  // SHA1(UserDataRecord) v1
+    kTypeIdGlobalDataRecord    = 0xAFE61ABE + 1,  // SHA1(GlobalDataRecord) v1
+
     kTypeIdActivityTrackerFree = ~kTypeIdActivityTracker,
+    kTypeIdUserDataRecordFree  = ~kTypeIdUserDataRecord,
   };
 
   // This is a thin wrapper around the thread-tracker's ScopedActivity that
@@ -499,6 +615,17 @@ class BASE_EXPORT GlobalActivityTracker {
   // Releases the activity-tracker for the current thread (for testing only).
   void ReleaseTrackerForCurrentThreadForTesting();
 
+  // Gets a reference to memory for holding user-defined activity data. If
+  // the reference is valid, it's memory will be returned. If not, then a
+  // new reference will be created (and stored) and that memory returned.
+  void* GetUserDataMemory(PersistentMemoryAllocator::Reference* reference);
+
+  // Releases memory for user-defined activity data.
+  void ReleaseUserDataMemory(PersistentMemoryAllocator::Reference* reference);
+
+  // Accesses the global data record for storing arbitrary key/value pairs.
+  ActivityUserData& user_data() { return user_data_; }
+
  private:
   friend class ActivityTrackerTest;
 
@@ -507,6 +634,7 @@ class BASE_EXPORT GlobalActivityTracker {
     // more than this number run concurrently, tracking of new ones may cease.
     kMaxThreadCount = 100,
     kCachedThreadMemories = 10,
+    kCachedUserDataMemories = 10,
   };
 
   // A thin wrapper around the main thread-tracker that keeps additional
@@ -562,6 +690,13 @@ class BASE_EXPORT GlobalActivityTracker {
   ActivityTrackerMemoryAllocator thread_tracker_allocator_;
   base::Lock thread_tracker_allocator_lock_;
 
+  // A caching memory allocator for user data attached to activity data.
+  ActivityTrackerMemoryAllocator user_data_allocator_;
+  base::Lock user_data_allocator_lock_;
+
+  // An object for holding global arbitrary key value pairs.
+  ActivityUserData user_data_;

[manzagop (departed), 2016/11/11 17:01:42]

This has a thread checker, so it should only be used from 1 thread?

[bcwhite, 2016/11/14 15:08:43]

Done.

+
   // The active global activity tracker.
   static GlobalActivityTracker* g_tracker_;