Handle zero-size allocations properly in heap profiler

base/trace_event/heap_profiler_heap_dump_writer.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_heap_dump_writer.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <iterator>
@@ skipping 52 matching lines @@
 }
 
 // Groups the allocations in the bucket by |breakBy|. The buckets in the
 // returned list will have |backtrace_cursor| advanced or
 // |is_broken_down_by_type_name| set depending on the property to group by.
 std::vector<Bucket> GetSubbuckets(const Bucket& bucket, BreakDownMode breakBy) {
   base::hash_map<const char*, Bucket> breakdown;
 
   if (breakBy == BreakDownMode::kByBacktrace) {
     for (const auto& context_and_size : bucket.bytes_by_context) {
+      DCHECK(context_and_size.second > 0);

[Primiano Tucci (use gerrit), 2016/04/05 18:38:48]

I think you don't need these ones. These are internal private functions.

       const Backtrace& backtrace = context_and_size.first->backtrace;
       const char* const* begin = std::begin(backtrace.frames);
       const char* const* end = std::end(backtrace.frames);
       const char* const* cursor = begin + bucket.backtrace_cursor;
 
       // The backtrace in the context is padded with null pointers, but these
       // should not be considered for breakdown. Adjust end to point past the
       // last non-null frame.
       while (begin != end && *(end - 1) == nullptr)
         end--;
 
       DCHECK_LE(cursor, end);
 
       if (cursor != end) {
         Bucket& subbucket = breakdown[*cursor];
         subbucket.size += context_and_size.second;
         subbucket.bytes_by_context.push_back(context_and_size);
         subbucket.backtrace_cursor = bucket.backtrace_cursor + 1;
         subbucket.is_broken_down_by_type_name =
             bucket.is_broken_down_by_type_name;
         DCHECK_GT(subbucket.size, 0u);
       }
     }
   } else if (breakBy == BreakDownMode::kByTypeName) {
     if (!bucket.is_broken_down_by_type_name) {
       for (const auto& context_and_size : bucket.bytes_by_context) {
+        DCHECK(context_and_size.second > 0);

[Primiano Tucci (use gerrit), 2016/04/05 18:38:48]

ditto

         const AllocationContext* context = context_and_size.first;
         Bucket& subbucket = breakdown[context->type_name];
         subbucket.size += context_and_size.second;
         subbucket.bytes_by_context.push_back(context_and_size);
         subbucket.backtrace_cursor = bucket.backtrace_cursor;
         subbucket.is_broken_down_by_type_name = true;
         DCHECK_GT(subbucket.size, 0u);
       }
     }
   }
@@ skipping 25 matching lines @@
   // part that contains the max-heap of small buckets.
   size_t accounted_for = 0;
   std::vector<Bucket>::iterator it;
   for (it = buckets.end(); it != buckets.begin(); --it) {
     // Compute the contribution to the number of bytes accounted for as a
     // fraction of 125 (in increments of 0.8 percent). Anything less than 1/125
     // is rounded down to 0 due to integer division. Buckets are iterated by
     // descending size, so later buckets cannot have a larger contribution than
     // this one.
     accounted_for += buckets.front().size;
-    size_t contribution = buckets.front().size * 125 / accounted_for;

[Dmitry Skiba, 2016/04/05 18:54:13]

With new checks, do we still need this change? Version with 'contribution'
variable was clearer, and inlined with the comment above.

-    if (contribution == 0)
+    if (buckets.front().size < (accounted_for / 125))
       break;
 
     // Put the largest bucket in [begin, it) at |it - 1| and max-heapify
     // [begin, it - 1). This puts the next largest bucket at |buckets.front()|.
     std::pop_heap(buckets.begin(), it);
   }
 
   // At this point, |buckets| looks like this (numbers are bucket sizes):
   //
   // <-- max-heap of small buckets --->
@@ skipping 71 matching lines @@
       BreakDown(subbucket);
 }
 
 const std::set<Entry>& HeapDumpWriter::Summarize(
     const hash_map<AllocationContext, size_t>& bytes_by_context) {
   // Start with one bucket that represents the entire heap. Iterate by
   // reference, because the allocation contexts are going to point to allocation
   // contexts stored in |bytes_by_context|.
   Bucket root_bucket;
   for (const auto& context_and_size : bytes_by_context) {
+    DCHECK(context_and_size.second > 0);

[Primiano Tucci (use gerrit), 2016/04/05 18:38:48]

Right you just need this one.
Nit: DCHECK_GT(context_and_size.second, 0) is more Chromey

     const AllocationContext* context = &context_and_size.first;
     const size_t size = context_and_size.second;
     root_bucket.bytes_by_context.push_back(std::make_pair(context, size));
     root_bucket.size += size;
   }
 
   AddEntryForBucket(root_bucket);
 
   // Recursively break down the heap and fill |entries_| with entries to dump.
   BreakDown(root_bucket);
@@ skipping 46 matching lines @@
     const hash_map<AllocationContext, size_t>& bytes_by_size,
     StackFrameDeduplicator* stack_frame_deduplicator,
     TypeNameDeduplicator* type_name_deduplicator) {
   internal::HeapDumpWriter writer(stack_frame_deduplicator,
                                   type_name_deduplicator);
   return Serialize(writer.Summarize(bytes_by_size));
 }
 
 }  // namespace trace_event
 }  // namespace base