metrics: Connect leak detector to allocator

metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests

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

[Simon Que, 2016-02-03 02:33:50]

Description was changed from

==========
metrics: Connect leak detector to allocator

BUG==chromium:382705
TEST=build successfully, run components_unittests
==========

to

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

See leak_detector.cc.

BUG==chromium:382705
TEST=build successfully, run components_unittests
==========

[Simon Que, 2016-02-03 02:33:50]

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

[Simon Que, 2016-02-03 02:54:21]

Here's an outline of what this patch contains:
- base/allocator/allocator_extension.*: Added functions for registering
alloc/free hooks, and for unwinding the call stack.
- leak_detector.h: Provides an interface for enabling/disabling the leak
detector and getting notified when a leak report is generated. Note that this
does not contain a class, only functions.
- leak_detector.cc: Registers alloc/free hook functions with the allocator.
Initializes LeakDetectorImpl. Performs sampling of incoming allocs/frees and
passes the sampled data to LeakDetectorImpl. Notifies observers when leak
reports are generated.
- leak_detector_unittest.cc: Contains a test for triggering a leak report
through the whole leak detector stack. Unfortunately, this cannot be run if
frame pointers are not enabled. AFAIK, the CQ trybots don't have frame pointer
support, but this test can still be run locally.

[Will Harris, 2016-02-03 05:02:33]

On 2016/02/03 02:54:21, Simon Que wrote:
> Here's an outline of what this patch contains:
> - base/allocator/allocator_extension.*: Added functions for registering
> alloc/free hooks, and for unwinding the call stack.
> - leak_detector.h: Provides an interface for enabling/disabling the leak
> detector and getting notified when a leak report is generated. Note that this
> does not contain a class, only functions.
> - leak_detector.cc: Registers alloc/free hook functions with the allocator.
> Initializes LeakDetectorImpl. Performs sampling of incoming allocs/frees and
> passes the sampled data to LeakDetectorImpl. Notifies observers when leak
> reports are generated.
> - leak_detector_unittest.cc: Contains a test for triggering a leak report
> through the whole leak detector stack. Unfortunately, this cannot be run if
> frame pointers are not enabled. AFAIK, the CQ trybots don't have frame pointer
> support, but this test can still be run locally.

I will look at this more in detail this week, but two initial questions to mull
over tonight:

1. why use a pthread_spinlock and not the existing lock primitives in base e.g.
base::Lock

2. In leak_detector.h - I think I'd prefer the interface be exposed an an object
with a constructor/destructor rather than Initialize/Shutdown so it can be
placed into a scoped_ptr<> by the caller rather than just having plan functions
exposed at the namespace. This matches other similar interfaces.

[Simon Que, 2016-02-03 17:51:12]

On 2016/02/03 05:02:33, Will Harris wrote:
> On 2016/02/03 02:54:21, Simon Que wrote:
> > Here's an outline of what this patch contains:
> > - base/allocator/allocator_extension.*: Added functions for registering
> > alloc/free hooks, and for unwinding the call stack.
> > - leak_detector.h: Provides an interface for enabling/disabling the leak
> > detector and getting notified when a leak report is generated. Note that
this
> > does not contain a class, only functions.
> > - leak_detector.cc: Registers alloc/free hook functions with the allocator.
> > Initializes LeakDetectorImpl. Performs sampling of incoming allocs/frees and
> > passes the sampled data to LeakDetectorImpl. Notifies observers when leak
> > reports are generated.
> > - leak_detector_unittest.cc: Contains a test for triggering a leak report
> > through the whole leak detector stack. Unfortunately, this cannot be run if
> > frame pointers are not enabled. AFAIK, the CQ trybots don't have frame
pointer
> > support, but this test can still be run locally.
> 
> I will look at this more in detail this week, but two initial questions to
mull
> over tonight:
> 
> 1. why use a pthread_spinlock and not the existing lock primitives in base
e.g.
> base::Lock

I originally wrote it for tcmalloc, which could not use src/base. I can change
it to use base::Lock or something similar.

> 
> 2. In leak_detector.h - I think I'd prefer the interface be exposed an an
object
> with a constructor/destructor rather than Initialize/Shutdown so it can be
> placed into a scoped_ptr<> by the caller rather than just having plan
functions
> exposed at the namespace. This matches other similar interfaces.

I can take a look at some other places where this is done. Got any examples?

[Will Harris, 2016-02-03 18:43:57]

On 2016/02/03 17:51:12, Simon Que wrote:
> On 2016/02/03 05:02:33, Will Harris wrote:
> > On 2016/02/03 02:54:21, Simon Que wrote:
> > > Here's an outline of what this patch contains:
> > > - base/allocator/allocator_extension.*: Added functions for registering
> > > alloc/free hooks, and for unwinding the call stack.
> > > - leak_detector.h: Provides an interface for enabling/disabling the leak
> > > detector and getting notified when a leak report is generated. Note that
> this
> > > does not contain a class, only functions.
> > > - leak_detector.cc: Registers alloc/free hook functions with the
allocator.
> > > Initializes LeakDetectorImpl. Performs sampling of incoming allocs/frees
and
> > > passes the sampled data to LeakDetectorImpl. Notifies observers when leak
> > > reports are generated.
> > > - leak_detector_unittest.cc: Contains a test for triggering a leak report
> > > through the whole leak detector stack. Unfortunately, this cannot be run
if
> > > frame pointers are not enabled. AFAIK, the CQ trybots don't have frame
> pointer
> > > support, but this test can still be run locally.
> > 
> > I will look at this more in detail this week, but two initial questions to
> mull
> > over tonight:
> > 
> > 1. why use a pthread_spinlock and not the existing lock primitives in base
> e.g.
> > base::Lock
> 
> I originally wrote it for tcmalloc, which could not use src/base. I can change
> it to use base::Lock or something similar.
thanks.

> 
> > 
> > 2. In leak_detector.h - I think I'd prefer the interface be exposed an an
> object
> > with a constructor/destructor rather than Initialize/Shutdown so it can be
> > placed into a scoped_ptr<> by the caller rather than just having plan
> functions
> > exposed at the namespace. This matches other similar interfaces.
> 
> I can take a look at some other places where this is done. Got any examples?

look at BrowserMainLoop::PostMainMessageLoopStart where a lot of globals end up
being created and put in scoped_ptrs.

[Simon Que, 2016-02-04 00:52:21]

Description was changed from

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

See leak_detector.cc.

BUG==chromium:382705
TEST=build successfully, run components_unittests
==========

to

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG==chromium:382705
TEST=build successfully, run components_unittests
==========

[Will Harris, 2016-02-04 03:41:52]

more top level questions before getting to the nittygritty

https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:74: std::list<LeakDetector*>*
g_leak_detector_object_list;
Are you expecting multiple customers of leak detector i.e. multiple areas
instantiating them? which code? How do you expect this to be used?

if so, I presume you want one actual instance, typically this is done by a
singleton pattern.

you can expose the singleton via g_browser_process

maybe if you explain more how you expect this to be used, or point me at a
design, then can come up with the right pattern to use so you don't have to keep
changing it.

[Simon Que, 2016-02-04 03:47:20]

On 2016/02/04 03:41:52, Will Harris wrote:
> more top level questions before getting to the nittygritty
> 
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> File components/metrics/leak_detector/leak_detector.cc (right):
> 
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> components/metrics/leak_detector/leak_detector.cc:74:
std::list<LeakDetector*>*
> g_leak_detector_object_list;
> Are you expecting multiple customers of leak detector i.e. multiple areas
> instantiating them? which code? How do you expect this to be used?
> 
> if so, I presume you want one actual instance, typically this is done by a
> singleton pattern.
> 
> you can expose the singleton via g_browser_process
> 
> maybe if you explain more how you expect this to be used, or point me at a
> design, then can come up with the right pattern to use so you don't have to
keep
> changing it.

There should be only one instance of the leak detector, because there is only
one instance of the allocator. The initialization should be done in only one
place.

[Will Harris, 2016-02-04 04:03:23]

On 2016/02/04 03:47:20, Simon Que wrote:
> On 2016/02/04 03:41:52, Will Harris wrote:
> > more top level questions before getting to the nittygritty
> > 
> >
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> > File components/metrics/leak_detector/leak_detector.cc (right):
> > 
> >
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> > components/metrics/leak_detector/leak_detector.cc:74:
> std::list<LeakDetector*>*
> > g_leak_detector_object_list;
> > Are you expecting multiple customers of leak detector i.e. multiple areas
> > instantiating them? which code? How do you expect this to be used?
> > 
> > if so, I presume you want one actual instance, typically this is done by a
> > singleton pattern.
> > 
> > you can expose the singleton via g_browser_process
> > 
> > maybe if you explain more how you expect this to be used, or point me at a
> > design, then can come up with the right pattern to use so you don't have to
> keep
> > changing it.
> 
> There should be only one instance of the leak detector, because there is only
> one instance of the allocator. The initialization should be done in only one
> place.

I don't see the purpose of g_leak_detector_object_list and all that management
logic then, can't this just be a singleton, or more simply always just accessed
via the plain scoped object from e.g. g_browser_process, with perhaps a CHECK
somewhere to make sure someone doesn't ever try to mistakenly construct it twice
(as this should never happen because as you say you only want to initialize the
hooks once).

[Simon Que, 2016-02-04 05:00:27]

On 2016/02/04 04:03:23, Will Harris wrote:
> On 2016/02/04 03:47:20, Simon Que wrote:
> > On 2016/02/04 03:41:52, Will Harris wrote:
> > > more top level questions before getting to the nittygritty
> > > 
> > >
> >
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> > > File components/metrics/leak_detector/leak_detector.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1665553002/diff/40001/components/metrics/leak...
> > > components/metrics/leak_detector/leak_detector.cc:74:
> > std::list<LeakDetector*>*
> > > g_leak_detector_object_list;
> > > Are you expecting multiple customers of leak detector i.e. multiple areas
> > > instantiating them? which code? How do you expect this to be used?
> > > 
> > > if so, I presume you want one actual instance, typically this is done by a
> > > singleton pattern.
> > > 
> > > you can expose the singleton via g_browser_process
> > > 
> > > maybe if you explain more how you expect this to be used, or point me at a
> > > design, then can come up with the right pattern to use so you don't have
to
> > keep
> > > changing it.
> > 
> > There should be only one instance of the leak detector, because there is
only
> > one instance of the allocator. The initialization should be done in only one
> > place.
> 
> I don't see the purpose of g_leak_detector_object_list and all that management
> logic then, can't this just be a singleton, or more simply always just
accessed
> via the plain scoped object from e.g. g_browser_process, with perhaps a CHECK
> somewhere to make sure someone doesn't ever try to mistakenly construct it
twice
> (as this should never happen because as you say you only want to initialize
the
> hooks once).

