components/metrics: Add runtime memory leak detector

components/metrics: Add runtime memory leak detector

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

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

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

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

[Simon Que, 2015-03-05 23:11:41]

This is not ready for official review. I've uploaded this CL so people can look
at it and cherry-pick it if they so desire.

[Simon Que, 2015-03-19 21:13:55]

Patchset #13 (id:240001) has been deleted

[haraken, 2015-03-23 03:39:46]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[haraken, 2015-03-23 04:34:39]

Do you have a design document that describes what the leak detector is doing at
a high level?

[Simon Que, 2015-03-23 19:18:54]

On 2015/03/23 04:34:39, haraken wrote:
> Do you have a design document that describes what the leak detector is doing
at
> a high level?

Here is one I wrote a few months ago. The design is mostly the same.
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...

However, I am working on a new presentation that directly describes the current
implementation.

[haraken, 2015-03-23 23:24:20]

On 2015/03/23 19:18:54, Simon Que wrote:
> On 2015/03/23 04:34:39, haraken wrote:
> > Do you have a design document that describes what the leak detector is doing
> at
> > a high level?
> 
> Here is one I wrote a few months ago. The design is mostly the same.
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> 
> However, I am working on a new presentation that directly describes the
current
> implementation.

Thanks, a design doc is very helpful for this kind of complex change :)

[Simon Que, 2015-03-24 01:21:09]

On 2015/03/23 23:24:20, haraken wrote:
> On 2015/03/23 19:18:54, Simon Que wrote:
> > On 2015/03/23 04:34:39, haraken wrote:
> > > Do you have a design document that describes what the leak detector is
doing
> > at
> > > a high level?
> > 
> > Here is one I wrote a few months ago. The design is mostly the same.
> >
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> > 
> > However, I am working on a new presentation that directly describes the
> current
> > implementation.
> 
> Thanks, a design doc is very helpful for this kind of complex change :)

Keep in mind that this will likely move to base/allocator. See the email
discussion thread.

[haraken, 2015-03-24 01:24:07]

On 2015/03/24 01:21:09, Simon Que wrote:
> On 2015/03/23 23:24:20, haraken wrote:
> > On 2015/03/23 19:18:54, Simon Que wrote:
> > > On 2015/03/23 04:34:39, haraken wrote:
> > > > Do you have a design document that describes what the leak detector is
> doing
> > > at
> > > > a high level?
> > > 
> > > Here is one I wrote a few months ago. The design is mostly the same.
> > >
> >
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> > > 
> > > However, I am working on a new presentation that directly describes the
> > current
> > > implementation.
> > 
> > Thanks, a design doc is very helpful for this kind of complex change :)
> 
> Keep in mind that this will likely move to base/allocator. See the email
> discussion thread.

FWIW, added some comments on the doc.

[Simon Que, 2015-03-24 01:40:48]

On 2015/03/24 01:24:07, haraken wrote:
> On 2015/03/24 01:21:09, Simon Que wrote:
> > On 2015/03/23 23:24:20, haraken wrote:
> > > On 2015/03/23 19:18:54, Simon Que wrote:
> > > > On 2015/03/23 04:34:39, haraken wrote:
> > > > > Do you have a design document that describes what the leak detector is
> > doing
> > > > at
> > > > > a high level?
> > > > 
> > > > Here is one I wrote a few months ago. The design is mostly the same.
> > > >
> > >
> >
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> > > > 
> > > > However, I am working on a new presentation that directly describes the
> > > current
> > > > implementation.
> > > 
> > > Thanks, a design doc is very helpful for this kind of complex change :)
> > 
> > Keep in mind that this will likely move to base/allocator. See the email
> > discussion thread.
> 
> FWIW, added some comments on the doc.

Thanks. Also note that this is not meant to be the patch that enables this on
machines in the field. With this CL, leak detection still has to be manually
compiled in and enabled locally by developers.

[Simon Que, 2015-04-04 03:32:20]

I moved most of the code into base/allocator. Still getting a few presubmit
check errors:

======== ERROR 1 =========

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  base/allocator/call_stack_table.cc
      Illegal include: "rlz/lib/crc32.h"
    Because of no rule applying. \
  base/allocator/leak_detector_impl.cc
      Illegal include: "rlz/lib/crc32.h"
    Because of no rule applying.

I think this is because I'm including something that's not in base/ in a base/
source file.

========= ERROR 2 ==========

The following files contain a third-party license but are not in a listed
third-party directory and are not whitelisted. You must add the following files
to the whitelist file android_webview/tools/third_party_files_whitelist.txt:
  base/allocator/address_map-inl.h

I copied this file over from tcmalloc because my leak detector implementation
requires it.

=========================

Given the above errors, and the fact that I had to leave some code in tcmalloc,
perhaps it would be easier to keep it in tcmalloc.

[haraken, 2015-04-30 02:05:53]

A couple of my thoughts:

- As I commented in the document, I'm still a bit skeptical about the ability of
the leak detector in a sense that the leak detector can find leaks but cannot
find the retainers of the leaks. As far as I've worked on leak detection in
Blink, the hardest part is not how to find leaks but how to find the retainers
of the leaks. (In case of Blink, we can easily find leaks just by observing the
number of documents because documents are going to leak in most (serious) leaks
in Blink.)

- That being said, the above is just my personal view on leaks in Blink. Given
that your leak detector has succeeded in finding and fixing leaks in Chromium,
I'm fine with landing the detector if the detector does not have any negative
impact on performance.

- To mitigate the performance concerns, would you provide the following data?

1) Explain what instructions are introduced to hot call paths to tcmalloc
methods.

2) Create a super-micro benchmark that will be most affected by the
instructions. Measure the performance and clarify the worst-case overhead.

3) If you observe some regression in 2), measure performance using some
realistic benchmarks that heavily use tcmalloc. If you don't observe any
regression in 2), you don't need this step.

[Benoit L, 2015-05-04 12:03:28]

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

[Benoit L, 2015-05-04 12:03:29]

Random drive-by comment (may very well be completely wrong) !

https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector_impl.cc:143:
address_map_.Insert(ptr, alloc_info);
If I understand this correctly, this add an AddressMap entry for *each*
allocation.
Has the memory cost of this been quantified?

[Benoit L, 2015-05-04 12:12:03]

https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector_impl.cc:143:
address_map_.Insert(ptr, alloc_info);
On 2015/05/04 12:03:29, lizeb wrote:
> If I understand this correctly, this add an AddressMap entry for *each*
> allocation.
> Has the memory cost of this been quantified?

Replying to myself.
Turns out that this is only recording for the fraction of allocation we look at.
With a sampling rate of 1/4, this may still be large.

[Simon Que, 2015-05-04 20:52:57]

On 2015/05/04 12:12:03, lizeb wrote:
>
https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
> File third_party/tcmalloc/chromium/src/leak_detector_impl.cc (right):
> 
>
https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/leak_detector_impl.cc:143:
> address_map_.Insert(ptr, alloc_info);
> On 2015/05/04 12:03:29, lizeb wrote:
> > If I understand this correctly, this add an AddressMap entry for *each*
> > allocation.
> > Has the memory cost of this been quantified?
> 
> Replying to myself.
> Turns out that this is only recording for the fraction of allocation we look
at.
> With a sampling rate of 1/4, this may still be large.

I added instrumentation for heap memory usage of the leak detector. In each
case, I let it run for a few minutes. All numbers are in bytes.

Sampling rate  Browser process   Renderer process
=============  ===============   ================
    1/16               6407864            1608712
     1/4               9935544            2193872
     100%             21382792            4264456

At a sampling rate of 1/4 (too high IMO), the browser process takes about 10 MB
and the renderer about 2 MB. So a formula for the total usage would be roughly:
  leak_detector_usage_in_MB = 10 + 2 * num_tabs;

If we have 20 tabs open, we'd be looking at 50 MB of memory usage.

If that's a concern, we can find some way to randomly select which renderer
processes have leak detection. e.g. by creation time or by pid.

[Simon Que, 2015-05-05 01:04:44]

On 2015/05/04 20:52:57, Simon Que wrote:
> On 2015/05/04 12:12:03, lizeb wrote:
> >
>
https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
> > File third_party/tcmalloc/chromium/src/leak_detector_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/986503002/diff/540001/third_party/tcmalloc/ch...
> > third_party/tcmalloc/chromium/src/leak_detector_impl.cc:143:
> > address_map_.Insert(ptr, alloc_info);
> > On 2015/05/04 12:03:29, lizeb wrote:
> > > If I understand this correctly, this add an AddressMap entry for *each*
> > > allocation.
> > > Has the memory cost of this been quantified?
> > 
> > Replying to myself.
> > Turns out that this is only recording for the fraction of allocation we look
> at.
> > With a sampling rate of 1/4, this may still be large.
> 
> I added instrumentation for heap memory usage of the leak detector. In each
> case, I let it run for a few minutes. All numbers are in bytes.
> 
> Sampling rate  Browser process   Renderer process
> =============  ===============   ================
>     1/16               6407864            1608712
>      1/4               9935544            2193872
>      100%             21382792            4264456
> 
> At a sampling rate of 1/4 (too high IMO), the browser process takes about 10
MB
> and the renderer about 2 MB. So a formula for the total usage would be
roughly:
>   leak_detector_usage_in_MB = 10 + 2 * num_tabs;
> 
> If we have 20 tabs open, we'd be looking at 50 MB of memory usage.
> 
> If that's a concern, we can find some way to randomly select which renderer
> processes have leak detection. e.g. by creation time or by pid.

TL;DR: The address map takes up the most heap, and can be optimized to become
several times smaller.

It turns out that most of the memory usage comes from the address map. The
internal implementation is such that if one entry exists in a 1-MB address
cluster, an 8192-element array of pointers will be allocated for that cluster.
On a 64-bit system, the array will be 64 KiB in size.

I've found that the browser process has around 50-100 clusters and renderer
process has about 20. The latter number means that 20 individual address samples
across different clusters will result in 1280 KiB = 1.25 MiB of heap usage per
renderer process. This is far too inefficient.

I've reworked the address map's internals to use less memory, without
introducing much extra lookup overhead (something that can be mitigated by
sampling). Now I have gotten the address map down to about 250 KiB for the
renderer process, resulting in 420 KiB of memory usage per tab. The browser
process is at 1.6 MiB.

With a whopping 50 tabs open, we're looking at around 25 MB of RAM usage. On a
2-GB system, that's still about 1.25%.

[Simon Que, 2015-05-07 00:51:46]

On 2015/04/30 02:05:53, haraken wrote:
> A couple of my thoughts:
> 
> - As I commented in the document, I'm still a bit skeptical about the ability
of
> the leak detector in a sense that the leak detector can find leaks but cannot
> find the retainers of the leaks. As far as I've worked on leak detection in
> Blink, the hardest part is not how to find leaks but how to find the retainers
> of the leaks. (In case of Blink, we can easily find leaks just by observing
the
> number of documents because documents are going to leak in most (serious)
leaks
> in Blink.)
> 
> - That being said, the above is just my personal view on leaks in Blink. Given
> that your leak detector has succeeded in finding and fixing leaks in Chromium,
> I'm fine with landing the detector if the detector does not have any negative
> impact on performance.
> 
> - To mitigate the performance concerns, would you provide the following data?
> 
> 1) Explain what instructions are introduced to hot call paths to tcmalloc
> methods.
> 
> 2) Create a super-micro benchmark that will be most affected by the
> instructions. Measure the performance and clarify the worst-case overhead.
> 
> 3) If you observe some regression in 2), measure performance using some
> realistic benchmarks that heavily use tcmalloc. If you don't observe any
> regression in 2), you don't need this step.

haraken: In case it is not clear, this CL will not affect the performance of
production/release builds, as this leak detector will be disabled by default.

I'd like to get this change in so it can be used by Chromium developers. After
it is in, we can continue to work on the performance and memory footprint.
Eventually we'd like to have it enabled in production, but we can work on
improving performance between now (this CL) and then.

BTW, I may have just found another leak using the leak detector running locally.
I will file a bug soon.

[haraken, 2015-05-07 01:11:06]

On 2015/05/07 00:51:46, Simon Que wrote:
> On 2015/04/30 02:05:53, haraken wrote:
> > A couple of my thoughts:
> > 
> > - As I commented in the document, I'm still a bit skeptical about the
ability
> of
> > the leak detector in a sense that the leak detector can find leaks but
cannot
> > find the retainers of the leaks. As far as I've worked on leak detection in
> > Blink, the hardest part is not how to find leaks but how to find the
retainers
> > of the leaks. (In case of Blink, we can easily find leaks just by observing
> the
> > number of documents because documents are going to leak in most (serious)
> leaks
> > in Blink.)
> > 
> > - That being said, the above is just my personal view on leaks in Blink.
Given
> > that your leak detector has succeeded in finding and fixing leaks in
Chromium,
> > I'm fine with landing the detector if the detector does not have any
negative
> > impact on performance.
> > 
> > - To mitigate the performance concerns, would you provide the following
data?
> > 
> > 1) Explain what instructions are introduced to hot call paths to tcmalloc
> > methods.
> > 
> > 2) Create a super-micro benchmark that will be most affected by the
> > instructions. Measure the performance and clarify the worst-case overhead.
> > 
> > 3) If you observe some regression in 2), measure performance using some
> > realistic benchmarks that heavily use tcmalloc. If you don't observe any
> > regression in 2), you don't need this step.
> 
> haraken: In case it is not clear, this CL will not affect the performance of
> production/release builds, as this leak detector will be disabled by default.
> 
> I'd like to get this change in so it can be used by Chromium developers. After
> it is in, we can continue to work on the performance and memory footprint.
> Eventually we'd like to have it enabled in production, but we can work on
> improving performance between now (this CL) and then.

We can work on the performance & memory footprint issue later, but I think it is
important to roughly clarify the implications of the performance & memory
footprint before landing things to trunk even if it's disabled on production
builds at the moment (i.e., it is important to roughly make sure that we can
ship it in a reasonable timeline with this approach before landing complicated
things like this to trunk).

[Simon Que, 2015-05-07 20:33:42]

On 2015/05/07 01:11:06, haraken wrote:
> On 2015/05/07 00:51:46, Simon Que wrote:
> > On 2015/04/30 02:05:53, haraken wrote:
> > > A couple of my thoughts:
> > > 
> > > - As I commented in the document, I'm still a bit skeptical about the
> ability
> > of
> > > the leak detector in a sense that the leak detector can find leaks but
> cannot
> > > find the retainers of the leaks. As far as I've worked on leak detection
in
> > > Blink, the hardest part is not how to find leaks but how to find the
> retainers
> > > of the leaks. (In case of Blink, we can easily find leaks just by
observing
> > the
> > > number of documents because documents are going to leak in most (serious)
> > leaks
> > > in Blink.)
> > > 
> > > - That being said, the above is just my personal view on leaks in Blink.
> Given
> > > that your leak detector has succeeded in finding and fixing leaks in
> Chromium,
> > > I'm fine with landing the detector if the detector does not have any
> negative
> > > impact on performance.
> > > 
> > > - To mitigate the performance concerns, would you provide the following
> data?
> > > 
> > > 1) Explain what instructions are introduced to hot call paths to tcmalloc
> > > methods.
> > > 
> > > 2) Create a super-micro benchmark that will be most affected by the
> > > instructions. Measure the performance and clarify the worst-case overhead.
> > > 
> > > 3) If you observe some regression in 2), measure performance using some
> > > realistic benchmarks that heavily use tcmalloc. If you don't observe any
> > > regression in 2), you don't need this step.
> > 
> > haraken: In case it is not clear, this CL will not affect the performance of
> > production/release builds, as this leak detector will be disabled by
default.
> > 
> > I'd like to get this change in so it can be used by Chromium developers.
After
> > it is in, we can continue to work on the performance and memory footprint.
> > Eventually we'd like to have it enabled in production, but we can work on
> > improving performance between now (this CL) and then.
> 
> We can work on the performance & memory footprint issue later, but I think it
is
> important to roughly clarify the implications of the performance & memory
> footprint before landing things to trunk even if it's disabled on production
> builds at the moment (i.e., it is important to roughly make sure that we can
> ship it in a reasonable timeline with this approach before landing complicated
> things like this to trunk).

That's reasonable. I'm working on some major memory reductions. I'll let you
know when I have it done, and I'll post some numbers.

[bjanakiraman1, 2015-05-12 21:22:17]

On Thu, May 7, 2015 at 1:33 PM, <sque@chromium.org> wrote:

>
>  We can work on the performance & memory footprint issue later, but I
>> think it
>>
> is
>
>> important to roughly clarify the implications of the performance & memory
>> footprint before landing things to trunk even if it's disabled on
>> production
>> builds at the moment (i.e., it is important to roughly make sure that we
>> can
>> ship it in a reasonable timeline with this approach before landing
>> complicated
>> things like this to trunk).
>>
>
> That's reasonable. I'm working on some major memory reductions. I'll let
> you
> know when I have it done, and I'll post some numbers.
>
> https://codereview.chromium.org/986503002/



I think its reasonable to get performance & memory numbers, but I don't see
a need to try to reduce memory footprint as a blocker for getting this
checked in.

This tool has already proved to be useful in detecting memory leaks, the
library is only linked in with a special gyp flag and does not affect
either *dev* or *production* builds (or any builds unless the flag is
used).

We've been discussing this for a while; if we have more objections in
getting this checked in, I'd like to have a meeting and discuss the issues
f2f,

Thanks,
Bhaskar

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2015-05-12 21:44:27]

On 2015/05/12 21:22:17, bjanakiraman1 wrote:
> On Thu, May 7, 2015 at 1:33 PM, <mailto:sque@chromium.org> wrote:
> 
> >
> >  We can work on the performance & memory footprint issue later, but I
> >> think it
> >>
> > is
> >
> >> important to roughly clarify the implications of the performance & memory
> >> footprint before landing things to trunk even if it's disabled on
> >> production
> >> builds at the moment (i.e., it is important to roughly make sure that we
> >> can
> >> ship it in a reasonable timeline with this approach before landing
> >> complicated
> >> things like this to trunk).
> >>
> >
> > That's reasonable. I'm working on some major memory reductions. I'll let
> > you
> > know when I have it done, and I'll post some numbers.
> >
> > https://codereview.chromium.org/986503002/
> 
> 
> 
> I think its reasonable to get performance & memory numbers, but I don't see
> a need to try to reduce memory footprint as a blocker for getting this
> checked in.

Yes, I don't think that we need to fix performance & memory issues before
landing this change.

I just want to understand the performance & memory implications of this change.
If I'm not missing the discussion, we haven't gotten any performance numbers
about this change.

[haraken, 2015-05-12 21:56:47]

On 2015/05/12 21:44:27, haraken wrote:
> On 2015/05/12 21:22:17, bjanakiraman1 wrote:
> > On Thu, May 7, 2015 at 1:33 PM, <mailto:sque@chromium.org> wrote:
> > 
> > >
> > >  We can work on the performance & memory footprint issue later, but I
> > >> think it
> > >>
> > > is
> > >
> > >> important to roughly clarify the implications of the performance & memory
> > >> footprint before landing things to trunk even if it's disabled on
> > >> production
> > >> builds at the moment (i.e., it is important to roughly make sure that we
> > >> can
> > >> ship it in a reasonable timeline with this approach before landing
> > >> complicated
> > >> things like this to trunk).
> > >>
> > >
> > > That's reasonable. I'm working on some major memory reductions. I'll let
> > > you
> > > know when I have it done, and I'll post some numbers.
> > >
> > > https://codereview.chromium.org/986503002/
> > 
> > 
> > 
> > I think its reasonable to get performance & memory numbers, but I don't see
> > a need to try to reduce memory footprint as a blocker for getting this
> > checked in.
> 
> Yes, I don't think that we need to fix performance & memory issues before
> landing this change.
> 
> I just want to understand the performance & memory implications of this
change.
> If I'm not missing the discussion, we haven't gotten any performance numbers
> about this change.

I scanned the slide again.

> No perceptible performance impact with a low sampling rate (1/16).
> Perf profiling reveals that it takes about 3% of active CPU cycles.

What does this mean? With the sampling rate of 1/16, do we hit 3% more CPU
cycles but don't observe the performance impact? Also what benchmark did you
use?

[bjanakiraman1, 2015-05-12 22:08:45]

On Tue, May 12, 2015 at 2:44 PM, <haraken@chromium.org> wrote:

> On 2015/05/12 21:22:17, bjanakiraman1 wrote:
>
>> On Thu, May 7, 2015 at 1:33 PM, <mailto:sque@chromium.org> wrote:
>>
>
>  >
>> >  We can work on the performance & memory footprint issue later, but I
>> >> think it
>> >>
>> > is
>> >
>> >> important to roughly clarify the implications of the performance &
>> memory
>> >> footprint before landing things to trunk even if it's disabled on
>> >> production
>> >> builds at the moment (i.e., it is important to roughly make sure that
>> we
>> >> can
>> >> ship it in a reasonable timeline with this approach before landing
>> >> complicated
>> >> things like this to trunk).
>> >>
>> >
>> > That's reasonable. I'm working on some major memory reductions. I'll let
>> > you
>> > know when I have it done, and I'll post some numbers.
>> >
>> > https://codereview.chromium.org/986503002/
>>
>
>
>
>  I think its reasonable to get performance & memory numbers, but I don't
>> see
>> a need to try to reduce memory footprint as a blocker for getting this
>> checked in.
>>
>
> Yes, I don't think that we need to fix performance & memory issues before
> landing this change.
>
> I just want to understand the performance & memory implications of this
> change.
> If I'm not missing the discussion, we haven't gotten any performance
> numbers
> about this change.
>
>
> https://codereview.chromium.org/986503002/



Yes. Simon said he's going to send performance/memory data that he's
collected.
Bhaskar

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2015-05-12 23:37:06]

On 2015/05/12 22:08:45, bjanakiraman1 wrote:
> On Tue, May 12, 2015 at 2:44 PM, <mailto:haraken@chromium.org> wrote:
> 
> > On 2015/05/12 21:22:17, bjanakiraman1 wrote:
> >
> >> On Thu, May 7, 2015 at 1:33 PM, <mailto:sque@chromium.org> wrote:
> >>
> >
> >  >
> >> >  We can work on the performance & memory footprint issue later, but I
> >> >> think it
> >> >>
> >> > is
> >> >
> >> >> important to roughly clarify the implications of the performance &
> >> memory
> >> >> footprint before landing things to trunk even if it's disabled on
> >> >> production
> >> >> builds at the moment (i.e., it is important to roughly make sure that
> >> we
> >> >> can
> >> >> ship it in a reasonable timeline with this approach before landing
> >> >> complicated
> >> >> things like this to trunk).
> >> >>
> >> >
> >> > That's reasonable. I'm working on some major memory reductions. I'll let
> >> > you
> >> > know when I have it done, and I'll post some numbers.
> >> >
> >> > https://codereview.chromium.org/986503002/
> >>
> >
> >
> >
> >  I think its reasonable to get performance & memory numbers, but I don't
> >> see
> >> a need to try to reduce memory footprint as a blocker for getting this
> >> checked in.
> >>
> >
> > Yes, I don't think that we need to fix performance & memory issues before
> > landing this change.
> >
> > I just want to understand the performance & memory implications of this
> > change.
> > If I'm not missing the discussion, we haven't gotten any performance
> > numbers
> > about this change.
> >
> >
> > https://codereview.chromium.org/986503002/
> 
> 
> 
> Yes. Simon said he's going to send performance/memory data that he's
> collected.
> Bhaskar

BTW, I'm not intending to block your project by requesting lots of
performance/memory data. If there is a reviewer who is more supportive to this
project, I'm happy to defer this to the person.

[Simon Que, 2015-05-13 00:28:22]

On 2015/05/12 23:37:06, haraken wrote:
> On 2015/05/12 22:08:45, bjanakiraman1 wrote:
> > Yes. Simon said he's going to send performance/memory data that he's
> > collected.
> > Bhaskar
> 
> BTW, I'm not intending to block your project by requesting lots of
> performance/memory data. If there is a reviewer who is more supportive to this
> project, I'm happy to defer this to the person.

