Reland of: components/metrics: Add runtime memory leak detector

Reland of: components/metrics: Add runtime memory leak detector

Reverted because it was failing some buildbots. The fixes have
been squashed into this one but they can be seen here as separate
patches.
http://crrev.com/1468873002
http://crrev.com/1471623003

==== Original commit message ====

This patch adds heuristic-based memory leak detector. Unlike
traditional leak detectors like valgrind, it doesn't wait until
a process terminates to check for leftover allocations. Instead,
it analyzes allocation patterns over time.

This code is not thread-safe. It is up to the caller of this code
to ensure thread safety.

BUG=382705
Signed-off-by: Simon Que <sque@chromium.org>;

Committed: https://crrev.com/096af097febf8a07d1323593e8cf94e83467ba71
Cr-Commit-Position: refs/heads/master@{#363836}

[Simon Que, 2015-11-23 19:38:59]

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

[Simon Que, 2015-11-23 19:39:00]

The original patch broke some builders and got reverted. I have fixed the two
problems. The individual fixes are linked in the commit message.

[jochen (gone - plz use gerrit), 2015-11-24 13:02:18]

trybots?

[Simon Que, 2015-11-24 19:35:08]

On 2015/11/24 13:02:18, jochen wrote:
> trybots?

These two:

http://build.chromium.org/p/chromium.lkgr/builders/Win%20x64/builds/1964/step...
https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28...

[Simon Que, 2015-11-24 21:53:08]

The CQ bit was checked by sque@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 21:54:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1472763003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1472763003/1

[commit-bot: I haz the power, 2015-11-24 21:54:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 21:54:56]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Simon Que, 2015-11-25 05:57:39]

On 2015/11/24 13:02:18, jochen wrote:
> trybots?

I'm not sure what you meant here. If you're suggesting that I run some trybots
first, neither of the failing trybots is listed on here. Nor do they get run as
part of the CQ. Is there some other command to run one of these?

Also, I've linked to the two fixes as separate CLs in the patch description, so
you can see what was changed from the reverted CL.

Let me know, thanks.

[jochen (gone - plz use gerrit), 2015-11-25 08:39:57]

you should be able to trigger tryjobs using "git cl try"

[Simon Que, 2015-11-26 05:40:46]

On 2015/11/25 08:39:57, jochen wrote:
> you should be able to trigger tryjobs using "git cl try"

linux_android_rel_ng keeps failing due to some infrastructure issue. I'll have
to file a bug.
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

[jochen (gone - plz use gerrit), 2015-11-26 13:21:29]

lgtm

[dcheng, 2015-11-30 21:05:47]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-11-30 21:05:48]

https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
File components/metrics/leak_detector/call_stack_manager.h (right):

https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
components/metrics/leak_detector/call_stack_manager.h:89:
base::hash_set<CallStack*,
Can we just use std::unique_ptr with a custom deleter? This seems entirely too
clever.

A simple custom deleter:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

More complicated custom deleters:
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/scoped_nss_...

[Simon Que, 2015-11-30 22:10:41]

https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
File components/metrics/leak_detector/call_stack_manager.h (right):

https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
components/metrics/leak_detector/call_stack_manager.h:89:
base::hash_set<CallStack*,
On 2015/11/30 21:05:48, dcheng wrote:
> Can we just use std::unique_ptr with a custom deleter? This seems entirely too
> clever.

Do you mean scoped_ptr? I don't think std::unique_ptr is allowed yet.

This would only move existing code around. The freeing code in
call_stack_manager.cc would just end up in the custom deleter.

[dcheng, 2015-11-30 23:10:10]

On 2015/11/30 at 22:10:41, sque wrote:
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> File components/metrics/leak_detector/call_stack_manager.h (right):
> 
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> components/metrics/leak_detector/call_stack_manager.h:89:
base::hash_set<CallStack*,
> On 2015/11/30 21:05:48, dcheng wrote:
> > Can we just use std::unique_ptr with a custom deleter? This seems entirely
too
> > clever.
> 
> Do you mean scoped_ptr? I don't think std::unique_ptr is allowed yet.
> 
> This would only move existing code around. The freeing code in
> call_stack_manager.cc would just end up in the custom deleter.