OK, I'll do that.

[Simon Que, 2016-02-04 19:06:30]

Is this class a good example of a singleton class? LoginState:

https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/log...

[Will Harris, 2016-02-04 19:22:12]

On 2016/02/04 19:06:30, Simon Que wrote:
> Is this class a good example of a singleton class? LoginState:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/log...

you have a number of options, you could not use a singleton at all and just
manage the lifetime manually in e.g. BrowserProcess.

When will you want to be constructed, how early does this have to happen?

When will you want the destructor to be called, do you want to stay
loaded/present throughout browser lifetime or are you happy to be destructed
during shutdown?

You coudl use a leaky lazy instance if you never want your destructor to be
called. Perhaps sketching out in a doc or here exactly how you expect to be
called where you expect to be constructed and destructed would help.

Look at examples for base::LazyInstance<>::Leaky perhaps? Look for examples of
other things initialized in chrome_browser_main.cc and see which most suits what
you want to do (if that's where you want to initialize - if you want to
initialize earlier then a lazy instance created elsewhere might be better)

TL;DR; I need to know more about how you expect this to be plumbed into Chrome
startup/shutdown to really give you concrete guidance here...

[Simon Que, 2016-02-04 19:32:33]

On 2016/02/04 19:22:12, Will Harris wrote:
> On 2016/02/04 19:06:30, Simon Que wrote:
> > Is this class a good example of a singleton class? LoginState:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/log...
> 
> you have a number of options, you could not use a singleton at all and just
> manage the lifetime manually in e.g. BrowserProcess.
>
> When will you want to be constructed, how early does this have to happen?

Instantiation doesn't need to happen very early on.
 
I'm thinking it could be constructed in ChromeOSMetricsProvider, since that is
where a class that serves a similar purpose (PerfProvider) is constructed.

> When will you want the destructor to be called, do you want to stay
> loaded/present throughout browser lifetime or are you happy to be destructed
> during shutdown?

It should ideally be destructed during shutdown. I haven't looked at the UMA
shutdown mechanics but there could be some mechanism to send all the
collected-but-not-submitted data at shutdown. If that's the case, then I'd want
the leak detector's data to be sent at shutdown as well.

> You coudl use a leaky lazy instance if you never want your destructor to be
> called. Perhaps sketching out in a doc or here exactly how you expect to be
> called where you expect to be constructed and destructed would help.
>
> Look at examples for base::LazyInstance<>::Leaky perhaps? Look for examples of
> other things initialized in chrome_browser_main.cc and see which most suits
what
> you want to do (if that's where you want to initialize - if you want to
> initialize earlier then a lazy instance created elsewhere might be better)
> 
> TL;DR; I need to know more about how you expect this to be plumbed into Chrome
> startup/shutdown to really give you concrete guidance here...

[Will Harris, 2016-02-05 04:38:17]

looks like PerfProvider is a simple (non singleton or lazyinstance) class that's
just a member variable of ChromeOSMetricsProvider

I don't know much about the lifetime of that class, I recommend you consult with
the metrics team about it.

Once you've got l-g-t-m from metrics on how to structure the LeakDetector class
into ChromeOSMetricsProvider I can review the interface with the allocator,
which I think is pretty much indepdendent of how the lifetime of the detector is
managed. I think it's safe to assume only one of these will ever be constructed,
so no need for any complex tracking logic in the class (e.g.
g_leak_detector_object_list) - keep it simple.

[Simon Que, 2016-02-05 05:49:02]

On 2016/02/05 04:38:17, Will Harris wrote:
> looks like PerfProvider is a simple (non singleton or lazyinstance) class
that's
> just a member variable of ChromeOSMetricsProvider
> 
> I don't know much about the lifetime of that class, I recommend you consult
with
> the metrics team about it.
> 
> Once you've got l-g-t-m from metrics on how to structure the LeakDetector
class
> into ChromeOSMetricsProvider I can review the interface with the allocator,
> which I think is pretty much indepdendent of how the lifetime of the detector
is
> managed. I think it's safe to assume only one of these will ever be
constructed,
> so no need for any complex tracking logic in the class (e.g.
> g_leak_detector_object_list) - keep it simple.

Actually, that's not correct. The next step will be to create a
LeakDetectorMonitor class, which extends LeakDetector::Observer. The plan is to
put LeakDetectorMonitor in ChromeOSMetricsProvider, and during initialization of
Chrome, it will construct the single instance of LeakDetector.

See the class/module instantiation hierarchy here:
https://docs.google.com/drawings/d/1HbGWDzeCuaqncumW6i3JfaueO4RIwQBU3I1K_k-Ok...

However, your concern regarding ChromeOSMetricsProvider is still valid. I'll
send them an email.

[Simon Que, 2016-02-08 18:38:47]

PTAL.

[Will Harris, 2016-02-08 19:37:58]

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

[Will Harris, 2016-02-08 19:42:26]

I've added asvitkine to the review as an owner of base/metrics he will be able
to advise on the interface with the rest of metrics

I am still concerned by your use of globals here, they can almost all just be
members of the LeakDetector class. If you are concerned about LeakDetector being
instantiated mulitple times, then add a CHECK or a DCHECK in the constructor.

As said before, I'm happy to review the interface with base/allocator once the
code in components/metrics is good.

[Simon Que, 2016-02-09 22:31:07]

On 2016/02/08 19:42:26, Will Harris (ooo until 25 Feb) wrote:
> I've added asvitkine to the review as an owner of base/metrics he will be able
> to advise on the interface with the rest of metrics
> 
> I am still concerned by your use of globals here, they can almost all just be
> members of the LeakDetector class. If you are concerned about LeakDetector
being
> instantiated mulitple times, then add a CHECK or a DCHECK in the constructor.
> 
> As said before, I'm happy to review the interface with base/allocator once the
> code in components/metrics is good.

I'm not convinced that it would be a good idea to make many of the static stuff
into members of the class. The hook functions and IterateLoadedObjects, for
example, are required to be standalone functions. I could make them static
members but they'd be private so there's no point. And for the sake of
performance, I'd prefer not to add an extra access from the hook function into
the LeakDetector object's member, unless it was something that is within the
sampled block, in which case the performance hit would be less.

Alexei, Will is OOO for a week. To continue without being blocked by Will's
absence, I'm thinking that I will create a separate CL consisting of a
stubbed-out LeakDetector class that only exports the observer interface and
registration. Then, I can create the observer class that is instantiated from
ChromeOSMetricsProvider. When Will returns, I can continue with the contents of
this CL and complete the pipeline. What do you think?

[Alexei Svitkine (slow), 2016-02-09 22:34:23]

That sgtm.
On 9 Feb 2016 5:31 p.m., <sque@chromium.org> wrote:

>
> On 2016/02/08 19:42:26, Will Harris (ooo until 25 Feb) wrote:
> > I've added asvitkine to the review as an owner of base/metrics he will be
able
> > to advise on the interface with the rest of metrics
> >
> > I am still concerned by your use of globals here, they can almost all just
be
> > members of the LeakDetector class. If you are concerned about LeakDetector
> being
> > instantiated mulitple times, then add a CHECK or a DCHECK in the
constructor.
> >
> > As said before, I'm happy to review the interface with base/allocator once
the
> > code in components/metrics is good.
>
> I'm not convinced that it would be a good idea to make many of the static
stuff
> into members of the class. The hook functions and IterateLoadedObjects, for
> example, are required to be standalone functions. I could make them static
> members but they'd be private so there's no point. And for the sake of
> performance, I'd prefer not to add an extra access from the hook function
into
> the LeakDetector object's member, unless it was something that is within the
> sampled block, in which case the performance hit would be less.
>
> Alexei, Will is OOO for a week. To continue without being blocked by Will's
> absence, I'm thinking that I will create a separate CL consisting of a
> stubbed-out LeakDetector class that only exports the observer interface and
> registration. Then, I can create the observer class that is instantiated from
> ChromeOSMetricsProvider. When Will returns, I can continue with the contents
of
> this CL and complete the pipeline. What do you think?
>
> https://codereview.chromium.org/1665553002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2016-02-09 22:45:54]

I've marked all the globals that I think can be moved to LeakDetector class or
removed altogether.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:35: } chrome_mapping;
Member of LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:81: LeakDetectorImpl*
g_leak_detector_impl;
Member of LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:88: uint64_t
g_last_analysis_alloc_size;
Member of LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:102: int g_max_stack_depth;
Member of LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:106: uint64_t
g_analysis_interval_bytes;
Member of LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:111: int
g_size_suspicion_threshold;
Delete

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:112: int
g_call_stack_suspicion_threshold;
Delete

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:275: bool Initialize() {
Move to LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:328: bool Initialize(int
sampling_factor,
Move to LeakDetector.

https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:350: bool Shutdown() {
Move to LeakDetector.

[Simon Que, 2016-02-10 02:14:29]

Description was changed from

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG==chromium:382705
TEST=build successfully, run components_unittests
==========

to

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests
==========

[Simon Que, 2016-02-26 02:09:24]

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

[Simon Que, 2016-02-26 02:09:25]

Will, I've rebased this on top of the other CL. While we're waiting for that one
to get committed, let's proceed with this one.

I've moved a lot of the variables and functions into the LeakDetector class.
I've also removed some unit tests. PTAL.

[Simon Que, 2016-02-27 00:45:35]

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

[Simon Que, 2016-02-27 00:45:36]

PTAL, the other patch has landed.

[Simon Que, 2016-02-29 21:04:39]

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

[Primiano Tucci (use gerrit), 2016-03-01 18:54:37]

Gave a first pass. 
My main comment here is:
 - the API surface in base can be simplified.
 - The current scoped disabler seems a bit fragile. It sounds it can be make way
simpler and more robust, see comments inline.
 - That phdr iterate is a bit scary, as relies on linker internals. As long as
CrOS folks are fine with that it's OK, just let's make it explicitly #ifdef CrOS
only, because that is not guaranteed to work on other platforms.

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
base/allocator/allocator_extension.cc:38: AllocHookFunc
SetSingleAllocHook(AllocHookFunc func) {
What I would do here is:
1) Have just a Set function with two parameters.
2) Have a  bool g_alloc_hooks_installed
3) When you reach here DCHECK( (!alloc_hook && !free_hook) ||
!g_alloc_hooks_installed)) 
which means: either clear the hooks or check that you are not doubly-setting
them

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
base/allocator/allocator_extension.cc:81: // frames to skip is determined
internally.
so why not omitting the skip_count argument at all?

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
File base/allocator/allocator_extension.h (right):

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
base/allocator/allocator_extension.h:19: using AllocHookFunc =
std::add_pointer<void(const void*, size_t)>::type;
What is the benefit of std::add_pointer as opposite to just a function pointer
(see below). I don't see any precedence in the codebase nor it seems whitelisted
in https://chromium-cpp.appspot.com/.

