Add leakDetectionMetric for tracing

Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters between 1st & last memory dump. This is a part of Real-World Leak Detector project (Design doc: https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0 ).

BUG=chromium:763280
Review-Url: https://chromiumcodereview.appspot.com/3012153002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/3f9a1ba77bbbe6ccf9551db7d05354296c555526

[yuzuchan, 2017-09-11 06:50:00]

Description was changed from

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).
BUG=catapult:#
==========

to

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).

BUG=763280
==========

[yuzuchan, 2017-09-11 07:11:50]

yuzus@chromium.org changed reviewers:
+ haraken@chromium.org, keishi@chromium.org

[yuzuchan, 2017-09-11 07:11:52]

PTAL :) 

This is how I feel about this patch: https://i.imgur.com/wSyzUa7.gif

[keishi, 2017-09-11 07:34:16]

Looks good so far. I think every metric requires a test.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:41:
histogram.addSample(sumCounter[key]);
I think we should skip adding a sample if chromeDumps doesn't have two dumps to
compare.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:48: const
timeDurationInMs_smallerIsBetter =
unused?

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:68: function
createNumericForBlinkObjectCounts(name) {
nit: Existing similar functions seem to use use singular

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:82: function
splitGlobalDumpsByBrowserName(model) {
I think we shouldn't have duplicate code so maybe move this to
system_health/memory_utils.html and import that?

[yuzuchan, 2017-09-13 04:46:44]

Description was changed from

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).

BUG=763280
==========

to

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).

BUG=chromium:763280
==========

[yuzuchan, 2017-09-13 04:48:57]

I haven't written a test yet, but addressed some of the comments.

I am going to use this patch to run cluster telemetry, so no review is needed at
this point before I clean up the code & add a test.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:41:
histogram.addSample(sumCounter[key]);
On 2017/09/11 at 07:34:16, keishi wrote:
> I think we should skip adding a sample if chromeDumps doesn't have two dumps
to compare.

Added an error logic here. Thanks.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:48: const
timeDurationInMs_smallerIsBetter =
On 2017/09/11 at 07:34:16, keishi wrote:
> unused?

True! Done.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:68: function
createNumericForBlinkObjectCounts(name) {
On 2017/09/11 at 07:34:16, keishi wrote:
> nit: Existing similar functions seem to use use singular

Done.

https://codereview.chromium.org/3012153002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/blink/leak_detection_metric.html:82: function
splitGlobalDumpsByBrowserName(model) {
On 2017/09/11 at 07:34:16, keishi wrote:
> I think we shouldn't have duplicate code so maybe move this to
system_health/memory_utils.html and import that?

Done.

[yuzuchan, 2017-09-13 11:32:03]

PTAL again :) 

This is how I feel about this patch: https://i.imgur.com/wC3u6cx.gif

[yuzuchan, 2017-09-14 09:23:34]

yuzus@chromium.org changed reviewers:
+ benjhayden@chromium.org

[yuzuchan, 2017-09-14 09:23:35]

PTAL :) 

This is how I feel about this patch: https://i.imgur.com/uLetEul.gif

[keishi, 2017-09-14 09:40:46]