Here is a spreadsheet with some data I collected:
https://docs.google.com/spreadsheets/d/1GGkIHrDFj0tSpCwQ4Zbogxnh1RrrCX5HX-ah_...

I ran with some local changes to improve performance and memory usage. I will
not be checking these in as part of the CL, as this already has too many patch
sets. But they show the possible performance and memory usage.

Sampling ratio was 1/16.

For a micro-benchmark, I suggest using the number of cycles in leak detector
relative to the number of cycles in new/delete, as measured by perf.

Performance overhead relative to new/delete was 84% for browser process and 66%
for renderer process, after the tab had finished loading. The relative
performance for tcmalloc hooks (part that cannot be reduced by sampling) was 19%
and 25%, respectively. The other performance overhead can be reduced with a
lower sampling ratio.

Heap usage is 1.2 MB for browser process and 320 KB for renderer process, after
running about 15 minutes.

[haraken, 2015-05-13 09:36:59]

On 2015/05/13 00:28:22, Simon Que wrote:
> On 2015/05/12 23:37:06, haraken wrote:
> > On 2015/05/12 22:08:45, bjanakiraman1 wrote:
> > > Yes. Simon said he's going to send performance/memory data that he's
> > > collected.
> > > Bhaskar
> > 
> > BTW, I'm not intending to block your project by requesting lots of
> > performance/memory data. If there is a reviewer who is more supportive to
this
> > project, I'm happy to defer this to the person.
> 
> Here is a spreadsheet with some data I collected:
>
https://docs.google.com/spreadsheets/d/1GGkIHrDFj0tSpCwQ4Zbogxnh1RrrCX5HX-ah_...
> 
> I ran with some local changes to improve performance and memory usage. I will
> not be checking these in as part of the CL, as this already has too many patch
> sets. But they show the possible performance and memory usage.
> 
> Sampling ratio was 1/16.
> 
> For a micro-benchmark, I suggest using the number of cycles in leak detector
> relative to the number of cycles in new/delete, as measured by perf.
> 
> Performance overhead relative to new/delete was 84% for browser process and
66%
> for renderer process, after the tab had finished loading. The relative
> performance for tcmalloc hooks (part that cannot be reduced by sampling) was
19%
> and 25%, respectively. The other performance overhead can be reduced with a
> lower sampling ratio.

Thanks for the detailed investigations!

