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

base/trace_event/heap_profiler_allocation_register.h

 // 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.
 
 #ifndef BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_REGISTER_H_
 #define BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_REGISTER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 10 matching lines @@
 namespace base {
 namespace trace_event {
 
 class AllocationRegisterTest;
 
 namespace internal {
 
 // Allocates a region of virtual address space of |size| rounded up to the
 // system page size. The memory is zeroed by the system. A guard page is
 // added after the end.
+//
+// TODO(awong): Remove the guarded memory. This isn't currently being used and

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

You say "this isn't currently used" but that is what allowed me to spot the
previous bug about the sentinel refcounting..
true, but how is removing the guard page making anything less complex? You talk
as if by removing the guard page we could make this hashtable, or the logic
around that, way simpler. unless I am missing something, that's not true.

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

I'll remove it.  The code just has too many concepts in one file. That's yet
another thing to think through and drift. For example, we had a logic divergence
where the code comment talked about relying on guard pages to crash and then
someone added a CHECK fail later. I agree it's benign, but I would prefer
removing
it. On the otherhand, this discussion has already pushed me beyond how much I
care to discuss it.

+// this code is complex looking enough that this costs more than its bug-bashing
+// weight.
 void* AllocateGuardedVirtualMemory(size_t size);
 
 // Frees a region of virtual address space allocated by a call to
 // |AllocateVirtualMemory|.
 void FreeGuardedVirtualMemory(void* address, size_t allocated_size);
 
 // Hash map that mmaps memory only once in the constructor. Its API is
 // similar to std::unordered_map, only index (KVIndex) is used to address
 template <size_t NumBuckets, class Key, class Value, class KeyHasher>
 class FixedHashMap {
   // To keep things simple we don't call destructors.
   static_assert(is_trivially_destructible<Key>::value &&
                     is_trivially_destructible<Value>::value,
                 "Key and Value shouldn't have destructors");
  public:
   using KVPair = std::pair<const Key, Value>;
 
   // For implementation simplicity API uses integer index instead
   // of iterators. Most operations (except Find) on KVIndex are O(1).
   using KVIndex = size_t;
   enum : KVIndex { kInvalidKVIndex = static_cast<KVIndex>(-1) };
 
   // Capacity controls how many items this hash map can hold, and largely
   // affects memory footprint.
-  FixedHashMap(size_t capacity)
+  explicit FixedHashMap(size_t capacity)
       : num_cells_(capacity),
         num_inserts_dropped_(0),
         cells_(static_cast<Cell*>(
             AllocateGuardedVirtualMemory(num_cells_ * sizeof(Cell)))),
         buckets_(static_cast<Bucket*>(
             AllocateGuardedVirtualMemory(NumBuckets * sizeof(Bucket)))),
         free_list_(nullptr),
         next_unused_cell_(0) {}
 
   ~FixedHashMap() {
@@ skipping 179 matching lines @@
 };
 
 }  // namespace internal
 
 class TraceEventMemoryOverhead;
 
 // The allocation register keeps track of all allocations that have not been
 // freed. Internally it has two hashtables: one for Backtraces and one for
 // actual allocations. Sizes of both hashtables are fixed, and this class
 // allocates (mmaps) only in its constructor.
+//
+// When either hash table hits max size, new inserts are dropped.
 class BASE_EXPORT AllocationRegister {
  public:
   // Details about an allocation.
   struct Allocation {
     const void* address;
     size_t size;
     AllocationContext context;
   };
 
   // An iterator that iterates entries in no particular order.
@@ skipping 86 matching lines @@
   struct AddressHasher {
     size_t operator () (const void* address) const;
   };
 
   using AllocationMap = internal::FixedHashMap<
       kAllocationBuckets,
       const void*,
       AllocationInfo,
       AddressHasher>;
 
+  // This method should be called once to initialize
+  // |out_of_storage_backtrace_index_|. The first call will create a unique
+  // sentinel entry. The return value of multiple calls is undefined.
+  BacktraceMap::KVIndex CreateBacktraceSentinel();
+
   BacktraceMap::KVIndex InsertBacktrace(const Backtrace& backtrace);
   void RemoveBacktrace(BacktraceMap::KVIndex index);
 
   Allocation GetAllocation(AllocationMap::KVIndex) const;
 
   AllocationMap allocations_;
   BacktraceMap backtraces_;
 
-  // Sentinel used when we run out of backtraces_ storage.
-  BacktraceMap::KVIndex out_of_storage_backtrace_index_;
+  // Sentinel used when the |backtraces_| table is full.
+  const BacktraceMap::KVIndex out_of_storage_backtrace_index_;
 
   DISALLOW_COPY_AND_ASSIGN(AllocationRegister);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_HEAP_PROFILER_ALLOCATION_REGISTER_H_