Tracked objects: Bump cumulative byte count storage to 64 bits to avoid saturation

base/tracked_objects.cc

 // Copyright (c) 2012 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.
 
 #include "base/tracked_objects.h"
 
 #include <ctype.h>
 #include <limits.h>
 #include <stdlib.h>
 
@@ skipping 92 matching lines @@
 
 DeathData::DeathData()
     : count_(0),
       sample_probability_count_(0),
       run_duration_sum_(0),
       queue_duration_sum_(0),
       run_duration_max_(0),
       queue_duration_max_(0),
       alloc_ops_(0),
       free_ops_(0),
-      allocated_bytes_(0),
-      freed_bytes_(0),
-      alloc_overhead_bytes_(0),
+#if !defined(ARCH_CPU_64_BITS)
+      byte_update_counter_(0),
+#endif
+      allocated_bytes_(),
+      freed_bytes_(),
+      alloc_overhead_bytes_(),
       max_allocated_bytes_(0),
       run_duration_sample_(0),
       queue_duration_sample_(0),
-      last_phase_snapshot_(nullptr) {}
+      last_phase_snapshot_(nullptr) {
+}
 
 DeathData::DeathData(const DeathData& other)
     : count_(other.count_),
       sample_probability_count_(other.sample_probability_count_),
       run_duration_sum_(other.run_duration_sum_),
       queue_duration_sum_(other.queue_duration_sum_),
       run_duration_max_(other.run_duration_max_),
       queue_duration_max_(other.queue_duration_max_),
       alloc_ops_(other.alloc_ops_),
       free_ops_(other.free_ops_),
+#if !defined(ARCH_CPU_64_BITS)
+      byte_update_counter_(0),
+#endif
       allocated_bytes_(other.allocated_bytes_),
       freed_bytes_(other.freed_bytes_),
       alloc_overhead_bytes_(other.alloc_overhead_bytes_),
       max_allocated_bytes_(other.max_allocated_bytes_),
       run_duration_sample_(other.run_duration_sample_),
       queue_duration_sample_(other.queue_duration_sample_),
       last_phase_snapshot_(nullptr) {
   // This constructor will be used by std::map when adding new DeathData values
   // to the map.  At that point, last_phase_snapshot_ is still NULL, so we don't
   // need to worry about ownership transfer.
@@ skipping 56 matching lines @@
     base::subtle::NoBarrier_Store(&run_duration_sample_, run_duration);
   }
 }
 
 void DeathData::RecordAllocations(const uint32_t alloc_ops,
                                   const uint32_t free_ops,
                                   const uint32_t allocated_bytes,
                                   const uint32_t freed_bytes,
                                   const uint32_t alloc_overhead_bytes,
                                   const uint32_t max_allocated_bytes) {
+#if !defined(ARCH_CPU_64_BITS)
+  // On 32 bit systems, we use an even/odd locking scheme to make possible to
+  // read 64 bit sums consistently. Note that since writes are bound to the
+  // thread owning this DeathData, there's no race on these writes.

[gab, 2017/05/02 21:17:12]

Can we have a base::ThreadChecker check confirm that these writes are always on
the same thread? This scheme depends on there being a single writer of
|byte_update_counter_|.

[Sigurður Ásgeirsson, 2017/05/03 15:23:44]

I looked into this, and it's not straightforward to do. The object that has
thread affinity is ThreadData, which in turn keeps a stash of these DeathData
objects. The problem is that now LuckyLuke is churning threads, the ThreadData
objects are thread-promiscuous, though they associate with zero or one threads
at any time.
The threading model here is ancient as time, quite well documented and deeply
embedded throughout this implementation. I'd beg off from fitting a
ThreadChecker in there, unless you strongly object.

+  int32_t counter_val =
+      base::subtle::NoBarrier_AtomicIncrement(&byte_update_counter_, 1);
+  // The counter must be odd.
+  DCHECK_EQ(1, counter_val & 1);
+#endif
+
   // Use saturating arithmetic.
   SaturatingMemberAdd(alloc_ops, &alloc_ops_);
   SaturatingMemberAdd(free_ops, &free_ops_);
-  SaturatingMemberAdd(allocated_bytes, &allocated_bytes_);
-  SaturatingMemberAdd(freed_bytes, &freed_bytes_);
-  SaturatingMemberAdd(alloc_overhead_bytes, &alloc_overhead_bytes_);
+  SaturatingByteCountMemberAdd(allocated_bytes, &allocated_bytes_);
+  SaturatingByteCountMemberAdd(freed_bytes, &freed_bytes_);
+  SaturatingByteCountMemberAdd(alloc_overhead_bytes, &alloc_overhead_bytes_);
 
   int32_t max = base::saturated_cast<int32_t>(max_allocated_bytes);
   if (max > max_allocated_bytes_)
     base::subtle::NoBarrier_Store(&max_allocated_bytes_, max);
+
+#if !defined(ARCH_CPU_64_BITS)
+  // Now release the value while rolling to even.
+  counter_val =
+      base::subtle::Barrier_AtomicIncrement(&byte_update_counter_, -1);
+  DCHECK_EQ(0, counter_val & 1);
+#endif
 }
 
 void DeathData::OnProfilingPhaseCompleted(int profiling_phase) {
   // Snapshotting and storing current state.
   last_phase_snapshot_ =
       new DeathDataPhaseSnapshot(profiling_phase, *this, last_phase_snapshot_);
 
   // Not touching fields for which a delta can be computed by comparing with a
   // snapshot from the previous phase. Resetting other fields.  Sample values
   // will be reset upon next death recording because sample_probability_count_
@@ skipping 17 matching lines @@
   // The damage is limited to selecting a wrong sample, which is not something
   // that can cause accumulating or cascading effects.
   // If there were no inconsistencies caused by race conditions, we never send a
   // sample for the previous phase in the next phase's snapshot because
   // ThreadData::SnapshotExecutedTasks doesn't send deltas with 0 count.
   base::subtle::NoBarrier_Store(&sample_probability_count_, 0);
   base::subtle::NoBarrier_Store(&run_duration_max_, 0);
   base::subtle::NoBarrier_Store(&queue_duration_max_, 0);
 }
 