e.g. using AllocHookFunc = void (*)(const void*, size_t);

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
base/allocator/allocator_extension.h:37: BASE_EXPORT AllocHookFunc
SetSingleAllocHook(AllocHookFunc func);
I'd drop Single from the name. The fact that you have just one should be clear
from the "Set" (as opposite to Add) in the name.
Single is a bit misleading, for a moment I thought you were using this to hook
only one allocation and then self-uninstall.

Also I think is saner to make the CHECK you have in components here in base, and
make this return void to keep it simple, unless you really need the old hook for
reasons other than just checking. More details in the comment in the .cc file

Also, very likely you want to set both these hook, not just one of them, so why
not just making this a single function which takes both argument, ie.

SetAllocatorHooks(AllocHookFunc, FreeHookFunct) ?

https://codereview.chromium.org/1665553002/diff/100001/base/allocator/allocat...
base/allocator/allocator_extension.h:52: //   skip_count:     Indicates how many
initial call stack levels to skip before
It looks like this is really not used in the .cc file. What's the point of
adding a dead parameter?

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:47: class MallocHookDisabler {
Hmm the design of this seems very prone to races. What happens if a thread is
doing an allocation while you uninstall the hooks here?

From the comment sounds like what you really want here is a scoped thread-local
disabler.
What I would do here is:
 - never uninstall-reinstall the hooks. Keep them always installed.
 - Make this class just increment and decrement a thread local integer variable
(you can use base::ThreadLocalPointer as a counter).
 - When you enter the hooks, check if that variable is >0, if so return (i.e.
make the hook a noop)

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:86: int
IterateLoadedObjects(struct dl_phdr_info* shared_object,
I cannot speak for CrOs, but on other platforms this is very error-prone.
Can I ask that, at very least, we make this #ifdef CrOS only? On android this
for sure will fail, so would be great to not end up using this by accident.

My suggestion here would be: do you really need to know the relative offset in
the client chrome? or probably you really need them only server-side. If this is
the case, what I would do here is merely return the offset from a well known
symbol (pick just any of the random function, or define a new one for this
purpose).
Then on server-side you reconstruct the real address by doing the reverse math
based on the knowledge of the virtual address of the known symbol + the offset
from it.
This will not require any client-side black magic.

However, I understandt his might change your design. Personally, as long as this
stays a CrOS only thingy.  Don't know enough of CrOS to say whether this is safe
or not, so, as long as somebody from CrOs is okay with that and we ifdef this
I'm fine.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:115: inline uint64_t
PointerToHash(const void* ptr) {
just doublecheckin, do you know that we have a SuperFastHash in base/ ?

[Simon Que, 2016-03-01 19:09:53]

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:47: class MallocHookDisabler {
On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> Hmm the design of this seems very prone to races. What happens if a thread is
> doing an allocation while you uninstall the hooks here?
> 
> From the comment sounds like what you really want here is a scoped
thread-local
> disabler.
> What I would do here is:
>  - never uninstall-reinstall the hooks. Keep them always installed.
>  - Make this class just increment and decrement a thread local integer
variable
> (you can use base::ThreadLocalPointer as a counter).
>  - When you enter the hooks, check if that variable is >0, if so return (i.e.
> make the hook a noop)

I am aware that uninstalling and reinstalling the hooks incurs a non-negligible
overhead, as I've seen from profiling my code locally. However...
1. InternalAlloc and InternalFree are meant to be called from CustomAllocator,
which is always to be called under lock.
2. I plan to implement a local allocator in the future for small sizes (e.g. <
128 bytes), which make up over 99.99% of the allocations in the leak detector.
When that is implemented, InternalAlloc will only be called in two cases: (a)
For larger sizes (e.g. >= 128 bytes); (b) When the local allocator needs more
memory to fulfill allocs. I've actually already written the code and the
uninstall-reinstall of hooks no longer shows up in perf profiles.

Your suggestion might work but it would also incur that extra overhead in the
hooks, esp if it's using something more exotic like pthread_getspecific(), which
is apparently the internal implementation of base::ThreadLocalPointer.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:86: int
IterateLoadedObjects(struct dl_phdr_info* shared_object,
On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> I cannot speak for CrOs, but on other platforms this is very error-prone.
> Can I ask that, at very least, we make this #ifdef CrOS only? On android this
> for sure will fail, so would be great to not end up using this by accident.
> 
> My suggestion here would be: do you really need to know the relative offset in
> the client chrome? or probably you really need them only server-side. If this
is
> the case, what I would do here is merely return the offset from a well known
> symbol (pick just any of the random function, or define a new one for this
> purpose).
> Then on server-side you reconstruct the real address by doing the reverse math
> based on the knowledge of the virtual address of the known symbol + the offset
> from it.
> This will not require any client-side black magic.
> 
> However, I understandt his might change your design. Personally, as long as
this
> stays a CrOS only thingy.  Don't know enough of CrOS to say whether this is
safe
> or not, so, as long as somebody from CrOs is okay with that and we ifdef this
> I'm fine.

I'll get someone from CrOS to take a look. Meanwhile, I'll also look into
specific symbols in the Chrome binary.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:115: inline uint64_t
PointerToHash(const void* ptr) {
On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> just doublecheckin, do you know that we have a SuperFastHash in base/ ?

Yes. I plan to further simplify this function by returning the result of the
multiplication without shifting. Instead, the sampling factor (which gets
compared against this value) would be shifted the other way during
initialization, which saves the extra shift each time this function is called.

[Simon Que, 2016-03-01 19:12:40]

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

[Simon Que, 2016-03-01 19:12:41]

+llozano

Luis, is it safe to call dl_iterate_phdr() to get the Chrome binary mapping info
on CrOS? Primiano says it would not work on some systems, such as Android, and
we want to make sure it would not break on CrOS.

[llozano, 2016-03-01 19:43:13]

On 2016/03/01 19:12:41, Simon Que wrote:
> +llozano
> 
> Luis, is it safe to call dl_iterate_phdr() to get the Chrome binary mapping
info
> on CrOS? Primiano says it would not work on some systems, such as Android, and
> we want to make sure it would not break on CrOS.

we have used dl_iterate_phdr in ChromeOS before without any issues.

[Primiano Tucci (use gerrit), 2016-03-01 21:10:28]

Ok, so from the base/allocator viewpoint I think this is ok if we make the API
simpler (see comments inline).
Not sure if I should review also components/ part. Happy to do that, but let me
know if i am supposed to or if I am going to interfere with somebody else
review.

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:40: return MallocHook::SetNewHook(func);
Just looked at malloc_hook.h, it looks SetNewHook is marked as deprecated.
Looks like what you want here is AddNewHook

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:47: return
MallocHook::SetDeleteHook(func);
And RemoveNewHook here

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:62: AllocHookFunc existing_hook =
MallocHook::SetNewHook(nullptr);
This is racy by design, how do you prevent that something else doesn't
AddNewHook in the meantime?
This cannot be a base API as is.
Let's keep the base api simple, and have just a void
SetAllocatorHooks(malloc_func_ptr, free_func_ptr) method. Inside the method do a
DCHECK with a bool as in my previous comment to check that you are not
overriding a previously hooks.
There doesn't seem to be a sane way of implementing a Get here, so let's not
have a get.
Doesn't really seem you need it for anything else other than CHECK.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:115: inline uint64_t
PointerToHash(const void* ptr) {
On 2016/03/01 19:09:53, Simon Que wrote:
> On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> > just doublecheckin, do you know that we have a SuperFastHash in base/ ?
> 
> Yes. I plan to further simplify this function by returning the result of the
> multiplication without shifting. Instead, the sampling factor (which gets
> compared against this value) would be shifted the other way during
> initialization, which saves the extra shift each time this function is called.

ok sounds like you have a plan here.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:201: CHECK(g_instance);
How are you preventing to hit this check when an allocation happens while
shutting down the ~LeakDetector.
In other words, how do you prevent that:

Thread A (who owns the Leak Detector) writes:
  g_instance = nullptr;
  SetHook (nullptr)

and Thread B:
 invokes AllocHook between the two instructions above?

note that even if you swap the order of execution of thread A that is still not
going to be thread safe, unless you use locks there and here in the check.

Probably here you want just:
if (!g_instance)
  return

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:218: //
|LeakDetectorImpl::ShouldGetStackTraceForSize()| is const; there is no
what happens if you are here while you destroy the impl on another thread?

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:229:
g_instance->RecordAlloc(ptr, size, depth, stack.data());
in the header of this class you say that this class is not thread safe, but here
you call RecordAlloc without any lock

[Simon Que, 2016-03-02 06:25:09]

Re: thread safety in the hook functions: you have a valid point. I could just
put the entire contents of the hook functions under a global lock. The same lock
will be used to destroy the LeakDetector object and set |g_instance| to null,
which is why the lock needs to be global.

That would produce poor performance, but it would guarantee thread safety. I'm
willing to do that for the sake of getting this checked in, and then optimizing
for performance later. How does that sound?

That said, here are some ideas for future iterations:
- I think a rwlock is what's needed to prevent race conditions on accessing the
pointer |g_instance| itself. Otherwise there would be too much contention
between a bunch of threads that will only read |g_instance|. The destructor will
be called only once so there will not be any necessary contention on this lock
until the end.
- Another thing I'm considering is to have different locks for different parts
of the LeakDetector struct. e.g. one for |impl_| and one for
|total_alloc_size_|.
- I'll also consider using thread-specific vars in some places.

If you have any other design pattern ideas, I'd like to hear them.

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:40: return MallocHook::SetNewHook(func);
On 2016/03/01 21:10:28, Primiano (throttled til Mar 4) wrote:
> Just looked at malloc_hook.h, it looks SetNewHook is marked as deprecated.
> Looks like what you want here is AddNewHook

I specifically went with the deprecated version because of performance. There is
a substantial overhead to using the new hook system because the hooks are copied
into a local array before being invoked, to allow for hook functions to
add/remove hooks.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

The deprecated system has a single hook which is invoked directly, and incurs no
extra overhead.

Running locally, I see a total of 1.5% overhead from Invoke{New|Delete}HookSlow.

See also:
https://docs.google.com/presentation/d/1DTc7d1e7CIFSIpAhfm7f41PepkwSReInn-3se...

That said, I could still simplify this into SetHooks(newhook, deletehook) that
sets the deprecated hook.

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:62: AllocHookFunc existing_hook =
MallocHook::SetNewHook(nullptr);
On 2016/03/01 21:10:27, Primiano (throttled til Mar 4) wrote:
> This is racy by design, how do you prevent that something else doesn't
> AddNewHook in the meantime?
> This cannot be a base API as is.
> Let's keep the base api simple, and have just a void
> SetAllocatorHooks(malloc_func_ptr, free_func_ptr) method. Inside the method do
a
> DCHECK with a bool as in my previous comment to check that you are not
> overriding a previously hooks.
> There doesn't seem to be a sane way of implementing a Get here, so let's not
> have a get.
> Doesn't really seem you need it for anything else other than CHECK.

Doesn't quite work that way, since the non-deprecated hook system uses a list of
hooks. I can add a Remove(newhook,deletehook) function though.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:86: int
IterateLoadedObjects(struct dl_phdr_info* shared_object,
On 2016/03/01 19:09:53, Simon Que wrote:
> On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> > I cannot speak for CrOs, but on other platforms this is very error-prone.
> > Can I ask that, at very least, we make this #ifdef CrOS only? On android
this
> > for sure will fail, so would be great to not end up using this by accident.
> > 
> > My suggestion here would be: do you really need to know the relative offset
in
> > the client chrome? or probably you really need them only server-side. If
this
> is
> > the case, what I would do here is merely return the offset from a well known
> > symbol (pick just any of the random function, or define a new one for this
> > purpose).
> > Then on server-side you reconstruct the real address by doing the reverse
math
> > based on the knowledge of the virtual address of the known symbol + the
offset
> > from it.
> > This will not require any client-side black magic.
> > 
> > However, I understandt his might change your design. Personally, as long as
> this
> > stays a CrOS only thingy.  Don't know enough of CrOS to say whether this is
> safe
> > or not, so, as long as somebody from CrOs is okay with that and we ifdef
this
> > I'm fine.
> 
> I'll get someone from CrOS to take a look. Meanwhile, I'll also look into
> specific symbols in the Chrome binary.

Done.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:115: inline uint64_t
PointerToHash(const void* ptr) {
On 2016/03/01 21:10:28, Primiano (throttled til Mar 4) wrote:
> On 2016/03/01 19:09:53, Simon Que wrote:
> > On 2016/03/01 18:54:37, Primiano (throttled til Mar 4) wrote:
> > > just doublecheckin, do you know that we have a SuperFastHash in base/ ?
> > 
> > Yes. I plan to further simplify this function by returning the result of the
> > multiplication without shifting. Instead, the sampling factor (which gets
> > compared against this value) would be shifted the other way during
> > initialization, which saves the extra shift each time this function is
called.
> 
> ok sounds like you have a plan here. 

Done.

https://codereview.chromium.org/1665553002/diff/120001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:201: CHECK(g_instance);
On 2016/03/01 21:10:28, Primiano (throttled til Mar 4) wrote:
> How are you preventing to hit this check when an allocation happens while
> shutting down the ~LeakDetector.
> In other words, how do you prevent that:
> 
> Thread A (who owns the Leak Detector) writes:
>   g_instance = nullptr;
>   SetHook (nullptr)
> 
> and Thread B:
>  invokes AllocHook between the two instructions above?
> 
> note that even if you swap the order of execution of thread A that is still
not
> going to be thread safe, unless you use locks there and here in the check.
> 
> Probably here you want just:
> if (!g_instance)
>   return
Done

[Primiano Tucci (use gerrit), 2016-03-02 06:39:54]

> Re: thread safety in the hook functions: you have a valid point. 
Just to doublecheck, if we go for the deprecated hooks path, can we get rid of
the getters in base/allocator right?

>put the entire contents of the hook functions under a global lock. The same
lock
>will be used to destroy the LeakDetector object and set |g_instance| to null,
>which is why the lock needs to be global.
If you have Set(null) and Set(hook) you don't need a lock anymore, right?

>>- I think a rwlock is what's needed to prevent race conditions on accessing
the
>pointer |g_instance| itself.
IIRC we don't have rwlocks in base, right? If that is the case we don't want to
invent them here.

> Otherwise there would be too much contention
> between a bunch of threads that will only read |g_instance|. The destructor
will
> be called only once so there will not be any necessary contention on this lock
> until the end.
Here's an idea: do you really need to destroy the LeakDetector. By looking at
the existing code chromeos_metrics_provider.cc looks like you never destroy it.
What if you make deilberately the leakdetector a LeakySingletonTraits, so you
know that it can't be possibly destroyed, and don't have to worry about races on
dtor?

>- Another thing I'm considering is to have different locks for different parts
>of the LeakDetector struct. e.g. one for |impl_| and one for
>|total_alloc_size_|.
My advise it to start with something simple and solid, and do these fine grained
smart tuning later. By personal experience every time I tried to do smart things
on locks I ended up forgetting something and causing bugs that did backfire in
~weeks.

> If you have any other design pattern ideas, I'd like to hear them.
Yup, leak the leakdetector (the name already advises that). you don't have to
care about races on destructor if you never destroy it :)

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:40: return MallocHook::SetNewHook(func);
On 2016/03/02 06:25:09, Simon Que wrote:
> On 2016/03/01 21:10:28, Primiano (throttled til Mar 4) wrote:
> > Just looked at malloc_hook.h, it looks SetNewHook is marked as deprecated.
> > Looks like what you want here is AddNewHook
> 
> I specifically went with the deprecated version because of performance. There
is
> a substantial overhead to using the new hook system because the hooks are
copied
> into a local array before being invoked, to allow for hook functions to
> add/remove hooks.

Ok let's keep these deprecated ones. Worst case when they become deprectated for
real should not be a great deal refactoring the code in base to make it work.
Also, very likely the shim layer will happen before.

https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
base/allocator/allocator_extension.cc:62: AllocHookFunc existing_hook =
MallocHook::SetNewHook(nullptr);
On 2016/03/02 06:25:09, Simon Que wrote:
> On 2016/03/01 21:10:27, Primiano (throttled til Mar 4) wrote:
> > This is racy by design, how do you prevent that something else doesn't
> > AddNewHook in the meantime?
> > This cannot be a base API as is.
> > Let's keep the base api simple, and have just a void
> > SetAllocatorHooks(malloc_func_ptr, free_func_ptr) method. Inside the method
do
> a
> > DCHECK with a bool as in my previous comment to check that you are not
> > overriding a previously hooks.
> > There doesn't seem to be a sane way of implementing a Get here, so let's not
> > have a get.
> > Doesn't really seem you need it for anything else other than CHECK.
> 
> Doesn't quite work that way, since the non-deprecated hook system uses a list
of
> hooks. I can add a Remove(newhook,deletehook) function though.

Is this problem solved (i.e. can we get rid of these getters) if we use the
deprectaed setters?

[Simon Que, 2016-03-02 08:52:40]

On 2016/03/02 06:39:54, Primiano (throttled til Mar 4) wrote:
> > Re: thread safety in the hook functions: you have a valid point. 
> Just to doublecheck, if we go for the deprecated hooks path, can we get rid of
> the getters in base/allocator right?
> 
> >put the entire contents of the hook functions under a global lock. The same
> lock
> >will be used to destroy the LeakDetector object and set |g_instance| to null,
> >which is why the lock needs to be global.
> If you have Set(null) and Set(hook) you don't need a lock anymore, right?

Not going to work. The Set() incurs major overhead.

> >>- I think a rwlock is what's needed to prevent race conditions on accessing
> the
> >pointer |g_instance| itself.
> IIRC we don't have rwlocks in base, right? If that is the case we don't want
to
> invent them here.

No, but there's always the option to let base::Lock be configurable between
pthread_mutex (current implementation), pthread_spinlock, and pthread_rwlock.

If we wanted to keep the solution local, I don't think it's that hard to
implement since it's a one-shot usage. Have a shared counter variable that
starts from 0. Each time the hook func is called, increment, and then decrement
when the hook func returns. When the destructor unregisters the hooks (while
under lock), it will then wait for the counter to drop to 0, if it's not already
there.


> >  there would be too much contention
> > between a bunch of threads that will only read |g_instance|. The destructor
> will
> > be called only once so there will not be any necessary contention on this
lock
> > until the end.
> Here's an idea: do you really need to destroy the LeakDetector. By looking at
> the existing code chromeos_metrics_provider.cc looks like you never destroy
it.
> What if you make deilberately the leakdetector a LeakySingletonTraits, so you
> know that it can't be possibly destroyed, and don't have to worry about races
on
> dtor?

Not sure how much controversy that would generate. I think it was one of the UMA
guys who said I should make LeakDetector a member of ChromeOSMetricsProvider (as
a scoped_ptr).

> 
> >- Another thing I'm considering is to have different locks for different
parts
> >of the LeakDetector struct. e.g. one for |impl_| and one for
> >|total_alloc_size_|.
> My advise it to start with something simple and solid, and do these fine
grained
> smart tuning later. By personal experience every time I tried to do smart
things
> on locks I ended up forgetting something and causing bugs that did backfire in
> ~weeks.
> 
> > If you have any other design pattern ideas, I'd like to hear them.
> Yup, leak the leakdetector (the name already advises that). you don't have to
> care about races on destructor if you never destroy it :)
> 
>
https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
> File base/allocator/allocator_extension.cc (right):
> 
>
https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
> base/allocator/allocator_extension.cc:40: return MallocHook::SetNewHook(func);
> On 2016/03/02 06:25:09, Simon Que wrote:
> > On 2016/03/01 21:10:28, Primiano (throttled til Mar 4) wrote:
> > > Just looked at malloc_hook.h, it looks SetNewHook is marked as deprecated.
> > > Looks like what you want here is AddNewHook
> > 
> > I specifically went with the deprecated version because of performance.
There
> is
> > a substantial overhead to using the new hook system because the hooks are
> copied
> > into a local array before being invoked, to allow for hook functions to
> > add/remove hooks.
> 
> Ok let's keep these deprecated ones. Worst case when they become deprectated
for
> real should not be a great deal refactoring the code in base to make it work.
> Also, very likely the shim layer will happen before.
> 
>
https://codereview.chromium.org/1665553002/diff/120001/base/allocator/allocat...
> base/allocator/allocator_extension.cc:62: AllocHookFunc existing_hook =
> MallocHook::SetNewHook(nullptr);
> On 2016/03/02 06:25:09, Simon Que wrote:
> > On 2016/03/01 21:10:27, Primiano (throttled til Mar 4) wrote:
> > > This is racy by design, how do you prevent that something else doesn't
> > > AddNewHook in the meantime?
> > > This cannot be a base API as is.
> > > Let's keep the base api simple, and have just a void
> > > SetAllocatorHooks(malloc_func_ptr, free_func_ptr) method. Inside the
method
> do
> > a
> > > DCHECK with a bool as in my previous comment to check that you are not
> > > overriding a previously hooks.
> > > There doesn't seem to be a sane way of implementing a Get here, so let's
not
> > > have a get.
> > > Doesn't really seem you need it for anything else other than CHECK.
> > 
> > Doesn't quite work that way, since the non-deprecated hook system uses a
list
> of
> > hooks. I can add a Remove(newhook,deletehook) function though.
> 
> Is this problem solved (i.e. can we get rid of these getters) if we use the
> deprectaed setters?

Yes. Setting the hook is atomic, and the return value is the previously set
value. These two operations (read, then write) together are not atomic, and
cannot be used to make sure that you're not overwriting the previous hook.

[Primiano Tucci (use gerrit), 2016-03-02 18:13:27]

Ok let's do this let'd discuss the major things first and leave the other things
for later.
Let's establish that calling the deprecated hooks like you are doing here is OK.
I can't really understand why you need the getters (as in GetSingleAllocHook)
here.
The only use that I see in this CL is just logging/CHECKS, and nothing that
couldn't be done with a bool in components.
I might be missing something, but can you please explain why do we need the
GetSingleAllocHook in base?
The reason why I ask is because I don't seem a sane way to expose this function
in base in a way which is not thread safe, so I'd like to not expose that at
all, especially if it's not really needed, as it seems to be the case here

[Simon Que, 2016-03-02 18:45:56]

We probably don't need it.

I'm not sure if there's a thread-safe way to check in SetAllocHook() if
there's an existing hook that will be overwritten, short of using a lock.
The scenario I'm thinking of:

0. No hook is set (nullptr).
1. Thread A reads value of hook, which is nullptr => OK.
2. Thread B reads value of hook, which is nullptr => OK.
3. Thread A sets hook to HookA().
4. Thread B sets hook to HookB(), overwriting HookA() and without Thread
A's knowledge.

But setting hooks is expected to be an infrequent operation so we could
have a static/local lock to make it thread safe.

On Wed, Mar 2, 2016 at 10:13 AM, <primiano@chromium.org> wrote:

> Ok let's do this let'd discuss the major things first and leave the other
> things
> for later.
> Let's establish that calling the deprecated hooks like you are doing here
> is OK.
> I can't really understand why you need the getters (as in
> GetSingleAllocHook)
> here.
> The only use that I see in this CL is just logging/CHECKS, and nothing that
> couldn't be done with a bool in components.
> I might be missing something, but can you please explain why do we need the
> GetSingleAllocHook in base?
> The reason why I ask is because I don't seem a sane way to expose this
> function
> in base in a way which is not thread safe, so I'd like to not expose that
> at
> all, especially if it's not really needed, as it seems to be the case here
>
>
> https://codereview.chromium.org/1665553002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2016-03-02 19:06:36]

this is the thing I don't get. why do you need to read the value of the hooks at
all? what is the use case here that justifies your need of knowing the fptr of
the current hooks? by reading your code it seems that you really want to
maintain a on/off state. 


On 2016/03/02 18:45:56, Simon Que wrote:
> We probably don't need it.
> 
> I'm not sure if there's a thread-safe way to check in SetAllocHook() if
> there's an existing hook that will be overwritten, short of using a lock.
> The scenario I'm thinking of:
> 
> 0. No hook is set (nullptr).
> 1. Thread A reads value of hook, which is nullptr => OK.
> 2. Thread B reads value of hook, which is nullptr => OK.
> 3. Thread A sets hook to HookA().
> 4. Thread B sets hook to HookB(), overwriting HookA() and without Thread
> A's knowledge.
> 
> But setting hooks is expected to be an infrequent operation so we could
> have a static/local lock to make it thread safe.
> 
> On Wed, Mar 2, 2016 at 10:13 AM, <mailto:primiano@chromium.org> wrote:
> 
> > Ok let's do this let'd discuss the major things first and leave the other
> > things
> > for later.
> > Let's establish that calling the deprecated hooks like you are doing here
> > is OK.
> > I can't really understand why you need the getters (as in
> > GetSingleAllocHook)
> > here.
> > The only use that I see in this CL is just logging/CHECKS, and nothing that
> > couldn't be done with a bool in components.
> > I might be missing something, but can you please explain why do we need the
> > GetSingleAllocHook in base?
> > The reason why I ask is because I don't seem a sane way to expose this
> > function
> > in base in a way which is not thread safe, so I'd like to not expose that
> > at
> > all, especially if it's not really needed, as it seems to be the case here
> >
> >
> > https://codereview.chromium.org/1665553002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2016-03-02 19:10:14]

In case another module comes along and attempts to register another pair of
hook functions, overwriting the leak detector's hooks.

On Wed, Mar 2, 2016 at 11:06 AM, <primiano@chromium.org> wrote:

> this is the thing I don't get. why do you need to read the value of the
> hooks at
> all? what is the use case here that justifies your need of knowing the
> fptr of
> the current hooks? by reading your code it seems that you really want to
> maintain a on/off state.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2016-03-02 19:56:16]

On 2016/03/02 19:10:14, Simon Que wrote:
> In case another module comes along and attempts to register another pair of
> hook functions, overwriting the leak detector's hooks.

Which:
1) is imho unrealistic :)
2) we can prevent that to happen (with sufficient approximation) by just adding
a dcheck in the base/allocator code that verifies that you don't overwrite hooks
if they are already set (aforementioned g_boolean) some replies ago.

We could prevent 2) to happen at all by allowing to set the hooks only once, but
this means that you'd have to add an if (!disabled) check in your hooks, and is
my understanding you don't want to do that for performance reasons.
Both options (1. a weaker check, 2. allowing the hooks to be set only once) seem
viable to me. IMHO It is reasonable to expect that nothing else will install
other hooks while you temporarily uninstall yours.
Also, more importantly the way you are proposing to prevent this is not
race-safe either and seems to make the code in base more complex and prone to be
misused for no reason.

