[sanitizer] Atomic access to StackDepot stats

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

129

Missing ;

135

This is a more expensive operation than the one used before. Probably does not matter, but please check that you are not slowing things down.

[vitalybuka]

rebase

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

135

Did you look at perf with msan? Stackdepot is much busier with msan. This is an atomic r/m/w of a global location...

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

135

In fact. Why can't we add up seq[i] numbers for each part when we read the stats, instead of maintaining an extra global counter?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

135

no, but I run large test on Asan where Put() costs solid 6% and see no difference.
Note that this is a slow path. Only fraction of Most ::Put calls endup here, and we already acquired a lock.
We need try hard to slowdown this code if we change anything after line 116.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

135

In fact. Why can't we add up seq[i] numbers for each part when we read the stats, instead of maintaining an extra global counter?

We can, but this is unrelated change.
This code is wrong as-is and this patch fixes it for free.
Later when I figure out what to do with our main issue - memory, I'll consider this micro optimizations.

compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h

  public:
   typedef typename Node::args_type args_type;
   typedef typename Node::handle_type handle_type;
   // Maps stack trace to an unique id.
   handle_type Put(args_type args, bool *inserted = nullptr);
   // Retrieves a stored stack trace by the id.
   args_type Get(u32 id);
 
-  StackDepotStats GetStats() const { return stats; }
+  StackDepotStats GetStats() const {
+    return {
+        atomic_load_relaxed(&n_uniq_ids),
+        atomic_load_relaxed(&allocated),
+    };
+  }
 
   void LockAll();
   void UnlockAll();
   void PrintAll();
 
  private:
   static Node *find(Node *s, args_type args, u32 hash);
   static Node *lock(atomic_uintptr_t *p);
   static void unlock(atomic_uintptr_t *p, Node *s);
 
   static const int kTabSize = 1 << kTabSizeLog;  // Hash table size.
   static const int kPartBits = 8;
   static const int kPartShift = sizeof(u32) * 8 - kPartBits - kReservedBits;
   static const int kPartCount =
       1 << kPartBits;  // Number of subparts in the table.
   static const int kPartSize = kTabSize / kPartCount;
   static const int kMaxId = 1 << kPartShift;
 
   atomic_uintptr_t tab[kTabSize];   // Hash table of Node's.
   atomic_uint32_t seq[kPartCount];  // Unique id generators.
 
-  StackDepotStats stats;
+  atomic_uintptr_t n_uniq_ids;
+  atomic_uintptr_t allocated;
 
   friend class StackDepotReverseMap;
 };
 
 template <class Node, int kReservedBits, int kTabSizeLog>
 Node *StackDepotBase<Node, kReservedBits, kTabSizeLog>::find(Node *s,
                                                              args_type args,
                                                              u32 hash) {
if (s2 != s) {
     node = find(s2, args, h);
     if (node) {
       unlock(p, s2);
       return node->get_handle();
     }
   }
   uptr part = (h % kTabSize) / kPartSize;
   u32 id = atomic_fetch_add(&seq[part], 1, memory_order_relaxed) + 1;
-  stats.n_uniq_ids++;
+  atomic_fetch_add(&n_uniq_ids, 1, memory_order_relaxed);
   CHECK_LT(id, kMaxId);
   id |= part << kPartShift;
   CHECK_NE(id, 0);
   CHECK_EQ(id & (((u32)-1) >> kReservedBits), id);
   uptr memsz = Node::storage_size(args);
   s = (Node *)PersistentAlloc(memsz);
-  stats.allocated += memsz;
+  atomic_fetch_add(&allocated, memsz, memory_order_relaxed);
   s->id = id;
   s->store(args, h);
   s->link = s2;
   unlock(p, s);
   if (inserted) *inserted = true;
   return s->get_handle();
 }