I just want to confirm but does this mean that the added hooks are going to
regress performance of each allocation by 19% or 25% even if the sampling rate
is low enough? If that's the case, the overhead is too large. I don't think you
need to make it 0% at the moment (because it isn't yet enabled on trunk) but I
might want to see a performance number that convinces me that we'll be able to
enable the feature in a reasonable timeline.

That said, I'm not an expert of tcmalloc or familiar with use cases in Chromium
OS (I'm doing this review just because no one was willing to review this CL). I
don't want to block your project on my (probably negative) reviews, so feel free
to find another reviewer who is more supportive to the project. (Though you'll
need to get one LG on the Intent-to-Implement thread in chromium-dev.)

[bccheng, 2015-05-13 13:53:15]

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

[bccheng, 2015-05-13 13:53:17]

Thanks for the time and effort of collecting the data and reviewing the code.

Using the browser process as an example, if I understand the numbers correctly
they mean the current implementation of the leak detector increases the overall
CPU usage by 8.82%. Since the cycles spent on new/delete/malloc/free are 10.52%,
it can be considered that each heap allocation/free is increased by 83.82% (ie
8.82/10.52). Out of the 83.82%, 18.79% is from the existing "hook"
implementation in tcmalloc and is orthogonal to the sampling hash function.

I think we should focus on the 8.82% overhead number as it has better
correlation to the overall performance degradation. Going forward we can try the
deprecated version of the hook traversal function whose overhead should be
lower, as well as cheaper hash function implementations.

[bjanakiraman1, 2015-05-13 16:30:41]

So, I'm looking at the overhead, and yeah, its really high, its not clear
we'll be able to get this down without a lot of work.

On the other hand, this detector has proved that it can detect long lived
leaks in Chrome that other tools cannot find, and has a legitimate case for
a buildbot similar to the LSAN one.

How about we implement it similar to LSAN, ie, for ChromiumOS build,
replace tcmalloc with a new allocator (tcmalloc+detetctor)? It would be
completely off the critical path for Chrome. Another advantage is that we'd
be free to upgrade tcmalloc to the latest version, and perhaps work on
reducing the malloc hook overhead or experimenting with different
allocators?


On Wed, May 13, 2015 at 6:53 AM, <bccheng@chromium.org> wrote:

> Thanks for the time and effort of collecting the data and reviewing the
> code.
>
> Using the browser process as an example, if I understand the numbers
> correctly
> they mean the current implementation of the leak detector increases the
> overall
> CPU usage by 8.82%. Since the cycles spent on new/delete/malloc/free are
> 10.52%,
> it can be considered that each heap allocation/free is increased by 83.82%
> (ie
> 8.82/10.52). Out of the 83.82%, 18.79% is from the existing "hook"
> implementation in tcmalloc and is orthogonal to the sampling hash function.
>
> I think we should focus on the 8.82% overhead number as it has better
> correlation to the overall performance degradation. Going forward we can
> try the
> deprecated version of the hook traversal function whose overhead should be
> lower, as well as cheaper hash function implementations.
>
> https://codereview.chromium.org/986503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-05-13 17:37:59]

I got some new numbers, this time running with a sampling rate of 1/256 and with
only hooks. The previous numbers had more going on in the hook functions than
just sampling, so they should not be considered an accurate reflection of the
minimum overhead.

The spreadsheet now has new sheets for the new data.

For 1/256 sampling, the browser/renderer process relative overhead was 12.1% and
12.5%, respectively.
For hooks only, the browser/renderer process relative overhead was 12.8% and
11.2%, respectively.

So we're looking at a relative overhead of around 12%.

These numbers are already obtained with deprecated hooks, which do not
contribute any measurable overhead.

[haraken, 2015-05-14 00:33:37]

> So, I'm looking at the overhead, and yeah, its really high, its not clear
> we'll be able to get this down without a lot of work.
> 
> On the other hand, this detector has proved that it can detect long lived
> leaks in Chrome that other tools cannot find, and has a legitimate case for
> a buildbot similar to the LSAN one.
> 
> How about we implement it similar to LSAN, ie, for ChromiumOS build,
> replace tcmalloc with a new allocator (tcmalloc+detetctor)? It would be
> completely off the critical path for Chrome. Another advantage is that we'd
> be free to upgrade tcmalloc to the latest version, and perhaps work on
> reducing the malloc hook overhead or experimenting with different
> allocators?

This sounds like a reasonable way to go :9

[gpike, 2015-06-11 21:01:09]

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

[gpike, 2015-06-11 21:01:11]

A quick note; I'll have more comments later today.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/crc32.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/crc32.h:14: // Returns a CRC-32 hash of |len|
bytes of data referenced by |data|.
CRCs are OK for error detection, but you want a general purpose hash function
instead. Please add FarmHash (to third_party/farmhash?) and use that.  Other
places in Chrome would likely benefit from FarmHash as well.

[gpike, 2015-06-11 22:04:15]

One style question: For pointers are you going to stick to "if (x)" or "if (x !=
NULL)" or "if (x != 0)"? I see a mix.

https://codereview.chromium.org/986503002/diff/600001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/986503002/diff/600001/content/browser/browser...
content/browser/browser_main_loop.cc:591: #if defined (ENABLE_LEAK_DETECTOR)
Add comment explaining this.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.h:21: // the number of
allocations from that call stack.
It must be more than that or you would be using hash_map<> or something like
that. Please expand the comment and explain why existing hash map types are not
sufficient.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:227: if (suspected->valid &&
suspected->value == entries[i].value) {
Please make this more obvious (if-then-else-if) instead of relying on tricky
control-flow and the invariant that an invalid SuspicionEntry must have a score
of 0.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:263: int ranking_size_;
const

(This also applies to some other fields.)

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:282: // List of allocated
values that passed the suspicion threshold and are being
List?

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:86: // Keep track of the
total number of bytes allocated.
Add comments that explain locking philosophy. Are globals all supposed to be
read/written only with heap_lock held?

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:105: inline unsigned int
PointerToHash(const void* ptr) {
Replace with something stronger.  E.g., multiply by a constant and return the
top 8 bits of the result.  The constant should be a random-looking odd number
with relatively few long strings of 0s or 1s in its binary representation.  The
FarmHash source code contains some constants that would be fine.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:113: unsigned int key =
PointerToHash(ptr) & 0xff;
This is reasonable if PointerToHash does a good job of hashing a pointer, but
that is currently not the case.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:123: if (!ptr ||
!ShouldSample(ptr))
Performance should improve if you first check ShouldSample(ptr).

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:129: if
(leak_detector->ShouldGetStackTraceForSize(size)) {
If leak_dectector is NULL here you crash. Later you check if leak_dectector is
NULL. This looks like a bug.

[Simon Que, 2015-06-12 22:18:55]

https://codereview.chromium.org/986503002/diff/600001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/986503002/diff/600001/content/browser/browser...
content/browser/browser_main_loop.cc:591: #if defined (ENABLE_LEAK_DETECTOR)
On 2015/06/11 22:04:16, gpike wrote:
> Add comment explaining this.

This is to make sure LeakDetector is linked in. I can explain it with a comment
but I'm not sure if it is the right way to do this. Seems very hacky.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.h:21: // the number of
allocations from that call stack.
On 2015/06/11 22:04:16, gpike wrote:
> It must be more than that or you would be using hash_map<> or something like
> that. Please expand the comment and explain why existing hash map types are
not
> sufficient.

Done.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/crc32.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/crc32.h:14: // Returns a CRC-32 hash of |len|
bytes of data referenced by |data|.
On 2015/06/11 21:01:11, gpike wrote:
> CRCs are OK for error detection, but you want a general purpose hash function
> instead. Please add FarmHash (to third_party/farmhash?) and use that.  Other
> places in Chrome would likely benefit from FarmHash as well.

I'll look into that. However, I think it should be added locally to tcmalloc as
well. I don't see other instances of tcmalloc code including stuff from outside
of it. But it may just be a convention due to it being forked from upstream,
rather than a hard technical requirement.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:227: if (suspected->valid &&
suspected->value == entries[i].value) {
On 2015/06/11 22:04:16, gpike wrote:
> Please make this more obvious (if-then-else-if) instead of relying on tricky
> control-flow and the invariant that an invalid SuspicionEntry must have a
score
> of 0.

Done.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:263: int ranking_size_;
On 2015/06/11 22:04:16, gpike wrote:
> const
> 
> (This also applies to some other fields.)

Done.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:282: // List of allocated
values that passed the suspicion threshold and are being
On 2015/06/11 22:04:16, gpike wrote:
> List?

Done.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:86: // Keep track of the
total number of bytes allocated.
On 2015/06/11 22:04:17, gpike wrote:
> Add comments that explain locking philosophy. Are globals all supposed to be
> read/written only with heap_lock held?

I'm not actually sure what the philosophy is. I copied this behavior over from
heap-profiler. I did send an email to the gperftools mailing list asking about
why the locks are needed but never got a response.

I did run it without the lock and it crashed.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:105: inline unsigned int
PointerToHash(const void* ptr) {
On 2015/06/11 22:04:16, gpike wrote:
> Replace with something stronger.  E.g., multiply by a constant and return the
> top 8 bits of the result.  The constant should be a random-looking odd number
> with relatively few long strings of 0s or 1s in its binary representation. 
The
> FarmHash source code contains some constants that would be fine.

I'll look into that while using farmhash.

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:123: if (!ptr ||
!ShouldSample(ptr))
On 2015/06/11 22:04:17, gpike wrote:
> Performance should improve if you first check ShouldSample(ptr).

Done. Can you explain why?

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:129: if
(leak_detector->ShouldGetStackTraceForSize(size)) {
On 2015/06/11 22:04:16, gpike wrote:
> If leak_dectector is NULL here you crash. Later you check if leak_dectector is
> NULL. This looks like a bug.

Done.

[Simon Que, 2015-06-12 23:14:08]

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/crc32.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/crc32.h:14: // Returns a CRC-32 hash of |len|
bytes of data referenced by |data|.
On 2015/06/12 22:18:55, Simon Que wrote:
> On 2015/06/11 21:01:11, gpike wrote:
> > CRCs are OK for error detection, but you want a general purpose hash
function
> > instead. Please add FarmHash (to third_party/farmhash?) and use that.  Other
> > places in Chrome would likely benefit from FarmHash as well.
> 
> I'll look into that. However, I think it should be added locally to tcmalloc
as
> well. I don't see other instances of tcmalloc code including stuff from
outside
> of it. But it may just be a convention due to it being forked from upstream,
> rather than a hard technical requirement.

Done.

[gpike, 2015-06-16 20:35:57]

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types because they
will call new/delete. Instead, this class takes a custom
I thought STL hash_map and std::unordered_* allow you to specify an allocator.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:86: // Keep track of the
total number of bytes allocated.
This (and perhaps other globals) are modified racily. Let's not do that. Please
start with a simple-and-correct implementation, especially since the code is not
going to slow down Chrome users in the field (yet...).  Improving the speed can
be a later step.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:104: // Convert a pointer to
a fash hash value.
fash

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector_impl.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector_impl.h:92: // in the same slot
as a linked list.
Please explain in the comment why this is done, instead of using an existing
library container type or implementing your own double-in-size-as-needed scheme.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/ranked_list.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:79: // The current number of
items int he list.
int he -> in the

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:142: // Don't add it if it's
smaller than the min size and the list is full.
"smaller than the min size" is not quite right. You are sorting by count, not
size.

[Simon Que, 2015-06-16 20:55:16]

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types because they
will call new/delete. Instead, this class takes a custom
On 2015/06/16 20:35:57, gpike wrote:
> I thought STL hash_map and std::unordered_* allow you to specify an allocator.

I had tried that a while back and didn't get it to work, so I went with this
solution.

I'll give it another shot.

[Simon Que, 2015-06-16 21:09:21]

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:86: // Keep track of the
total number of bytes allocated.
On 2015/06/16 20:35:57, gpike wrote:
> This (and perhaps other globals) are modified racily. Let's not do that.
Please
> start with a simple-and-correct implementation, especially since the code is
not
> going to slow down Chrome users in the field (yet...).  Improving the speed
can
> be a later step.

I've enclosed the incrementing of |total_alloc_size| in a spinlock. Is there
anything else that needs to be done?

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:104: // Convert a pointer to
a fash hash value.
On 2015/06/16 20:35:57, gpike wrote:
> fash

Done.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/ranked_list.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:79: // The current number of
items int he list.
On 2015/06/16 20:35:57, gpike wrote:
> int he -> in the

Done.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:142: // Don't add it if it's
smaller than the min size and the list is full.
On 2015/06/16 20:35:57, gpike wrote:
> "smaller than the min size" is not quite right. You are sorting by count, not
> size.

Done.

[Simon Que, 2015-06-16 21:09:22]

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:86: // Keep track of the
total number of bytes allocated.
On 2015/06/16 20:35:57, gpike wrote:
> This (and perhaps other globals) are modified racily. Let's not do that.
Please
> start with a simple-and-correct implementation, especially since the code is
not
> going to slow down Chrome users in the field (yet...).  Improving the speed
can
> be a later step.

I've enclosed the incrementing of |total_alloc_size| in a spinlock. Is there
anything else that needs to be done?

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:104: // Convert a pointer to
a fash hash value.
On 2015/06/16 20:35:57, gpike wrote:
> fash

Done.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/ranked_list.h (right):

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:79: // The current number of
items int he list.
On 2015/06/16 20:35:57, gpike wrote:
> int he -> in the

Done.

https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:142: // Don't add it if it's
smaller than the min size and the list is full.
On 2015/06/16 20:35:57, gpike wrote:
> "smaller than the min size" is not quite right. You are sorting by count, not
> size.

Done.

[Simon Que, 2015-06-16 21:45:03]

On 2015/06/16 20:55:16, Simon Que wrote:
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> File third_party/tcmalloc/chromium/src/call_stack_table.h (right):
> 
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types because they
> will call new/delete. Instead, this class takes a custom
> On 2015/06/16 20:35:57, gpike wrote:
> > I thought STL hash_map and std::unordered_* allow you to specify an
allocator.
> 
> I had tried that a while back and didn't get it to work, so I went with this
> solution.
> 
> I'll give it another shot.

The syntax of providing a custom hash/equal_to/allocator isn't very transparent
or well-documented. Can you provide a link to an example of how this can be
done?

[gpike, 2015-06-16 21:56:20]

On 2015/06/16 21:45:03, Simon Que wrote:
> On 2015/06/16 20:55:16, Simon Que wrote:
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > File third_party/tcmalloc/chromium/src/call_stack_table.h (right):
> > 
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types because
they
> > will call new/delete. Instead, this class takes a custom
> > On 2015/06/16 20:35:57, gpike wrote:
> > > I thought STL hash_map and std::unordered_* allow you to specify an
> allocator.
> > 
> > I had tried that a while back and didn't get it to work, so I went with this
> > solution.
> > 
> > I'll give it another shot.
> 
> The syntax of providing a custom hash/equal_to/allocator isn't very
transparent
> or well-documented. Can you provide a link to an example of how this can be
> done?

http://docs.roguewave.com/sourcepro/11.1/html/toolsug/11-6.html

[Simon Que, 2015-06-17 22:59:19]

On 2015/06/16 21:56:20, gpike wrote:
> On 2015/06/16 21:45:03, Simon Que wrote:
> > On 2015/06/16 20:55:16, Simon Que wrote:
> > >
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > > File third_party/tcmalloc/chromium/src/call_stack_table.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > > third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types because
> they
> > > will call new/delete. Instead, this class takes a custom
> > > On 2015/06/16 20:35:57, gpike wrote:
> > > > I thought STL hash_map and std::unordered_* allow you to specify an
> > allocator.
> > > 
> > > I had tried that a while back and didn't get it to work, so I went with
this
> > > solution.
> > > 
> > > I'll give it another shot.
> > 
> > The syntax of providing a custom hash/equal_to/allocator isn't very
> transparent
> > or well-documented. Can you provide a link to an example of how this can be
> > done?
> 
> http://docs.roguewave.com/sourcepro/11.1/html/toolsug/11-6.html

Thanks. I've implemented it, along with a few other minor changes. See patch set
description.

I haven't tested this for correctness since that can take a long time. I'll let
you do another review first.

[Simon Que, 2015-06-18 00:54:56]

On 2015/06/17 22:59:19, Simon Que wrote:
> On 2015/06/16 21:56:20, gpike wrote:
> > On 2015/06/16 21:45:03, Simon Que wrote:
> > > On 2015/06/16 20:55:16, Simon Que wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > > > File third_party/tcmalloc/chromium/src/call_stack_table.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/986503002/diff/640001/third_party/tcmalloc/ch...
> > > > third_party/tcmalloc/chromium/src/call_stack_table.h:22: // types
because
> > they
> > > > will call new/delete. Instead, this class takes a custom
> > > > On 2015/06/16 20:35:57, gpike wrote:
> > > > > I thought STL hash_map and std::unordered_* allow you to specify an
> > > allocator.
> > > > 
> > > > I had tried that a while back and didn't get it to work, so I went with
> this
> > > > solution.
> > > > 
> > > > I'll give it another shot.
> > > 
> > > The syntax of providing a custom hash/equal_to/allocator isn't very
> > transparent
> > > or well-documented. Can you provide a link to an example of how this can
be
> > > done?
> > 
> > http://docs.roguewave.com/sourcepro/11.1/html/toolsug/11-6.html
> 
> Thanks. I've implemented it, along with a few other minor changes. See patch
set
> description.
> 
> I haven't tested this for correctness since that can take a long time. I'll
let
> you do another review first.

There is also another hash table in leak_detector_impl.cc. That is copied over
from heap-profiler-table.cc. Not sure if it would be worth it to replace it with
std::unordered_map, as it would require another Alloc class to be used by
STL_Allocator. We already have CallStackTable::Alloc. It would be a redundant
definition if we were to create LeakDetectorImpl::Alloc.

Geoff, do you have any suggestions?

[Simon Que, 2015-06-18 22:53:46]

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:227: if (suspected->valid &&
suspected->value == entries[i].value) {
On 2015/06/12 22:18:55, Simon Que wrote:
> On 2015/06/11 22:04:16, gpike wrote:
> > Please make this more obvious (if-then-else-if) instead of relying on tricky
> > control-flow and the invariant that an invalid SuspicionEntry must have a
> score
> > of 0.
> 
> Done.

I got this wrong. It's actually setting the |suspected->valid| flag in the first
if() block. The second if() block will execute if the first if block executes,
but will also execute if there is another slot that is already valid and has a
matching value.

This is somewhat confusing so I will add a comment to better explain this. Feel
free to suggest an alternative way of constructing it to look less confusing.

[gpike, 2015-07-01 21:38:21]

Do you need to add farmhash.* as well?

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/600001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:227: if (suspected->valid &&
suspected->value == entries[i].value) {
On 2015/06/18 22:53:46, Simon Que wrote:
> On 2015/06/12 22:18:55, Simon Que wrote:
> > On 2015/06/11 22:04:16, gpike wrote:
> > > Please make this more obvious (if-then-else-if) instead of relying on
tricky
> > > control-flow and the invariant that an invalid SuspicionEntry must have a
> > score
> > > of 0.
> > 
> > Done.
> 
> I got this wrong. It's actually setting the |suspected->valid| flag in the
first
> if() block. The second if() block will execute if the first if block executes,
> but will also execute if there is another slot that is already valid and has a
> matching value.
> 
> This is somewhat confusing so I will add a comment to better explain this.
Feel
> free to suggest an alternative way of constructing it to look less confusing.

How about:
if (suspected->valid && suspected->value == entries[i].value) {
  ...
  break;
} else if (!suspected->valid) {
  ...
  break;
}

A small amount of code duplication in the cases is fine; the compiler can "undo"
that for you in the generated code, so it shouldn't affect performance.

https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.cc:135: if (free)
why free?

[Simon Que, 2015-07-10 00:09:06]

Added farmhash code.

https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/call_stack_table.cc:135: if (free)
On 2015/07/01 21:38:21, gpike wrote:
> why free?

Meant dealloc_.

[Simon Que, 2015-07-10 22:50:08]

On 2015/07/01 21:38:21, gpike wrote:
> How about:
> if (suspected->valid && suspected->value == entries[i].value) {
>   ...
>   break;
> } else if (!suspected->valid) {
>   ...
>   break;
> }
> 
> A small amount of code duplication in the cases is fine; the compiler can
"undo"
> that for you in the generated code, so it shouldn't affect performance.
> 
>
https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
> File third_party/tcmalloc/chromium/src/call_stack_table.cc (right):
> 
>
https://codereview.chromium.org/986503002/diff/720001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/call_stack_table.cc:135: if (free)
> why free?

This is no longer relevant. The underlying array has been replaced with an STL
map.

[Simon Que, 2015-07-10 22:53:26]

FTR, here's some more performance numbers, this time from data collected from
the field through CWP. The spreadsheet shows the total overhead of tcmalloc in
Chrome OS:
https://drive.google.com/corp/drive/u/0/folders/0BzPK_1nFytaUfkhpck9UUGRWUjc3...

tcmalloc takes up 2.5-3% of CPU cycles on Chrome OS machines in the field. Using
an observed ratio of leak detector overhead to new/delete overhead of 10%, we
can expect an overhead of 0.25-0.3% in the field. This is several times lower
than what I observed from running locally (see earlier discussion in this CL).

[Simon Que, 2015-07-11 01:41:14]

Patchset #41 (id:820001) has been deleted

[Simon Que, 2015-07-11 01:41:56]

Patchset #40 (id:800001) has been deleted

[gpike, 2015-07-14 19:16:08]

lgtm

I still would like to see more comments in leak_detector.cc explaining things
like what vars are protected by the lock and what functions should only be
invoked with the lock held.

Overall it looks good though.

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:85: uint64
last_free_dump_size = 0;
unused?

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:88: // have been allocated
since the last time this function was called.
this function was called -> that was done

[gpike, 2015-07-14 19:16:13]

lgtm

I still would like to see more comments in leak_detector.cc explaining things
like what vars are protected by the lock and what functions should only be
invoked with the lock held.

Overall it looks good though.

[Simon Que, 2015-07-14 23:31:20]

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_detector.cc (right):

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:85: uint64
last_free_dump_size = 0;
On 2015/07/14 19:16:08, gpike wrote:
> unused?

Done.

https://codereview.chromium.org/986503002/diff/860001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_detector.cc:88: // have been allocated
since the last time this function was called.
On 2015/07/14 19:16:08, gpike wrote:
> this function was called -> that was done

Done.

[Simon Que, 2015-07-14 23:37:36]

PTAL

https://codereview.chromium.org/986503002/diff/880001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/986503002/diff/880001/content/browser/browser...
content/browser/browser_main_loop.cc:643: LeakDetectorDummy();
I still find this to be really weird. It'd be good if there were another
alternative.

This is necessary right now because there are no other calls from the rest of
Chrome into the leak detector. This call forces the leak detector to be linked
into Chrome.

However, in the future this will not be necessary, once we have the CWP code in
Chrome calling the leak detector to retrieve leak reports.

I am thinking that this should be marked with a TODO for that goal.

[Simon Que, 2015-07-14 23:44:36]

https://codereview.chromium.org/986503002/diff/880001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/986503002/diff/880001/content/browser/browser...
content/browser/browser_main_loop.cc:643: LeakDetectorDummy();
On 2015/07/14 23:37:36, Simon Que wrote:
> I still find this to be really weird. It'd be good if there were another
> alternative.
> 
> This is necessary right now because there are no other calls from the rest of
> Chrome into the leak detector. This call forces the leak detector to be linked
> into Chrome.
> 
> However, in the future this will not be necessary, once we have the CWP code
in
> Chrome calling the leak detector to retrieve leak reports.
> 
> I am thinking that this should be marked with a TODO for that goal.

Alternatively I could put this in some code that we own. That would avoid the
potential headache of getting an owner of browser_main_loop.cc to approve this.

[Simon Que, 2015-07-16 18:47:51]

Patchset #43 (id:900001) has been deleted

[Nico, 2015-07-16 19:31:22]

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

[Nico, 2015-07-16 19:31:24]

build/ lgtm

https://codereview.chromium.org/986503002/diff/920001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/986503002/diff/920001/build/common.gypi#newco...
build/common.gypi:3827: '-g',
Including full debug symbols in all chromeos builds seems…ambitious to me. I
guess this is off by default so it's ok?

[Will Harris, 2015-07-16 19:39:38]

On 2015/03/23 19:18:54, Simon Que wrote:
> On 2015/03/23 04:34:39, haraken wrote:
> > Do you have a design document that describes what the leak detector is doing
> at
> > a high level?
> 
> Here is one I wrote a few months ago. The design is mostly the same.
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> 
> However, I am working on a new presentation that directly describes the
current
> implementation.

can you make this design doc available to @chromium.org users (or even better,
public)?

[Nico, 2015-07-16 19:49:09]

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:121: #if
defined(ENABLE_PROFILING) || defined(ENABLE_LEAK_DETECTOR)
This too means you can never ship this to users – enabling these flags causes
static initializers, and we don't allow static initializers in the shipping
product

[Simon Que, 2015-07-16 19:52:19]

On 2015/07/16 19:39:38, Will Harris wrote:
> On 2015/03/23 19:18:54, Simon Que wrote:
> > On 2015/03/23 04:34:39, haraken wrote:
> > > Do you have a design document that describes what the leak detector is
doing
> > at
> > > a high level?
> > 
> > Here is one I wrote a few months ago. The design is mostly the same.
> >
>
https://docs.google.com/document/d/1Ps5yTwO8wdUwdBK747yVQhO3D2W2XN0BtZ-8nBpJk...
> > 
> > However, I am working on a new presentation that directly describes the
> current
> > implementation.
> 
> can you make this design doc available to @chromium.org users (or even better,
> public)?

I made a public copy of it under my chromium.org account:
https://docs.google.com/document/d/1Rq7NCQ4IaMhT5jO3Z1nsq1tZ1YAC6NR7X2MWsEVqZDA/

[jar (doing other things), 2015-07-16 20:59:46]

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

[jar (doing other things), 2015-07-16 20:59:49]

Is this based on an upstream change?... or is this a novel addition to our
branch?

[Simon Que, 2015-07-16 21:13:57]

On 2015/07/16 20:59:49, jar (doing other things) wrote:
> Is this based on an upstream change?... or is this a novel addition to our
> branch?

It's a new addition to our branch.

[Simon Que, 2015-07-16 21:14:03]

On 2015/07/16 20:59:49, jar (doing other things) wrote:
> Is this based on an upstream change?... or is this a novel addition to our
> branch?

It's a new addition to our branch.

[Simon Que, 2015-07-16 21:15:09]

https://codereview.chromium.org/986503002/diff/920001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/986503002/diff/920001/build/common.gypi#newco...
build/common.gypi:3827: '-g',
On 2015/07/16 19:31:24, Nico wrote:
> Including full debug symbols in all chromeos builds seems…ambitious to me. I
> guess this is off by default so it's ok?

The '-g' isn't necessary; I'm getting rid of it.

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:121: #if
defined(ENABLE_PROFILING) || defined(ENABLE_LEAK_DETECTOR)
On 2015/07/16 19:49:09, Nico wrote:
> This too means you can never ship this to users – enabling these flags
causes
> static initializers, and we don't allow static initializers in the shipping
> product

Then the problem may extend beyond this file. I am using
REGISTER_MODULE_INITIALIZER() in leak_detector.cc ... is that a problem too?

In any case, the current CL is not for the shipping product. We will eventually
add more integration with Chrome. At that time, we can provide these flags
through Chrome (along with other parts of initialization), without having to
read environment variables or create static initializers.

[Nico, 2015-07-16 21:25:17]

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/base/commandlineflags.h (right):

https://codereview.chromium.org/986503002/diff/920001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/base/commandlineflags.h:121: #if
defined(ENABLE_PROFILING) || defined(ENABLE_LEAK_DETECTOR)
On 2015/07/16 21:15:08, Simon Que wrote:
> On 2015/07/16 19:49:09, Nico wrote:
> > This too means you can never ship this to users – enabling these flags
> causes
> > static initializers, and we don't allow static initializers in the shipping
> > product
> 
> Then the problem may extend beyond this file. I am using
> REGISTER_MODULE_INITIALIZER() in leak_detector.cc ... is that a problem too?

Yes.
 
> In any case, the current CL is not for the shipping product. We will
eventually
> add more integration with Chrome. At that time, we can provide these flags
> through Chrome (along with other parts of initialization), without having to
> read environment variables or create static initializers.

Makes sense :-)

[Simon Que, 2015-07-20 19:49:23]

wfh and jar:

I'd like to get this CL in soon, as it is four months old. Let me know what
further action or explanation you need from me.

[jar (doing other things), 2015-07-20 22:19:03]

That fact that this code is off by default, and will only be turned on in
developer builds, is very good.  That makes it very tempting to approve,
although I may be a bit of a rubber stamp.

I'm hopeful it does not slow builds.  Can you comment?

I don't have the time to do a detailed walk-through review, but there are a few
comments below.

I'd also really like to find some folks to be TCMalloc owners... so maybe you
are also up for that job... or folks on the current reviewer list can chime in.

https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:46: LeakAnalyzer(int
ranking_size, int num_suspicions_threshold,
Is this a fork of external code?  I not, I think that Chromium code style
suggests one arg per line (in definition and declaration) when you can't fit all
args on one line.

https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/leak_analyzer.h:83: int size_remaining =
buffer_size;
why is this giant routine placed in a header, and not in a cc file?

https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
File third_party/tcmalloc/chromium/src/ranked_list.h (right):

https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
third_party/tcmalloc/chromium/src/ranked_list.h:84: int
RankedList<T>::GetInsertionIndex(int count) const {
I'm again a bit surprised to see larger code blocks (even templated) in headers.
 Is this perf critical code... used in very few places... so inlining is needed?

[Simon Que, 2015-07-20 22:41:35]

On 2015/07/20 22:19:03, jar (doing other things) wrote:
> That fact that this code is off by default, and will only be turned on in
> developer builds, is very good.  That makes it very tempting to approve,
> although I may be a bit of a rubber stamp.
> 
> I'm hopeful it does not slow builds.  Can you comment?

See allocator.gyp. I've added a "sources!" section to exclude these new source
files if the leak_detector flag is not set.

> I don't have the time to do a detailed walk-through review, but there are a
few
> comments below.
> 
> I'd also really like to find some folks to be TCMalloc owners... so maybe you
> are also up for that job... or folks on the current reviewer list can chime
in.

Yes, there was some talk about it at a summit a few months ago. I got the
impression that no one really owns it. I'm willing to consider being an owner.

>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):
> 
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/leak_analyzer.h:46: LeakAnalyzer(int
> ranking_size, int num_suspicions_threshold,
> Is this a fork of external code?  I not, I think that Chromium code style
> suggests one arg per line (in definition and declaration) when you can't fit
all
> args on one line.

I'll fix it.

> 
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/leak_analyzer.h:83: int size_remaining =
> buffer_size;
> why is this giant routine placed in a header, and not in a cc file?
> 
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> File third_party/tcmalloc/chromium/src/ranked_list.h (right):
> 
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> third_party/tcmalloc/chromium/src/ranked_list.h:84: int
> RankedList<T>::GetInsertionIndex(int count) const {
> I'm again a bit surprised to see larger code blocks (even templated) in
headers.
>  Is this perf critical code... used in very few places... so inlining is
needed?

The only reason for those two big inline classes is because of templating. There
is no performance issue.

Can you suggest a way to do these templated classes without inlining them in the
header? The inlining is done because I use them for two types: uint32_t (size of
allocation) and HeapProfileBucket (call stack).

[Simon Que, 2015-07-23 01:02:43]

On 2015/07/20 22:41:35, Simon Que wrote:
> On 2015/07/20 22:19:03, jar (doing other things) wrote:
> > That fact that this code is off by default, and will only be turned on in
> > developer builds, is very good.  That makes it very tempting to approve,
> > although I may be a bit of a rubber stamp.
> > 
> > I'm hopeful it does not slow builds.  Can you comment?
> 
> See allocator.gyp. I've added a "sources!" section to exclude these new source
> files if the leak_detector flag is not set.
> 
> > I don't have the time to do a detailed walk-through review, but there are a
> few
> > comments below.
> > 
> > I'd also really like to find some folks to be TCMalloc owners... so maybe
you
> > are also up for that job... or folks on the current reviewer list can chime
> in.
> 
> Yes, there was some talk about it at a summit a few months ago. I got the
> impression that no one really owns it. I'm willing to consider being an owner.
> 
> >
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> > File third_party/tcmalloc/chromium/src/leak_analyzer.h (right):
> > 
> >
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> > third_party/tcmalloc/chromium/src/leak_analyzer.h:46: LeakAnalyzer(int
> > ranking_size, int num_suspicions_threshold,
> > Is this a fork of external code?  I not, I think that Chromium code style
> > suggests one arg per line (in definition and declaration) when you can't fit
> all
> > args on one line.
> 
> I'll fix it.
> 
> > 
> >
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> > third_party/tcmalloc/chromium/src/leak_analyzer.h:83: int size_remaining =
> > buffer_size;
> > why is this giant routine placed in a header, and not in a cc file?
> > 
> >
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> > File third_party/tcmalloc/chromium/src/ranked_list.h (right):
> > 
> >
>
https://codereview.chromium.org/986503002/diff/960001/third_party/tcmalloc/ch...
> > third_party/tcmalloc/chromium/src/ranked_list.h:84: int
> > RankedList<T>::GetInsertionIndex(int count) const {
> > I'm again a bit surprised to see larger code blocks (even templated) in
> headers.
> >  Is this perf critical code... used in very few places... so inlining is
> needed?
> 
> The only reason for those two big inline classes is because of templating.
There
> is no performance issue.
> 
> Can you suggest a way to do these templated classes without inlining them in
the
> header? The inlining is done because I use them for two types: uint32_t (size
of
> allocation) and HeapProfileBucket (call stack).

I've removed templates from the inlined functions and am instead using an
alternative method. Jim, what do you think? See leak_detector_value_type.h,
which encapsulates the two possible types.

[jar (doing other things), 2015-07-28 06:17:52]

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/farmhash.cc (right):

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:76: #if
defined(FARMHASH_NO_BUILTIN_EXPECT) ||
(defined(FARMHASH_OPTIONAL_BUILTIN_EXPECT) && !defined(HAVE_BUILTIN_EXPECT))
nit: Probably use a backslash to get this to wrap at 80 columns... and still be
parsed correctly.  Probably indent the "second line" by 4 from the left edge.

You might even put the backslash just a character space after the "||" and then
I *think* the second line will just fit ;-).

You already did this sort of indent down around line 105.

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:568: uint64_t
Hash64WithSeeds(const char *s, size_t len, uint64_t seed0, uint64_t seed1);
nit: 80 characters.

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:574: uint64_t
Hash64WithSeeds(const char *s, size_t len, uint64_t seed0, uint64_t seed1) {
nit: 80 characters.

If possible, please consider the Chromium style of one arg per line in a decl of
definition, when they don't all fit on a line.

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:676: w =
farmhashna::WeakHashLen32WithSeeds(s + 32, z + w.second, y + Fetch(s + 16));
nit: 80 characters

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:750: uint64_t
Hash64WithSeeds(const char *s, size_t len, uint64_t seed0, uint64_t seed1) {
nit: 80 chars

https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/farmhash.cc:791: STATIC_INLINE __m128i
Mul(__m128i x, __m128i y) { return _mm_mullo_epi32(x, y); }
nit: 80 chars here and later in file.

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/leak_detector_value_type.h:75: const
CallStack* call_stack_;
Better would be a UniquePtr.

Does this class leak?  Where is the destructor that would have freed this
member?

(UniquePtr would fix that)

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/leak_detector_value_type.h:76: };
Are these menat to be copyable and assignable??  If not... standard macro...

[Simon Que, 2015-07-28 06:45:26]

On 2015/07/28 06:17:52, jar (doing other things) wrote:
>
https://codereview.chromium.org/986503002/diff/1020001/third_party/tcmalloc/c...
> File third_party/tcmalloc/chromium/src/farmhash.cc (right):

Good to see you back in the saddle. I'm willing to make the corrections but keep
in mind farmhash comes from an external source. Do your comments still hold?

[Simon Que, 2015-07-28 18:10:56]

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/leak_detector_value_type.h:75: const
CallStack* call_stack_;
On 2015/07/28 06:17:51, jar (doing other things) wrote:
> Better would be a UniquePtr.
> 
> Does this class leak?  Where is the destructor that would have freed this
> member?
> 
> (UniquePtr would fix that)

This is not owned by LeakDetectorValueType.

LeakDetectorImpl owns all existing CallStack objects, stored in an
unordered_set. Pointers to these objects are passed to other classes as
references to unique call stacks.

Is there a container that would be appropriate for this? e.g. some ref-counted
ptr.

https://codereview.chromium.org/986503002/diff/1040001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/leak_detector_value_type.h:76: };
On 2015/07/28 06:17:51, jar (doing other things) wrote:
> Are these menat to be copyable and assignable??  If not... standard macro...

Yes, this is supposed to be copyable and assignable. The contents are just POD.

[jar (doing other things), 2015-08-06 00:18:43]

jar@chromium.org changed reviewers:
+ mal@chromium.org

[jar (doing other things), 2015-08-06 00:18:45]

mal@: Can you figure out who should help with this landing?  I think it is
yet-another third-party library... and then the intent is to connect it to
tcmalloc.  Truth is... we can also use a new owner for tcmalloc (while you're at
it).

--------------------------------

Regarding my comments about adjusting style in this cl, and your suggestion that
they may not apply since this is third party code... I think you are right....

...but...

The problem is that this directory structure is a branch of third party
(TCMalloc) code.  It has a vendor branch directory, showing what we have merged
in, and folks periodically update that branch, and merge in changes.  

I don't think this (large) body of code should be landed here.  

Instead I think that only a minimal shim should land in this directory.

I suspect that if you want similar style-guide-exception dispensation for your
third-party code, you should create a parallel library under third_party,
including a vendor branch (showing what was pulled in from a third party). 

There are also issues to take care of regarding licensing, and I'm surprised to
see the chromium header dropped into place.  I'm going to copy Mark Larson...
and maybe he can direct you to the person that could discuss/facilitate the
creation of a new third-party directory structure (I just don't know what the
current rules and processes are).

[Simon Que, 2015-08-06 00:34:23]

On 2015/08/06 00:18:45, jar (doing other things) wrote:
> mal@: Can you figure out who should help with this landing?  I think it is
> yet-another third-party library... and then the intent is to connect it to
> tcmalloc.  Truth is... we can also use a new owner for tcmalloc (while you're
at
> it).
> 

Are you referring to just farmhash or to my entire CL?

> --------------------------------
> 
> Regarding my comments about adjusting style in this cl, and your suggestion
that
> they may not apply since this is third party code... I think you are right....
> 
> ...but...
> 
> The problem is that this directory structure is a branch of third party
> (TCMalloc) code.  It has a vendor branch directory, showing what we have
merged
> in, and folks periodically update that branch, and merge in changes.  
> 
> I don't think this (large) body of code should be landed here. 

Again, is this referring to farmhash or to the whole CL?
 
> 
> Instead I think that only a minimal shim should land in this directory.
> 
> I suspect that if you want similar style-guide-exception dispensation for your
> third-party code, you should create a parallel library under third_party,
> including a vendor branch (showing what was pulled in from a third party). 
> 
> There are also issues to take care of regarding licensing, and I'm surprised
to
> see the chromium header dropped into place.  I'm going to copy Mark Larson...
> and maybe he can direct you to the person that could discuss/facilitate the
> creation of a new third-party directory structure (I just don't know what the
> current rules and processes are).

[jar (doing other things), 2015-08-06 00:50:50]

On 2015/08/06 00:34:23, Simon Que wrote:
> On 2015/08/06 00:18:45, jar (doing other things) wrote:
> > mal@: Can you figure out who should help with this landing?  I think it is
> > yet-another third-party library... and then the intent is to connect it to
> > tcmalloc.  Truth is... we can also use a new owner for tcmalloc (while
you're
> at
> > it).
> > 
> 
> Are you referring to just farmhash or to my entire CL?
> 
> > --------------------------------
> > 
> > Regarding my comments about adjusting style in this cl, and your suggestion
> that
> > they may not apply since this is third party code... I think you are
right....
> > 
> > ...but...
> > 
> > The problem is that this directory structure is a branch of third party
> > (TCMalloc) code.  It has a vendor branch directory, showing what we have
> merged
> > in, and folks periodically update that branch, and merge in changes.  
> > 
> > I don't think this (large) body of code should be landed here. 
> 
> Again, is this referring to farmhash or to the whole CL?
>  
> > 
> > Instead I think that only a minimal shim should land in this directory.
> > 
> > I suspect that if you want similar style-guide-exception dispensation for
your
> > third-party code, you should create a parallel library under third_party,
> > including a vendor branch (showing what was pulled in from a third party). 
> > 
> > There are also issues to take care of regarding licensing, and I'm surprised
> to
> > see the chromium header dropped into place.  I'm going to copy Mark
Larson...
> > and maybe he can direct you to the person that could discuss/facilitate the
> > creation of a new third-party directory structure (I just don't know what
the
> > current rules and processes are).

Certainly any code that requires style dispensation because it third party code,
should be landed in a third party library so we can update it as needed (with
less confusion).  The only reason to get dispensation is that we want to
minimize diffs with an external tree, to make merges easier.

When code is added to our TCMalloc branch directiory, it is best to minimize
changes (in that directory), exposing only a minimal API change, and then land
new (larger bodies of) code in chromium (somewhere) to interact with and
manipulate that API.

Perhaps mal@ (or someone he points you at) can take this CL in other (more
recent?) directions.  I don't follow all the modularity plans these days, so I
don't know what the "standard"(?) procedure is these days.

[Mark Larson, 2015-08-06 02:23:16]

I'm not in a great position to parse this code review, but I hear jar@'s
request to find a new shepherd. I've sent email to the eng-review team for
advice, and I hope someone more capable than I will jump in and take it
from here.

On Wed, Aug 5, 2015 at 5:50 PM, <jar@chromium.org> wrote:

> On 2015/08/06 00:34:23, Simon Que wrote:
>
>> On 2015/08/06 00:18:45, jar (doing other things) wrote:
>> > mal@: Can you figure out who should help with this landing?  I think
>> it is
>> > yet-another third-party library... and then the intent is to connect it
>> to
>> > tcmalloc.  Truth is... we can also use a new owner for tcmalloc (while
>>
> you're
>
>> at
>> > it).
>> >
>>
>
> Are you referring to just farmhash or to my entire CL?
>>
>
> > --------------------------------
>> >
>> > Regarding my comments about adjusting style in this cl, and your
>> suggestion
>> that
>> > they may not apply since this is third party code... I think you are
>>
> right....
>
>> >
>> > ...but...
>> >
>> > The problem is that this directory structure is a branch of third party
>> > (TCMalloc) code.  It has a vendor branch directory, showing what we have
>> merged
>> > in, and folks periodically update that branch, and merge in changes.
>> >
>> > I don't think this (large) body of code should be landed here.
>>
>
> Again, is this referring to farmhash or to the whole CL?
>>
>
> >
>> > Instead I think that only a minimal shim should land in this directory.
>> >
>> > I suspect that if you want similar style-guide-exception dispensation
>> for
>>
> your
>
>> > third-party code, you should create a parallel library under
>> third_party,
>> > including a vendor branch (showing what was pulled in from a third
>> party).
>> >
>> > There are also issues to take care of regarding licensing, and I'm
>> surprised
>> to
>> > see the chromium header dropped into place.  I'm going to copy Mark
>>
> Larson...
>
>> > and maybe he can direct you to the person that could discuss/facilitate
>> the
>> > creation of a new third-party directory structure (I just don't know
>> what
>>
> the
>
>> > current rules and processes are).
>>
>
> Certainly any code that requires style dispensation because it third party
> code,
> should be landed in a third party library so we can update it as needed
> (with
> less confusion).  The only reason to get dispensation is that we want to
> minimize diffs with an external tree, to make merges easier.
>
> When code is added to our TCMalloc branch directiory, it is best to
> minimize
> changes (in that directory), exposing only a minimal API change, and then
> land
> new (larger bodies of) code in chromium (somewhere) to interact with and
> manipulate that API.
>
> Perhaps mal@ (or someone he points you at) can take this CL in other (more
> recent?) directions.  I don't follow all the modularity plans these days,
> so I
> don't know what the "standard"(?) procedure is these days.
>
>
>
> https://codereview.chromium.org/986503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-08-06 02:42:10]

I talked to jar@ offline. The issue is that the leak detector uses
tcmalloc's internal hooks (much as what the already existing deep memory
profiler does), so its hard to keep it as a separate piece.  We didn't
think this was a big issue since the code is enabled only with a special
build like the deep profiler.

The possible alternate options are (a) upstream the change to tcmalloc and
bring a new version into chromium, or (b) try to see if we can separate out
the code with a minimal tcmalloc patch.

Simon is going to look into (b).

+1 to finding an owner. The tcmalloc in chromium has not been updated in a
long time, there may be performance upsides to this.

Bhaskar
On Aug 5, 2015 7:23 PM, "Mark Larson (Google)" <mal@chromium.org> wrote:

> I'm not in a great position to parse this code review, but I hear jar@'s
> request to find a new shepherd. I've sent email to the eng-review team for
> advice, and I hope someone more capable than I will jump in and take it
> from here.
>
> On Wed, Aug 5, 2015 at 5:50 PM, <jar@chromium.org> wrote:
>
>> On 2015/08/06 00:34:23, Simon Que wrote:
>>
>>> On 2015/08/06 00:18:45, jar (doing other things) wrote:
>>> > mal@: Can you figure out who should help with this landing?  I think
>>> it is
>>> > yet-another third-party library... and then the intent is to connect
>>> it to
>>> > tcmalloc.  Truth is... we can also use a new owner for tcmalloc (while
>>>
>> you're
>>
>>> at
>>> > it).
>>> >
>>>
>>
>> Are you referring to just farmhash or to my entire CL?
>>>
>>
>> > --------------------------------
>>> >
>>> > Regarding my comments about adjusting style in this cl, and your
>>> suggestion
>>> that
>>> > they may not apply since this is third party code... I think you are
>>>
>> right....
>>
>>> >
>>> > ...but...
>>> >
>>> > The problem is that this directory structure is a branch of third party
>>> > (TCMalloc) code.  It has a vendor branch directory, showing what we
>>> have
>>> merged
>>> > in, and folks periodically update that branch, and merge in changes.
>>> >
>>> > I don't think this (large) body of code should be landed here.
>>>
>>
>> Again, is this referring to farmhash or to the whole CL?
>>>
>>
>> >
>>> > Instead I think that only a minimal shim should land in this directory.
>>> >
>>> > I suspect that if you want similar style-guide-exception dispensation
>>> for
>>>
>> your
>>
>>> > third-party code, you should create a parallel library under
>>> third_party,
>>> > including a vendor branch (showing what was pulled in from a third
>>> party).
>>> >
>>> > There are also issues to take care of regarding licensing, and I'm
>>> surprised
>>> to
>>> > see the chromium header dropped into place.  I'm going to copy Mark
>>>
>> Larson...
>>
>>> > and maybe he can direct you to the person that could
>>> discuss/facilitate the
>>> > creation of a new third-party directory structure (I just don't know
>>> what
>>>
>> the
>>
>>> > current rules and processes are).
>>>
>>
>> Certainly any code that requires style dispensation because it third
>> party code,
>> should be landed in a third party library so we can update it as needed
>> (with
>> less confusion).  The only reason to get dispensation is that we want to
>> minimize diffs with an external tree, to make merges easier.
>>
>> When code is added to our TCMalloc branch directiory, it is best to
>> minimize
>> changes (in that directory), exposing only a minimal API change, and then
>> land
>> new (larger bodies of) code in chromium (somewhere) to interact with and
>> manipulate that API.
>>
>> Perhaps mal@ (or someone he points you at) can take this CL in other
>> (more
>> recent?) directions.  I don't follow all the modularity plans these days,
>> so I
>> don't know what the "standard"(?) procedure is these days.
>>
>>
>>
>> https://codereview.chromium.org/986503002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bccheng, 2015-08-06 23:19:08]

I just want to add a bit more context about the memory allocator situation for
Chrome - Chrome on ChromeOS is the only variant that uses TCMalloc as other
platforms have switched to use the system allocators. Given there has been no
owner for TCMalloc for years and only one OS is using it, it is unlikely that
the TCMalloc branch will receive another update. Instead, the likely move for
ChromeOS Chrome is to switch to the system allocator in the future too.

Given most Chromebooks only have 2GB/4GB of RAM, we have been on the forefront
to deal with more memory pressure problems caused by memory leaks than other
platforms. That's why we are motivated to invent a low-overhead leak detector
that can be enabled in the production environment. The tricky part of reducing
the overhead is to reduce the time spent when invoking the additional hook
functions used to track memory allocations and frees, and the most optimal place
is to directly modify the function bodies like malloc/free/calloc/realloc.

Since TCMalloc is near its end of service time, directly adding the leak
detector code into the third_party repository is actually not as destructive as
it appears. If we are allowed to mess with it directly, it will give us the
best-case numbers about the low-overhead instrumentation. If not, it is totally
understandable and we will try plan (b) as Bhaskar indicated earlier.

[Simon Que, 2015-08-07 01:43:20]

jar: I've moved everything to components/metrics.

I think farmhash will have to be moved to third_party in a future patch set.

However, there are presubmit errors due to including third_party/tcmalloc code.

Elsewhere in the codebase, a tcmalloc include file is included:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...


** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  components/metrics/leak_detector/call_stack_table.cc
      Illegal include: "third_party/tcmalloc/chromium/src/heap-profile-stats.h"
    Because of no rule applying. \
  components/metrics/leak_detector/custom_allocator.cc
      Illegal include:
"third_party/tcmalloc/chromium/src/base/low_level_alloc.h"
    Because of no rule applying. \
  components/metrics/leak_detector/leak_detector.cc
      Illegal include: "third_party/tcmalloc/chromium/src/base/googleinit.h"
    Because of no rule applying. \
  components/metrics/leak_detector/leak_detector.cc
      Illegal include: "third_party/tcmalloc/chromium/src/tcmalloc_guard.h"
    Because of no rule applying. \
  components/metrics/leak_detector/leak_detector_impl.h
      Illegal include: "third_party/tcmalloc/chromium/src/addressmap-inl.h"
    Because of no rule applying. \
  components/metrics/leak_detector/leak_detector_impl.h
      Illegal include: "third_party/tcmalloc/chromium/src/heap-profile-stats.h"
    Because of no rule applying. \
  components/metrics/leak_detector/spin_lock_wrapper.cc
      Illegal include: "third_party/tcmalloc/chromium/src/base/spinlock.h"
    Because of no rule applying.

[Simon Que, 2015-08-07 01:56:15]

On 2015/08/07 01:43:20, Simon Que wrote:
> jar: I've moved everything to components/metrics.
> 
> I think farmhash will have to be moved to third_party in a future patch set.
> 
> However, there are presubmit errors due to including third_party/tcmalloc
code.
> 
> Elsewhere in the codebase, a tcmalloc include file is included:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
> 
> 
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   components/metrics/leak_detector/call_stack_table.cc
>       Illegal include:
"third_party/tcmalloc/chromium/src/heap-profile-stats.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/custom_allocator.cc
>       Illegal include:
> "third_party/tcmalloc/chromium/src/base/low_level_alloc.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/leak_detector.cc
>       Illegal include: "third_party/tcmalloc/chromium/src/base/googleinit.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/leak_detector.cc
>       Illegal include: "third_party/tcmalloc/chromium/src/tcmalloc_guard.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/leak_detector_impl.h
>       Illegal include: "third_party/tcmalloc/chromium/src/addressmap-inl.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/leak_detector_impl.h
>       Illegal include:
"third_party/tcmalloc/chromium/src/heap-profile-stats.h"
>     Because of no rule applying. \
>   components/metrics/leak_detector/spin_lock_wrapper.cc
>       Illegal include: "third_party/tcmalloc/chromium/src/base/spinlock.h"
>     Because of no rule applying.

Let me rephrase that because it might not have been clear... I'm getting these
presubmit errors for including tcmalloc code. However, there are existing source
files that already include stuff from tcmalloc:
https://code.google.com/p/chromium/codesearch#search/&q=%23include%20%5C%22th...

I do notice that these are under tcmalloc's gperftools directory. So it seems
like everything outside of gperftools cannot be included (spinlock, addressmap,
low_level_alloc, etc).

And there's too much in these modules for us to copy all of them into
components/metrics/. I'm thinking some of them should be moved into gperftools,
or otherwise create a shim layer there.

[gpike, 2015-08-07 17:35:11]

> Since TCMalloc is near its end of service time, directly adding the leak
> detector code into the third_party repository is actually not as destructive

Given the performance goals of Chrome/Chromium, I don't think it is
wise to phase TCMalloc out without more investigation. The system
allocators on various platforms have improved in recent years, but so
has TCMalloc. I would love to see a newer version of TCMalloc in
Chromium, and some parameter tuning to make it work well against your
current set of benchmarks. I would be happy to be involved but I can't
devote too much time to it due to other responsibilities (my day job
is outside of Chrome/ChromeOS/Chromium).

Also, I'm sure you've thought of this: does Chrome have heavily-used
data structures that cause a lot of heap allocation? I would be happy
to help you look at that also. Sometimes easy changes to container
libraries etc. can reduce the amount of time spent allocating and
deallocating.

Geoff

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-08-13 22:17:06]

Patchset #53 (id:1120001) has been deleted

[jar (doing other things), 2015-08-14 02:34:38]

I only started to review the code... but had a number of comments.  Could you
take another pass over it before I dig in deeper.

Thanks,

Jim

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:42: for (size_t i = 0; i <
ranked_entries_.size() && size_remaining > 0; ++i) {
I think you want size_remaining > 1 here, and in lines below.  You definitely
want it here so that the loop can end early (if you run out of space).  

When the buffer size_remaining is 1, then at most a null character will be
written, and one will be returned by snprintf().  It doesn't include the cost of
writing the final null terminator... even though you always give it room to do
so.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:53: sprintf(prev_entry_buffer,
"(%10d)", entry.count - prev_entry->count);
snprintf is probably better, to avoid buffer overruns.  Yes... I can see that
you are way safe... but better would be a defined constant:

const BUFFER_SIZE = 256;
and then use that in the declaration, as well as in the snprintf().

... or use sizeof(prev_entry_buffer) as the limit.

Your choice.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:78: // Erase the last comma
and space.
How do you know you wrote a comma, and didn't run out of space?  How do you know
you had suspected leaks to print?

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:89: if (size_remaining >= 0)
This will always be at least 1.

What do you want to return?  Do you want to include the \0 in your tally?

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:107: }
nit: Personal preference: I like shorter code with less repetitions... and since
both branches of the "if" do an increment on the iter, better might be:

auto erase_iter = iter++;
if (erase_iter->second == 0) {
  suspected_historgram.erase(erase_iter);
}

If you hate this... or this is really external code that you don't want to
improve... ok.... but it is a personal preference.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:162: return
&prev_ranked_entries_.entry(i);
This concerned me a bit... why do you want to return a pointer into such a
structure, when there is other code that mutates these structures (I think).  I
spend less time reviewing code when it is clearly safer.

In both (private) call sites, all you seem to use in the "->count".

Could you return that count, and be a bit safer?

If you're struggling to return a count, and a success/failure, you could pass in
a pointer to place the count, and return a success/fail return code, or such. 
Your choice.

[Simon Que, 2015-08-14 04:21:26]

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:42: for (size_t i = 0; i <
ranked_entries_.size() && size_remaining > 0; ++i) {
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> I think you want size_remaining > 1 here, and in lines below.  You definitely
> want it here so that the loop can end early (if you run out of space).  
> 
> When the buffer size_remaining is 1, then at most a null character will be
> written, and one will be returned by snprintf().  It doesn't include the cost
of
> writing the final null terminator... even though you always give it room to do
> so.

Done.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:53: sprintf(prev_entry_buffer,
"(%10d)", entry.count - prev_entry->count);
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> snprintf is probably better, to avoid buffer overruns.  Yes... I can see that
> you are way safe... but better would be a defined constant:
> 
> const BUFFER_SIZE = 256;
> and then use that in the declaration, as well as in the snprintf().
> 
> ... or use sizeof(prev_entry_buffer) as the limit.
> 
> Your choice.

Done.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:78: // Erase the last comma
and space.
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> How do you know you wrote a comma, and didn't run out of space?  How do you
know
> you had suspected leaks to print?

Done.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:89: if (size_remaining >= 0)
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> This will always be at least 1.
> 
> What do you want to return?  Do you want to include the \0 in your tally?

Done.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:107: }
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> nit: Personal preference: I like shorter code with less repetitions... and
since
> both branches of the "if" do an increment on the iter, better might be:
> 
> auto erase_iter = iter++;
> if (erase_iter->second == 0) {
>   suspected_historgram.erase(erase_iter);
> }
> 
> If you hate this... or this is really external code that you don't want to
> improve... ok.... but it is a personal preference.

This is not external code. Done.

https://codereview.chromium.org/986503002/diff/1140001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:162: return
&prev_ranked_entries_.entry(i);
On 2015/08/14 02:34:38, jar (doing other things) wrote:
> This concerned me a bit... why do you want to return a pointer into such a
> structure, when there is other code that mutates these structures (I think). 
I
> spend less time reviewing code when it is clearly safer.
> 
> In both (private) call sites, all you seem to use in the "->count".
> 
> Could you return that count, and be a bit safer?
> 
> If you're struggling to return a count, and a success/failure, you could pass
in
> a pointer to place the count, and return a success/fail return code, or such. 
> Your choice.
> 

Done.

[jar (doing other things), 2015-08-15 03:29:01]

Here are some more comments.

Again, please try to clean up other portions of the CL with similar changes.  I
have not finished reading the code.

Thanks,

Jim

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:40: return
call_stack->hash;
Does this give a warning as a hash of type uint64_t is converted to a long?

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:68: if (iter ==
entry_map_.end())
Is this an internal error, or a regular occurrence?

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:96: if (size_left > 0) {
Please correct this throughout this CL.

This should probably be a test for > 1.  Size left should never be zero.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:102: if (size_left > 0)
This is always true.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:34: long operator() (const
CallStack* call_stack) const;
nit: Style: Structs should only be used to hold passive values.  This should
probably be a class.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:37: CallStackTable(int
call_stack_suspicion_threshold);
nit: explicit

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:45: int Dump(char* buffer,
int size) const;
nit: define meaning of return value.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:57: int attempted_size =
nit: Why don't you use the attempted_size declared in the outer scope?

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:82: // Erase the last comma
and space.
As mentioned in my earlier comment: How do you know that you didn't run out of
space?  If you did run out (in the any of the iterations of the for loop on line
74), then the last item may not be a comma and space. 

It is also fairly extreme... but if this function is called with a buffer of
length 2, then you can actually undeflow the buffer with this pointer adjustment
on line 84.  

I think the first issue is more worrisome... though I don't know how "terrible"
it will be if we truncate some charaters that are not trailing commas... but I
don't know how the output will be processed.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:114: for (int i = 0; i <
static_cast<int>(ranked_deltas.size()) - 1; ++i) {
nit: Prefer to not use casts.  I'm not sure I'm following the logic here...
but... perhaps...

In this case, I think(?) your concerns is that you don't want to subtract 1 from
a size(), when size could be zero.  

Can you avoid the problem by instead itterating:

for (size_t i = 1; i < ranked_delats.size(); ++i)

and then using i-1 and i rather than (what you're using now) i and i+1

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:132: for (int i = 0;
found_drop && i < drop_index; ++i) {
nit: This was a little confusing that you tested found_drop constantly, but yet
never changed it in this loop.  It would probably read more clearly as:

if (found_drop) {
  for (...) {
    ...
  }

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:25: public:
nit: declare a virtual destructor.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:39: };
Are you buying anything by using this class?  Do you really have a number of
derived classes?

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

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:42: // Remove existing macro
definitions for the following functions so there is no
I didn't understand this comment.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:47: ? (default_value)
nit: delete parens  (line 53 as well).

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:167: const ElfW(Phdr)& header
= info->dlpi_phdr[i];
Can you provide better names?

[Simon Que, 2015-08-15 23:28:30]

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:40: return
call_stack->hash;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Does this give a warning as a hash of type uint64_t is converted to a long?

No, but this should all be size_t, according to the unordered_map
specifications.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:68: if (iter ==
entry_map_.end())
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Is this an internal error, or a regular occurrence?

This should not happen unless there was an error elsewhere, e.g:
- A double free.
- Improper sampling of allocs and frees in the leak detector

It's not a regular occurrence but it could happen if there was bad code
elsewhere.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:96: if (size_left > 0) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Please correct this throughout this CL.
> 
> This should probably be a test for > 1.  Size left should never be zero.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:102: if (size_left > 0)
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> This is always true.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:34: long operator() (const
CallStack* call_stack) const;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Style: Structs should only be used to hold passive values.  This should
> probably be a class.

Then it would need a "public:" access specifier. I thought the use of a struct
to pass in a Hash to STL containers is pretty standard.

e.g. see KeyHash here:
http://en.cppreference.com/w/cpp/container/unordered_map/unordered_map

Also in existing Chrome code:
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ha...

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:37: CallStackTable(int
call_stack_suspicion_threshold);
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: explicit

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:45: int Dump(char* buffer,
int size) const;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: define meaning of return value.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:57: int attempted_size =
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Why don't you use the attempted_size declared in the outer scope?  

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:82: // Erase the last comma
and space.
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> As mentioned in my earlier comment: How do you know that you didn't run out of
> space?  If you did run out (in the any of the iterations of the for loop on
line
> 74), then the last item may not be a comma and space. 
> 
> It is also fairly extreme... but if this function is called with a buffer of
> length 2, then you can actually undeflow the buffer with this pointer
adjustment
> on line 84.  
> 
> I think the first issue is more worrisome... though I don't know how
"terrible"
> it will be if we truncate some charaters that are not trailing commas... but I
> don't know how the output will be processed.

All this logging code is meant to be temporary. In a future iteration, I intend
to replace it with passing data structures to UMA.

That said, I've fixed this.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:114: for (int i = 0; i <
static_cast<int>(ranked_deltas.size()) - 1; ++i) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Prefer to not use casts.  I'm not sure I'm following the logic here...
> but... perhaps...
> 
> In this case, I think(?) your concerns is that you don't want to subtract 1
from
> a size(), when size could be zero.  
> 
> Can you avoid the problem by instead itterating:
> 
> for (size_t i = 1; i < ranked_delats.size(); ++i)
> 
> and then using i-1 and i rather than (what you're using now) i and i+1
> 

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:132: for (int i = 0;
found_drop && i < drop_index; ++i) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: This was a little confusing that you tested found_drop constantly, but
yet
> never changed it in this loop.  It would probably read more clearly as:
> 
> if (found_drop) {
>   for (...) {
>     ...
>   }

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:25: public:
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: declare a virtual destructor.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:39: };
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Are you buying anything by using this class?  Do you really have a number of
> derived classes?

There are two derived classes. Although I could move it into class ValueType.

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

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:42: // Remove existing macro
definitions for the following functions so there is no
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> I didn't understand this comment.

Forgot to delete this from earlier code.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:47: ? (default_value)
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: delete parens  (line 53 as well).

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:167: const ElfW(Phdr)& header
= info->dlpi_phdr[i];
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Can you provide better names?

Done.

[Simon Que, 2015-08-15 23:28:31]

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:40: return
call_stack->hash;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Does this give a warning as a hash of type uint64_t is converted to a long?

No, but this should all be size_t, according to the unordered_map
specifications.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:68: if (iter ==
entry_map_.end())
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Is this an internal error, or a regular occurrence?

This should not happen unless there was an error elsewhere, e.g:
- A double free.
- Improper sampling of allocs and frees in the leak detector

It's not a regular occurrence but it could happen if there was bad code
elsewhere.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:96: if (size_left > 0) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Please correct this throughout this CL.
> 
> This should probably be a test for > 1.  Size left should never be zero.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:102: if (size_left > 0)
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> This is always true.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:34: long operator() (const
CallStack* call_stack) const;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Style: Structs should only be used to hold passive values.  This should
> probably be a class.

Then it would need a "public:" access specifier. I thought the use of a struct
to pass in a Hash to STL containers is pretty standard.

e.g. see KeyHash here:
http://en.cppreference.com/w/cpp/container/unordered_map/unordered_map

Also in existing Chrome code:
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ha...

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:37: CallStackTable(int
call_stack_suspicion_threshold);
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: explicit

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:45: int Dump(char* buffer,
int size) const;
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: define meaning of return value.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:57: int attempted_size =
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Why don't you use the attempted_size declared in the outer scope?  

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:82: // Erase the last comma
and space.
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> As mentioned in my earlier comment: How do you know that you didn't run out of
> space?  If you did run out (in the any of the iterations of the for loop on
line
> 74), then the last item may not be a comma and space. 
> 
> It is also fairly extreme... but if this function is called with a buffer of
> length 2, then you can actually undeflow the buffer with this pointer
adjustment
> on line 84.  
> 
> I think the first issue is more worrisome... though I don't know how
"terrible"
> it will be if we truncate some charaters that are not trailing commas... but I
> don't know how the output will be processed.

All this logging code is meant to be temporary. In a future iteration, I intend
to replace it with passing data structures to UMA.

That said, I've fixed this.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:114: for (int i = 0; i <
static_cast<int>(ranked_deltas.size()) - 1; ++i) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: Prefer to not use casts.  I'm not sure I'm following the logic here...
> but... perhaps...
> 
> In this case, I think(?) your concerns is that you don't want to subtract 1
from
> a size(), when size could be zero.  
> 
> Can you avoid the problem by instead itterating:
> 
> for (size_t i = 1; i < ranked_delats.size(); ++i)
> 
> and then using i-1 and i rather than (what you're using now) i and i+1
> 

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:132: for (int i = 0;
found_drop && i < drop_index; ++i) {
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: This was a little confusing that you tested found_drop constantly, but
yet
> never changed it in this loop.  It would probably read more clearly as:
> 
> if (found_drop) {
>   for (...) {
>     ...
>   }

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:25: public:
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: declare a virtual destructor.

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:39: };
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Are you buying anything by using this class?  Do you really have a number of
> derived classes?

There are two derived classes. Although I could move it into class ValueType.

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

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:42: // Remove existing macro
definitions for the following functions so there is no
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> I didn't understand this comment.

Forgot to delete this from earlier code.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:47: ? (default_value)
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> nit: delete parens  (line 53 as well).

Done.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:167: const ElfW(Phdr)& header
= info->dlpi_phdr[i];
On 2015/08/15 03:29:01, jar (doing other things) wrote:
> Can you provide better names?

Done.

[Simon Que, 2015-08-18 22:34:05]

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:37: CallStackTable(int
call_stack_suspicion_threshold);
On 2015/08/15 23:28:30, Simon Que wrote:
> On 2015/08/15 03:29:01, jar (doing other things) wrote:
> > nit: explicit
> 
> Done.

Also done in ranked_list.h

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:39: };
On 2015/08/15 23:28:31, Simon Que wrote:
> On 2015/08/15 03:29:01, jar (doing other things) wrote:
> > Are you buying anything by using this class?  Do you really have a number of
> > derived classes?
> 
> There are two derived classes. Although I could move it into class ValueType.

Done.

[Simon Que, 2015-08-18 23:17:03]

jar: I've made some more changes along the lines of your feedback. PTAL

[Simon Que, 2015-08-20 00:08:18]

Patchset #1 (id:1) has been deleted

[Simon Que, 2015-08-20 00:08:41]

Patchset #1 (id:20001) has been deleted

[Simon Que, 2015-08-20 00:09:00]

Patchset #1 (id:40001) has been deleted

[Simon Que, 2015-08-20 00:09:20]

Patchset #1 (id:60001) has been deleted

[Simon Que, 2015-08-20 00:09:36]

Patchset #1 (id:80001) has been deleted

[Simon Que, 2015-08-20 00:09:53]

Patchset #1 (id:100001) has been deleted

[Simon Que, 2015-08-20 00:10:12]

Patchset #1 (id:120001) has been deleted

[Simon Que, 2015-08-20 00:10:30]

Patchset #1 (id:140001) has been deleted

[Simon Que, 2015-08-20 00:10:51]

Patchset #1 (id:160001) has been deleted

[Simon Que, 2015-08-20 00:11:09]

Patchset #1 (id:180001) has been deleted

[Simon Que, 2015-08-20 00:11:47]

Patchset #1 (id:200001) has been deleted

[Simon Que, 2015-08-20 00:12:37]

Patchset #1 (id:220001) has been deleted

[Simon Que, 2015-08-20 00:12:54]

Patchset #1 (id:260001) has been deleted

[Simon Que, 2015-08-20 00:13:17]

Patchset #1 (id:280001) has been deleted

[Simon Que, 2015-08-20 00:13:38]

Patchset #1 (id:300001) has been deleted

[Simon Que, 2015-08-20 00:13:53]

Patchset #1 (id:320001) has been deleted

[Simon Que, 2015-08-20 00:14:14]

Patchset #1 (id:340001) has been deleted

[Simon Que, 2015-08-20 00:15:11]

Patchset #1 (id:360001) has been deleted

[Simon Que, 2015-08-20 00:15:26]

Patchset #1 (id:380001) has been deleted

[Simon Que, 2015-08-20 00:16:07]

Patchset #1 (id:400001) has been deleted

[Simon Que, 2015-08-20 00:29:02]

https://codereview.chromium.org/986503002/diff/1300001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1300001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:48: //    << "n=" << n << " is
too big to allocate";
Should clean this up.

[Simon Que, 2015-08-20 04:21:55]

https://codereview.chromium.org/986503002/diff/1300001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1300001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:48: //    << "n=" << n << " is
too big to allocate";
On 2015/08/20 00:29:02, Simon Que wrote:
> Should clean this up.

Done.

[jar (doing other things), 2015-08-21 02:48:44]

Most critically, all this code needs unit tests.

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1220001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:34: long operator() (const
CallStack* call_stack) const;
On 2015/08/15 23:28:30, Simon Que wrote:
> On 2015/08/15 03:29:01, jar (doing other things) wrote:
> > nit: Style: Structs should only be used to hold passive values.  This should
> > probably be a class.
> 
> Then it would need a "public:" access specifier. I thought the use of a struct
> to pass in a Hash to STL containers is pretty standard.
> 
> e.g. see KeyHash here:
> http://en.cppreference.com/w/cpp/container/unordered_map/unordered_map
> 
> Also in existing Chrome code:
> 

I'm not used to seeing this... but a readability reviewer agreed (subject to not
having C++11 lambda support)....

So you can leave it as is.

Thanks for the correction!

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:13: prev_ranked_entries_ =
ranked_entries_;
This seems to instigate a vector copy... which presumably causes an
allocation... and a deallocation of the old vector.  Is this really what you
wanted to do?  ...and is the ongoing cost contained??

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:25:
ranked_deltas.Add(entry.value, entry.count - prev_count);
Since each Add() is an O(N) operation (to do the emplace vector insertion), it
would appear that iterating over this vector to accumulate the deltas (this way)
is an O(N^2) operation.  Is this what your plan was?  How large is this vector
expected to routinely be when this method is called?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:37: to_string_buffer[0] =
'\0';
nit: What is the significance of this line?  Should it be deleted?  I think you
mutate this buffer below, destroying this initial value (before reuse), so it is
a bit misleading to ever have this setting made.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:53: continue;
*IF* you assume RankedEntries are indeed in reverse sorted order, can't you
conclude that all the other counts will be zero, and we should break here?  Is
there a reason not to?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:99: size_remaining +=
length_of_comma_and_space;
Note that this "backtracking" has left you buffer without a null termination.  
I guess your test on line 102 is now guaranteed to pass... so line 103 will
probably get you back to a null-terminated state... but this probably makes your
code more fragile.  better would be to write a null here as well.

nit: Personal preference: A better pattern (than backtracking) for putting in
commas between entries is to have a bi-valued variable that indicates that
you've printed at least one... and when that becomes true, always *prepend* ", "
before you print the next item.

For example:
char* optional_comma = "";  // Instead of wrote_suspected_leak.
for (...leak_values : suspected_leaks_) {
   ... snprint(buffer, size_remaining, "%s%s",
         optional_commana, leak_value.ToString())
   optional_comma = ", ";
   ...
}

It is your choice of which way to write the code....  but the latter will be
simpler.... and *slightly* more correct, as your current code would often
(sometimes?) kill off characters that were not trailing spaces and commas.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:29: score_threshold_((1 <<
num_suspicions_threshold) * 2 - 1),
Is it worth a CHECK_LT(num_suspicions_threshold, 30u) in the body, to be sure
these are all as expected (no overflow)?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:50: int Dump(char* buffer,
const int buffer_size) const;
nit: mutable operands (like buffer), used to convey return values, should come
last.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:112: const uint64_t
kMultiplier = 0x9ddfea08eb382d69ULL;
Please add a comment citing the source of this hash function.  I found it on the
web in a few places... but it shouldn't be listed without an explanation or
citation.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:260:
CustomAllocator::Free(g_heap_lock, sizeof(*g_heap_lock));
I'm a little surprised that you can clean up this much.  This would leave this
method unsafe to call (line 245 would be dangerous), but you you have a return
on line 248 that *looks* like it is here to prevent doing a second free of the
g_leak_detector.

Can you explain the logic a bit?  I'm suspicious you should leak the lock.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:14: // Singleton management
functions.
nit: This sure looks like a namespace, rather than a class.  Why is a class
used?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:51: buf = ptr + 1;
nit: Perhaps it would be cleaner to not mutate the input that you are given...
and you should also restore the '\n' that you replaced with a null?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:52: ptr = strchr(buf,
'\n');
It looks like you won't print the final segment of this supplied string, unless
the input string is null terminated.  At the same time, the code on line 42
appears to be "set up" to handle a null terminated string (with no newline).

What was the intention here?

I *suspect* that you should move this line up to in front of line 41, and not
bother to calculate it on line 39.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:59: int result =
static_cast<int>(size /= sizeof(uint32_t));
nit: I suspect you didn't mean to use "/=".  You don't seem to use size anywhere
else.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:88:
CustomAllocator::Free(call_stack, sizeof(CallStack));
nit: Although not necessary, it would probably be cleaner to reset these values
to null when you free them, or make it harder to have a duplicate free in some
other ways.  The cost is negligible, but the debugging time is beyond measure if
you  have a bug.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:165:
size_leak_analyzer_.Dump(buf, sizeof(buf));
nit: Is it ok for this dump to be empty?  Perhaps instead of null terminating on
line 164, testing the return result would tell you if it was worth printing on
line 166.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:236: return
base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
Two things:
a) I'm surprised you took the address of ptr, which is an argument on the stack.
 Is that what was intended?

b) I suspect the intent was to provide the size of the first arg to Hash(), as
the second argumnet, in which case you should pass "sizeof(char*)".

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:22: const char*
LeakDetectorValueType::ToString(char* buffer,
Please put the mutable target last.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:35: :
type_(kCallStack), call_stack_(call_stack) {}
Please initialize all POD members, in both constructors.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:18: if (entries_.size() >
max_size_)
Wouldn't this test be better (with slight adjustment) before line 15, so that we
avoid potentially reallocating (larger than the reserved size)?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:22: int
RankedList::GetInsertionIndex(int count) const {
Have you considered using std::binary_search?

There are a lot of ways to write this code incorrectly... not dealing with
underflows, overflows, or edge conditions... and it is much easier to use (the
faster) standard function.

On the other hand.... given that (I think) you routinely do numerous O(N)
operations when you do a binary search (before and after), I bet a linear search
would be about as efficient.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:35: // Otherwise it's somewhere
in the middle, so look for it there.
nit: undent

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:22: class RankedList {
This name is IMO slightly deceptive. A "list" usually means (to me?) a linked
(or doubly-linked) list.  This data structure is really a vector, with all the
complexity that entails (when adding members).  If you want to keep this name,
you should at *least* surface this fact in the method comments (as noted below).

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:47: // with the same value.
nit: Add comment that this is an O(N) insertion into a vector.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:53: int GetInsertionIndex(int
count) const;
nit: Add comment that this is a binary search, of complexity O(log(N)).

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:41: STL_Allocator(const
STL_Allocator&) {}
nit: explicit?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
nit: Clearer and faster is probably:

n <= max_size()

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:56: size_type max_size() const
{ return size_t(-1) / sizeof(T); }
Rather than size_t(-1), you should probably use
std::numeric_limits<size_t>::max()

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14: class
CustomAllocator {
Why are we using a class, rather than a namespace?

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/gperftools/spin_lock_wrapper.h (right):

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/gperftools/spin_lock_wrapper.h:11: // This
avoids users of this class to directly include those classes, causing
nit: typos in sentence.  I don't know what you meant to write.

[Simon Que, 2015-08-21 05:44:05]

Regarding all the printing code: I suggest not being too worried about it as it
is meant to be replaced in the next CL with actual passing of a leak report data
structure. I already have that code written up.

Regarding unit tests: Since this code is not going to be run, and it would make
no sense to add the leak detector unit tests to the unit_test binary right now,
how about in a later CL? I can add a TODO for it. Otherwise we'll just have some
unit tests that aren't being run regularly, either.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:13: prev_ranked_entries_ =
ranked_entries_;
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> This seems to instigate a vector copy... which presumably causes an
> allocation... and a deallocation of the old vector.  Is this really what you
> wanted to do?  ...and is the ongoing cost contained??
> 

I can optimize this.

However, keep in mind that the leak analysis doesn't run that frequently...
something on the order of once per few seconds. The performance here is not a
bottleneck.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:25:
ranked_deltas.Add(entry.value, entry.count - prev_count);
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> Since each Add() is an O(N) operation (to do the emplace vector insertion), it
> would appear that iterating over this vector to accumulate the deltas (this
way)
> is an O(N^2) operation.  Is this what your plan was?  How large is this vector
> expected to routinely be when this method is called?

N=16.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:14: // Singleton management
functions.
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> nit: This sure looks like a namespace, rather than a class.  Why is a class
> used?

I haven't seen many modules in Chrome that weren't a class, but if making it a
namespace is fine, I'll make it a namespace.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:236: return
base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Two things:
> a) I'm surprised you took the address of ptr, which is an argument on the
stack.
>  Is that what was intended?

Is there a problem with accessing an argument on the stack? The idea here is
that I'm trying to hash the value of the pointer.

> 
> b) I suspect the intent was to provide the size of the first arg to Hash(), as
> the second argumnet, in which case you should pass "sizeof(char*)".

No, the second arg is the size of the pointer itself. The first arg is the
address of the pointer variable.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:22: int
RankedList::GetInsertionIndex(int count) const {
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Have you considered using std::binary_search?
> 
> There are a lot of ways to write this code incorrectly... not dealing with
> underflows, overflows, or edge conditions... and it is much easier to use (the
> faster) standard function.
> 
> On the other hand.... given that (I think) you routinely do numerous O(N)
> operations when you do a binary search (before and after), I bet a linear
search
> would be about as efficient.

Yes I could consider a linear search.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Clearer and faster is probably:
> 
> n <= max_size()

That wouldn't catch the overflow of |n * sizeof(T)|, which is the value passed
to Allocate().

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14: class
CustomAllocator {
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Why are we using a class, rather than a namespace?

This is required as a template type for STL_Allocator. Is it possible to pass a
namespace as a template type? I didn't think it is possible.

[jar (doing other things), 2015-08-21 17:32:32]

You shouldn't land code without unit tests.  A todo is not sufficient.  You
should be able to write unittest code that only requires modules (cc files) to
compile, and does not require you to link-into the main binary.  There should
also be build file changes that always run your unittest code.

With regard to printing code: Your code is very low level, forcing you to
manipulate buffers.  When you go this route, the code needs to be correct, and
clean. 


Thanks in advance,

Jim

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:236: return
base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > Two things:
> > a) I'm surprised you took the address of ptr, which is an argument on the
> stack.
> >  Is that what was intended?
> 
> Is there a problem with accessing an argument on the stack? The idea here is
> that I'm trying to hash the value of the pointer.
> 
> > 
> > b) I suspect the intent was to provide the size of the first arg to Hash(),
as
> > the second argumnet, in which case you should pass "sizeof(char*)".
> 
> No, the second arg is the size of the pointer itself. The first arg is the
> address of the pointer variable.
> 

You are correct.  My comment was mistaken.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:243:
sizeof(*(call_stack->stack)) * call_stack->depth);
Is there any padding in "*(call_stack->stack)"?  If so, is there any reason to
believe it is consistently initialized, such that a hash would be meaningful?  
...or how are you assured that there is no padding between these instances?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > nit: Clearer and faster is probably:
> > 
> > n <= max_size()
> 
> That wouldn't catch the overflow of |n * sizeof(T)|, which is the value passed
> to Allocate().

I must be missing something.  Isn't max_size() the largest legal value?

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14: class
CustomAllocator {
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > Why are we using a class, rather than a namespace?
> 
> This is required as a template type for STL_Allocator. Is it possible to pass
a
> namespace as a template type? I didn't think it is possible.

I see.  SGTM.

[bjanakiraman1, 2015-08-21 17:54:42]

On Fri, Aug 21, 2015 at 10:32 AM, <jar@chromium.org> wrote:

> You shouldn't land code without unit tests.  A todo is not sufficient.  You
> should be able to write unittest code that only requires modules (cc
> files) to
> compile, and does not require you to link-into the main binary.  There
> should
> also be build file changes that always run your unittest code.
>
> With regard to printing code: Your code is very low level, forcing you to
> manipulate buffers.  When you go this route, the code needs to be correct,
> and
> clean.
>
>
> Thanks in advance,
>
> Jim



Simon,
I think its okay to have unit tests that are not being run on a regular
basis, as long as there's something for people to test against when changes
are made to the code.

If/When we enable this by default, we should get the unit tests running
regularly.

Jim, would that be okay with you?
Bhaskar



>
>
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> File components/metrics/leak_detector/leak_detector_impl.cc (right):
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> components/metrics/leak_detector/leak_detector_impl.cc:236: return
> base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
> On 2015/08/21 05:44:05, Simon Que wrote:
>
>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>> > Two things:
>> > a) I'm surprised you took the address of ptr, which is an argument
>>
> on the
>
>> stack.
>> >  Is that what was intended?
>>
>
> Is there a problem with accessing an argument on the stack? The idea
>>
> here is
>
>> that I'm trying to hash the value of the pointer.
>>
>
> >
>> > b) I suspect the intent was to provide the size of the first arg to
>>
> Hash(), as
>
>> > the second argumnet, in which case you should pass "sizeof(char*)".
>>
>
> No, the second arg is the size of the pointer itself. The first arg is
>>
> the
>
>> address of the pointer variable.
>>
>
>
> You are correct.  My comment was mistaken.
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> components/metrics/leak_detector/leak_detector_impl.cc:243:
> sizeof(*(call_stack->stack)) * call_stack->depth);
> Is there any padding in "*(call_stack->stack)"?  If so, is there any
> reason to believe it is consistently initialized, such that a hash would
> be meaningful?   ...or how are you assured that there is no padding
> between these instances?
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> File components/metrics/leak_detector/stl_allocator.h (right):
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n *
> sizeof(T)) / sizeof(T) == n);
> On 2015/08/21 05:44:05, Simon Que wrote:
>
>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>> > nit: Clearer and faster is probably:
>> >
>> > n <= max_size()
>>
>
> That wouldn't catch the overflow of |n * sizeof(T)|, which is the
>>
> value passed
>
>> to Allocate().
>>
>
> I must be missing something.  Isn't max_size() the largest legal value?
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
> File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h
> (right):
>
>
>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
> third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14:
> class CustomAllocator {
> On 2015/08/21 05:44:05, Simon Que wrote:
>
>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>> > Why are we using a class, rather than a namespace?
>>
>
> This is required as a template type for STL_Allocator. Is it possible
>>
> to pass a
>
>> namespace as a template type? I didn't think it is possible.
>>
>
> I see.  SGTM.
>
> https://codereview.chromium.org/986503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jar (doing other things), 2015-08-21 19:40:52]

Chromium never (to my knowledge) allows code to be landed without unit
tests. ... and tests need to be run regularly/automatically (so that
incidental bustage is instantly detected... and it is not up to a developer
to seek out "extra" tests that need to be run to check compatibility of a
change.)  This also prevents bit-rot as incompatible underlying changes are
made.

Is there any reason why the unit tests for this code can't be run
regularly?   There are hashing functions, and ordered lists... and no unit
tests.    I've cited potential problems in the code... and there is not
even evidence that the code works as planned.


Jim





On Fri, Aug 21, 2015 at 10:54 AM, Bhaskar Janakiraman <
bjanakiraman@chromium.org> wrote:

> On Fri, Aug 21, 2015 at 10:32 AM, <jar@chromium.org> wrote:
>
>> You shouldn't land code without unit tests.  A todo is not sufficient.
>> You
>> should be able to write unittest code that only requires modules (cc
>> files) to
>> compile, and does not require you to link-into the main binary.  There
>> should
>> also be build file changes that always run your unittest code.
>>
>> With regard to printing code: Your code is very low level, forcing you to
>> manipulate buffers.  When you go this route, the code needs to be
>> correct, and
>> clean.
>>
>>
>> Thanks in advance,
>>
>> Jim
>
>
>
> Simon,
> I think its okay to have unit tests that are not being run on a regular
> basis, as long as there's something for people to test against when changes
> are made to the code.
>
> If/When we enable this by default, we should get the unit tests running
> regularly.
>
> Jim, would that be okay with you?
> Bhaskar
>
>
>
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>> File components/metrics/leak_detector/leak_detector_impl.cc (right):
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>> components/metrics/leak_detector/leak_detector_impl.cc:236: return
>> base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
>> On 2015/08/21 05:44:05, Simon Que wrote:
>>
>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>> > Two things:
>>> > a) I'm surprised you took the address of ptr, which is an argument
>>>
>> on the
>>
>>> stack.
>>> >  Is that what was intended?
>>>
>>
>> Is there a problem with accessing an argument on the stack? The idea
>>>
>> here is
>>
>>> that I'm trying to hash the value of the pointer.
>>>
>>
>> >
>>> > b) I suspect the intent was to provide the size of the first arg to
>>>
>> Hash(), as
>>
>>> > the second argumnet, in which case you should pass "sizeof(char*)".
>>>
>>
>> No, the second arg is the size of the pointer itself. The first arg is
>>>
>> the
>>
>>> address of the pointer variable.
>>>
>>
>>
>> You are correct.  My comment was mistaken.
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>> components/metrics/leak_detector/leak_detector_impl.cc:243:
>> sizeof(*(call_stack->stack)) * call_stack->depth);
>> Is there any padding in "*(call_stack->stack)"?  If so, is there any
>> reason to believe it is consistently initialized, such that a hash would
>> be meaningful?   ...or how are you assured that there is no padding
>> between these instances?
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>> File components/metrics/leak_detector/stl_allocator.h (right):
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>> components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n *
>> sizeof(T)) / sizeof(T) == n);
>> On 2015/08/21 05:44:05, Simon Que wrote:
>>
>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>> > nit: Clearer and faster is probably:
>>> >
>>> > n <= max_size()
>>>
>>
>> That wouldn't catch the overflow of |n * sizeof(T)|, which is the
>>>
>> value passed
>>
>>> to Allocate().
>>>
>>
>> I must be missing something.  Isn't max_size() the largest legal value?
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>> File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h
>> (right):
>>
>>
>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>> third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14:
>> class CustomAllocator {
>> On 2015/08/21 05:44:05, Simon Que wrote:
>>
>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>> > Why are we using a class, rather than a namespace?
>>>
>>
>> This is required as a template type for STL_Allocator. Is it possible
>>>
>> to pass a
>>
>>> namespace as a template type? I didn't think it is possible.
>>>
>>
>> I see.  SGTM.
>>
>> https://codereview.chromium.org/986503002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wtc, 2015-08-21 20:02:09]

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

[wtc, 2015-08-21 20:02:12]

Just some drive-by review comments, which you may ignore.

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:15: #include
<gperftools/custom_allocator.h>

This header file name should probably be put in "" instead of <>.
If the gperftools headers are considered system headers, then
they should be listed with other system headers above.

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:98: #endif  //
CALL_STACK_TABLE_H_

Nit: this comment should say
COMPONENTS_METRICS_LEAK_DETECTOR_CALL_STACK_TABLE_H_

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

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:8: #include <stdint.h>

Nit: don't need to include <stdint.h>

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:9: #include <stdint.h>

Nit: <stdint.h> can be omitted since we already include <inttypes.h>, which is a
superset.

See http://pubs.opengroup.org/onlinepubs/009604599/basedefs/inttypes.h.html

[bjanakiraman1, 2015-08-21 20:09:38]

On Fri, Aug 21, 2015 at 12:40 PM, Jim Roskind <jar@chromium.org> wrote:

> Chromium never (to my knowledge) allows code to be landed without unit
> tests. ... and tests need to be run regularly/automatically (so that
> incidental bustage is instantly detected... and it is not up to a developer
> to seek out "extra" tests that need to be run to check compatibility of a
> change.)  This also prevents bit-rot as incompatible underlying changes are
> made.
>
> Is there any reason why the unit tests for this code can't be run
> regularly?   There are hashing functions, and ordered lists... and no unit
> tests.    I've cited potential problems in the code... and there is not
> even evidence that the code works as planned.
>
>
Agreed.

Simon, lets plan to also have unit tests.

I don't know how to make them run automatically for ChromeOS, perhaps
someone on this thread knows?
Bhaskar


> Jim
>
>
>
>
>
> On Fri, Aug 21, 2015 at 10:54 AM, Bhaskar Janakiraman <
> bjanakiraman@chromium.org> wrote:
>
>> On Fri, Aug 21, 2015 at 10:32 AM, <jar@chromium.org> wrote:
>>
>>> You shouldn't land code without unit tests.  A todo is not sufficient.
>>> You
>>> should be able to write unittest code that only requires modules (cc
>>> files) to
>>> compile, and does not require you to link-into the main binary.  There
>>> should
>>> also be build file changes that always run your unittest code.
>>>
>>> With regard to printing code: Your code is very low level, forcing you to
>>> manipulate buffers.  When you go this route, the code needs to be
>>> correct, and
>>> clean.
>>>
>>>
>>> Thanks in advance,
>>>
>>> Jim
>>
>>
>>
>> Simon,
>> I think its okay to have unit tests that are not being run on a regular
>> basis, as long as there's something for people to test against when changes
>> are made to the code.
>>
>> If/When we enable this by default, we should get the unit tests running
>> regularly.
>>
>> Jim, would that be okay with you?
>> Bhaskar
>>
>>
>>
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>> File components/metrics/leak_detector/leak_detector_impl.cc (right):
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>> components/metrics/leak_detector/leak_detector_impl.cc:236: return
>>> base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>
>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>> > Two things:
>>>> > a) I'm surprised you took the address of ptr, which is an argument
>>>>
>>> on the
>>>
>>>> stack.
>>>> >  Is that what was intended?
>>>>
>>>
>>> Is there a problem with accessing an argument on the stack? The idea
>>>>
>>> here is
>>>
>>>> that I'm trying to hash the value of the pointer.
>>>>
>>>
>>> >
>>>> > b) I suspect the intent was to provide the size of the first arg to
>>>>
>>> Hash(), as
>>>
>>>> > the second argumnet, in which case you should pass "sizeof(char*)".
>>>>
>>>
>>> No, the second arg is the size of the pointer itself. The first arg is
>>>>
>>> the
>>>
>>>> address of the pointer variable.
>>>>
>>>
>>>
>>> You are correct.  My comment was mistaken.
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>> components/metrics/leak_detector/leak_detector_impl.cc:243:
>>> sizeof(*(call_stack->stack)) * call_stack->depth);
>>> Is there any padding in "*(call_stack->stack)"?  If so, is there any
>>> reason to believe it is consistently initialized, such that a hash would
>>> be meaningful?   ...or how are you assured that there is no padding
>>> between these instances?
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>> File components/metrics/leak_detector/stl_allocator.h (right):
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>> components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n *
>>> sizeof(T)) / sizeof(T) == n);
>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>
>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>> > nit: Clearer and faster is probably:
>>>> >
>>>> > n <= max_size()
>>>>
>>>
>>> That wouldn't catch the overflow of |n * sizeof(T)|, which is the
>>>>
>>> value passed
>>>
>>>> to Allocate().
>>>>
>>>
>>> I must be missing something.  Isn't max_size() the largest legal value?
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>>> File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h
>>> (right):
>>>
>>>
>>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>>> third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14:
>>> class CustomAllocator {
>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>
>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>> > Why are we using a class, rather than a namespace?
>>>>
>>>
>>> This is required as a template type for STL_Allocator. Is it possible
>>>>
>>> to pass a
>>>
>>>> namespace as a template type? I didn't think it is possible.
>>>>
>>>
>>> I see.  SGTM.
>>>
>>> https://codereview.chromium.org/986503002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-08-21 21:03:26]

The only concern I had was other developers objecting to running a test for
something that is not used. That increases the overall runtime of the unit
tests.

I know how to make them run only for Chrome OS... there are conditions that
can be set in the gyp files.


On Fri, Aug 21, 2015 at 1:09 PM, Bhaskar Janakiraman <
bjanakiraman@chromium.org> wrote:

> On Fri, Aug 21, 2015 at 12:40 PM, Jim Roskind <jar@chromium.org> wrote:
>
>> Chromium never (to my knowledge) allows code to be landed without unit
>> tests. ... and tests need to be run regularly/automatically (so that
>> incidental bustage is instantly detected... and it is not up to a developer
>> to seek out "extra" tests that need to be run to check compatibility of a
>> change.)  This also prevents bit-rot as incompatible underlying changes are
>> made.
>>
>> Is there any reason why the unit tests for this code can't be run
>> regularly?   There are hashing functions, and ordered lists... and no unit
>> tests.    I've cited potential problems in the code... and there is not
>> even evidence that the code works as planned.
>>
>>
> Agreed.
>
> Simon, lets plan to also have unit tests.
>
> I don't know how to make them run automatically for ChromeOS, perhaps
> someone on this thread knows?
> Bhaskar
>
>
>> Jim
>>
>>
>>
>>
>>
>> On Fri, Aug 21, 2015 at 10:54 AM, Bhaskar Janakiraman <
>> bjanakiraman@chromium.org> wrote:
>>
>>> On Fri, Aug 21, 2015 at 10:32 AM, <jar@chromium.org> wrote:
>>>
>>>> You shouldn't land code without unit tests.  A todo is not sufficient.
>>>> You
>>>> should be able to write unittest code that only requires modules (cc
>>>> files) to
>>>> compile, and does not require you to link-into the main binary.  There
>>>> should
>>>> also be build file changes that always run your unittest code.
>>>>
>>>> With regard to printing code: Your code is very low level, forcing you
>>>> to
>>>> manipulate buffers.  When you go this route, the code needs to be
>>>> correct, and
>>>> clean.
>>>>
>>>>
>>>> Thanks in advance,
>>>>
>>>> Jim
>>>
>>>
>>>
>>> Simon,
>>> I think its okay to have unit tests that are not being run on a regular
>>> basis, as long as there's something for people to test against when changes
>>> are made to the code.
>>>
>>> If/When we enable this by default, we should get the unit tests running
>>> regularly.
>>>
>>> Jim, would that be okay with you?
>>> Bhaskar
>>>
>>>
>>>
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>>> File components/metrics/leak_detector/leak_detector_impl.cc (right):
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>>> components/metrics/leak_detector/leak_detector_impl.cc:236: return
>>>> base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
>>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>>
>>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>>> > Two things:
>>>>> > a) I'm surprised you took the address of ptr, which is an argument
>>>>>
>>>> on the
>>>>
>>>>> stack.
>>>>> >  Is that what was intended?
>>>>>
>>>>
>>>> Is there a problem with accessing an argument on the stack? The idea
>>>>>
>>>> here is
>>>>
>>>>> that I'm trying to hash the value of the pointer.
>>>>>
>>>>
>>>> >
>>>>> > b) I suspect the intent was to provide the size of the first arg to
>>>>>
>>>> Hash(), as
>>>>
>>>>> > the second argumnet, in which case you should pass "sizeof(char*)".
>>>>>
>>>>
>>>> No, the second arg is the size of the pointer itself. The first arg is
>>>>>
>>>> the
>>>>
>>>>> address of the pointer variable.
>>>>>
>>>>
>>>>
>>>> You are correct.  My comment was mistaken.
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>>> components/metrics/leak_detector/leak_detector_impl.cc:243:
>>>> sizeof(*(call_stack->stack)) * call_stack->depth);
>>>> Is there any padding in "*(call_stack->stack)"?  If so, is there any
>>>> reason to believe it is consistently initialized, such that a hash would
>>>> be meaningful?   ...or how are you assured that there is no padding
>>>> between these instances?
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>>> File components/metrics/leak_detector/stl_allocator.h (right):
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
>>>> components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n *
>>>> sizeof(T)) / sizeof(T) == n);
>>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>>
>>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>>> > nit: Clearer and faster is probably:
>>>>> >
>>>>> > n <= max_size()
>>>>>
>>>>
>>>> That wouldn't catch the overflow of |n * sizeof(T)|, which is the
>>>>>
>>>> value passed
>>>>
>>>>> to Allocate().
>>>>>
>>>>
>>>> I must be missing something.  Isn't max_size() the largest legal value?
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>>>> File third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h
>>>> (right):
>>>>
>>>>
>>>>
https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
>>>> third_party/tcmalloc/chromium/src/gperftools/custom_allocator.h:14:
>>>> class CustomAllocator {
>>>> On 2015/08/21 05:44:05, Simon Que wrote:
>>>>
>>>>> On 2015/08/21 02:48:43, jar (doing other things) wrote:
>>>>> > Why are we using a class, rather than a namespace?
>>>>>
>>>>
>>>> This is required as a template type for STL_Allocator. Is it possible
>>>>>
>>>> to pass a
>>>>
>>>>> namespace as a template type? I didn't think it is possible.
>>>>>
>>>>
>>>> I see.  SGTM.
>>>>
>>>> https://codereview.chromium.org/986503002/
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-08-21 21:59:20]