Right, scoped_ptr.

Using a custom deleter will be a lot less boilerplate, unless I'm missing
something.

[Simon Que, 2015-12-01 01:21:52]

On 2015/11/30 23:10:10, dcheng wrote:
> On 2015/11/30 at 22:10:41, sque wrote:
> >
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > File components/metrics/leak_detector/call_stack_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > components/metrics/leak_detector/call_stack_manager.h:89:
> base::hash_set<CallStack*,
> > On 2015/11/30 21:05:48, dcheng wrote:
> > > Can we just use std::unique_ptr with a custom deleter? This seems entirely
> too
> > > clever.
> > 
> > Do you mean scoped_ptr? I don't think std::unique_ptr is allowed yet.
> > 
> > This would only move existing code around. The freeing code in
> > call_stack_manager.cc would just end up in the custom deleter.
> 
> Right, scoped_ptr.
> 
> Using a custom deleter will be a lot less boilerplate, unless I'm missing
> something.

Here's a copy of the changes to call_stack_manager.*:
https://codereview.chromium.org/1471623003/

I'm having trouble in two places in GetCallStack() -- see my comments in that
CL.

[Simon Que, 2015-12-01 01:23:14]

On 2015/12/01 01:21:52, Simon Que wrote:
> On 2015/11/30 23:10:10, dcheng wrote:
> > On 2015/11/30 at 22:10:41, sque wrote:
> > >
> >
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > > File components/metrics/leak_detector/call_stack_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > > components/metrics/leak_detector/call_stack_manager.h:89:
> > base::hash_set<CallStack*,
> > > On 2015/11/30 21:05:48, dcheng wrote:
> > > > Can we just use std::unique_ptr with a custom deleter? This seems
entirely
> > too
> > > > clever.
> > > 
> > > Do you mean scoped_ptr? I don't think std::unique_ptr is allowed yet.
> > > 
> > > This would only move existing code around. The freeing code in
> > > call_stack_manager.cc would just end up in the custom deleter.
> > 
> > Right, scoped_ptr.
> > 
> > Using a custom deleter will be a lot less boilerplate, unless I'm missing
> > something.
> 
> Here's a copy of the changes to call_stack_manager.*:
> https://codereview.chromium.org/1471623003/
> 
> I'm having trouble in two places in GetCallStack() -- see my comments in that
> CL.

Also, notice that the number of lines of code in the .cc file doesn't change by
much. OTOH I did have to add several lines to the .h file.

[Simon Que, 2015-12-01 02:58:23]

Patchset #5 (id:80001) has been deleted

[dcheng, 2015-12-01 18:55:49]

On 2015/12/01 at 01:23:14, sque wrote:
> On 2015/12/01 01:21:52, Simon Que wrote:
> > On 2015/11/30 23:10:10, dcheng wrote:
> > > On 2015/11/30 at 22:10:41, sque wrote:
> > > >
> > >
> >
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > > > File components/metrics/leak_detector/call_stack_manager.h (right):
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/1472763003/diff/60001/components/metrics/leak...
> > > > components/metrics/leak_detector/call_stack_manager.h:89:
> > > base::hash_set<CallStack*,
> > > > On 2015/11/30 21:05:48, dcheng wrote:
> > > > > Can we just use std::unique_ptr with a custom deleter? This seems
entirely
> > > too
> > > > > clever.
> > > > 
> > > > Do you mean scoped_ptr? I don't think std::unique_ptr is allowed yet.
> > > > 
> > > > This would only move existing code around. The freeing code in
> > > > call_stack_manager.cc would just end up in the custom deleter.
> > > 
> > > Right, scoped_ptr.
> > > 
> > > Using a custom deleter will be a lot less boilerplate, unless I'm missing
> > > something.
> > 
> > Here's a copy of the changes to call_stack_manager.*:
> > https://codereview.chromium.org/1471623003/
> > 
> > I'm having trouble in two places in GetCallStack() -- see my comments in
that
> > CL.
> 
> Also, notice that the number of lines of code in the .cc file doesn't change
by much. OTOH I did have to add several lines to the .h file.