[Simon Que, 2016-03-02 21:56:24]

On 2016/03/02 19:56:16, Primiano (throttled til Mar 4) wrote:
> On 2016/03/02 19:10:14, Simon Que wrote:
> > In case another module comes along and attempts to register another pair of
> > hook functions, overwriting the leak detector's hooks.
> 
> Which:
> 1) is imho unrealistic :)
> 2) we can prevent that to happen (with sufficient approximation) by just
adding
> a dcheck in the base/allocator code that verifies that you don't overwrite
hooks
> if they are already set (aforementioned g_boolean) some replies ago.
> 
> We could prevent 2) to happen at all by allowing to set the hooks only once,
but
> this means that you'd have to add an if (!disabled) check in your hooks, and
is
> my understanding you don't want to do that for performance reasons.
> Both options (1. a weaker check, 2. allowing the hooks to be set only once)
seem
> viable to me. IMHO It is reasonable to expect that nothing else will install
> other hooks while you temporarily uninstall yours.
> Also, more importantly the way you are proposing to prevent this is not
> race-safe either and seems to make the code in base more complex and prone to
be
> misused for no reason.

Updated CL with simplified alloc hook registration and locking.

[Primiano Tucci (use gerrit), 2016-03-02 22:16:11]

base/allocator looks way better, thanks. There are just some comments left
pending.
Just don't know if you want me to cover also components/metrics.
I am not an owner there but happy to help with the review if you want and if
other owners are overloaded there.

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.cc:48: #endif
Ok this looks great now, thanks :)

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.cc:51: bool RemoveHooks(AllocHookFunc
alloc_hook, FreeHookFunc free_hook) {
Is this a leftover? This doesn't seem neither declared in the header nor used
anywhere else in the code in this cl.

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
File base/allocator/allocator_extension.h (right):

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.h:19: using AllocHookFunc =
std::add_pointer<void(const void*, size_t)>::type;
I think there is an unresolved comment here, maybe it just got lost in the
thread:

copy-pasting:
std::add_pointer<void(const void*, size_t)>::type;
What is the benefit of std::add_pointer as opposite to just a function pointer
(see below). I don't see any precedence in the codebase nor it seems whitelisted
in https://chromium-cpp.appspot.com/.

e.g. using AllocHookFunc = void (*)(const void*, size_t);

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.h:51: BASE_EXPORT int GetCallStack(void**
stack, int max_stack_size, int skip_count);
There is another unresolved comment here:
It looks like this is really not used in the .cc file. What's the point of
adding a dead parameter?

[Simon Que, 2016-03-02 22:35:25]

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
File base/allocator/allocator_extension.cc (right):

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.cc:51: bool RemoveHooks(AllocHookFunc
alloc_hook, FreeHookFunc free_hook) {
On 2016/03/02 22:16:11, Primiano (throttled til Mar 4) wrote:
> Is this a leftover? This doesn't seem neither declared in the header nor used
> anywhere else in the code in this cl.

Done.

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
File base/allocator/allocator_extension.h (right):

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.h:19: using AllocHookFunc =
std::add_pointer<void(const void*, size_t)>::type;
On 2016/03/02 22:16:11, Primiano (throttled til Mar 4) wrote:
> I think there is an unresolved comment here, maybe it just got lost in the
> thread:
> 
> copy-pasting:
> std::add_pointer<void(const void*, size_t)>::type;
> What is the benefit of std::add_pointer as opposite to just a function pointer
> (see below). I don't see any precedence in the codebase nor it seems
whitelisted
> in https://chromium-cpp.appspot.com/.
> 
> e.g. using AllocHookFunc = void (*)(const void*, size_t);

Done.

https://codereview.chromium.org/1665553002/diff/160001/base/allocator/allocat...
base/allocator/allocator_extension.h:51: BASE_EXPORT int GetCallStack(void**
stack, int max_stack_size, int skip_count);
On 2016/03/02 22:16:11, Primiano (throttled til Mar 4) wrote:
> There is another unresolved comment here:
> It looks like this is really not used in the .cc file. What's the point of
> adding a dead parameter?

Done.

[Primiano Tucci (use gerrit), 2016-03-02 22:46:15]

//base/allocator/ LGTM

Let me know if you want me to look at the rest or just shut up.

[Simon Que, 2016-03-02 22:47:35]

On 2016/03/02 22:46:15, Primiano (throttled til Mar 4) wrote:
> //base/allocator/ LGTM
> 
> Let me know if you want me to look at the rest or just shut up.

Please look at the rest. I still need Will to LGTM the other files but your
feedback would be welcome.

[Primiano Tucci (use gerrit), 2016-03-03 16:23:44]

Some comments here. Sorry for the delay, I am In a conference, will finish and
give a final pass around noon.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:62: static
base::allocator::AllocHookFunc alloc_hook_;
It seems that this variable is only ever set to be = to &AllocHook;
Why don't you directly SetHooks(&AllocHook, &FreeHook) in the dtor, instead of
keeping a copy of something you already know in these statics?

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:136: base::Lock*
g_instance_ptr_lock = nullptr;
This lock as is is not really solving any problem. You are preventing that you
access an inconsistent g_instance ptr but now you moved the race to the lock.
How can prevent to access an inconsistent lock if you hit a hook function while
you are destroying the LeakDetector (and nulling the lock)

I think what you really want is the lock here to be static. What I typically see
in these cases in chromium is 

base::LazyInstance<base::Lock>::Leaky g_instance_ptr_lock =
LAZY_INSTANCE_INITIALIZER;

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:149: : total_alloc_size_(0),
Can you reset  binary_mapping_addr_ and  binary_mapping_size_ here? If someone
in future takes this piece of code and doesn't realize the #ifdef part below
that would be left uninitialized

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:156: CHECK(!g_instance) <<
"Cannot instantiate multiple instances of this class.";
don't you need the g_instance_ptr_lock to make this check?

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:215: base::AutoLock
lock(*g_instance_ptr_lock);
At this point you already have to take a lock every time you enter this (and the
free hooks).
Can't you leverage the g_isntance check (or the ShouldSample check) to bypass
the hooks without having to uninstall and reinstall them in the scoped enabler.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:21: #include
"components/metrics/leak_detector/leak_detector_impl.h"
Are you adding this just for the scoped_ptr? I think you can forward declare it
and including this to the .cc?

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:97: // unwinds call stack if
necessary. Passes the allocated memory |ptr| and
These functions seem internal only and hence no need to be exposed in the .h
file.
It seems the case that these should be anonymous functions in an unnamed
namespace in the .cc file, as indicated by our codestyle 
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

(unless I am missing some code and you need them in tests, in which case the
only option is keeping them here)

[Primiano Tucci (use gerrit), 2016-03-03 17:51:30]

Some final comments inline.
I think I understand what you are doing here, but my overall feeling is that you
are making the design of your leak detector way more complicated than what you
need.

You are in this state where some of the things are done in the LeakDetector
class, which is not a singleton but you treat as a "temporary singleton" by
assuming that you have only one instance and caching its ptr into a global
variable. Also you say that you need a lock only to access the singleton ptr,
but in reality you *really* want a lock to avoid races.

Feel free to design as more convenient for your purposes, my $0.02 is that this
design as is will cause lot of debugging headaches once you start shipping it.
You ended up having your own STL containers, which use a custom allocator, which
invoke functions pointers that you set here, which disable the hooks before
performing each allocation.
IMHO  if something will go wrong here in the wild you (or who will have to
maintain the code for you) will have a hard time debugging all this.

I would have kept all this way more simple (have a real leaky singleton class,
use a lock to avoid thread races and an counter that you set when you enter each
hook to prevent reentrancy, user simpler types and not invent own STL
allocators). Anyways I don't want this to block your work, take it as a
suggestion and use your best judgement on what to do here.
I'll keep reviewing and as long as the patterns feel sane and not visibly racy,
LG when we get there.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:135: // For atomic access to
|g_instance| -- the pointer, not the pointed-to object.
You say "not the pointed-to object" but in reality you use this lock (and need
it) to avoid concurrent accesses to the LeaakDetectorImpl class.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:255: // This should be
modified only with a lock because it uses the shared
You can make this comment more explicit by adding a :

g_lock->AssertAcquired() (which is a noop in release, but still gives you a
dcheck)

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:266: // - Possibly generates a
vector of LeakReports using CustomAllocator.
Suggestion (maybe for a future CL): probably would be less intrusive if you did
all this leak analysis below in a posted task, which would give you two
benefits:
you would avoid races by being sure that everything happens on the same thread,
and will avoid to block random threads during their allocation fastpaths

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:275: std::vector<LeakReport>
leak_reports_for_observers;
isn't std::vector cause mallocs and going to re-enter the LeakDetector?

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:291:
NotifyObservers(leak_reports_for_observers);
NotifyObserver does all sort of thigs (PosTask, invoking a list of observers).
How do you guarantee that they don't re-enter the hooks? PostTask very likely
will do some malloc. Similarly you have no control of your observers.
All this seems to work only if none of that malloc's

[Simon Que, 2016-03-04 01:59:18]

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:62: static
base::allocator::AllocHookFunc alloc_hook_;
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> It seems that this variable is only ever set to be = to &AllocHook;
> Why don't you directly SetHooks(&AllocHook, &FreeHook) in the dtor, instead of
> keeping a copy of something you already know in these statics?

That would require AllocHook/FreeHook to be public, or defined as a non-member
function locally. It'll depends on how the rest of the code is structured, based
on the rest of this discussion. Let's revisit this later when we have the rest
of the design laid out.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:136: base::Lock*
g_instance_ptr_lock = nullptr;
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> This lock as is is not really solving any problem. You are preventing that you
> access an inconsistent g_instance ptr but now you moved the race to the lock.
> How can prevent to access an inconsistent lock if you hit a hook function
while
> you are destroying the LeakDetector (and nulling the lock)
> 
> I think what you really want is the lock here to be static. What I typically
see
> in these cases in chromium is 
> 
> base::LazyInstance<base::Lock>::Leaky g_instance_ptr_lock =
> LAZY_INSTANCE_INITIALIZER;

What we really need here is a one-shot rwlock. I'm not sure that a standard lock
is the best option since it has to cover all the code that uses |g_instance| --
meaning it locks a large section of code.

If you're averse to using a pthread_rwlock, I can make my own -- it's easy
because the only writer is the dtor. Replace this with an int counter that
represents the number of threads using g_instance. Each time the hook function
runs, increment the counter, and then decrement on exit.

In the dtor, the hooks will be unregistered first. Then the dtor will wait for
the counter to reach 0. The dtor will end up waiting for any threads currently
in the hook to finish, and there will be no more hook calls.

Then, I can change hook code to use the lock in fewer places since there there
are no longer any thread contention issues with |g_instance| as a ptr.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:149: : total_alloc_size_(0),
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> Can you reset  binary_mapping_addr_ and  binary_mapping_size_ here? If someone
> in future takes this piece of code and doesn't realize the #ifdef part below
> that would be left uninitialized

Done.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:156: CHECK(!g_instance) <<
"Cannot instantiate multiple instances of this class.";
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> don't you need the g_instance_ptr_lock to make this check?

The DCHECK above should ensure that the ctor is always on the same thread.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:266: // - Possibly generates a
vector of LeakReports using CustomAllocator.
On 2016/03/03 17:51:30, Primiano (throttled til Mar 4) wrote:
> Suggestion (maybe for a future CL): probably would be less intrusive if you
did
> all this leak analysis below in a posted task, which would give you two
> benefits:
> you would avoid races by being sure that everything happens on the same
thread,
> and will avoid to block random threads during their allocation fastpaths

Added a TODO.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:275: std::vector<LeakReport>
leak_reports_for_observers;
On 2016/03/03 17:51:30, Primiano (throttled til Mar 4) wrote:
> isn't std::vector cause mallocs and going to re-enter the LeakDetector?

Yes. Recursive calls to the allocator are actually OK as long as they don't
cause a deadlock -- e.g. called from an unlocked location.

This code was originally written as an unlocked location.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:21: #include
"components/metrics/leak_detector/leak_detector_impl.h"
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> Are you adding this just for the scoped_ptr? I think you can forward declare
it
> and including this to the .cc?

Done.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:97: // unwinds call stack if
necessary. Passes the allocated memory |ptr| and
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> These functions seem internal only and hence no need to be exposed in the .h
> file.
> It seems the case that these should be anonymous functions in an unnamed
> namespace in the .cc file, as indicated by our codestyle 
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...
> 
> (unless I am missing some code and you need them in tests, in which case the
> only option is keeping them here)

These functions need to access the contents of the LeakDetector object. (impl_,
total_size_alloc_, and RecordAlloc/RecordFree).

[Simon Que, 2016-03-04 02:15:36]

On 2016/03/03 17:51:30, Primiano (throttled til Mar 4) wrote:
> Some final comments inline.
> I think I understand what you are doing here, but my overall feeling is that
you
> are making the design of your leak detector way more complicated than what you
> need.
> 
> You are in this state where some of the things are done in the LeakDetector
> class, which is not a singleton but you treat as a "temporary singleton" by
> assuming that you have only one instance and caching its ptr into a global
> variable. Also you say that you need a lock only to access the singleton ptr,
> but in reality you *really* want a lock to avoid races.
> 
> Feel free to design as more convenient for your purposes, my $0.02 is that
this
> design as is will cause lot of debugging headaches once you start shipping it.
> You ended up having your own STL containers, which use a custom allocator,
which
> invoke functions pointers that you set here, which disable the hooks before
> performing each allocation.

The current setup is not acceptable for shipping. I still have to actually put
some internal allocation logic into the custom allocator, so that it doesn't
need to call the main allocator each time. That code is relatively simple, about
200 lines.

In fact, the custom allocator actually makes it easier to profile the leak
detector since all STL objects that use it will contain the namespace name
"leak_detector" in their signatures.

> IMHO  if something will go wrong here in the wild you (or who will have to
> maintain the code for you) will have a hard time debugging all this.
> 
> I would have kept all this way more simple (have a real leaky singleton class,
> use a lock to avoid thread races and an counter that you set when you enter
each
> hook to prevent reentrancy, user simpler types and not invent own STL
> allocators). Anyways I don't want this to block your work, take it as a
> suggestion and use your best judgement on what to do here.
> I'll keep reviewing and as long as the patterns feel sane and not visibly
racy,
> LG when we get there.

The implementation of this class as an instantiatable object came from a
discussion with Will and the UMA guys. I've forwarded you the email.

Re-entrance prevention is not practical because it interferes with the
deterministic sampling scheme. Right now, whether or not to enter is determined
solely by the value of the ptr. This way, the same ptr can be passed to
AllocHook and FreeHook.

[Simon Que, 2016-03-04 02:48:47]

On 2016/03/04 02:15:36, Simon Que wrote:
> On 2016/03/03 17:51:30, Primiano (throttled til Mar 4) wrote:
> > Some final comments inline.
> > I think I understand what you are doing here, but my overall feeling is that
> you
> > are making the design of your leak detector way more complicated than what
you
> > need.
> > 
> > You are in this state where some of the things are done in the LeakDetector
> > class, which is not a singleton but you treat as a "temporary singleton" by
> > assuming that you have only one instance and caching its ptr into a global
> > variable. Also you say that you need a lock only to access the singleton
ptr,
> > but in reality you *really* want a lock to avoid races.
> > 
> > Feel free to design as more convenient for your purposes, my $0.02 is that
> this
> > design as is will cause lot of debugging headaches once you start shipping
it.
> > You ended up having your own STL containers, which use a custom allocator,
> which
> > invoke functions pointers that you set here, which disable the hooks before
> > performing each allocation.
> 
> The current setup is not acceptable for shipping. I still have to actually put
> some internal allocation logic into the custom allocator, so that it doesn't
> need to call the main allocator each time. That code is relatively simple,
about
> 200 lines.
> 
> In fact, the custom allocator actually makes it easier to profile the leak
> detector since all STL objects that use it will contain the namespace name
> "leak_detector" in their signatures.
> 
> > IMHO  if something will go wrong here in the wild you (or who will have to
> > maintain the code for you) will have a hard time debugging all this.
> > 
> > I would have kept all this way more simple (have a real leaky singleton
class,
> > use a lock to avoid thread races and an counter that you set when you enter
> each
> > hook to prevent reentrancy, user simpler types and not invent own STL
> > allocators). Anyways I don't want this to block your work, take it as a
> > suggestion and use your best judgement on what to do here.
> > I'll keep reviewing and as long as the patterns feel sane and not visibly
> racy,
> > LG when we get there.
> 
> The implementation of this class as an instantiatable object came from a
> discussion with Will and the UMA guys. I've forwarded you the email.
> 
> Re-entrance prevention is not practical because it interferes with the
> deterministic sampling scheme. Right now, whether or not to enter is
determined
> solely by the value of the ptr. This way, the same ptr can be passed to
> AllocHook and FreeHook.

BTW, would you like to discuss this in person while you're in town? I'm free in
the afternoon after 2pm and I can come to wherever you're sitting.

[Simon Que, 2016-03-04 02:55:19]

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:62: static
base::allocator::AllocHookFunc alloc_hook_;
On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> It seems that this variable is only ever set to be = to &AllocHook;
> Why don't you directly SetHooks(&AllocHook, &FreeHook) in the dtor, instead of
> keeping a copy of something you already know in these statics?

One argument in favor of the current design: what happens when the LeakDetector
dtor has unregistered the hooks, but there are still some other threads that
have not finished executing in the hooks? If they instantiate and destroy one of
these, they must not re-install the hooks.

If you think that's a good reason to keep it this way, I can add a comment here
explaining the rationale.

[Primiano Tucci (use gerrit), 2016-03-04 06:57:35]

> Re-entrance prevention is not practical because it interferes with the
deterministic sampling scheme. Right now, whether or not to enter is determined

Writing re-entrant code is extremely hard and error prone. Also, I don't see how
here you need reentrancy-safe code. This is something that you are self-imposing
and I don't see an actual reason for that.
Why don't you just disable/bypass the hooks once you are in the hooks function
themselves, instead of relying on fine-grained hook-bypassing containers? Are
you really concerned in tracking leaks in the leak detector itself?
 
I think that with this design you are going to make your near future life hard,
and more importantly, you are imposing a *huge* maintenance burden to the
unlucky person that will have to maintain this code.
You ended up having a class that, at the same time is:
- owned via a scoped_ptr (suggesting that you can create as much instances as
you want)
- used like a singleton, assuming you can have at most one instance at a time,
caching a global g_instance ptr.
- giving out weak pointers.
- called by multiple threads (for which you suggest you want to invent your own
locking mechanism)
- called with reentrancy.

Which sounds a recipe for a maintenance nightmare.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:62: static
base::allocator::AllocHookFunc alloc_hook_;
> One argument in favor of the current design: what happens when the
LeakDetector
> dtor has unregistered the hooks, but there are still some other threads that
> have not finished executing in the hooks? 
That's why I was suggesting to have a boolean state to keep track of that.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:62: static
base::allocator::AllocHookFunc alloc_hook_;
On 2016/03/04 01:59:18, Simon Que wrote:
> That would require AllocHook/FreeHook to be public, or defined as a non-member
> function locally. 
I am not sure I follow. AllocHook and FreeHook here are already static functions
here.
As I suggested in my previous comment, they don't even need to be such, they
could be just functions in the anonymous namespace.
Even if you want to keep them static functions of LeakDetector, very likely what
you want is the disabler to be an internal class of the LeakDetector, or just
use friend.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:136: base::Lock*
g_instance_ptr_lock = nullptr;
> If you're averse to using a pthread_rwlock
Is not that I am adverse. As usually in chrome the pattern is: is there
precedent for doing this? If not why?
base does not expose a rwlock. Neither I can find any non-third_party code which
uses pthread_rwlock.
There are a bunch of discussions on chromium-dev about the topic. 


>, I can make my own -- it's easy
> because the only writer is the dtor. Replace this with an int counter that
> represents the number of threads using g_instance. Each time the hook function
> runs, increment the counter, and then decrement on exit.
Yes, and now you run into another small problem: how do you make the concurrent
accesses to this counter sane? Now you need another lock for the counter.


> In the dtor, the hooks will be unregistered first. Then the dtor will wait for
> the counter to reach 0. The dtor will end up waiting for any threads currently
> in the hook to finish, and there will be no more hook calls.
So you have a lock for the counter, you have a counter and now you want a method
to wait for that: I think you have just re-invented base::ConditionVariable.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:156: CHECK(!g_instance) <<
"Cannot instantiate multiple instances of this class.";
On 2016/03/04 01:59:18, Simon Que wrote:
> On 2016/03/03 16:23:44, Primiano (throttled til Mar 4) wrote:
> > don't you need the g_instance_ptr_lock to make this check?
> 
> The DCHECK above should ensure that the ctor is always on the same thread.

Acknowledged.

https://codereview.chromium.org/1665553002/diff/200001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:275: std::vector<LeakReport>
leak_reports_for_observers;
On 2016/03/04 01:59:18, Simon Que wrote:
> Yes. Recursive calls to the allocator are actually OK as long as they don't
> cause a deadlock -- e.g. called from an unlocked location.
How can you say that re-entrant calls to RecordAlloc are OK?

1) You are using std::vector here, how can you be sure that std::vector is
reentrancy-safe  in the allocator path? That is an implementation detail.

2) Below you are invoking some observers. How can you be sure that the observers
are reentrancy-safe if you don't know who they are?

