Store alloc history data in memory leak report protobuf

Store alloc history data in memory leak report protobuf

BUG=chromium:382705
TEST=unit tests pass

Committed: https://crrev.com/26e212b95ec412ff489e0eb8679285cb5df20228
Cr-Commit-Position: refs/heads/master@{#387393}

[Simon Que, 2016-04-11 20:29:15]

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

[Simon Que, 2016-04-11 20:31:25]

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:53: // to merge the two.
I wonder if LeakDetector should just create a protobuf here and use MergeFrom()
in LeakDetectorController to merge with the template.

[Will Harris, 2016-04-11 21:11:51]

not too familiar with the protobuf aspects of metrics, so I'm happy for
asvitkine to review this entire CL and since sque@ is owner of
components/metrics/leak_detector no l-g-t-m is needed from me. thanks!

[Will Harris, 2016-04-11 21:12:01]

Description was changed from

==========
Store alloc history data in memory leak report protobuf

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

to

==========
Store alloc history data in memory leak report protobuf

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

[Will Harris, 2016-04-11 21:12:01]

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

[Simon Que, 2016-04-11 21:13:15]

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

[Simon Que, 2016-04-11 21:13:15]

Adding Ilya as reviewer since he reviewed my protobuf code.

[Ilya Sherman, 2016-04-13 00:07:12]

LGTM % nits, thanks.

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

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:9: #include <algorithm>  //
For std::transform
nit: I'd omit the comment, since it's likely to drift out of date.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:9: #include <algorithm>  //
For std::transform
... though, actually, you don't seem to use std::transform in the code.  Is this
include just not needed in the latest patch set?

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:125:
report.alloc_breakdown_history().size());
I think you can skip this reserve() call, unless the vector size is actually
quite large.  It makes the code somewhat more complex to read, whereas I think
the performance benefit is likely to be negligible.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:128:
new_entry.counts_by_size.reserve(entry.counts_by_size.size());
Ditto here, especially since you write the vector contents in a single call,
rather than adding elements one at a time.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:53: // to merge the two.
On 2016/04/11 20:31:25, Simon Que wrote:
> I wonder if LeakDetector should just create a protobuf here and use
MergeFrom()
> in LeakDetectorController to merge with the template.

Yeah, that seems pretty reasonable.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:77:
std::vector<AllocationBreakdown> alloc_breakdown_history;
Optional nit: Mebbe s/alloc/allocation, given the type name?

[Simon Que, 2016-04-13 22:41:31]

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

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:9: #include <algorithm>  //
For std::transform
On 2016/04/13 00:07:12, Ilya Sherman wrote:
> ... though, actually, you don't seem to use std::transform in the code.  Is
this
> include just not needed in the latest patch set?

Yes, I forgot to delete it when I no longer used it. But I'm using it in the new
patch set.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:125:
report.alloc_breakdown_history().size());
On 2016/04/13 00:07:12, Ilya Sherman wrote:
> I think you can skip this reserve() call, unless the vector size is actually
> quite large.  It makes the code somewhat more complex to read, whereas I think
> the performance benefit is likely to be negligible.

Done.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:128:
new_entry.counts_by_size.reserve(entry.counts_by_size.size());
On 2016/04/13 00:07:12, Ilya Sherman wrote:
> Ditto here, especially since you write the vector contents in a single call,
> rather than adding elements one at a time.

Done.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:53: // to merge the two.
On 2016/04/13 00:07:12, Ilya Sherman wrote:
> On 2016/04/11 20:31:25, Simon Que wrote:
> > I wonder if LeakDetector should just create a protobuf here and use
> MergeFrom()
> > in LeakDetectorController to merge with the template.
> 
> Yeah, that seems pretty reasonable.

Done.

https://codereview.chromium.org/1868193003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:77:
std::vector<AllocationBreakdown> alloc_breakdown_history;
On 2016/04/13 00:07:12, Ilya Sherman wrote:
> Optional nit: Mebbe s/alloc/allocation, given the type name?

The corresponding field in the protobuf is "alloc_breakdown_history" so I
decided to match that.

[Ilya Sherman, 2016-04-14 01:08:46]

Thanks.  Still LGTM % nits:

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.cc:119:
proto->CheckTypeAndMergeFrom(report);
Hmm, is the "CheckTypeAnd" prefix necessary?  That is, could you just call
MergeFrom()?

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller_unittest.cc (right):

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller_unittest.cc:20: // is only used
for.
nit: s/is only/is (or otherwise rephrase)

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

https://codereview.chromium.org/1868193003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:111: for (auto
call_stack_entry : report.call_stack())
nit: Most Chromium code that I've seen prefers to always use curly braces with
loops, even one-line loops.

[Simon Que, 2016-04-14 01:24:31]

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.cc:119:
proto->CheckTypeAndMergeFrom(report);
On 2016/04/14 01:08:45, Ilya Sherman wrote:
> Hmm, is the "CheckTypeAnd" prefix necessary?  That is, could you just call
> MergeFrom()?