Just to close the loop, we ended up having a discussion. CustomAllocator is
indeed required, since we don't want to go through the normal allocator (but
that wasn't clear from the comments in the code). Also, base::hash_set (which I
failed to notice this patch was using) doesn't support move-only types.

[Will Harris, 2015-12-01 20:48:51]

lgtm

[Simon Que, 2015-12-01 22:13:25]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-12-01 22:13:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1472763003/#ps100001
(title: "Indicate reason for using CustomAllocator")

[commit-bot: I haz the power, 2015-12-01 22:16:10]

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

[commit-bot: I haz the power, 2015-12-01 22:27:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 22:27:26]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Simon Que, 2015-12-02 00:35:05]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-12-02 00:41:25]

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

[commit-bot: I haz the power, 2015-12-02 01:53:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 01:53:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2015-12-02 02:04:02]

On 2015/12/02 01:53:45, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

base/move.h has changed recently:
https://chromium.googlesource.com/chromium/src/+/e1b0277c20198c31b8782f567634...

[Will Harris, 2015-12-02 02:12:20]

looks like this needs a rebase.

[Simon Que, 2015-12-02 02:32:51]

thakis: Need your input on whether move constructors are allowed in
ranked_list.(cc|h).

Previously I was using MOVE_ONLY_TYPE_FOR_CPP_03 instead of C++11 move
semantics. Now that crrev.com/1407443002 is in, anything that used the move
macro seems to need to define move constructors.

Move semantics are used in RankedList because it is passed to another class
(LeakAnalyzer::AddSample()) and then stored there. It saves an extra copy.
However, I can use swap() if move semantics are a no-go.

[Simon Que, 2015-12-04 21:32:28]

Patchset #6 (id:120001) has been deleted

[Simon Que, 2015-12-07 18:58:47]

On 2015/12/02 02:32:51, Simon Que wrote:
> thakis: Need your input on whether move constructors are allowed in
> ranked_list.(cc|h).
> 
> Previously I was using MOVE_ONLY_TYPE_FOR_CPP_03 instead of C++11 move
> semantics. Now that crrev.com/1407443002 is in, anything that used the move
> macro seems to need to define move constructors.
> 
> Move semantics are used in RankedList because it is passed to another class
> (LeakAnalyzer::AddSample()) and then stored there. It saves an extra copy.
> However, I can use swap() if move semantics are a no-go.

dcheng: Will is OOO, can you take a look at my latest changes? I still haven't
heard from Nico.

[dcheng, 2015-12-08 02:04:51]

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.cc (right):

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:18:
entries_.swap(other.entries_);
entries_(std::move(other.entries_)) to keep the two consistent?

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:30: Entry{ValueType(), count});
Note that uniform initialization syntax isn't yet allowed.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:37: entries_.insert(iter,
Entry({value, count}));
Ditto.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.h (right):

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:13: #include "base/move.h"
Remove this #include (see below for more info).

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:26:
MOVE_ONLY_TYPE_FOR_CPP_03(RankedList);
This macro is being removed in https://codereview.chromium.org/1501793003. Just
use DISALLOW_COPY_AND_ASSIGN() in the standard location at the bottom of the
class declaration.

[Simon Que, 2015-12-08 02:24:16]

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.cc (right):

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:18:
entries_.swap(other.entries_);
On 2015/12/08 02:04:51, dcheng wrote:
> entries_(std::move(other.entries_)) to keep the two consistent?

Done.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:30: Entry{ValueType(), count});
On 2015/12/08 02:04:51, dcheng wrote:
> Note that uniform initialization syntax isn't yet allowed.