Jim, I've addressed most of your comments that involved minor code changes.

I'd like to rethink the implementation of RankedList so that'll be in a separate
patch set.

And unit tests will be in a separate patch set, assuming we agree on whether it
is okay to run them in the main Chrome unit_test binary for ChromeOS.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:13: prev_ranked_entries_ =
ranked_entries_;
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:42, jar (doing other things) wrote:
> > This seems to instigate a vector copy... which presumably causes an
> > allocation... and a deallocation of the old vector.  Is this really what you
> > wanted to do?  ...and is the ongoing cost contained??
> > 
> 
> I can optimize this.
> 
> However, keep in mind that the leak analysis doesn't run that frequently...
> something on the order of once per few seconds. The performance here is not a
> bottleneck.

I'm going to hold off on implementing this until I've reworked RankedList into a
form that we're both satisfied with.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:37: to_string_buffer[0] =
'\0';
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> nit: What is the significance of this line?  Should it be deleted?  I think
you
> mutate this buffer below, destroying this initial value (before reuse), so it
is
> a bit misleading to ever have this setting made.

Deleted.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:53: continue;
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> *IF* you assume RankedEntries are indeed in reverse sorted order, can't you
> conclude that all the other counts will be zero, and we should break here?  Is
> there a reason not to?

You're right, it makes more sense to break.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:99: size_remaining +=
length_of_comma_and_space;
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> Note that this "backtracking" has left you buffer without a null termination. 