+int64_t DeathData::CumulativeByteCountRead(const CumulativeByteCount* count) {
+#if defined(ARCH_CPU_64_BITS)
+  return *count;
+#else
+  return static_cast<int64_t>(count->hi_word) << 32 |
+         static_cast<uint32_t>(count->lo_word);
+#endif
+}
+
+int64_t DeathData::ConsistentCumulativeByteCountRead(
+    const CumulativeByteCount* count) const {
+#if defined(ARCH_CPU_64_BITS)
+  return base::subtle::NoBarrier_Load(count);
+#else
+  // We're on a 32 bit system, this is going to be complicated.
+  while (true) {

[gab, 2017/05/02 21:17:12]

This can theoretically starve readers. I assume this data is only consumed in
developer-only UIs where this matters less? Is the API clear about that?

[Sigurður Ásgeirsson, 2017/05/03 14:10:10]

I don't know where you'd see this documented, but this is only active under
"--enable-heap-profiling=task-profiler", as sadly there's sufficient performance
impact to this that it shows on benchmarks.

+    int32_t update_counter = 0;
+    // Acquire the starting count, spin until it's even.
+    do {
+      update_counter = base::subtle::NoBarrier_Load(&byte_update_counter_);
+    } while (update_counter & 1);

[gab, 2017/05/02 21:17:12]

PlatformThread::YieldCurrentThread() after each failed cycle

[Sigurður Ásgeirsson, 2017/05/03 14:10:10]

Mmmm, I added a spin count to yield. These reads happen on the main UI thread,
as I understand it...

+
+    DCHECK_EQ(update_counter & 1, 0);
+
+    int64_t value =
+        static_cast<int64_t>(base::subtle::NoBarrier_Load(&count->hi_word))
+            << 32 |
+        static_cast<uint32_t>(base::subtle::NoBarrier_Load(&count->lo_word));

[gab, 2017/05/02 21:17:12]

I'm not convinced that NoBarrier is what you want here and elsewhere.

NoBarrier means that reads/writes can be reordered (i.e. you could see the
desired |update_counter| but only half the writes to |count| -- and the final
validity check in this method doesn't change that).

NoBarrier is fine in the single variable use case because it's merely
reads/writes of a single piece of data (without caring about relative ordering
of other reads/writes on involved threads), but now ordering does matter as you
depend on one piece of state being set one way to assume things about other
pieces of state.

[Sigurður Ásgeirsson, 2017/05/03 14:10:10]

The final (now-)increment to the guarding counter is a barrier write, which - to
my understanding - makes all previous writes on that thread/core consistent to
other cores.
These reads can then be inconsistent only if there are concurrent updates, which
is (now) guarded by the update counter.

+
+    // If the count has not changed, the read is consistent.
+    // Otherwise go around and try again.
+    if (update_counter == base::subtle::NoBarrier_Load(&byte_update_counter_))

[gab, 2017/05/02 21:17:12]

Can't this check see the same value twice while there was a write in the middle
since you're decrementing at the end of DeathData::RecordAllocation() instead of
rolling forward to even? Should it merely roll forward? If merely moving forward
and back, it's no better than a "writing"/"not writing" flag which has this
issue as well.

[Sigurður Ásgeirsson, 2017/05/03 14:10:10]

You're absolutely right and that was my intent. I guess I zoned in a copy/paste
someplace. Thanks!

+      return value;
+  }
+#endif
+}
+
 void DeathData::SaturatingMemberAdd(const uint32_t addend,
                                     base::subtle::Atomic32* sum) {
   // Bail quick if no work or already saturated.
   if (addend == 0U || *sum == INT_MAX)
     return;
 
   base::CheckedNumeric<int32_t> new_sum = *sum;
   new_sum += addend;
   base::subtle::NoBarrier_Store(sum, new_sum.ValueOrDefault(INT_MAX));
 }
 
+void DeathData::SaturatingByteCountMemberAdd(const uint32_t addend,
+                                             CumulativeByteCount* sum) {
+  // Bail quick if no work or already saturated.
+  if (addend == 0U || CumulativeByteCountRead(sum) == LONG_MAX)

[gab, 2017/05/02 21:17:12]

Use std::numeric_limits<int64_t>::max() instead of LONG_MAX here and below

[Sigurður Ásgeirsson, 2017/05/03 14:10:10]

Ah, new hotness, thanks!

+    return;
+
+  base::CheckedNumeric<int64_t> new_sum = CumulativeByteCountRead(sum);
+  new_sum += addend;
+  int64_t new_value = new_sum.ValueOrDefault(LONG_MAX);
+// Update our value.
+#if defined(ARCH_CPU_64_BITS)
+  base::subtle::NoBarrier_Store(sum, new_value);
+#else
+  base::subtle::NoBarrier_Store(&sum->hi_word,
+                                static_cast<int32_t>(new_value >> 32));
+  base::subtle::NoBarrier_Store(&sum->lo_word,
+                                static_cast<int32_t>(new_value & 0xFFFFFFFF));
+#endif
+}
+
 //------------------------------------------------------------------------------
 DeathDataSnapshot::DeathDataSnapshot()
     : count(-1),
       run_duration_sum(-1),
       run_duration_max(-1),
       run_duration_sample(-1),
       queue_duration_sum(-1),
       queue_duration_max(-1),
       queue_duration_sample(-1),
       alloc_ops(-1),
       free_ops(-1),
       allocated_bytes(-1),
       freed_bytes(-1),
       alloc_overhead_bytes(-1),
       max_allocated_bytes(-1) {}
 
 DeathDataSnapshot::DeathDataSnapshot(int count,
                                      int32_t run_duration_sum,
                                      int32_t run_duration_max,
                                      int32_t run_duration_sample,
                                      int32_t queue_duration_sum,
                                      int32_t queue_duration_max,
                                      int32_t queue_duration_sample,
                                      int32_t alloc_ops,
                                      int32_t free_ops,
-                                     int32_t allocated_bytes,
-                                     int32_t freed_bytes,
-                                     int32_t alloc_overhead_bytes,
+                                     int64_t allocated_bytes,
+                                     int64_t freed_bytes,
+                                     int64_t alloc_overhead_bytes,
                                      int32_t max_allocated_bytes)
     : count(count),
       run_duration_sum(run_duration_sum),
       run_duration_max(run_duration_max),
       run_duration_sample(run_duration_sample),
       queue_duration_sum(queue_duration_sum),
       queue_duration_max(queue_duration_max),
       queue_duration_sample(queue_duration_sample),
       alloc_ops(alloc_ops),
       free_ops(free_ops),
@@ skipping 774 matching lines @@
 #endif
 }
 
 ProcessDataSnapshot::ProcessDataSnapshot(const ProcessDataSnapshot& other) =
     default;
 
 ProcessDataSnapshot::~ProcessDataSnapshot() {
 }
 
 }  // namespace tracked_objects