3) In the code below you are doing a vector.push_back(). How is that
re-entrant-safe? (push_back while you are pushing_back in the same vector?)

[Primiano Tucci (use gerrit), 2016-03-04 16:03:56]

On 2016/03/04 02:48:47, Simon Que wrote:
> BTW, would you like to discuss this in person while you're in town? I'm free
in
> the afternoon after 2pm and I can come to wherever you're sitting.

I am on an offsite until noon-ish but should be back to MTV from 2-3 PM. Let's
chat then?

[Simon Que, 2016-03-11 06:38:33]

I've updated this CL based on my discussion with Primiano. PTAL.

[Simon Que, 2016-03-11 06:40:37]

sque@chromium.org changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[Simon Que, 2016-03-11 06:40:38]

Adding Alexei and Ilya to review changes to chrome/browser/metrics.

[Primiano Tucci (use gerrit), 2016-03-11 12:10:17]

I'm a bit short of bandwidth today due to unexpected fires.
I had a quick look and this looks good, % the fact that during the internal
thread I think everybody converged towards LazyInstance::Leaky instead of
singleton.
Beyond that (that should be a few lines fix) this looks to me way simpler to
reason about.

P.S.: the shim layer just landed and seems to stick. At some point in the near
future we might want to get rid of the SetHooks changes in //base/allocator and
use the shim.
For the moment, though, let's leave things in this patch as they are and let's
keep that as a near future cleanup.
Maybe just add a TODO for now.