> I guess your test on line 102 is now guaranteed to pass... so line 103 will
> probably get you back to a null-terminated state... but this probably makes
your
> code more fragile.  better would be to write a null here as well.
> 
> nit: Personal preference: A better pattern (than backtracking) for putting in
> commas between entries is to have a bi-valued variable that indicates that
> you've printed at least one... and when that becomes true, always *prepend* ",
"
> before you print the next item.
> 
> For example:
> char* optional_comma = "";  // Instead of wrote_suspected_leak.
> for (...leak_values : suspected_leaks_) {
>    ... snprint(buffer, size_remaining, "%s%s",
>          optional_commana, leak_value.ToString())
>    optional_comma = ", ";
>    ...
> }
> 
> It is your choice of which way to write the code....  but the latter will be
> simpler.... and *slightly* more correct, as your current code would often
> (sometimes?) kill off characters that were not trailing spaces and commas.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:29: score_threshold_((1 <<
num_suspicions_threshold) * 2 - 1),
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> Is it worth a CHECK_LT(num_suspicions_threshold, 30u) in the body, to be sure
> these are all as expected (no overflow)?

I simplified the score system. Now it goes linearly instead of exponentially.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.h:50: int Dump(char* buffer,
const int buffer_size) const;
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> nit: mutable operands (like buffer), used to convey return values, should come
> last.

