Handle zero-size allocations properly in heap profiler

Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.

Committed: https://crrev.com/a29434e19fa91646e9c40afa5078e64e0ae73903
Cr-Commit-Position: refs/heads/master@{#385426}

[kraynov-google, 2016-04-05 17:13:22]

Description was changed from

==========
Fix div-by-zero in heap profiler dump writer

BUG=
==========

to

==========
Fix div-by-zero in heap profiler dump writer

Dump bucket size could be zero.
This change performs safe percentage calculation.
==========

[kraynov-google, 2016-04-05 17:13:22]

kraynov@google.com changed reviewers:
+ primiano@chromium.org

[kraynov-google, 2016-04-05 17:14:02]

kraynov@google.com changed reviewers:
+ kraynov@google.com

[kraynov-google, 2016-04-05 17:14:02]

Please take a look

[Primiano Tucci (use gerrit), 2016-04-05 17:26:40]

primiano@chromium.org changed reviewers:
+ dskiba@chromium.org

[Primiano Tucci (use gerrit), 2016-04-05 17:26:40]

Many thanks for looking at this.

I think this is a consequence of something that dskiba recently pointed out: we
record 0-sized allocations. 
I think the right fix here is preventing 0-sized allocations in the first place
to be recorded, which feels like we should:
- early out in AllocationRegister::Insert.
- DCHECK in HeapDumpWriter::Summarize that we don't have zero-size buckets.

Can you plz do that?

[kraynov-google, 2016-04-05 18:18:49]

Description was changed from

==========
Fix div-by-zero in heap profiler dump writer

Dump bucket size could be zero.
This change performs safe percentage calculation.
==========

to

==========
Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.
==========

[kraynov-google, 2016-04-05 18:22:11]

Updated

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

LGTM with comments addressed.

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:73:
DCHECK(context_and_size.second > 0);
I think you don't need these ones. These are internal private functions.

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:100:
DCHECK(context_and_size.second > 0);
ditto

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:238:
DCHECK(context_and_size.second > 0);
Right you just need this one.
Nit: DCHECK_GT(context_and_size.second, 0) is more Chromey

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

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2016-04-05 18:54:14]

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer.cc (left):

https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:144: size_t contribution =
buckets.front().size * 125 / accounted_for;
With new checks, do we still need this change? Version with 'contribution'
variable was clearer, and inlined with the comment above.

[kraynov-google, 2016-04-05 19:09:40]

On 2016/04/05 18:54:14, Dmitry Skiba wrote:
>
https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
> File base/trace_event/heap_profiler_heap_dump_writer.cc (left):
> 
>
https://codereview.chromium.org/1859143003/diff/20001/base/trace_event/heap_p...
> base/trace_event/heap_profiler_heap_dump_writer.cc:144: size_t contribution =
> buckets.front().size * 125 / accounted_for;
> With new checks, do we still need this change? Version with 'contribution'
> variable was clearer, and inlined with the comment above.

Multiplying by 125 can lead to integer overflow.

[kraynov-google, 2016-04-05 22:14:05]

Take a look please,
btw, tests are fixed

[Primiano Tucci (use gerrit), 2016-04-06 08:20:45]

PS6 LGTM with two comments addressed

Ah, can you also add a "tracing: " prefix before the CL title and description,
i.e.
tracing: Handle zero-size allocations in heap profiler

Thanks for fixing this :)

https://codereview.chromium.org/1859143003/diff/100001/base/trace_event/heap_...
File base/trace_event/heap_profiler_allocation_register_unittest.cc (right):

https://codereview.chromium.org/1859143003/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_register_unittest.cc:62:
reg.Insert(reinterpret_cast<void*>(1), 0, ctx);
I think this (and line 64) can be moved to the InsertRemove test below. IMHO not
worth making that a dedicated TEST_F.
Just add a comment before saying
// Zero-sized allocations should be discarded.

https://codereview.chromium.org/1859143003/diff/100001/base/trace_event/heap_...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1859143003/diff/100001/base/trace_event/heap_...
base/trace_event/heap_profiler_heap_dump_writer.cc:139: // If next bucket
increase this amount by less then 0.8% just skip the tail
I'd add a "(1/125)" after 0.8% otherwise It's not clear where it comes from.
Also, nit: missing trailing dots.

[kraynov, 2016-04-06 08:43:17]

The CQ bit was checked by kraynov@chromium.org

[kraynov, 2016-04-06 08:43:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1859143003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-04-06 08:43:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1859143003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1859143003/140001

[commit-bot: I haz the power, 2016-04-06 09:53:43]

Description was changed from

==========
Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.
==========

to

==========
Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.
==========

[commit-bot: I haz the power, 2016-04-06 09:53:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-06 09:54:46]

Description was changed from

==========
Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.
==========

to

==========
Handle zero-size allocations properly in heap profiler

Now zero-size allocations are ignored for heap profiling.
Percentage calculation is originally changed to avoid div-by-zero,
but it also solves integer overflow issue with >32768 Kb allocations
on platforms with 32-bit size_t.

Committed: https://crrev.com/a29434e19fa91646e9c40afa5078e64e0ae73903
Cr-Commit-Position: refs/heads/master@{#385426}
==========

[commit-bot: I haz the power, 2016-04-06 09:54:47]

Patchset 8 (id:??) landed as
https://crrev.com/a29434e19fa91646e9c40afa5078e64e0ae73903
Cr-Commit-Position: refs/heads/master@{#385426}