[Simon Que, 2016-03-11 21:21:34]

On 2016/03/11 12:10:17, Primiano wrote:
> I'm a bit short of bandwidth today due to unexpected fires.
> I had a quick look and this looks good, % the fact that during the internal
> thread I think everybody converged towards LazyInstance::Leaky instead of
> singleton.
> Beyond that (that should be a few lines fix) this looks to me way simpler to
> reason about.

Done.

> P.S.: the shim layer just landed and seems to stick. At some point in the near
> future we might want to get rid of the SetHooks changes in //base/allocator
and
> use the shim.
> For the moment, though, let's leave things in this patch as they are and let's
> keep that as a near future cleanup.
> Maybe just add a TODO for now.

Done.

[Will Harris, 2016-03-12 01:44:22]

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:37: // This class is
thread-safe as it uses locks for critical sections.
it looks like only addobserver and removeobserver are thread safe, the other
methods are using a thread_checker_ and will dcheck if called on the wrong
thread.

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_unittest.cc (right):

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_unittest.cc:71: // Use a
scoped_ptr to hold the test object so it can be destroyed before the
nit:update comment

[Simon Que, 2016-03-14 19:17:01]

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:37: // This class is
thread-safe as it uses locks for critical sections.
On 2016/03/12 01:44:22, Will Harris wrote:
> it looks like only addobserver and removeobserver are thread safe, the other
> methods are using a thread_checker_ and will dcheck if called on the wrong
> thread.

