Record call site history in LeakDetectorImpl

Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass

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

[Simon Que, 2016-04-08 22:07:31]

Description was changed from

==========
Record call site history in LeakDetectorImpl

BUG=chromium:382705
TEST=unit tests pass
==========

to

==========
Record call site history in LeakDetectorImpl

BUG=chromium:382705
TEST=unit tests pass
==========

[Simon Que, 2016-04-08 22:07:31]

sque@chromium.org changed reviewers:
+ wfh@chromium.org

[Simon Que, 2016-04-09 00:15:58]

https://codereview.chromium.org/1870233003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/1870233003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:552: }
Still need to check the call stack breakdown.

[Simon Que, 2016-04-09 01:04:32]

https://codereview.chromium.org/1870233003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/1870233003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:552: }
On 2016/04/09 00:15:58, Simon Que wrote:
> Still need to check the call stack breakdown.

Done.

[Will Harris, 2016-04-09 19:39:02]

can you give a little more background on why this is needed and how it fits into
the Grand Plan, perhaps in the CL description?

[Simon Que, 2016-04-09 19:54:44]

Description was changed from

==========
Record call site history in LeakDetectorImpl

BUG=chromium:382705
TEST=unit tests pass
==========

to

==========
Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass
==========

[Simon Que, 2016-04-09 19:54:53]

On 2016/04/09 19:39:02, Will Harris wrote:
> can you give a little more background on why this is needed and how it fits
into
> the Grand Plan, perhaps in the CL description?

Done. Let me know if you have more questions.

[Will Harris, 2016-04-09 20:10:23]

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_impl.cc:272:
InternalVector<AllocationBreakdown>* dest = &report->alloc_breakdown_history_;
why take this into a ptr, can't you just operate directly on the
report->alloc_breakdown_history - or just to shorten line length?

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/ranked_set.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/ranked_set.cc:55: // whole Entry struct.
can't you just implement the operators in Entry rather than implementing Find?

[Simon Que, 2016-04-09 21:04:11]

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_impl.cc:272:
InternalVector<AllocationBreakdown>* dest = &report->alloc_breakdown_history_;
On 2016/04/09 20:10:23, Will Harris wrote:
> why take this into a ptr, can't you just operate directly on the
> report->alloc_breakdown_history - or just to shorten line length?

Kind of. I am trying to make the rest of this function more clear by having a
src/dest duality ... e.g. src.size()/dest->size, src_iter/dest_iter.

However, I could hold off until around line 283 (where src is defined) to define
dest, and use the full name of the var before that.

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/ranked_set.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/ranked_set.cc:55: // whole Entry struct.
On 2016/04/09 20:10:23, Will Harris wrote:
> can't you just implement the operators in Entry rather than implementing Find?

The operator would have to look something like:

  bool operator==(const Entry& other) const {
    return value == other.value && count == other.count;
  }

However, in this case we don't know the count. The alternative would be to have
an == operator that compares |value| only, but it would be strange to someone
looking at the code.

[Will Harris, 2016-04-11 17:26:37]

lgtm

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_impl.cc:272:
InternalVector<AllocationBreakdown>* dest = &report->alloc_breakdown_history_;
On 2016/04/09 21:04:11, Simon Que wrote:
> On 2016/04/09 20:10:23, Will Harris wrote:
> > why take this into a ptr, can't you just operate directly on the
> > report->alloc_breakdown_history - or just to shorten line length?
> 
> Kind of. I am trying to make the rest of this function more clear by having a
> src/dest duality ... e.g. src.size()/dest->size, src_iter/dest_iter.
> 
> However, I could hold off until around line 283 (where src is defined) to
define
> dest, and use the full name of the var before that.

no that's fine. I understand why now.

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/ranked_set.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/ranked_set.cc:55: // whole Entry struct.
On 2016/04/09 21:04:11, Simon Que wrote:
> On 2016/04/09 20:10:23, Will Harris wrote:
> > can't you just implement the operators in Entry rather than implementing
Find?
> 
> The operator would have to look something like:
> 
>   bool operator==(const Entry& other) const {
>     return value == other.value && count == other.count;
>   }
> 
> However, in this case we don't know the count. The alternative would be to
have
> an == operator that compares |value| only, but it would be strange to someone
> looking at the code.

I think as long as it's commented (perhaps explain what specifically is
"unusual") then it'll be fine. thanks.

[Simon Que, 2016-04-11 18:41:08]

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/ranked_set.cc (right):

https://codereview.chromium.org/1870233003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/ranked_set.cc:55: // whole Entry struct.
On 2016/04/11 17:26:37, Will Harris wrote:
> On 2016/04/09 21:04:11, Simon Que wrote:
> > On 2016/04/09 20:10:23, Will Harris wrote:
> > > can't you just implement the operators in Entry rather than implementing
> Find?
> > 
> > The operator would have to look something like:
> > 
> >   bool operator==(const Entry& other) const {
> >     return value == other.value && count == other.count;
> >   }
> > 
> > However, in this case we don't know the count. The alternative would be to
> have
> > an == operator that compares |value| only, but it would be strange to
someone
> > looking at the code.
> 
> I think as long as it's commented (perhaps explain what specifically is
> "unusual") then it'll be fine. thanks.

Done.

[Simon Que, 2016-04-11 21:14:54]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-04-11 21:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1870233003/#ps40001
(title: "Clarify comments in RankedSet")

[commit-bot: I haz the power, 2016-04-11 21:15:21]

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

[commit-bot: I haz the power, 2016-04-11 22:35:34]

Description was changed from

==========
Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass
==========

to

==========
Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass
==========

[commit-bot: I haz the power, 2016-04-11 22:35:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-11 22:36:59]

Description was changed from

==========
Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass
==========

to

==========
Record call site history in LeakDetectorImpl

Recording a history of the call sites with the most allocations gives us a
better picture of the allocation patterns leading up to a suspected leak
being found. This is useful data that is relatively easy to obtain -- it is
already being processed by the Leak Detector, just have to record it.

BUG=chromium:382705
TEST=unit tests pass

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

[commit-bot: I haz the power, 2016-04-11 22:37:00]

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

[Lei Zhang, 2016-04-12 00:41:10]

We have red bots:

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20...

Revert?

[Lei Zhang, 2016-04-12 01:09:08]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1878863004/ by thestig@chromium.org.

The reason for reverting is: Broke ChromeOS ASAN and MSAN bots.

Links to failures are on the original code review..