Done.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:37: entries_.insert(iter,
Entry({value, count}));
On 2015/12/08 02:04:51, dcheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.h (right):

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:13: #include "base/move.h"
On 2015/12/08 02:04:51, dcheng wrote:
> Remove this #include (see below for more info).

Done.

https://codereview.chromium.org/1472763003/diff/140001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:26:
MOVE_ONLY_TYPE_FOR_CPP_03(RankedList);
On 2015/12/08 02:04:51, dcheng wrote:
> This macro is being removed in https://codereview.chromium.org/1501793003.
Just
> use DISALLOW_COPY_AND_ASSIGN() in the standard location at the bottom of the
> class declaration.

Done.

[dcheng, 2015-12-08 05:22:01]

lgtm

https://codereview.chromium.org/1472763003/diff/160001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.cc (right):

https://codereview.chromium.org/1472763003/diff/160001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:40: new_entry.value = value;
I don't think there's any harm in just initializing this on line 30 with the
rest of the init.

[Simon Que, 2015-12-08 06:03:42]

https://codereview.chromium.org/1472763003/diff/160001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list.cc (right):

https://codereview.chromium.org/1472763003/diff/160001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:40: new_entry.value = value;
On 2015/12/08 05:22:00, dcheng wrote:
> I don't think there's any harm in just initializing this on line 30 with the
> rest of the init.

Done.

[Simon Que, 2015-12-08 06:03:49]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-12-08 06:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
wfh@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1472763003/#ps180001
(title: "Initialize RankedList::Entry fields together")

[commit-bot: I haz the power, 2015-12-08 06:04:07]

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

[commit-bot: I haz the power, 2015-12-08 06:06:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 06:06:04]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Simon Que, 2015-12-08 08:25:23]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-12-08 08:25:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1472763003/#ps200001
(title: "Rebased")

[commit-bot: I haz the power, 2015-12-08 08:26:11]

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

[commit-bot: I haz the power, 2015-12-08 08:57:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 08:57:35]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Simon Que, 2015-12-08 09:19:41]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-12-08 09:20:03]

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

[commit-bot: I haz the power, 2015-12-08 09:44:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 09:44:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Simon Que, 2015-12-08 23:18:56]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2015-12-08 23:25:42]

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

[commit-bot: I haz the power, 2015-12-09 00:21:04]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-12-09 00:22:41]

Description was changed from

==========
Reland of: components/metrics: Add runtime memory leak detector

Reverted because it was failing some buildbots. The fixes have
been squashed into this one but they can be seen here as separate
patches.
http://crrev.com/1468873002
http://crrev.com/1471623003

==== Original commit message ====

This patch adds heuristic-based memory leak detector. Unlike
traditional leak detectors like valgrind, it doesn't wait until
a process terminates to check for leftover allocations. Instead,
it analyzes allocation patterns over time.

This code is not thread-safe. It is up to the caller of this code
to ensure thread safety.

BUG=382705

Signed-off-by: Simon Que <sque@chromium.org>
==========

to

==========
Reland of: components/metrics: Add runtime memory leak detector

Reverted because it was failing some buildbots. The fixes have
been squashed into this one but they can be seen here as separate
patches.
http://crrev.com/1468873002
http://crrev.com/1471623003

==== Original commit message ====

This patch adds heuristic-based memory leak detector. Unlike
traditional leak detectors like valgrind, it doesn't wait until
a process terminates to check for leftover allocations. Instead,
it analyzes allocation patterns over time.

This code is not thread-safe. It is up to the caller of this code
to ensure thread safety.

BUG=382705

Signed-off-by: Simon Que <sque@chromium.org>

Committed: https://crrev.com/096af097febf8a07d1323593e8cf94e83467ba71
Cr-Commit-Position: refs/heads/master@{#363836}
==========

[commit-bot: I haz the power, 2015-12-09 00:22:42]

Patchset 9 (id:??) landed as
https://crrev.com/096af097febf8a07d1323593e8cf94e83467ba71
Cr-Commit-Position: refs/heads/master@{#363836}