Done.

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_unittest.cc (right):

https://codereview.chromium.org/1665553002/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_unittest.cc:71: // Use a
scoped_ptr to hold the test object so it can be destroyed before the
On 2016/03/12 01:44:22, Will Harris wrote:
> nit:update comment

Done.

[Ilya Sherman, 2016-03-14 22:38:07]

Just looked at leak_detector_controller.{h, cc}.  Those files LGTM.  Please let
me know if you'd like me to also look at the leak_detector implementation files
-- otherwise, I'll assume that Will's review suffices.

[Will Harris, 2016-03-15 04:13:35]

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:51: MappingInfo* mapping =
static_cast<MappingInfo*>(data);
reinterpret_cast for pointers

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:118: return &g_instance.Get();
.Pointer()

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:148: new
leak_detector::LeakDetectorImpl(mapping.addr, mapping.size,
mapping.addr and mapping.size might be 0 here, should the leak detector still be
instantiated in that case?

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:182: LeakDetector* detector =
&g_instance.Get();
.Pointer()

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:204:
detector->impl_lock_.Acquire();
what is the performance hit like for having a lock around every alloc and free?

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:236:
GetReportsForObservers(leak_reports, &leak_reports_for_observers);
two threads can call GetReportsForObservers at the same time, I think?

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:245: LeakDetector* detector =
&g_instance.Get();
.Pointer()

[Simon Que, 2016-03-15 06:39:32]

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:51: MappingInfo* mapping =
static_cast<MappingInfo*>(data);
On 2016/03/15 04:13:35, Will Harris wrote:
> reinterpret_cast for pointers

Done.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:118: return &g_instance.Get();
On 2016/03/15 04:13:35, Will Harris wrote:
> .Pointer()

Done.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:148: new
leak_detector::LeakDetectorImpl(mapping.addr, mapping.size,
On 2016/03/15 04:13:35, Will Harris wrote:
> mapping.addr and mapping.size might be 0 here, should the leak detector still
be
> instantiated in that case?

Yes. If that's the case, call stacks in leak reports will simply have the
original addresses, not offsets.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:182: LeakDetector* detector =
&g_instance.Get();
On 2016/03/15 04:13:35, Will Harris wrote:
> .Pointer()

Done.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:204:
detector->impl_lock_.Acquire();
On 2016/03/15 04:13:35, Will Harris wrote:
> what is the performance hit like for having a lock around every alloc and
free?

The overhead of the leak detector at 1/256 sampling is 2.0%. This is designed
for clarity over performance. I do plan to optimize it in a later CL.

I've measured some optimizations and have the overhead at 1.1% per process. It
may be possible to optimize it even more.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:236:
GetReportsForObservers(leak_reports, &leak_reports_for_observers);
On 2016/03/15 04:13:35, Will Harris wrote:
> two threads can call GetReportsForObservers at the same time, I think?

Both arguments to this function are local. tcmalloc itself is handling the heap
allocation of |leak_reports_for_observers|. Is there another safety issue?

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:245: LeakDetector* detector =
&g_instance.Get();
On 2016/03/15 04:13:35, Will Harris wrote:
> .Pointer()

Done.

[Will Harris, 2016-03-15 06:49:43]

lgtm. thanks for patience while reviewing.

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/300001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:236:
GetReportsForObservers(leak_reports, &leak_reports_for_observers);
On 2016/03/15 06:39:31, Simon Que wrote:
> On 2016/03/15 04:13:35, Will Harris wrote:
> > two threads can call GetReportsForObservers at the same time, I think?
> 
> Both arguments to this function are local. tcmalloc itself is handling the
heap
> allocation of |leak_reports_for_observers|. Is there another safety issue?

yes sorry you're right, I did not see that GetReportsForObservers was in anon
namespace.

[Primiano Tucci (use gerrit), 2016-03-15 11:49:24]

Apologies for the lateness. I join the LGTM train with few minor comments.
I think that if you use one lock for impl_lock_ and alloc_size_lock_ it might be
even faster, I guess right now you pay more for the lock/unlock atomics than the
number crunching you do inside :)

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:171:
DCHECK(thread_checker_.CalledOnValidThread());
I don't think this DCHECK can possibly be hit.
the thread_checker_ is intiialized here in the ctor. When constructed, it
automatically sets the TID internally, so this condition will be necessarily
true.

In other words, if you expand the ThreadChecker ctor + this line, this reads as:
thread_checker.valid_thread_id_ = current_id
DCHECK(thread_checker.valid_thread_id == current_tid)

It makes sense, instead, that you keep the check elsewhere

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:187:
detector->alloc_size_lock_.Acquire();
Just a minor stylistic nit, I usually see this as a forced scope + autolock:
{
  AutoLock lock(detector->alloc_size_lock_);
  detector->total_alloc_size_ += size;
}

this prevents that if somebody touches your code later they don't screw up the
locking (e.g. mistakenly cutting away the release).

ditto below

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:189:
detector->alloc_size_lock_.Release();
Note that you write here the total_alloc_size_ in a lock, but re-read it below
on line 219 outside the lock, which is potentially racy.
I think you want to store the total_alloc_size here within the lock, so
everything stays atomic.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:232:
detector->impl_lock_.Release();
You have two fine grained locks here (alloc_size_lock_ and impl_lock). I think
you introduced them to reduce contemption, however the length of the critical
section seems minimal for both.
IMHO it would be more readable and also more performant if you had only one
lock, and did everything inside this if under that lock.
Rationale: now, on every malloc that > threshold, you have to [acquire, release,
acquire, release]. If you had only one lock could do that only once. I am pretty
confident that if you profile this most of the time is spent for the
acquire/release themselves, not for the number crunching you do inside :)
But that's really up to you.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:266: void
LeakDetector::NotifyObservers(const std::vector<LeakReport>& reports) {
Suggestion for a future improvement, observer_list_threadsafe.h gives lot of
this threadsafe  + posttask stuff for free :)