Done.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:112: const uint64_t
kMultiplier = 0x9ddfea08eb382d69ULL;
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> Please add a comment citing the source of this hash function.  I found it on
the
> web in a few places... but it shouldn't be listed without an explanation or
> citation.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:260:
CustomAllocator::Free(g_heap_lock, sizeof(*g_heap_lock));
On 2015/08/21 02:48:42, jar (doing other things) wrote:
> I'm a little surprised that you can clean up this much.  This would leave this
> method unsafe to call (line 245 would be dangerous), but you you have a return
> on line 248 that *looks* like it is here to prevent doing a second free of the
> g_leak_detector.
> 
> Can you explain the logic a bit?  I'm suspicious you should leak the lock.

I've fixed the logic so that it first checks for IsInitialized() and returns if
false.

Similar to the beginning of Initialize().

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:51: buf = ptr + 1;
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Perhaps it would be cleaner to not mutate the input that you are given...
> and you should also restore the '\n' that you replaced with a null?

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:52: ptr = strchr(buf,
'\n');
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> It looks like you won't print the final segment of this supplied string,
unless
> the input string is null terminated.  At the same time, the code on line 42
> appears to be "set up" to handle a null terminated string (with no newline).
> 
> What was the intention here?
> 
> I *suspect* that you should move this line up to in front of line 41, and not
> bother to calculate it on line 39.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:59: int result =
static_cast<int>(size /= sizeof(uint32_t));
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: I suspect you didn't mean to use "/=".  You don't seem to use size
anywhere
> else.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:88:
CustomAllocator::Free(call_stack, sizeof(CallStack));
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Although not necessary, it would probably be cleaner to reset these
values
> to null when you free them, or make it harder to have a duplicate free in some
> other ways.  The cost is negligible, but the debugging time is beyond measure
if
> you  have a bug.

Done. I just cleared the containers.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:165:
size_leak_analyzer_.Dump(buf, sizeof(buf));
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Is it ok for this dump to be empty?  Perhaps instead of null terminating
on
> line 164, testing the return result would tell you if it was worth printing on
> line 166.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:236: return
base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
On 2015/08/21 17:32:31, jar (doing other things) wrote:
> On 2015/08/21 05:44:05, Simon Que wrote:
> > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > Two things:
> > > a) I'm surprised you took the address of ptr, which is an argument on the
> > stack.
> > >  Is that what was intended?
> > 
> > Is there a problem with accessing an argument on the stack? The idea here is
> > that I'm trying to hash the value of the pointer.
> > 
> > > 
> > > b) I suspect the intent was to provide the size of the first arg to
Hash(),
> as
> > > the second argumnet, in which case you should pass "sizeof(char*)".
> > 
> > No, the second arg is the size of the pointer itself. The first arg is the
> > address of the pointer variable.
> > 
> 
> You are correct.  My comment was mistaken.  

Perhaps it would be more clear to have this operator take a uintptr_t?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:243:
sizeof(*(call_stack->stack)) * call_stack->depth);
On 2015/08/21 17:32:31, jar (doing other things) wrote:
> Is there any padding in "*(call_stack->stack)"?  If so, is there any reason to
> believe it is consistently initialized, such that a hash would be meaningful? 

> ...or how are you assured that there is no padding between these instances?

|call_stack->stack| is an array of ptrs. And |call_stack->depth| limits the
hashing to the initialized part of |stack|.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:22: const char*
LeakDetectorValueType::ToString(char* buffer,
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Please put the mutable target last.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:35: :
type_(kCallStack), call_stack_(call_stack) {}
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Please initialize all POD members, in both constructors.

Done.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:35: // Otherwise it's somewhere
in the middle, so look for it there.
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: undent

Done.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:22: class RankedList {
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> This name is IMO slightly deceptive. A "list" usually means (to me?) a linked
> (or doubly-linked) list.  This data structure is really a vector, with all the
> complexity that entails (when adding members).  If you want to keep this name,
> you should at *least* surface this fact in the method comments (as noted
below).

I'm going to rethink this entire implementation and perhaps make it a STL list
instead.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:41: STL_Allocator(const
STL_Allocator&) {}
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: explicit?

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
On 2015/08/21 17:32:31, jar (doing other things) wrote:
> On 2015/08/21 05:44:05, Simon Que wrote:
> > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > nit: Clearer and faster is probably:
> > > 
> > > n <= max_size()
> > 
> > That wouldn't catch the overflow of |n * sizeof(T)|, which is the value
passed
> > to Allocate().
> 
> I must be missing something.  Isn't max_size() the largest legal value?

How could size_t n be > max_size()? Checking n <= max_size() is always going to
pass.

OTOH, suppose you have a 32-bit system with size_t = 32 bits. If you tried to
allocate N=2^24 copies of a struct of size 256, you'd be attempting to allocate
a total of N * 256 = 2^32 bytes. That is not possible because multiplying the
two would result in an overflow. And to check for this overflow:

  (n * sizeof(T)) / sizeof(T) == n

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:56: size_type max_size() const
{ return size_t(-1) / sizeof(T); }
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Rather than size_t(-1), you should probably use
> std::numeric_limits<size_t>::max()

Done.

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
File third_party/tcmalloc/chromium/src/gperftools/spin_lock_wrapper.h (right):

https://codereview.chromium.org/986503002/diff/1320001/third_party/tcmalloc/c...
third_party/tcmalloc/chromium/src/gperftools/spin_lock_wrapper.h:11: // This
avoids users of this class to directly include those classes, causing
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: typos in sentence.  I don't know what you meant to write.

Done.

[Simon Que, 2015-08-21 23:38:39]

Patchset #44 (id:1360001) has been deleted

[jar (doing other things), 2015-08-22 00:21:47]

You suggested:

   "And unit tests will be in a separate patch set".

This patch must not land until it includes unittests.

If you would rather find another reviewer, that would be fine.

... but reviewers should not allow code to land without unit tests.  If you find
another reviewer, please be SURE to indicate that a previous reviewer insisted
on you having unittest code. 

Thanks,

Jim

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:243:
sizeof(*(call_stack->stack)) * call_stack->depth);
On 2015/08/21 21:59:20, Simon Que wrote:
> On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > Is there any padding in "*(call_stack->stack)"?  If so, is there any reason
to
> > believe it is consistently initialized, such that a hash would be
meaningful? 
> 
> > ...or how are you assured that there is no padding between these instances?
> 
> |call_stack->stack| is an array of ptrs. And |call_stack->depth| limits the
> hashing to the initialized part of |stack|.

I should have been more explicit....

I believe that each struct in an array is required to be aligned, even if the
size of the array is not a multiple of an alignment factor.  To make this work,
the compiler will routinely "pad" any smaller (irregularly sized?) struct, and
misleadingly set the sizeof(your_struct) to be a multiple of the required
alignment value.  The data in this "trailing padding section" following each
useful-data-section of the struct is not guaranteed to be initialized memory. 
Moreover, entries inside a struct are aligned based on their type. This can
result in internal padding in a struct, the areas for such padding are also not
guaranteed to be initialized.

As a result, it is questionable as to whether you can "scan an array of these
structs" and get a meaningful (deterministic) result.  That was the point of my
comment.

It turns out that the **current** definition of the struct in question
"*(call_stack->stack)" has a leading member that is a uint32, followed by a
pointer.  I *think* that on a 64bit machine, that pointer is supposed to be "8
byte aligned," and hence instigate internal padding in your structure.  This
might be true on x86 64bit architectures, and I'm sure it is true on other
architectures.

Have you tested your code on a 64bit platform?

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
On 2015/08/21 21:59:20, Simon Que wrote:
> On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > On 2015/08/21 05:44:05, Simon Que wrote:
> > > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > > nit: Clearer and faster is probably:
> > > > 
> > > > n <= max_size()
> > > 
> > > That wouldn't catch the overflow of |n * sizeof(T)|, which is the value
> passed
> > > to Allocate().
> > 
> > I must be missing something.  Isn't max_size() the largest legal value?
> 
> How could size_t n be > max_size()? Checking n <= max_size() is always going
to
> pass.
> 
> OTOH, suppose you have a 32-bit system with size_t = 32 bits. If you tried to
> allocate N=2^24 copies of a struct of size 256, you'd be attempting to
allocate
> a total of N * 256 = 2^32 bytes. That is not possible because multiplying the
> two would result in an overflow. And to check for this overflow:
> 
>   (n * sizeof(T)) / sizeof(T) == n
> 

max_size() is defined on line 56 below.  It is the result of dividing the
largest size_t by sizeof(T).  So long as n is less than this value, the product
with sizeof(T) will always avoid an overflow (in a size_t capable variable).

What am I missing?

[Simon Que, 2015-08-22 00:35:21]

On 2015/08/22 00:21:47, jar (doing other things) wrote:
> You suggested:
> 
>    "And unit tests will be in a separate patch set".
> 
> This patch must not land until it includes unittests.
> 
> If you would rather find another reviewer, that would be fine.
> 
> ... but reviewers should not allow code to land without unit tests.  If you
find
> another reviewer, please be SURE to indicate that a previous reviewer insisted
> on you having unittest code. 
> 
> Thanks,
> 
> Jim

By patch set I mean further revisions to this CL, not a separate patch / CL.

Each time you upload a revision, rietveld calls it "Patch Set $N" ... that's
what I was referring to.


> 
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> File components/metrics/leak_detector/leak_detector_impl.cc (right):
> 
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> components/metrics/leak_detector/leak_detector_impl.cc:243:
> sizeof(*(call_stack->stack)) * call_stack->depth);
> On 2015/08/21 21:59:20, Simon Que wrote:
> > On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > > Is there any padding in "*(call_stack->stack)"?  If so, is there any
reason
> to
> > > believe it is consistently initialized, such that a hash would be
> meaningful? 
> > 
> > > ...or how are you assured that there is no padding between these
instances?
> > 
> > |call_stack->stack| is an array of ptrs. And |call_stack->depth| limits the
> > hashing to the initialized part of |stack|.
> 
> I should have been more explicit....
> 
> I believe that each struct in an array is required to be aligned, even if the
> size of the array is not a multiple of an alignment factor.  To make this
work,
> the compiler will routinely "pad" any smaller (irregularly sized?) struct, and
> misleadingly set the sizeof(your_struct) to be a multiple of the required
> alignment value.  The data in this "trailing padding section" following each
> useful-data-section of the struct is not guaranteed to be initialized memory. 
> Moreover, entries inside a struct are aligned based on their type. This can
> result in internal padding in a struct, the areas for such padding are also
not
> guaranteed to be initialized.
> 
> As a result, it is questionable as to whether you can "scan an array of these
> structs" and get a meaningful (deterministic) result.  That was the point of
my
> comment.
> 
> It turns out that the **current** definition of the struct in question
> "*(call_stack->stack)" has a leading member that is a uint32, followed by a
> pointer.  I *think* that on a 64bit machine, that pointer is supposed to be "8
> byte aligned," and hence instigate internal padding in your structure.  This
> might be true on x86 64bit architectures, and I'm sure it is true on other
> architectures.

|call_stack| is the struct.
|call_stack->stack| is a const void** (array of const ptrs)

> 
> Have you tested your code on a 64bit platform?

Yes it was primarily on x86_64

> 
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> File components/metrics/leak_detector/stl_allocator.h (right):
> 
>
https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
> components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T))
/
> sizeof(T) == n);
> On 2015/08/21 21:59:20, Simon Que wrote:
> > On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > > On 2015/08/21 05:44:05, Simon Que wrote:
> > > > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > > > nit: Clearer and faster is probably:
> > > > > 
> > > > > n <= max_size()
> > > > 
> > > > That wouldn't catch the overflow of |n * sizeof(T)|, which is the value
> > passed
> > > > to Allocate().
> > > 
> > > I must be missing something.  Isn't max_size() the largest legal value?
> > 
> > How could size_t n be > max_size()? Checking n <= max_size() is always going
> to
> > pass.
> > 
> > OTOH, suppose you have a 32-bit system with size_t = 32 bits. If you tried
to
> > allocate N=2^24 copies of a struct of size 256, you'd be attempting to
> allocate
> > a total of N * 256 = 2^32 bytes. That is not possible because multiplying
the
> > two would result in an overflow. And to check for this overflow:
> > 
> >   (n * sizeof(T)) / sizeof(T) == n
> > 
> 
> max_size() is defined on line 56 below.  It is the result of dividing the
> largest size_t by sizeof(T).  So long as n is less than this value, the
product
> with sizeof(T) will always avoid an overflow (in a size_t capable variable).
> 
> What am I missing?

Oh I see... I thought max_size() was some STL construct.

[Simon Que, 2015-08-22 22:41:51]

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.h (right):

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:15: #include
<gperftools/custom_allocator.h>
On 2015/08/21 20:02:12, wtc wrote:
> 
> This header file name should probably be put in "" instead of <>.
> If the gperftools headers are considered system headers, then
> they should be listed with other system headers above.

Done. The presubmit will fail if they are put in "" so I have instead moved them
up to the system headers.

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.h:98: #endif  //
CALL_STACK_TABLE_H_
On 2015/08/21 20:02:12, wtc wrote:
> 
> Nit: this comment should say
> COMPONENTS_METRICS_LEAK_DETECTOR_CALL_STACK_TABLE_H_

Done.

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

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:8: #include <stdint.h>
On 2015/08/21 20:02:12, wtc wrote:
> 
> Nit: don't need to include <stdint.h>

Done.

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:9: #include <stdint.h>
On 2015/08/21 20:02:12, wtc wrote:
> 
> Nit: <stdint.h> can be omitted since we already include <inttypes.h>, which is
a
> superset.
> 
> See http://pubs.opengroup.org/onlinepubs/009604599/basedefs/inttypes.h.html

Done.

[Simon Que, 2015-08-23 05:30:30]

Patchset #47 (id:1440001) has been deleted

[Simon Que, 2015-08-23 05:32:08]

Added unit tests for LeakAnalyzer and RankedList.

Will add unit tests for CallStackTable and LeakDetectorImpl, if possible.

[Simon Que, 2015-08-23 22:06:41]

Patchset #1 (id:420001) has been deleted

[Simon Que, 2015-08-23 22:07:04]

Patchset #1 (id:440001) has been deleted

[Simon Que, 2015-08-23 22:07:24]

Patchset #1 (id:460001) has been deleted

[Simon Que, 2015-08-23 22:07:44]

Patchset #1 (id:480001) has been deleted

[Simon Que, 2015-08-23 22:08:06]

Patchset #1 (id:500001) has been deleted

[Simon Que, 2015-08-23 22:08:31]

Patchset #1 (id:520001) has been deleted

[Simon Que, 2015-08-23 22:08:54]

Patchset #1 (id:540001) has been deleted

[Simon Que, 2015-08-23 22:09:12]

Patchset #1 (id:560001) has been deleted

[Simon Que, 2015-08-23 22:09:31]

Patchset #1 (id:580001) has been deleted

[Simon Que, 2015-08-23 22:10:12]

Patchset #1 (id:600001) has been deleted

[Simon Que, 2015-08-23 22:10:15]

Patchset #2 (id:620001) has been deleted

[Simon Que, 2015-08-23 22:10:19]

Patchset #3 (id:640001) has been deleted

[Simon Que, 2015-08-23 22:10:21]

Patchset #2 (id:660001) has been deleted

[Simon Que, 2015-08-23 22:10:24]

Patchset #3 (id:680001) has been deleted

[Simon Que, 2015-08-23 22:10:53]

Patchset #1 (id:660001) has been deleted

[Simon Que, 2015-08-23 22:11:41]

Patchset #1 (id:700001) has been deleted

[Simon Que, 2015-08-23 22:11:43]

Patchset #2 (id:720001) has been deleted

[Simon Que, 2015-08-23 22:11:46]

Patchset #3 (id:740001) has been deleted

[Simon Que, 2015-08-23 22:11:48]

Patchset #4 (id:760001) has been deleted

[Simon Que, 2015-08-23 22:11:48]

Patchset #5 (id:780001) has been deleted

[Simon Que, 2015-08-23 22:11:50]

Patchset #6 (id:840001) has been deleted

[Simon Que, 2015-08-23 22:11:52]

Patchset #7 (id:860001) has been deleted

[Simon Que, 2015-08-23 22:11:54]

Patchset #7 (id:880001) has been deleted

[Simon Que, 2015-08-23 22:11:56]

Patchset #7 (id:920001) has been deleted

[Simon Que, 2015-08-23 22:11:59]

Patchset #8 (id:940001) has been deleted

[Simon Que, 2015-08-23 22:12:29]

Patchset #1 (id:700001) has been deleted

[Simon Que, 2015-08-23 22:13:41]

Patchset #4 (id:860001) has been deleted

[Simon Que, 2015-08-23 22:13:52]

Patchset #4 (id:880001) has been deleted

[Simon Que, 2015-08-23 22:14:04]