I thought it was not available because MemoryLeakReportProto uses LITE_RUNTIME
and I didn't think MergeFrom() was part of MessageLite's API:
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...

It turns out that replacing CheckTypeAndMergeFrom() with MergeFrom() still
works, so maybe I'm missing something.

I also wonder if the params in MemoryLeakReportProto should be in their own
separate nested message field. This way we can just store
MemoryLeakReportProto_Params and copy it to the |params| field in each
MemoryLeakReportProto.

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller_unittest.cc (right):

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller_unittest.cc:20: // is only used
for.
On 2016/04/14 01:08:45, Ilya Sherman wrote:
> nit: s/is only/is (or otherwise rephrase)

Done.

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

https://codereview.chromium.org/1868193003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:111: for (auto
call_stack_entry : report.call_stack())
On 2016/04/14 01:08:45, Ilya Sherman wrote:
> nit: Most Chromium code that I've seen prefers to always use curly braces with
> loops, even one-line loops.

Done.

[Ilya Sherman, 2016-04-14 02:30:05]

(Still LGTM)

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1868193003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.cc:119:
proto->CheckTypeAndMergeFrom(report);
On 2016/04/14 01:24:30, Simon Que wrote:
> On 2016/04/14 01:08:45, Ilya Sherman wrote:
> > Hmm, is the "CheckTypeAnd" prefix necessary?  That is, could you just call
> > MergeFrom()?
> 
> I thought it was not available because MemoryLeakReportProto uses LITE_RUNTIME
> and I didn't think MergeFrom() was part of MessageLite's API:
>
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...
> 
> It turns out that replacing CheckTypeAndMergeFrom() with MergeFrom() still
> works, so maybe I'm missing something.

Yeah, the API docs on that site are not the clearest.  I think the message.h
docs include a bunch of docs about related classes as well, which are available
even when using MessageLite.  (I inferred that MergeFrom() would be usable here
because it was documented as being called from CheckTyepAndMergeFrom().)

> I also wonder if the params in MemoryLeakReportProto should be in their own
> separate nested message field. This way we can just store
> MemoryLeakReportProto_Params and copy it to the |params| field in each
> MemoryLeakReportProto.

Sure, that seems like a reasonable design choice.

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

https://codereview.chromium.org/1868193003/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:118:
proto_entry->add_counts_by_size(count);
nit: Curly braces here too

[Simon Que, 2016-04-14 03:04:49]

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

https://codereview.chromium.org/1868193003/diff/60001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:118:
proto_entry->add_counts_by_size(count);
On 2016/04/14 02:30:04, Ilya Sherman wrote:
> nit: Curly braces here too

Done.

[Simon Que, 2016-04-14 03:05:01]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-04-14 03:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1868193003/#ps80001
(title: "More curly braces")

[commit-bot: I haz the power, 2016-04-14 03:05:20]

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

[commit-bot: I haz the power, 2016-04-14 03:12:59]

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

[commit-bot: I haz the power, 2016-04-14 03:13:00]

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

[Simon Que, 2016-04-14 06:39:53]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-04-14 06:39:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1868193003/#ps100001
(title: "Update build deps")

[commit-bot: I haz the power, 2016-04-14 06:40:08]

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

[commit-bot: I haz the power, 2016-04-14 06:47:39]

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

[commit-bot: I haz the power, 2016-04-14 06:47:39]

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

[Simon Que, 2016-04-14 06:53:06]

Patchset #6 (id:100001) has been deleted

[Simon Que, 2016-04-14 07:20:01]

On 2016/04/14 06:47:39, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Getting this build error, which I am not able to reproduce locally:

In file included from
../../components/metrics/leak_detector/leak_detector_unittest.cc:5:
In file included from ../../components/metrics/leak_detector/leak_detector.h:21:
gen/components/metrics/proto/memory_leak_report.pb.h:9:10: fatal error:
'google/protobuf/stubs/common.h' file not found

[Simon Que, 2016-04-14 19:25:09]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-04-14 19:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1868193003/#ps140001
(title: "Add dependency on protobufs to leak detector tests")

[commit-bot: I haz the power, 2016-04-14 19:25:24]

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

[commit-bot: I haz the power, 2016-04-14 19:32:33]

Description was changed from

==========
Store alloc history data in memory leak report protobuf

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

to

==========
Store alloc history data in memory leak report protobuf

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

[commit-bot: I haz the power, 2016-04-14 19:32:34]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-04-14 19:34:07]

Description was changed from

==========
Store alloc history data in memory leak report protobuf

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

to

==========
Store alloc history data in memory leak report protobuf

BUG=chromium:382705
TEST=unit tests pass

Committed: https://crrev.com/26e212b95ec412ff489e0eb8679285cb5df20228
Cr-Commit-Position: refs/heads/master@{#387393}
==========

[commit-bot: I haz the power, 2016-04-14 19:34:09]

Patchset 7 (id:??) landed as
https://crrev.com/26e212b95ec412ff489e0eb8679285cb5df20228
Cr-Commit-Position: refs/heads/master@{#387393}