heap-profiler: Clean up some comments and C++.

base/trace_event/heap_profiler_allocation_register.cc

 // Copyright 2015 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/trace_event/heap_profiler_allocation_register.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/trace_event/trace_event_memory_overhead.h"
@@ skipping 58 matching lines @@
   const uintptr_t shift = 15;
   const uintptr_t h = (key * a) >> shift;
   return h;
 }
 
 AllocationRegister::AllocationRegister()
     : AllocationRegister(kAllocationCapacity, kBacktraceCapacity) {}
 
 AllocationRegister::AllocationRegister(size_t allocation_capacity,
                                        size_t backtrace_capacity)
-    : allocations_(allocation_capacity), backtraces_(backtrace_capacity) {
-  Backtrace sentinel = {};
-  sentinel.frames[0] = StackFrame::FromThreadName("[out of heap profiler mem]");
-  sentinel.frame_count = 1;
-
-  // Rationale for max / 2: in theory we could just start the sentinel with a
-  // refcount == 0. However, this optimization avoids to hit the 2nd condition
-  // of the "if" in RemoveBacktrace, hence reducing the chances of hurting the
-  // fastpath. From a functional viewpoint, the sentinel is safe even if we wrap
-  // over the refcount.
-  BacktraceMap::KVPair::second_type sentinel_refcount =
-      std::numeric_limits<BacktraceMap::KVPair::second_type>::max() / 2;
-  auto index_and_flag = backtraces_.Insert(sentinel, sentinel_refcount);
-  DCHECK(index_and_flag.second);
-  out_of_storage_backtrace_index_ = index_and_flag.first;
-}
+    : allocations_(allocation_capacity),
+      backtraces_(backtrace_capacity),
+      out_of_storage_backtrace_index_(CreateBacktraceSentinel()) {}

[Primiano Tucci (use gerrit), 2017/04/04 09:50:23]

awesome. I wanted to suggest this yesterday during our chat, but that didn't
want to cause too much trouble.
Along these lines, how about at this point:
- make const kOutOfStorageBackTraceIndex = 0;
- in this ctor just:
  KVIndex sentinel_index = CreateBacktraceSentinel();
  DCHECK_EQ(kOutOfStorageBackTraceIndex, sentinel_index)
 (although at this point you could argue there is no need for a dedicated
CreateBacktraceSentinel function anymore that that could be just in the body of
the ctor)

This should allow some more const propagation and emit code which is hopefully
be faster and smaller than the current one.
(having said this, not too strong on this, so feel free to keep the code as it
is if you prefer this way)

[awong, 2017/04/04 18:28:30]

Done.

 
 AllocationRegister::~AllocationRegister() {}
 
 bool AllocationRegister::Insert(const void* address,
                                 size_t size,
                                 const AllocationContext& context) {
   DCHECK(address != nullptr);
   if (size == 0) {
     return false;
   }
@@ skipping 49 matching lines @@
 
 void AllocationRegister::EstimateTraceMemoryOverhead(
     TraceEventMemoryOverhead* overhead) const {
   size_t allocated = sizeof(AllocationRegister);
   size_t resident = sizeof(AllocationRegister) +
                     allocations_.EstimateUsedMemory() +
                     backtraces_.EstimateUsedMemory();
   overhead->Add("AllocationRegister", allocated, resident);
 }
 
+AllocationRegister::BacktraceMap::KVIndex
+AllocationRegister::CreateBacktraceSentinel() {
+  Backtrace sentinel = {};
+  sentinel.frames[0] = StackFrame::FromThreadName("[out of heap profiler mem]");
+  sentinel.frame_count = 1;
+
+  // Rationale for max / 2: in theory we could just start the sentinel with a
+  // refcount == 0. However, using max / 2 allows short circuiting of the
+  // conditional in RemoveBacktrace() keeping the sentinel logic out of the fast
+  // path. From a functional viewpoint, the sentinel is safe even if we wrap
+  // over the refcount.
+  BacktraceMap::KVPair::second_type sentinel_refcount =
+      std::numeric_limits<BacktraceMap::KVPair::second_type>::max() / 2;
+  auto index_and_flag = backtraces_.Insert(sentinel, sentinel_refcount);
+  DCHECK(index_and_flag.second);
+
+  return index_and_flag.first;
+}
+
 AllocationRegister::BacktraceMap::KVIndex AllocationRegister::InsertBacktrace(
     const Backtrace& backtrace) {
   auto index = backtraces_.Insert(backtrace, 0).first;
   if (index == BacktraceMap::kInvalidKVIndex)
     return out_of_storage_backtrace_index_;
   auto& backtrace_and_count = backtraces_.Get(index);
   backtrace_and_count.second++;
   return index;
 }
 
@@ skipping 11 matching lines @@
   const auto& address_and_info = allocations_.Get(index);
   const auto& backtrace_and_count =
       backtraces_.Get(address_and_info.second.backtrace_index);
   return {address_and_info.first, address_and_info.second.size,
           AllocationContext(backtrace_and_count.first,
                             address_and_info.second.type_name)};
 }
 
 }  // namespace trace_event
 }  // namespace base