Patchset #9 (id:1040001) has been deleted

[Simon Que, 2015-08-23 22:14:27]

Patchset #6 (id:980001) has been deleted

[Simon Que, 2015-08-23 23:27:32]

Patchset #29 (id:1520001) has been deleted

[Simon Que, 2015-08-23 23:30:53]

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:14: // Singleton management
functions.
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:42, jar (doing other things) wrote:
> > nit: This sure looks like a namespace, rather than a class.  Why is a class
> > used?
> 
> I haven't seen many modules in Chrome that weren't a class, but if making it a
> namespace is fine, I'll make it a namespace.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:236: return
base::Hash(reinterpret_cast<const char*>(&ptr), sizeof(ptr));
On 2015/08/21 21:59:20, Simon Que wrote:
> On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > On 2015/08/21 05:44:05, Simon Que wrote:
> > > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > > Two things:
> > > > a) I'm surprised you took the address of ptr, which is an argument on
the
> > > stack.
> > > >  Is that what was intended?
> > > 
> > > Is there a problem with accessing an argument on the stack? The idea here
is
> > > that I'm trying to hash the value of the pointer.
> > > 
> > > > 
> > > > b) I suspect the intent was to provide the size of the first arg to
> Hash(),
> > as
> > > > the second argumnet, in which case you should pass "sizeof(char*)".
> > > 
> > > No, the second arg is the size of the pointer itself. The first arg is the
> > > address of the pointer variable.
> > > 
> > 
> > You are correct.  My comment was mistaken.  
> 
> Perhaps it would be more clear to have this operator take a uintptr_t?

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:35: :
type_(kCallStack), call_stack_(call_stack) {}
On 2015/08/21 21:59:20, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > Please initialize all POD members, in both constructors.
> 
> Done.

I just saw that I hadn't done this completely in a previous patch set. Done.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:18: if (entries_.size() >
max_size_)
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> Wouldn't this test be better (with slight adjustment) before line 15, so that
we
> avoid potentially reallocating (larger than the reserved size)?