non owner LGTM The test looks great.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:77: throw new
Error('Memory dump did not happen twice, and thus leak could' +
nit: Maybe something more concise like "Expected at least two memory dumps." or
"Missing two memory dumps".

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
File tracing/tracing/model/memory_allocator_dump.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
tracing/tracing/model/memory_allocator_dump.html:115: addChildren(array) {
I don't know about naming conventions in tracing but isn't this more like "set"
rather than "add"?

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
File tracing/tracing/model/memory_dump_test_utils.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
tracing/tracing/model/memory_dump_test_utils.html:112: *     numerics:
(!Object<string, (number|!tr.b.Scalar)>|undefined)
add jsdoc for children

[haraken, 2017-09-14 14:21:07]

non-owner LGTM

[benjhayden, 2017-09-14 19:18:51]

Mostly style nits.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:23: let sumCounter =
{};
Please use an ES6 Map instead:
let sumCounter = new Map();
sumCounter.set(key, count);
for (const [key, count] of sumCounter) {...}

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:25:
pids.forEach(function(pid) {
Please use for..of loops instead of forEach and for..in.
for (const pid of pids) { ... }
for (const [key, counter] of Object.entries(counters)) { ... }

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:26: if
(Object.keys(sumCounter).length === 0) {
Is this just merging the maps, or is there something trickier happening?

Merging maps is usually written more simply:
for (const [key, count] of countLeakedBlinkObjects(chromeDumps, pid)) {
  sumCounter.set(key, (sumCounter.get(key) || 0) + count);
}

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:38: const histogram =
createNumericForBlinkObjectCount(key);
Please use the existing helper function histograms.createHistogram() directly
instead of a new helper function so that I can help you maintain this if I need
to refactor the API.
histograms.createHistogram(key, tr.b.Unit.byName.count_smallerIsBetter,
sumCounter[key], {
  summaryOptions: {
    max: false,
    min: false,
    std: false,
  },
});
https://github.com/catapult-project/catapult/issues/3591

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:46: function
extractRendererPids(model) {
This looks short enough that you can inline it into its only caller.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:62: const n = new
tr.v.Histogram(name, count_smallerIsBetter);
Please prepend the name with something like 'leak:' to disambiguate this from
any other metric that might process these counters.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:82: const diffCounter =
{};
Please use an ES6 Map instead.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:94:
.find(isBlinkObjects);
Please use an arrow function instead:
find(dump => dump.fullName === 'blink_objects')

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric_test.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric_test.html:55: function
createBlinkObjectCount(renderer, name, value) {
This looks short enough that you can inline it to its only caller.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric_test.html:60: function
createMapFromHistograms(histograms) {
Please let the tests test the histograms directly instead of using this helper
function.
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

[yuzuchan, 2017-09-15 08:32:25]

Thank you so much for the review. PTAL again :)


This is how I feel about this patch: https://i.imgur.com/4SVFLTP.gif

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:23: let sumCounter =
{};
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please use an ES6 Map instead:
> let sumCounter = new Map();
> sumCounter.set(key, count);
> for (const [key, count] of sumCounter) {...}

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:25:
pids.forEach(function(pid) {
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please use for..of loops instead of forEach and for..in.
> for (const pid of pids) { ... }
> for (const [key, counter] of Object.entries(counters)) { ... }

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:26: if
(Object.keys(sumCounter).length === 0) {
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Is this just merging the maps, or is there something trickier happening?
> 
> Merging maps is usually written more simply:
> for (const [key, count] of countLeakedBlinkObjects(chromeDumps, pid)) {
>   sumCounter.set(key, (sumCounter.get(key) || 0) + count);
> }

Right! Thanks for teaching me the simple way.
Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:38: const histogram =
createNumericForBlinkObjectCount(key);
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please use the existing helper function histograms.createHistogram() directly
instead of a new helper function so that I can help you maintain this if I need
to refactor the API.
> histograms.createHistogram(key, tr.b.Unit.byName.count_smallerIsBetter,
sumCounter[key], {
>   summaryOptions: {
>     max: false,
>     min: false,
>     std: false,
>   },
> });
> https://github.com/catapult-project/catapult/issues/3591

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:46: function
extractRendererPids(model) {
On 2017/09/14 at 19:18:50, benjhayden wrote:
> This looks short enough that you can inline it into its only caller.

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:62: const n = new
tr.v.Histogram(name, count_smallerIsBetter);
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please prepend the name with something like 'leak:' to disambiguate this from
any other metric that might process these counters.

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:77: throw new
Error('Memory dump did not happen twice, and thus leak could' +
On 2017/09/14 at 09:40:46, keishi wrote:
> nit: Maybe something more concise like "Expected at least two memory dumps."
or "Missing two memory dumps".

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:82: const diffCounter =
{};
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please use an ES6 Map instead.

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:94:
.find(isBlinkObjects);
On 2017/09/14 at 19:18:50, benjhayden wrote:
> Please use an arrow function instead:
> find(dump => dump.fullName === 'blink_objects')

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric_test.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric_test.html:55: function
createBlinkObjectCount(renderer, name, value) {
On 2017/09/14 at 19:18:51, benjhayden wrote:
> This looks short enough that you can inline it to its only caller.

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric_test.html:60: function
createMapFromHistograms(histograms) {
On 2017/09/14 at 19:18:51, benjhayden wrote:
> Please let the tests test the histograms directly instead of using this helper
function.
>
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

Done.

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
File tracing/tracing/model/memory_allocator_dump.html (right):

https://codereview.chromium.org/3012153002/diff/140001/tracing/tracing/model/...
tracing/tracing/model/memory_allocator_dump.html:115: addChildren(array) {
On 2017/09/14 at 09:40:46, keishi wrote:
> I don't know about naming conventions in tracing but isn't this more like
"set" rather than "add"?

I mimicked the other functions in this file, so I'm pretty sure add is
appropriate here.

[benjhayden, 2017-09-15 17:30:46]

Thanks! Just a few more nits. This will probably be the last or penultimate
round of comments from me.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:21: if
(Object.keys(modelHelper).length === 0) {
getOrCreateHelper returns undefined if Chrome is not present, and
Object.keys(undefined) throws a TypeError.
if (modelHelper === undefined)

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:44: const histogram =
histograms.createHistogram('Leaked ' + key,
No need to define the unused variable 'histogram'.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:45:
tr.b.Unit.byName.count_smallerIsBetter, sumCounter.get(key), {
s/sumCounter.get(key)/count/

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:48: count: true,
This is the default value of count so it doesn't need to be specified.
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu...

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:52: sum: true,
This is the default value of sum.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:67: for (const key of
lastCounter.keys()) {
for (const [key, lastCount] of lastCounter) {
  diffCounter.set(key, lastCount - firstCounter.get(key));
}

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:76: if (pid in
processesMemoryDumps) {
We generally recommend using early-returns to reduce indentation:
if (processesMemoryDumps[pid] === undefined) return counter;

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:79:
blinkObjectsDump.children.forEach(function(v) {
Please use for (const v of blinkObjectsDump.children) { ... }

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/utils.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/utils.html:137: makeKeyUniqueAndSet,
It looks like this doesn't need to be exported.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/utils.html:138:
skipDumpsThatDoNotIntersectRange
It looks like this doesn't need to be exported.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/model/...
File tracing/tracing/model/memory_dump_test_utils.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/model/...
tracing/tracing/model/memory_dump_test_utils.html:127:
dump.addChildren(getOption(opt_options, 'children'));
MemoryAllocatorDump doesn't protect its children property, and newAllocatorDump
is test util, so I think it's ok for newAllocatorDump to set it directly instead
of adding a helper method to MemoryAllocatorDump:
const children = getOption(opt_options, 'children');
if (children) dump.children = children;

[yuzuchan, 2017-09-19 07:56:26]

Thank you very much again for the review! Hope I addressed all the points you
mentioned.
PTAL again :) 


This is how I feel about this patch: https://i.imgur.com/6ykmJLf.gif

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
File tracing/tracing/metrics/blink/leak_detection_metric.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:21: if
(Object.keys(modelHelper).length === 0) {
On 2017/09/15 at 17:30:46, benjhayden wrote:
> getOrCreateHelper returns undefined if Chrome is not present, and
Object.keys(undefined) throws a TypeError.
> if (modelHelper === undefined)

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:44: const histogram =
histograms.createHistogram('Leaked ' + key,
On 2017/09/15 at 17:30:46, benjhayden wrote:
> No need to define the unused variable 'histogram'.

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:45:
tr.b.Unit.byName.count_smallerIsBetter, sumCounter.get(key), {
On 2017/09/15 at 17:30:46, benjhayden wrote:
> s/sumCounter.get(key)/count/

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:48: count: true,
On 2017/09/15 at 17:30:46, benjhayden wrote:
> This is the default value of count so it doesn't need to be specified.
>
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu...

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:52: sum: true,
On 2017/09/15 at 17:30:45, benjhayden wrote:
> This is the default value of sum.

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:67: for (const key of
lastCounter.keys()) {
On 2017/09/15 at 17:30:46, benjhayden wrote:
> for (const [key, lastCount] of lastCounter) {
>   diffCounter.set(key, lastCount - firstCounter.get(key));
> }

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:76: if (pid in
processesMemoryDumps) {
On 2017/09/15 at 17:30:45, benjhayden wrote:
> We generally recommend using early-returns to reduce indentation:
> if (processesMemoryDumps[pid] === undefined) return counter;

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/blink/leak_detection_metric.html:79:
blinkObjectsDump.children.forEach(function(v) {
On 2017/09/15 at 17:30:46, benjhayden wrote:
> Please use for (const v of blinkObjectsDump.children) { ... }

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/utils.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/utils.html:137: makeKeyUniqueAndSet,
On 2017/09/15 at 17:30:46, benjhayden wrote:
> It looks like this doesn't need to be exported.

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/utils.html:138:
skipDumpsThatDoNotIntersectRange
On 2017/09/15 at 17:30:46, benjhayden wrote:
> It looks like this doesn't need to be exported.

Done.

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/model/...
File tracing/tracing/model/memory_dump_test_utils.html (right):

https://codereview.chromium.org/3012153002/diff/180001/tracing/tracing/model/...
tracing/tracing/model/memory_dump_test_utils.html:127:
dump.addChildren(getOption(opt_options, 'children'));
On 2017/09/15 at 17:30:46, benjhayden wrote:
> MemoryAllocatorDump doesn't protect its children property, and
newAllocatorDump is test util, so I think it's ok for newAllocatorDump to set it
directly instead of adding a helper method to MemoryAllocatorDump:
> const children = getOption(opt_options, 'children');
> if (children) dump.children = children;

Done.

[benjhayden, 2017-09-20 05:02:35]

lgtm, thanks!

[yuzuchan, 2017-09-20 06:34:35]

The CQ bit was checked by yuzus@chromium.org

[yuzuchan, 2017-09-20 06:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, keishi@chromium.org
Link to the patchset: https://codereview.chromium.org/3012153002/#ps220001
(title: "Reflect comments")

[commit-bot: I haz the power, 2017-09-20 06:34:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-20 07:06:51]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1505889275316020,
"parent_rev": "8a5fbb70cbd389a350928aa0bad3ca190c43168d", "commit_rev":
"3f9a1ba77bbbe6ccf9551db7d05354296c555526"}

[commit-bot: I haz the power, 2017-09-20 07:06:55]

Description was changed from

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).

BUG=chromium:763280
==========

to

==========
Add leakDetectionMetric for tracing

This CL adds leakDetectionMetric which shows you a diff of blink object counters
between 1st & last memory dump. This is a part of Real-World Leak Detector
project (Design doc:
https://docs.google.com/document/d/1wUWa7dWUdvr6dLdYHFfMQdnvgzt7lrrvzYfpAK-_6e0
).

BUG=chromium:763280

Review-Url: https://chromiumcodereview.appspot.com/3012153002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-09-20 07:06:57]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...