Support further atomic ops on ActivityTracker stored user-data values.

Change-Id: Id9fa420ba24a5ef1c5bc6b3be15a82c0b189ec8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036376
Commit-Queue: Brian White <bcwhite@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738316}

base/debug/activity_analyzer_unittest.cc

@@ -361,10 +361,12 @@ TEST_F(ActivityAnalyzerTest, GlobalUserDataTest) {
   ASSERT_NE(0U, process_data.id());
   process_data.Set("raw", "foo", 3);
   process_data.SetString("string", "bar");
-  process_data.SetChar("char", '9');
-  process_data.SetInt("int", -9999);
-  process_data.SetUint("uint", 9999);
-  process_data.SetBool("bool", true);
+  process_data.SetBool("bool1", false);
+  process_data.SetBool("bool2", false)->store(true, std::memory_order_relaxed);
+  process_data.SetChar("char1", '7');
+  process_data.SetChar("char2", '8')->store('9', std::memory_order_relaxed);
+  process_data.SetInt("int", -9998)->fetch_sub(1, std::memory_order_relaxed);
+  process_data.SetUint("uint", 9998)->fetch_add(1, std::memory_order_relaxed);
   process_data.SetReference("ref", string1, sizeof(string1));
   process_data.SetStringReference("sref", string2);
 
@@ -376,14 +378,18 @@ TEST_F(ActivityAnalyzerTest, GlobalUserDataTest) {
   EXPECT_EQ("foo", snapshot.at("raw").Get().as_string());
   ASSERT_TRUE(Contains(snapshot, "string"));
   EXPECT_EQ("bar", snapshot.at("string").GetString().as_string());
-  ASSERT_TRUE(Contains(snapshot, "char"));
-  EXPECT_EQ('9', snapshot.at("char").GetChar());
+  ASSERT_TRUE(Contains(snapshot, "bool1"));
+  EXPECT_FALSE(snapshot.at("bool1").GetBool());
+  ASSERT_TRUE(Contains(snapshot, "bool2"));
+  EXPECT_TRUE(snapshot.at("bool2").GetBool());
+  ASSERT_TRUE(Contains(snapshot, "char1"));
+  EXPECT_EQ('7', snapshot.at("char1").GetChar());
+  ASSERT_TRUE(Contains(snapshot, "char2"));
+  EXPECT_EQ('9', snapshot.at("char2").GetChar());
   ASSERT_TRUE(Contains(snapshot, "int"));
   EXPECT_EQ(-9999, snapshot.at("int").GetInt());
   ASSERT_TRUE(Contains(snapshot, "uint"));
   EXPECT_EQ(9999U, snapshot.at("uint").GetUint());
-  ASSERT_TRUE(Contains(snapshot, "bool"));
-  EXPECT_TRUE(snapshot.at("bool").GetBool());
   ASSERT_TRUE(Contains(snapshot, "ref"));
   EXPECT_EQ(string1, snapshot.at("ref").GetReference().data());
   EXPECT_EQ(sizeof(string1), snapshot.at("ref").GetReference().size());

base/debug/activity_tracker.cc

@@ -398,11 +398,15 @@ bool ActivityUserData::CreateSnapshot(Snapshot* output_snapshot) const {
       } break;
       case BOOL_VALUE:
       case CHAR_VALUE:
-        value.short_value_ = *reinterpret_cast<char*>(entry.second.memory);
+        value.short_value_ =
+            reinterpret_cast<std::atomic<char>*>(entry.second.memory)
+                ->load(std::memory_order_relaxed);
         break;
       case SIGNED_VALUE:
       case UNSIGNED_VALUE:
-        value.short_value_ = *reinterpret_cast<uint64_t*>(entry.second.memory);
+        value.short_value_ =
+            reinterpret_cast<std::atomic<uint64_t>*>(entry.second.memory)
+                ->load(std::memory_order_relaxed);
         break;
       case END_OF_VALUES:  // Included for completeness purposes.
         NOTREACHED();
@@ -446,17 +450,17 @@ bool ActivityUserData::GetOwningProcessId(const void* memory,
   return OwningProcess::GetOwningProcessId(&header->owner, out_id, out_stamp);
 }
 
-void ActivityUserData::Set(StringPiece name,
-                           ValueType type,
-                           const void* memory,
-                           size_t size) {
+void* ActivityUserData::Set(StringPiece name,
+                            ValueType type,
+                            const void* memory,
+                            size_t size) {
   DCHECK_GE(std::numeric_limits<uint8_t>::max(), name.length());
   size = std::min(std::numeric_limits<uint16_t>::max() - (kMemoryAlignment - 1),
                   size);
 
   // It's possible that no user data is being stored.
   if (!memory_)
-    return;
+    return nullptr;
 
   // The storage of a name is limited so use that limit during lookup.
   if (name.length() > kMaxUserDataNameLength)
@@ -482,7 +486,7 @@ void ActivityUserData::Set(StringPiece name,
     // now if there's not room enough for even this.
     size_t base_size = sizeof(FieldHeader) + name_extent;
     if (base_size > available_)
-      return;
+      return nullptr;
 
     // The "full size" is the size for storing the entire value.
     size_t full_size = std::min(base_size + value_extent, available_);
@@ -500,7 +504,7 @@ void ActivityUserData::Set(StringPiece name,
     if (size != 0) {
       size = std::min(full_size - base_size, size);
       if (size == 0)
-        return;
+        return nullptr;
     }
 
     // Allocate a chunk of memory.
@@ -542,6 +546,10 @@ void ActivityUserData::Set(StringPiece name,
   info->size_ptr->store(0, std::memory_order_seq_cst);
   memcpy(info->memory, memory, size);
   info->size_ptr->store(size, std::memory_order_release);
+
+  // The address of the stored value is returned so it can be re-used by the
+  // caller, so long as it's done in an atomic way.
+  return info->memory;
 }
 
 void ActivityUserData::SetReference(StringPiece name,
@@ -1229,12 +1237,12 @@ GlobalActivityTracker::ThreadSafeUserData::ThreadSafeUserData(void* memory,
 
 GlobalActivityTracker::ThreadSafeUserData::~ThreadSafeUserData() = default;
 
-void GlobalActivityTracker::ThreadSafeUserData::Set(StringPiece name,
-                                                    ValueType type,
-                                                    const void* memory,
-                                                    size_t size) {
+void* GlobalActivityTracker::ThreadSafeUserData::Set(StringPiece name,
+                                                     ValueType type,
+                                                     const void* memory,
+                                                     size_t size) {
   AutoLock lock(data_lock_);
-  ActivityUserData::Set(name, type, memory, size);
+  return ActivityUserData::Set(name, type, memory, size);
 }
 
 GlobalActivityTracker::ManagedActivityTracker::ManagedActivityTracker(

base/debug/activity_tracker.h

@@ -346,7 +346,7 @@ struct Activity {
 // additional information during debugging. It is also used to store arbitrary
 // global data. All updates must be done from the same thread though other
 // threads can read it concurrently if they create new objects using the same
-// memory.
+// memory. For a thread-safe version, see ThreadSafeUserData later on.
 class BASE_EXPORT ActivityUserData {
  public:
   // List of known value type. REFERENCE types must immediately follow the non-
@@ -423,6 +423,13 @@ class BASE_EXPORT ActivityUserData {
   // This information is stored on a "best effort" basis. It may be dropped if
   // the memory buffer is full or the associated activity is beyond the maximum
   // recording depth.
+  //
+  // Some methods return pointers to the stored value that can be further
+  // modified using normal std::atomic operations without having to go through
+  // this interface, thus avoiding the relatively expensive name lookup.
+  // ==> Use std::memory_order_relaxed as the "order" parameter to atomic ops.
+  // Remember that the return value will be nullptr if the value could not
+  // be stored!
   void Set(StringPiece name, const void* memory, size_t size) {
     Set(name, RAW_VALUE, memory, size);
   }
@@ -432,18 +439,22 @@ class BASE_EXPORT ActivityUserData {
   void SetString(StringPiece name, StringPiece16 value) {
     SetString(name, UTF16ToUTF8(value));
   }
-  void SetBool(StringPiece name, bool value) {
+  std::atomic<bool>* SetBool(StringPiece name, bool value) {
     char cvalue = value ? 1 : 0;
-    Set(name, BOOL_VALUE, &cvalue, sizeof(cvalue));
+    void* addr = Set(name, BOOL_VALUE, &cvalue, sizeof(cvalue));
+    return reinterpret_cast<std::atomic<bool>*>(addr);
   }
-  void SetChar(StringPiece name, char value) {
-    Set(name, CHAR_VALUE, &value, sizeof(value));
+  std::atomic<char>* SetChar(StringPiece name, char value) {
+    void* addr = Set(name, CHAR_VALUE, &value, sizeof(value));
+    return reinterpret_cast<std::atomic<char>*>(addr);
   }
-  void SetInt(StringPiece name, int64_t value) {
-    Set(name, SIGNED_VALUE, &value, sizeof(value));
+  std::atomic<int64_t>* SetInt(StringPiece name, int64_t value) {
+    void* addr = Set(name, SIGNED_VALUE, &value, sizeof(value));
+    return reinterpret_cast<std::atomic<int64_t>*>(addr);
   }
-  void SetUint(StringPiece name, uint64_t value) {
-    Set(name, UNSIGNED_VALUE, &value, sizeof(value));
+  std::atomic<uint64_t>* SetUint(StringPiece name, uint64_t value) {
+    void* addr = Set(name, UNSIGNED_VALUE, &value, sizeof(value));
+    return reinterpret_cast<std::atomic<uint64_t>*>(addr);
   }
 
   // These function as above but don't actually copy the data into the
@@ -478,10 +489,10 @@ class BASE_EXPORT ActivityUserData {
                                  int64_t* out_stamp);
 
  protected:
-  virtual void Set(StringPiece name,
-                   ValueType type,
-                   const void* memory,
-                   size_t size);
+  virtual void* Set(StringPiece name,
+                    ValueType type,
+                    const void* memory,
+                    size_t size);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(ActivityTrackerTest, UserDataTest);
@@ -1078,10 +1089,10 @@ class BASE_EXPORT GlobalActivityTracker {
     ~ThreadSafeUserData() override;
 
    private:
-    void Set(StringPiece name,
-             ValueType type,
-             const void* memory,
-             size_t size) override;
+    void* Set(StringPiece name,
+              ValueType type,
+              const void* memory,
+              size_t size) override;
 
     Lock data_lock_;