No longer an issue with list implementation.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:22: int
RankedList::GetInsertionIndex(int count) const {
On 2015/08/21 05:44:05, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > Have you considered using std::binary_search?
> > 
> > There are a lot of ways to write this code incorrectly... not dealing with
> > underflows, overflows, or edge conditions... and it is much easier to use
(the
> > faster) standard function.
> > 
> > On the other hand.... given that (I think) you routinely do numerous O(N)
> > operations when you do a binary search (before and after), I bet a linear
> search
> > would be about as efficient.
> 
> Yes I could consider a linear search.

Replaced with std::upper_bound.

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:22: class RankedList {
On 2015/08/21 21:59:20, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > This name is IMO slightly deceptive. A "list" usually means (to me?) a
linked
> > (or doubly-linked) list.  This data structure is really a vector, with all
the
> > complexity that entails (when adding members).  If you want to keep this
name,
> > you should at *least* surface this fact in the method comments (as noted
> below).
> 
> I'm going to rethink this entire implementation and perhaps make it a STL list
> instead.

Done.

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:47: // with the same value.
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Add comment that this is an O(N) insertion into a vector.

Now a O(1) insertion into a linked list. :)

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:53: int GetInsertionIndex(int
count) const;
On 2015/08/21 02:48:43, jar (doing other things) wrote:
> nit: Add comment that this is a binary search, of complexity O(log(N)).

Removed this function, replaced with upper_bound (linear search of ranked list)

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
File components/metrics/leak_detector/stl_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/stl_allocator.h:51: RAW_CHECK((n * sizeof(T)) /
sizeof(T) == n);
On 2015/08/22 00:21:47, jar (doing other things) wrote:
> On 2015/08/21 21:59:20, Simon Que wrote:
> > On 2015/08/21 17:32:31, jar (doing other things) wrote:
> > > On 2015/08/21 05:44:05, Simon Que wrote:
> > > > On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > > > > nit: Clearer and faster is probably:
> > > > > 
> > > > > n <= max_size()
> > > > 
> > > > That wouldn't catch the overflow of |n * sizeof(T)|, which is the value
> > passed
> > > > to Allocate().
> > > 
> > > I must be missing something.  Isn't max_size() the largest legal value?
> > 
> > How could size_t n be > max_size()? Checking n <= max_size() is always going
> to
> > pass.
> > 
> > OTOH, suppose you have a 32-bit system with size_t = 32 bits. If you tried
to
> > allocate N=2^24 copies of a struct of size 256, you'd be attempting to
> allocate
> > a total of N * 256 = 2^32 bytes. That is not possible because multiplying
the
> > two would result in an overflow. And to check for this overflow:
> > 
> >   (n * sizeof(T)) / sizeof(T) == n
> > 
> 
> max_size() is defined on line 56 below.  It is the result of dividing the
> largest size_t by sizeof(T).  So long as n is less than this value, the
product
> with sizeof(T) will always avoid an overflow (in a size_t capable variable).
> 
> What am I missing?

Done.

https://codereview.chromium.org/986503002/diff/1540001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1540001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:25: prev_ranked_entries_ =
std::move(ranked_entries_);
Now with std::move semantics.

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

https://codereview.chromium.org/986503002/diff/1540001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.cc:12: RankedList&
RankedList::operator= (RankedList&& other) {
Implemented std::move support.

https://codereview.chromium.org/986503002/diff/1540001/components/metrics/lea...
File components/metrics/leak_detector/ranked_list_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1540001/components/metrics/lea...
components/metrics/leak_detector/ranked_list_unittest.cc:267:
TEST_F(RankedListTest, MoveOperation) {
Test RankedList std::move support.

[Simon Que, 2015-08-23 23:31:37]

Unit test for CallStackTable has been added.

[haraken, 2015-08-23 23:33:19]

haraken@chromium.org changed reviewers:
- haraken@chromium.org

[Simon Que, 2015-08-25 04:13:50]

Patchset #26 (id:1480001) has been deleted

[Simon Que, 2015-08-25 04:20:33]

Patchset #29 (id:1560001) has been deleted

[Simon Que, 2015-08-26 22:36:33]

Patchset #29 (id:1580001) has been deleted

[Simon Que, 2015-08-27 20:41:32]

sque@chromium.org changed reviewers:
+ dhsharp@chromium.org, rapati@chromium.org

[Simon Que, 2015-08-27 22:05:34]

Patchset #29 (id:1600001) has been deleted

[Simon Que, 2015-08-31 05:47:03]

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

https://codereview.chromium.org/986503002/diff/1320001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:47: // with the same value.
On 2015/08/23 23:30:53, Simon Que wrote:
> On 2015/08/21 02:48:43, jar (doing other things) wrote:
> > nit: Add comment that this is an O(N) insertion into a vector.
> 
> Now a O(1) insertion into a linked list. :)

Actually that's wrong... it's O(N) due to ordering.

OTOH, the new implementation makes it trivial to replace the list with a
std::set if necessary.

[wtc, 2015-09-01 00:28:14]

wtc@chromium.org changed reviewers:
- wtc@chromium.org

[Simon Que, 2015-09-01 21:09:48]

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:417:
JuliaSet(false);
Comment on flag meaning.

[bjanakiraman1, 2015-09-01 21:27:24]

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

[bjanakiraman1, 2015-09-01 21:27:27]

I took a quick look at this main test that checks the leak detector, seems
reasonable, see inline comments

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:3: // found in
the LICENSE file.
Add more comments on what exactly this test is doing (brief explanation).

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:95: class
LeakDetectorImplTest : public ::testing::Test {
Add some comments on what methods / classes in the leak detector this is
testing.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:131: // Alloc
and free functions that automatically pass allocation info to
Alloc and free are emulating what tcmalloc hook function does. This is okay, add
some comments to that effect here,

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:191: // free the
leaked pointers at the end.
Explain why this is present (duplicate bookkeeping).

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:430: 
Explain where KrawStack* comes from.

[bjanakiraman1, 2015-09-01 21:42:37]

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

[commit-bot: I haz the power, 2015-09-01 21:43:41]

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

[commit-bot: I haz the power, 2015-09-01 21:46:22]

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

[commit-bot: I haz the power, 2015-09-01 21:46:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bjanakiraman1, 2015-09-01 21:48:55]

Mostly, comments about adding better explanations. Otherwise LGTM.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:138:
EXPECT_EQ(0U, manager.size());
In general, add a brief comment at the beginning saying what its testing.
Similar to the test above, which explains the purpose clearly.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:122: for (int i = 0;
i < 10; ++i) {
Instead of 10, use kDefaultLeakThreshold + <n>

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:235: for (int i = 0;
i < 100; ++i) {
Change 100 to kDefaultLeakThreshold + <n> (similar to previous comments)

[Simon Que, 2015-09-01 23:23:31]

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:138:
EXPECT_EQ(0U, manager.size());
On 2015/09/01 21:48:55, bjanakiraman1 wrote:
> In general, add a brief comment at the beginning saying what its testing.
> Similar to the test above, which explains the purpose clearly.

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:122: for (int i = 0;
i < 10; ++i) {
On 2015/09/01 21:48:55, bjanakiraman1 wrote:
> Instead of 10, use kDefaultLeakThreshold + <n> 

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:235: for (int i = 0;
i < 100; ++i) {
On 2015/09/01 21:48:55, bjanakiraman1 wrote:
> Change 100 to kDefaultLeakThreshold + <n> (similar to previous comments)

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:3: // found in
the LICENSE file.
On 2015/09/01 21:27:27, bjanakiraman1 wrote:
> Add more comments on what exactly this test is doing (brief explanation).

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:95: class
LeakDetectorImplTest : public ::testing::Test {
On 2015/09/01 21:27:27, bjanakiraman1 wrote:
> Add some comments on what methods / classes in the leak detector this is
> testing. 

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:131: // Alloc
and free functions that automatically pass allocation info to
On 2015/09/01 21:27:27, bjanakiraman1 wrote:
> Alloc and free are emulating what tcmalloc hook function does. This is okay,
add
> some comments to that effect here, 

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:191: // free the
leaked pointers at the end.
On 2015/09/01 21:27:27, bjanakiraman1 wrote:
> Explain why this is present (duplicate bookkeeping). 

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:417:
JuliaSet(false);
On 2015/09/01 21:09:48, Simon Que wrote:
> Comment on flag meaning.

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:430: 
On 2015/09/01 21:27:27, bjanakiraman1 wrote:
> Explain where KrawStack* comes from.

Done. No longer directly referencing kRawStack* anymore.

[jar (doing other things), 2015-09-04 18:01:56]

Incomplete review... I only looked at the unit tests....

I still need to look around to see if other files need tests... and then review
as a whole.

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
components/metrics/leak_detector/call_stack_manager_unittest.cc:20:
reinterpret_cast<const void*>(0x55667788),
I noticed all these addresses were 32 bits long.  Is this code intended to work
on 64 bit platforms?

You could use the address of various items to transparently acquire the "native"
pointer sizes.

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
components/metrics/leak_detector/call_stack_manager_unittest.cc:23: const void*
kRawStack1[] = {
For purposes of testing... it was nice that this stack had the same length as
kRawStack0. That strengthened the tests.

IMO, you may as well make it nearly identical, with only one central element
differing.  Maybe something like (I'm not sure if this is legal... but you could
do it with shared constants if not):

const void* kRawStack[] = {
  kRawStack0[0],
  kRawStack0[1],
  reinterpret_cast<const void*>(0x87654321),  // Or make it a function of the
related value.
  kRowStack0[2],
};

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
components/metrics/leak_detector/call_stack_manager_unittest.cc:121:
manager.GetCallStack(arraysize(kRawStack3), kRawStack3);
Please add:
EXPECT_EQ(arraysize(kRawStack3), stack->depth);

...or better... would be to add these expectations instead around lines 66-80,
so that it is clear that the attribute is established.

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
components/metrics/leak_detector/call_stack_manager_unittest.cc:133:
manager.GetCallStack(stack->depth - 4, stack->stack)->hash);
This is good... but it just checks that similar base stacks have distinct
hashes.  For instance, if only the last entry in the stack were consulted (and
earlier values were ignored), this would all pass.

You could probably add:
  EXPECT_NE(stack->hash,
            manager.GetCallStack(stack->depth - 1, stack->stack + 1)->hash);
  EXPECT_NE(stack->hash,
            manager.GetCallStack(stack->depth - 2, stack->stack + 2)->hash);
  EXPECT_NE(stack->hash,
            manager.GetCallStack(stack->depth - 3, stack->stack + 3)->hash);
  EXPECT_NE(stack->hash,
            manager.GetCallStack(stack->depth - 4, stack->stack + 4)->hash);

https://chromiumcodereview.appspot.com/986503002/diff/1660001/components/metr...
components/metrics/leak_detector/call_stack_manager_unittest.cc:145:
manager.GetCallStack(arraysize(kRawStack0), kRawStack0);
Better might be to replicate the stack into new memory before passing data to
GetCallStack().  This would make it clear that the contents of the stack were
replicated (when the internal records were kept), and the address of the stack
data argument was not used in the calculations (of the hash, etc.)

[Simon Que, 2015-09-07 06:40:32]

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:20:
reinterpret_cast<const void*>(0x55667788),
On 2015/09/04 18:01:56, jar (doing other things) wrote:
> I noticed all these addresses were 32 bits long.  Is this code intended to
work
> on 64 bit platforms?
> 
> You could use the address of various items to transparently acquire the
"native"
> pointer sizes.

Done. I'll rely on truncation from 64 to 32 bits on 32-bit platforms instead.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:23: const void*
kRawStack1[] = {
On 2015/09/04 18:01:56, jar (doing other things) wrote:
> For purposes of testing... it was nice that this stack had the same length as
> kRawStack0. That strengthened the tests.
> 
> IMO, you may as well make it nearly identical, with only one central element
> differing.  Maybe something like (I'm not sure if this is legal... but you
could
> do it with shared constants if not):
> 
> const void* kRawStack[] = {
>   kRawStack0[0],
>   kRawStack0[1],
>   reinterpret_cast<const void*>(0x87654321),  // Or make it a function of the
> related value.
>   kRowStack0[2],
> };

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:121:
manager.GetCallStack(arraysize(kRawStack3), kRawStack3);
On 2015/09/04 18:01:56, jar (doing other things) wrote:
> Please add:
> EXPECT_EQ(arraysize(kRawStack3), stack->depth);
> 
> ...or better... would be to add these expectations instead around lines 66-80,
> so that it is clear that the attribute is established.

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:133:
manager.GetCallStack(stack->depth - 4, stack->stack)->hash);
On 2015/09/04 18:01:56, jar (doing other things) wrote:
> This is good... but it just checks that similar base stacks have distinct
> hashes.  For instance, if only the last entry in the stack were consulted (and
> earlier values were ignored), this would all pass.
> 
> You could probably add:
>   EXPECT_NE(stack->hash,
>             manager.GetCallStack(stack->depth - 1, stack->stack + 1)->hash);
>   EXPECT_NE(stack->hash,
>             manager.GetCallStack(stack->depth - 2, stack->stack + 2)->hash);
>   EXPECT_NE(stack->hash,
>             manager.GetCallStack(stack->depth - 3, stack->stack + 3)->hash);
>   EXPECT_NE(stack->hash,
>             manager.GetCallStack(stack->depth - 4, stack->stack + 4)->hash);
> 
> 

Done.

https://codereview.chromium.org/986503002/diff/1660001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:145:
manager.GetCallStack(arraysize(kRawStack0), kRawStack0);
On 2015/09/04 18:01:56, jar (doing other things) wrote:
> Better might be to replicate the stack into new memory before passing data to
> GetCallStack().  This would make it clear that the contents of the stack were
> replicated (when the internal records were kept), and the address of the stack
> data argument was not used in the calculations (of the hash, etc.)

Done.

[Simon Que, 2015-09-11 18:18:12]

Patchset #36 (id:1760001) has been deleted

[Simon Que, 2015-09-11 18:18:25]

Patchset #35 (id:1740001) has been deleted

[jar (doing other things), 2015-09-25 18:00:34]

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

[jar (doing other things), 2015-09-25 18:00:36]

Adding Will Harris as a reviewer.  Hopefully he can give this the attention that
was overdue from me.

He has taken on a OWNER position in TCMalloc... and can approve the changes as
he sees fit.

[Simon Que, 2015-09-25 18:16:54]

On 2015/09/25 18:00:36, jar (doing other things) wrote:
> Adding Will Harris as a reviewer.  Hopefully he can give this the attention
that
> was overdue from me.
> 
> He has taken on a OWNER position in TCMalloc... and can approve the changes as
> he sees fit.

Actually I'm updating this so that we don't need to add anything to TCMalloc.
Will's review may not be needed after that.

I'm replacing the spinlock wrapper with using pthread_spinlock_t in
leak_detector.cc.
I'm replacing the custom allocator with a pool allocator that gets its memory
from the standard allocator (disables the hooks temporarily). See here for more
info:
https://docs.google.com/presentation/d/1Lm--ZX42cQODCH4vwh0f7qMVIKAXHvoT2Aidv...

[bjanakiraman1, 2015-09-25 19:23:35]

Simon,
If not Will, who is going to be reviewing the changes?

If this is removing complexity / reducing functionality, then its fine, but
if its adding more code to something that's not yet fully reviewed, its
problematic.

To be clear, I'm not opposed to using a pool allocator for performance
improvement, but we should get the basic functionality reviewed and checked
in.

Thanks,
Bhaskar


On Fri, Sep 25, 2015 at 11:16 AM, <sque@chromium.org> wrote:

> On 2015/09/25 18:00:36, jar (doing other things) wrote:
>
>> Adding Will Harris as a reviewer.  Hopefully he can give this the
>> attention
>>
> that
>
>> was overdue from me.
>>
>
> He has taken on a OWNER position in TCMalloc... and can approve the
>> changes as
>> he sees fit.
>>
>
> Actually I'm updating this so that we don't need to add anything to
> TCMalloc.
> Will's review may not be needed after that.
>
> I'm replacing the spinlock wrapper with using pthread_spinlock_t in
> leak_detector.cc.
> I'm replacing the custom allocator with a pool allocator that gets its
> memory
> from the standard allocator (disables the hooks temporarily). See here for
> more
> info:
>
>
https://docs.google.com/presentation/d/1Lm--ZX42cQODCH4vwh0f7qMVIKAXHvoT2Aidv...
>
> https://codereview.chromium.org/986503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-09-25 19:38:49]

On 2015/09/25 19:23:35, bjanakiraman1 wrote:
> Simon,
> If not Will, who is going to be reviewing the changes?
> 
> If this is removing complexity / reducing functionality, then its fine, but
> if its adding more code to something that's not yet fully reviewed, its
> problematic.
> 
> To be clear, I'm not opposed to using a pool allocator for performance
> improvement, but we should get the basic functionality reviewed and checked
> in.
> 
> Thanks,
> Bhaskar
> 

I just think it'd be poor form to add some wrapper classes to tcmalloc and then
have to remove them later when we don't use them.

I can skip the pool allocator and just have it disable the hooks before using
new/delete internally. That reduces the amount of extra code to review.

[Will Harris, 2015-09-26 05:09:06]

https://codereview.chromium.org/986503002/diff/1820001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/986503002/diff/1820001/build/common.gypi#newc...
build/common.gypi:2770: 'defines': ['ENABLE_REMOTING=1'],
driveby: this looks like a confused rebase?

[Simon Que, 2015-09-26 21:48:33]

https://codereview.chromium.org/986503002/diff/1820001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/986503002/diff/1820001/build/common.gypi#newc...
build/common.gypi:2770: 'defines': ['ENABLE_REMOTING=1'],
On 2015/09/26 05:09:06, Will Harris wrote:
> driveby: this looks like a confused rebase?

Done.

[Simon Que, 2015-10-28 23:00:45]

Here is the design doc viewable by chromium.org accounts:

https://docs.google.com/document/d/1Rq7NCQ4IaMhT5jO3Z1nsq1tZ1YAC6NR7X2MWsEVqZ...

[jar (doing other things), 2015-11-02 19:37:34]

You might consider adding at least "comment access" to the doc (currently
we can only "view," but say nothing).

Also, just as an FYI (which you may already know): You can visit the url:
about:tcmalloc
and see a lot of info about the heap.  This may lead to other hooks that
can be exploited... but perhaps you are already there.

Jim

On Wed, Oct 28, 2015 at 4:00 PM, <sque@chromium.org> wrote:

> Here is the design doc viewable by chromium.org accounts:
>
>
>
https://docs.google.com/document/d/1Rq7NCQ4IaMhT5jO3Z1nsq1tZ1YAC6NR7X2MWsEVqZ...
>
> https://codereview.chromium.org/986503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2015-11-05 01:39:05]

sque@chromium.org changed reviewers:
- bccheng@chromium.org, bjanakiraman@chromium.org, dhsharp@chromium.org,
gpike@chromium.org, jar@chromium.org, kouhei@chromium.org, lizeb@chromium.org,
mal@chromium.org, rapati@chromium.org

[Will Harris, 2015-11-11 17:33:02]

In general this looks okay for first commit of the core analytical code, but I
have a few questions and concerns.

1. Is this only for ChromeOS or will this ever be expanded to other platforms?

2. I really worry about how you are getting data out of the leak analyzer - it
seems to dump the results as LOG or into strings. I would like to see evidence
of a way forward for data to be pulled out of the leak analyzer some other way -
e.g. can you separate the part that does all of the output to logs to a separate
class LeakAnalyzerLogger or LeakAnalyzerDumper or something, that calls into
LeakAnalyzer and dumps the data. There shouldn't be LOG(ERROR) in the core code,
or any of the sprintfs. I'd like to see clean interfaces in/out of the
analytical engine.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:42: snprintf(line,
sizeof(line), "%d: %s\n", getpid(), str);
use base::GetCurrentProcId()

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:72: int result =
static_cast<int>(size / sizeof(uint32_t));
Max 64-bit unsigned int divided by 4 is larger than INT_MAX, what happens for
these large allocations? Perhaps check for this before the division?

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:160: if (iter ==
address_map_.end())
might it be good to catch these double frees?

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:192:
size_leak_analyzer_.AddSample(std::move(size_ranked_list));
I don't think C++11 move semantics are allowed in chromium yet. use
MOVE_ONLY_TYPE_FOR_CPP_03 in LeakAnalyzer class.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:274: if (ptr_value >=
mapping_addr_ && ptr_value < mapping_addr_ + mapping_size_)
can you explain how this works with allocations that are outside the mapping of
the main executable, or for binaries that have a lot of dynamically loaded
modules, is there one copy of LeakDetectorImpl per loaded module?

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.h (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.h:10: #include
<unordered_map>
can't use unordered_map yet - see https://chromium-cpp.appspot.com/ - use
base::hash_map instead.

(but see very recent chromium-dev post by thakis.)

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:65: uint32_t size_;
why is this not size_t?

[Simon Que, 2015-11-12 01:06:52]

Description was changed from

==========
components/metrics: Add memory leak detector

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

The leak detector is activated by a GYP flag that is set
to off by default. It must be manually built with the GYP
flag turned on.

BUG=chromium:382705

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

to

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

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

The leak detector is activated by a GYP flag that is set
to off by default. It must be manually built with the GYP
flag turned on.

BUG=chromium:382705

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

[Simon Que, 2015-11-12 17:29:24]

Removed logging.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.cc (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:42: snprintf(line,
sizeof(line), "%d: %s\n", getpid(), str);
On 2015/11/11 17:33:01, Will Harris wrote:
> use base::GetCurrentProcId()

Done.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:72: int result =
static_cast<int>(size / sizeof(uint32_t));
On 2015/11/11 17:33:01, Will Harris wrote:
> Max 64-bit unsigned int divided by 4 is larger than INT_MAX, what happens for
> these large allocations? Perhaps check for this before the division?

Done.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:160: if (iter ==
address_map_.end())
On 2015/11/11 17:33:01, Will Harris wrote:
> might it be good to catch these double frees?

Good idea. I'll leave a TODO here for a future revision.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:192:
size_leak_analyzer_.AddSample(std::move(size_ranked_list));
On 2015/11/11 17:33:01, Will Harris wrote:
> I don't think C++11 move semantics are allowed in chromium yet. use
> MOVE_ONLY_TYPE_FOR_CPP_03 in LeakAnalyzer class.

Done.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.cc:274: if (ptr_value >=
mapping_addr_ && ptr_value < mapping_addr_ + mapping_size_)
On 2015/11/11 17:33:01, Will Harris wrote:
> can you explain how this works with allocations that are outside the mapping
of
> the main executable, or for binaries that have a lot of dynamically loaded
> modules, is there one copy of LeakDetectorImpl per loaded module?

I'm not sure what those special cases are, as I only intend for this to work
with the main Chrome module. This code will be activated from the main Chrome
code (browser and renderer).

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl.h (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl.h:10: #include
<unordered_map>
On 2015/11/11 17:33:01, Will Harris wrote:
> can't use unordered_map yet - see https://chromium-cpp.appspot.com/ - use
> base::hash_map instead.
> 
> (but see very recent chromium-dev post by thakis.)

Done.

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1900001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:65: uint32_t size_;
On 2015/11/11 17:33:02, Will Harris wrote:
> why is this not size_t?

Done.

[Simon Que, 2015-11-12 19:44:14]

I've added links to the documents to the crbug (see BUG= line).

[Will Harris, 2015-11-12 22:38:01]

Description was changed from

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

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

The leak detector is activated by a GYP flag that is set
to off by default. It must be manually built with the GYP
flag turned on.

BUG=chromium:382705

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

to

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

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

BUG=382705

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

[Will Harris, 2015-11-13 17:46:37]

Almost there!

How are you managing thread safety in calls to RecordAlloc and RecordFree?
Depending on how you plan to shim the allocator these could possibly come in on
any thread - if the expectation is that this class is only called from the same
thread it's instantiated on, then this should be documented and checked for.

[Simon Que, 2015-11-13 18:23:42]

On 2015/11/13 17:46:37, Will Harris wrote:
> Almost there!
> 
> How are you managing thread safety in calls to RecordAlloc and RecordFree?
> Depending on how you plan to shim the allocator these could possibly come in
on
> any thread - if the expectation is that this class is only called from the
same
> thread it's instantiated on, then this should be documented and checked for.

That will be handled in leak_detector.cc, which stitches it all together with
the allocator. See previous patch sets that still contain leak_detector.cc, e.g.
https://codereview.chromium.org/986503002/diff/1880001/components/metrics/lea...

There are spinlocks that will enforce atomic access to LeakDetectorImpl and its
constituents.

[Will Harris, 2015-11-13 19:51:12]

On 2015/11/13 18:23:42, Simon Que wrote:
> On 2015/11/13 17:46:37, Will Harris wrote:
> > Almost there!
> > 
> > How are you managing thread safety in calls to RecordAlloc and RecordFree?
> > Depending on how you plan to shim the allocator these could possibly come in
> on
> > any thread - if the expectation is that this class is only called from the
> same
> > thread it's instantiated on, then this should be documented and checked for.
> 
> That will be handled in leak_detector.cc, which stitches it all together with
> the allocator. See previous patch sets that still contain leak_detector.cc,
e.g.
>
https://codereview.chromium.org/986503002/diff/1880001/components/metrics/lea...
> 
> There are spinlocks that will enforce atomic access to LeakDetectorImpl and
its
> constituents.

Okay that code can be reviewed in a future CL then. It might be nice to say in
the comments that this code is not thread safe.

lgtm on this CL.

Please ask a metrics owner to l-g-t-m adding this to components/metrics as per
email thread.

[Simon Que, 2015-11-13 20:15:33]

Description was changed from

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

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

BUG=382705

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

to

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

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

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

BUG=382705

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

[Simon Que, 2015-11-13 20:15:34]

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

[Alexei Svitkine (slow), 2015-11-13 20:51:28]

lgtm % comments

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/OWNERS (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/OWNERS:1: sque@chromium.org
OWNERS files are recommended to have at least two people. Is there a second
person that should be added here? Maybe Will?

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:22: void
LeakAnalyzer::AddSample(RankedList ranked_list) {
Why is this taking RankedList by value? Either it should be a const ref or a
pointer.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:44: if (ranked_deltas.size() >
1) {
Nit: !.empty()

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.cc (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:32: switch(type_) {
Nit: Missing space after switch. Suggest running "git cl format".

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:37: default:
Nit: It's best not to have default: cases in switches. This way, if a new switch
value is added, it forces the switches to be updated, since compilers will warn
otherwise.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:21: kNone,
Nit: enums should use FOO_BAR syntax per Chromium style guide

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

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:25: class RankedList {
Add a comment explaing what this class is for.

[Simon Que, 2015-11-13 23:44:57]

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/OWNERS (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/OWNERS:1: sque@chromium.org
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> OWNERS files are recommended to have at least two people. Is there a second
> person that should be added here? Maybe Will?

I'll add him once he confirms that it is OK.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer.cc (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:22: void
LeakAnalyzer::AddSample(RankedList ranked_list) {
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Why is this taking RankedList by value? Either it should be a const ref or a
> pointer.

This is meant to use move semantics, to avoid unnecessary copying when assigning
to |ranked_entries_|. I've updated the comment in the header file to reflect
this usage.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer.cc:44: if (ranked_deltas.size() >
1) {
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Nit: !.empty()

This is checking that there is more than 1 element.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.cc (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:32: switch(type_) {
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Nit: Missing space after switch. Suggest running "git cl format".

Done.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.cc:37: default:
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Nit: It's best not to have default: cases in switches. This way, if a new
switch
> value is added, it forces the switches to be updated, since compilers will
warn
> otherwise.

Done.

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_value_type.h (right):

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_value_type.h:21: kNone,
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Nit: enums should use FOO_BAR syntax per Chromium style guide

Done.

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

https://codereview.chromium.org/986503002/diff/1940001/components/metrics/lea...
components/metrics/leak_detector/ranked_list.h:25: class RankedList {
On 2015/11/13 20:51:27, Alexei Svitkine (slow) wrote:
> Add a comment explaing what this class is for.

Done.

[jar (doing other things), 2015-11-14 02:58:33]

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

[jar (doing other things), 2015-11-14 02:58:37]

high level comment: 

I didn't see how you would hook into the allocator (and free) calls, but I'm a
bit concerned that the allocator will be called on many threads, and there does
not seem to be code for mutex protecting access to your data structures.  Is the
assumption that some master/central lock will be held before any calls into your
code?... or did I miss the locking code?

Additional comments below.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:23: call_stack->depth *
sizeof(*call_stack->stack));
nit: Perhaps a tiny comment indicating why the structures pointed to (or are
they just addresses into the stack segment??) are not deleted.

nit: personal preference/cleanliness: I find it is less error prone to clear out
pointers as I free areas.  In this case, it seems nice to follow this Free with:
call_stack->stack = NULL;
call_stack->depth = 0;

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:34: temp.stack =
const_cast<const void**>(stack);
nit: perhaps it would be nicer to have declared the |stack| member as:

const void*const *

and then I think you wouldn't need the cast here.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:46: CallStack* call_stack
=
nit: Would it have been nicer to declare CallStack as a class, including a
constructor (so you don't need to clear the memory via memset), and probably a
class specific operator new and operator delete (so you don't need to keep
mentioning your custom allocator)?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:62: return c1->depth ==
c2->depth &&
nit: Is it worth also validating that the hash matches, as that should be much
cheaper than the full compare?... or are you presuming that we'd never be called
if the hashes didn't match?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager.h (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:18: 
It might be nice to have a pre-amble here, describing the structures, their
lifetimes, producers, and consumers.  That might reduce the amount of commentary
needed inside the actual classes and structs.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:22: const void** stack;   
                // Call stack as an array of addrs.
Can you provide more info about this data structure?

Who owns it?  Who needs to call free(?) or delete on the array of pointers?  

Who owns the items pointed to by pointers on the stack?  What are they?

I suspect that |depth| is the dimension of the |stack[]| array, but this should
be explicit.

Can there be nulls in stack[]? Is it null terminated?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:36: // call stack object.
This also confused me about ownership and lifetime.  Can you add a comment about
that?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:127:
EXPECT_NE(0U, stack0->hash);
The hash function doesn't guarantee non-zero return (I think).

What you can always assume is that with enough differences in the data, that the
hash will be different from any given constant.  To do this, you typically
iterate until you get a "different hash" and then declare victory.  It is
probably too much of a hasssle to try to validate the quality of the hash
function... but I think you are agreeing with your tests... and just want to
make suer the dependency is on the data you meant to depend upon.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:133:
EXPECT_NE(stack0->hash, stack1->hash);
The hash function doesn't guarantee that you won't have colisions.

What you can check is that the hash of the same stack does produce the same hash
in two distinct CallStackManagers.

You can also test that *eventually* (if you try enough stacks!) that you'll get
different hashes.

The code, as written, seems brittle, relative to the definition of the
base::Hash function.

It is probably not the end of the world... but uninitialized memory would be
enough to (usually) pass this test.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:150:
manager.GetCallStack(stack->depth - 1, stack->stack)->hash);
Again, this test seems backwards, as hash collisions are never precluded.  

I think you *can* test to see that the hash remains constant (i.e., a constant
function of the data you *expect* it to hash), when you modify the element
beyond |depth|.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:16: // Get the top
|kRankedListSize| entries.
I don't understand this comment.  Perhaps the comment could read:

We only maintain a list of the top |kRankedListSize| entries.

If that is what this variable is for, perhaps a better variable name might be:

kMaxCountOfSuspciousStacks

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:19: // Initial number of
hash table buckets.
nit: your choice... but the name of the variable seems clear... and this comment
then seems redundant (unless you wanted to add some subtlety).

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:46: entry =
&entry_map_[call_stack];
Would it have been enough to just do this operator[]?  Doesn't it work in both
cases (whether find succeeded or failed, inducing the insertion)?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:74: if
(entry.net_num_allocs > 0) {
Given line 65, which seems to discard any entries with net_num_allocs==0, when
is this condition false?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/custom_allocator.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/custom_allocator.cc:40: g_alloc_func = nullptr;
I'm curious how this will be done in a race-free manner.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/custom_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/custom_allocator.h:19: // functions to use as
an external allocator.
I'm not yet clear how you "hook in," but how do you prevent your custom
allocator from being instrumented (recording stats recursively in your leak
analyzer)?

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:104: // to run
|kDefaultLeakThreshold + 1| times to trigger a leak.
nit: Several times you said "...trigger a leak" but I think you meant "trigger a
report of a leak."

Same thing later in the file.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:339:
list.Add(Size(32), 10 + i * 40);  // *
nit: Remove these trailing comments.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:212: // The
center region of the complex plane that is the basis for our Julia set
I was amazed/surprised to see code working with Complex numbers.  What is the
point of this?

Please motivate this code.  This sure looks to be adding more complexity than is
desirable in a unit test.

[Simon Que, 2015-11-17 00:28:48]

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:23: call_stack->depth *
sizeof(*call_stack->stack));
On 2015/11/14 02:58:35, jar (doing other things) wrote:
> nit: Perhaps a tiny comment indicating why the structures pointed to (or are
> they just addresses into the stack segment??) are not deleted.

They are just addresses into the stack segment -- the addresses themselves are
the data values.

If this is not clear, I wonder if I'm better off storing the addresses as an
array of uintptr_t's.

> nit: personal preference/cleanliness: I find it is less error prone to clear
out
> pointers as I free areas.  In this case, it seems nice to follow this Free
with:
> call_stack->stack = NULL;
> call_stack->depth = 0;

Done

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:34: temp.stack =
const_cast<const void**>(stack);
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> nit: perhaps it would be nicer to have declared the |stack| member as:
> 
> const void*const *
> 
> and then I think you wouldn't need the cast here.

The problem is that we wouldn't be able to make a copy of the stack on line 54.
The |stack| member would need to be non-const. OTOH, I use this pointer
assignment to avoid a full-stack copy in case the CallStack object has already
been created.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:46: CallStack* call_stack
=
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> nit: Would it have been nicer to declare CallStack as a class, including a
> constructor (so you don't need to clear the memory via memset), and probably a
> class specific operator new and operator delete (so you don't need to keep
> mentioning your custom allocator)?
> 
> 

Actually the memset is not necessary, removed.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.cc:62: return c1->depth ==
c2->depth &&
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> nit: Is it worth also validating that the hash matches, as that should be much
> cheaper than the full compare?... or are you presuming that we'd never be
called
> if the hashes didn't match?

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager.h (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:18: 
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> It might be nice to have a pre-amble here, describing the structures, their
> lifetimes, producers, and consumers.  That might reduce the amount of
commentary
> needed inside the actual classes and structs.

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:22: const void** stack;   
                // Call stack as an array of addrs.
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> Can you provide more info about this data structure?
> 
> Who owns it?  Who needs to call free(?) or delete on the array of pointers?  
> 
> Who owns the items pointed to by pointers on the stack?  What are they?
> 
> I suspect that |depth| is the dimension of the |stack[]| array, but this
should
> be explicit.
> 
> Can there be nulls in stack[]? Is it null terminated?

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager.h:36: // call stack object.
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> This also confused me about ownership and lifetime.  Can you add a comment
about
> that?

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_manager_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:127:
EXPECT_NE(0U, stack0->hash);
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> The hash function doesn't guarantee non-zero return (I think).

Yes. The assumption here is that the test call stacks I've chosen all have
nonzero hashes. But thinking about it some more, these checks aren't that
useful.

> 
> What you can always assume is that with enough differences in the data, that
the
> hash will be different from any given constant.  To do this, you typically
> iterate until you get a "different hash" and then declare victory.  It is
> probably too much of a hasssle to try to validate the quality of the hash
> function... but I think you are agreeing with your tests... and just want to
> make suer the dependency is on the data you meant to depend upon.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:133:
EXPECT_NE(stack0->hash, stack1->hash);
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> The hash function doesn't guarantee that you won't have colisions.
> 
> What you can check is that the hash of the same stack does produce the same
hash
> in two distinct CallStackManagers.
> 
> You can also test that *eventually* (if you try enough stacks!) that you'll
get
> different hashes.
> 
> The code, as written, seems brittle, relative to the definition of the
> base::Hash function.
> 
> It is probably not the end of the world... but uninitialized memory would be
> enough to (usually) pass this test.

This test is not designed to make sure the hash function is generating unique
hashes, but that the CallStackManager is properly storing the hashes in
CallStack structs.

If there is a collision, then I should be choosing different sets of raw call
stacks that will generate unique hashes.

I've added a test to check for same hashes from different CallStackMgrs.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_manager_unittest.cc:150:
manager.GetCallStack(stack->depth - 1, stack->stack)->hash);
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> Again, this test seems backwards, as hash collisions are never precluded.  
> 
> I think you *can* test to see that the hash remains constant (i.e., a constant
> function of the data you *expect* it to hash), when you modify the element
> beyond |depth|.  

See comment above.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/call_stack_table.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:16: // Get the top
|kRankedListSize| entries.
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> I don't understand this comment.  Perhaps the comment could read:
> 
> We only maintain a list of the top |kRankedListSize| entries.
> 
> If that is what this variable is for, perhaps a better variable name might be:
> 
> kMaxCountOfSuspciousStacks

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:19: // Initial number of
hash table buckets.
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> nit: your choice... but the name of the variable seems clear... and this
comment
> then seems redundant (unless you wanted to add some subtlety).

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:46: entry =
&entry_map_[call_stack];
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> Would it have been enough to just do this operator[]?  Doesn't it work in both
> cases (whether find succeeded or failed, inducing the insertion)?

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/call_stack_table.cc:74: if
(entry.net_num_allocs > 0) {
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> Given line 65, which seems to discard any entries with net_num_allocs==0, when
> is this condition false?

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/custom_allocator.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/custom_allocator.cc:40: g_alloc_func = nullptr;
On 2015/11/14 02:58:36, jar (doing other things) wrote:
> I'm curious how this will be done in a race-free manner.

Again, see this code:
https://codereview.chromium.org/986503002/diff/1880001/components/metrics/lea...

Any access to LeakDetectorImpl will be guarded by a lock. All other classes are
dependencies of LeakDetectorImpl and will likewise be protected. See class
hierarchy here:
https://docs.google.com/drawings/d/1lp5VVFAOu7vg0aFTCa_4T4AQNh_HPFRGNZr0Tz8i4...

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/custom_allocator.h (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/custom_allocator.h:19: // functions to use as
an external allocator.
On 2015/11/14 02:58:37, jar (doing other things) wrote:
> I'm not yet clear how you "hook in," but how do you prevent your custom
> allocator from being instrumented (recording stats recursively in your leak
> analyzer)?

It was originally included in this CL, but since it's not being used anywhere,
I've deleted it. Here it is from a previous patch set:
https://codereview.chromium.org/986503002/diff/1880001/components/metrics/lea...

See "InternalAlloc" and "MallocHookDisabler" -- the tcmalloc hooks are removed
to do the allocation, and then reinstated when done.

This may or may not generate a lot of overhead but I also plan to add a local
allocator in a subsequent CL to reduce the number of allocs/frees along this
path. I already have it written up and it is much more efficient than the
LowLevelAlloc used in tcmalloc.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/leak_analyzer_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:104: // to run
|kDefaultLeakThreshold + 1| times to trigger a leak.
On 2015/11/14 02:58:37, jar (doing other things) wrote:
> nit: Several times you said "...trigger a leak" but I think you meant "trigger
a
> report of a leak."
> 
> Same thing later in the file.

Done.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_analyzer_unittest.cc:339:
list.Add(Size(32), 10 + i * 40);  // *
On 2015/11/14 02:58:37, jar (doing other things) wrote:
> nit: Remove these trailing comments.

They're designed to clarify to the reader which deltas will be selected.

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_impl_unittest.cc (right):

https://codereview.chromium.org/986503002/diff/1960001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_impl_unittest.cc:212: // The
center region of the complex plane that is the basis for our Julia set
On 2015/11/14 02:58:37, jar (doing other things) wrote:
> I was amazed/surprised to see code working with Complex numbers.  What is the
> point of this?
> 
> Please motivate this code.  This sure looks to be adding more complexity than
is
> desirable in a unit test.

I tried to come up with a self-contained program that does something other than
purposefully leak memory. This is to simulate a real-life leak scenario where
the developer is leaking in a not-so-obvious way -- there are several alloc
sites but not all of them are leaky, which simulates accidentally forgetting to
free at one site.

This code is not intended to be modified going forward except to fix bugs or
optimize for performance. If we were to add new functionality to
LeakDetectorImpl (e.g. false positive detection) in the future, we should write
a new piece of test code that has exactly that scenario, rather than adding some
false positive pattern to the JuliaSet code.

That said, I do think this function is too long. I don't know how to reduce it
significantly, except for choosing a lower value of |n| for the Julia Set
function. But that would result in fewer call sites... I'm trying to keep more
than one of both leaky and non-leaky call sites, currently 2 and 3 respectively.

[Simon Que, 2015-11-17 00:34:18]

On 2015/11/14 02:58:37, jar (doing other things) wrote:
> high level comment: 
> 
> I didn't see how you would hook into the allocator (and free) calls, but I'm a
> bit concerned that the allocator will be called on many threads, and there
does
> not seem to be code for mutex protecting access to your data structures.  Is
the
> assumption that some master/central lock will be held before any calls into
your
> code?... or did I miss the locking code?
> 

Yes, there will be. See my response to your comment in custom_allocator.cc.

I mentioned in the CL description that this code by itself is not thread-safe.
Does it need any other indicators of non-thread-safety?

[Will Harris, 2015-11-17 00:38:19]

> I mentioned in the CL description that this code by itself is not thread-safe.
> Does it need any other indicators of non-thread-safety?

Threading expectations should be in the code, not the CL, in case someone tries
to call it with a different expectation.

[Simon Que, 2015-11-18 04:06:29]

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

[Simon Que, 2015-11-18 04:06:33]

+jochen to review components/components_tests.gyp

[jochen (gone - plz use gerrit), 2015-11-18 16:15:35]

jochen@chromium.org changed reviewers:
+ bjanakiraman@chromium.org, gpike@chromium.org

[jochen (gone - plz use gerrit), 2015-11-18 16:15:37]

https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gyp...
components/metrics.gypi:107: 'target_name': 'metrics_leak_detector',
where's the gn version?

https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
File components/metrics/leak_detector/OWNERS (right):

https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
components/metrics/leak_detector/OWNERS:1: sque@chromium.org
only chromium committers can be owners

[Simon Que, 2015-11-19 23:44:57]

https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gypi
File components/metrics.gypi (right):

https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gyp...
components/metrics.gypi:107: 'target_name': 'metrics_leak_detector',
On 2015/11/18 16:15:37, jochen wrote:
> where's the gn version?

Done.

https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
File components/metrics/leak_detector/OWNERS (right):

https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
components/metrics/leak_detector/OWNERS:1: sque@chromium.org
On 2015/11/18 16:15:37, jochen wrote:
> only chromium committers can be owners

I'm pretty sure I'm a chromium committer... I see myself in the Chromium
committers list.

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

On 2015/11/19 at 23:44:57, sque wrote:
> https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gypi
> File components/metrics.gypi (right):
> 
>
https://codereview.chromium.org/986503002/diff/2040001/components/metrics.gyp...
> components/metrics.gypi:107: 'target_name': 'metrics_leak_detector',
> On 2015/11/18 16:15:37, jochen wrote:
> > where's the gn version?
> 
> Done.
> 
>
https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
> File components/metrics/leak_detector/OWNERS (right):
> 
>
https://codereview.chromium.org/986503002/diff/2040001/components/metrics/lea...
> components/metrics/leak_detector/OWNERS:1: sque@chromium.org
> On 2015/11/18 16:15:37, jochen wrote:
> > only chromium committers can be owners
> 
> I'm pretty sure I'm a chromium committer... I see myself in the Chromium
committers list.

oops, sorry, my mistake

[jochen (gone - plz use gerrit), 2015-11-20 13:39:48]

lgtm

[Simon Que, 2015-11-21 00:30:40]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-11-21 00:30:40]

The patchset sent to the CQ was uploaded after l-g-t-m from gpike@chromium.org,
thakis@chromium.org, bjanakiraman@chromium.org, wfh@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/986503002/#ps2080001
(title: "Add GN build dependency on chromeos")

[commit-bot: I haz the power, 2015-11-21 00:34:18]

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

[commit-bot: I haz the power, 2015-11-21 00:56:28]

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

[commit-bot: I haz the power, 2015-11-21 00:56:31]

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

[Simon Que, 2015-11-21 01:42:36]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-11-21 01:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
thakis@chromium.org, asvitkine@chromium.org, gpike@chromium.org,
bjanakiraman@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/986503002/#ps2100001
(title: "Remove explicit zero initialization of struct-vector (done
implicitly)")

[commit-bot: I haz the power, 2015-11-21 01:44:29]

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

[commit-bot: I haz the power, 2015-11-21 03:00:05]

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

[commit-bot: I haz the power, 2015-11-21 03:00:08]

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

[Simon Que, 2015-11-21 05:59:03]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2015-11-21 05:59:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
thakis@chromium.org, asvitkine@chromium.org, gpike@chromium.org,
bjanakiraman@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/986503002/#ps2120001
(title: "Mac build fixes: const arg in comparator, rm const in func return
type")

[commit-bot: I haz the power, 2015-11-21 05:59:21]

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

[commit-bot: I haz the power, 2015-11-21 08:04:05]

Committed patchset #52 (id:2120001)

[commit-bot: I haz the power, 2015-11-21 08:04:52]

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

[Will Harris, 2015-11-21 08:06:20]

On 2015/11/21 08:04:52, commit-bot: I haz the power wrote:
> Patchset 52 (id:??) landed as
> https://crrev.com/d02b109cb1f41b7ea0584d0910c8bb4cf21cb521
> Cr-Commit-Position: refs/heads/master@{#361012}

Woot :)

[tsergeant, 2015-11-23 00:06:56]

A revert of this CL (patchset #52 id:2120001) has been created in
https://codereview.chromium.org/1472693002/ by tsergeant@chromium.org.

The reason for reverting is: This CL has been causing the builder Linux
ChromiumOS Tests (dbg)(1) to fail:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....