[Simon Que, 2016-03-15 18:33:42]

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:171:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/03/15 11:49:24, Primiano wrote:
> I don't think this DCHECK can possibly be hit.
> the thread_checker_ is intiialized here in the ctor. When constructed, it
> automatically sets the TID internally, so this condition will be necessarily
> true.
> 
> In other words, if you expand the ThreadChecker ctor + this line, this reads
as:
> thread_checker.valid_thread_id_ = current_id
> DCHECK(thread_checker.valid_thread_id == current_tid)
> 
> It makes sense, instead, that you keep the check elsewhere

Done.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:187:
detector->alloc_size_lock_.Acquire();
On 2016/03/15 11:49:23, Primiano wrote:
> Just a minor stylistic nit, I usually see this as a forced scope + autolock:
> {
>   AutoLock lock(detector->alloc_size_lock_);
>   detector->total_alloc_size_ += size;
> }
> 
> this prevents that if somebody touches your code later they don't screw up the
> locking (e.g. mistakenly cutting away the release).
> 
> ditto below

Done.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:189:
detector->alloc_size_lock_.Release();
On 2016/03/15 11:49:24, Primiano wrote:
> Note that you write here the total_alloc_size_ in a lock, but re-read it below
> on line 219 outside the lock, which is potentially racy.
> I think you want to store the total_alloc_size here within the lock, so
> everything stays atomic.

I'm not that concerned about it since |total_alloc_size_| never decreases.
Nonetheless, I'll include that in the lock.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:232:
detector->impl_lock_.Release();
On 2016/03/15 11:49:24, Primiano wrote:
> You have two fine grained locks here (alloc_size_lock_ and impl_lock). I think
> you introduced them to reduce contemption, however the length of the critical
> section seems minimal for both.
> IMHO it would be more readable and also more performant if you had only one
> lock, and did everything inside this if under that lock.
> Rationale: now, on every malloc that > threshold, you have to [acquire,
release,
> acquire, release]. If you had only one lock could do that only once. I am
pretty
> confident that if you profile this most of the time is spent for the
> acquire/release themselves, not for the number crunching you do inside :)
> But that's really up to you.

Done.

https://codereview.chromium.org/1665553002/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:266: void
LeakDetector::NotifyObservers(const std::vector<LeakReport>& reports) {
On 2016/03/15 11:49:24, Primiano wrote:
> Suggestion for a future improvement, observer_list_threadsafe.h gives lot of
> this threadsafe  + posttask stuff for free :)

Done. Added a TODO.

[Simon Que, 2016-03-15 18:33:52]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-03-15 18:33:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, wfh@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1665553002/#ps360001
(title: "primiano's feedback; Register observer sooner")

[commit-bot: I haz the power, 2016-03-15 18:34:13]

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

[commit-bot: I haz the power, 2016-03-15 19:55:25]

Description was changed from

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests
==========

to

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests
==========

[commit-bot: I haz the power, 2016-03-15 19:55:27]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-15 19:56:50]

Description was changed from

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests
==========

to

==========
metrics: Connect leak detector to base::allocator

This patch introduces the leak detector's integration layer,
which gathers alloc/free info and passes it to the leak
detection logic in LeakDetectorImpl.

It registers callbacks with the allocator, which are called
during alloc and free. These callbacks intercept and sample
the alloc and free info.

It also provides a notification interface for external
objects to receive leak reports.

See leak_detector.cc for more info.

BUG=chromium:382705
TEST=build successfully, run components_unittests

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

[commit-bot: I haz the power, 2016-03-15 19:56:53]

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