tracing/tracing/metrics/blink/leak_detection_metric.html - Issue 3012153002: Add leakDetectionMetric for tracing

Side by Side Diff: tracing/tracing/metrics/blink/leak_detection_metric.html

Issue 3012153002: Add leakDetectionMetric for tracing (Closed)

Patch Set: Bug fix Created 3 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « tracing/tracing/metrics/all_metrics.html ('k') | tracing/tracing/metrics/blink/leak_detection_metric_test.html » ('j') | tracing/tracing/metrics/blink/leak_detection_metric_test.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <!--

	3 Copyright 2017 The Chromium Authors. All rights reserved.

	4 Use of this source code is governed by a BSD-style license that can be

	5 found in the LICENSE file.

	6 -->

	7

	8 <link rel="import" href="/tracing/base/unit.html">

	9 <link rel="import" href="/tracing/metrics/metric_registry.html">

	10 <link rel="import" href="/tracing/metrics/system_health/utils.html">

	11 <link rel="import" href="/tracing/value/histogram.html">

	12

	13 <script>

	14 'use strict';

	15

	16 tr.exportTo('tr.metrics.blink', function() {

	17 function leakDetectionMetric(histograms, model) {

	18 const pids = extractRendererPids(model);

	19

	20 const chromeDumps = tr.metrics.sh

	21 .splitGlobalDumpsByBrowserName(model, undefined).get('chrome');

	22

	23 let sumCounter = {};
	benjhayden 2017/09/14 19:18:50 Please use an ES6 Map instead: let sumCounter = ne Please use an ES6 Map instead: let sumCounter = new Map(); sumCounter.set(key, count); for (const [key, count] of sumCounter) {...} yuzuchan 2017/09/15 08:32:25 Done. Show quoted text 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.
	24 // Add up counters for all the renderer processes.

	25 pids.forEach(function(pid) {
	benjhayden 2017/09/14 19:18:50 Please use for..of loops instead of forEach and fo Please use for..of loops instead of forEach and for..in. for (const pid of pids) { ... } for (const [key, counter] of Object.entries(counters)) { ... } yuzuchan 2017/09/15 08:32:24 Done. Show quoted text 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.
	26 if (Object.keys(sumCounter).length === 0) {
	benjhayden 2017/09/14 19:18:50 Is this just merging the maps, or is there somethi 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); } yuzuchan 2017/09/15 08:32:24 Right! Thanks for teaching me the simple way. Done Show quoted text 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.
	27 sumCounter = countLeakedBlinkObjects(chromeDumps, pid);

	28 } else {

	29 const counterForAnotherRenderer =

	30 countLeakedBlinkObjects(chromeDumps, pid);

	31 for (const key in counters) {

	32 sumCounter[key] += counterForAnotherRenderer[key];

	33 }

	34 }

	35 });

	36

	37 for (const key in sumCounter) {

	38 const histogram = createNumericForBlinkObjectCount(key);
	benjhayden 2017/09/14 19:18:50 Please use the existing helper function histograms 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 yuzuchan 2017/09/15 08:32:25 Done. Show quoted text 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.
	39 histogram.addSample(sumCounter[key]);

	40 histograms.addHistogram(histogram);

	41 }

	42 }

	43

	44 tr.metrics.MetricRegistry.register(leakDetectionMetric);

	45

	46 function extractRendererPids(model) {
	benjhayden 2017/09/14 19:18:50 This looks short enough that you can inline it int This looks short enough that you can inline it into its only caller. yuzuchan 2017/09/15 08:32:25 Done. Show quoted text On 2017/09/14 at 19:18:50, benjhayden wrote: > This looks short enough that you can inline it into its only caller. Done.
	47 const modelHelper = model.getOrCreateHelper(

	48 tr.model.helpers.ChromeModelHelper);

	49 if (Object.keys(modelHelper).length === 0) {

	50 throw new Error('Chrome is not present.');

	51 }

	52 const rendererHelpers = modelHelper.rendererHelpers;

	53 if (Object.keys(rendererHelpers).length === 0) {

	54 throw new Error('Renderer process is not present.');

	55 }

	56 return Object.keys(rendererHelpers);

	57 }

	58

	59 const count_smallerIsBetter = tr.b.Unit.byName.count_smallerIsBetter;

	60

	61 function createNumericForBlinkObjectCount(name) {

	62 const n = new tr.v.Histogram(name, count_smallerIsBetter);
	benjhayden 2017/09/14 19:18:50 Please prepend the name with something like 'leak: Please prepend the name with something like 'leak:' to disambiguate this from any other metric that might process these counters. yuzuchan 2017/09/15 08:32:24 Done. Show quoted text 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.
	63 n.customizeSummaryOptions({

	64 avg: false,

	65 count: true,

	66 max: false,

	67 min: false,

	68 std: false,

	69 sum: true,

	70 percentile: []

	71 });

	72 return n;

	73 }

	74

	75 function countLeakedBlinkObjects(dumps, pid) {

	76 if (dumps === undefined \|\| dumps.length < 2) {

	77 throw new Error('Memory dump did not happen twice, and thus leak could' +
	keishi 2017/09/14 09:40:46 nit: Maybe something more concise like "Expected a nit: Maybe something more concise like "Expected at least two memory dumps." or "Missing two memory dumps". yuzuchan 2017/09/15 08:32:24 Done. Show quoted text 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.
	78 ' not be calculated.');

	79 }

	80 const firstCounter = countBlinkObjects(dumps[0], pid);

	81 const lastCounter = countBlinkObjects(dumps[dumps.length - 1], pid);

	82 const diffCounter = {};
	benjhayden 2017/09/14 19:18:50 Please use an ES6 Map instead. Please use an ES6 Map instead. yuzuchan 2017/09/15 08:32:24 Done. Show quoted text On 2017/09/14 at 19:18:50, benjhayden wrote: > Please use an ES6 Map instead. Done.
	83 for (const key in lastCounter) {

	84 diffCounter[key] = lastCounter[key] - firstCounter[key];

	85 }

	86 return diffCounter;

	87 }

	88

	89 function countBlinkObjects(dump, pid) {

	90 const counter = {};

	91 const processesMemoryDumps = dump.processMemoryDumps;

	92 if (pid in processesMemoryDumps) {

	93 const blinkObjectsDump = processesMemoryDumps[pid].memoryAllocatorDumps

	94 .find(isBlinkObjects);
	benjhayden 2017/09/14 19:18:50 Please use an arrow function instead: find(dump => Please use an arrow function instead: find(dump => dump.fullName === 'blink_objects') yuzuchan 2017/09/15 08:32:24 Done. Show quoted text On 2017/09/14 at 19:18:50, benjhayden wrote: > Please use an arrow function instead: > find(dump => dump.fullName === 'blink_objects') Done.
	95 blinkObjectsDump.children.forEach(function(v) {

	96 counter[v.name] = v.numerics.object_count.value;

	97 });

	98 }

	99 return counter;

	100 }

	101

	102 function isBlinkObjects(memoryDump) {

	103 return memoryDump.fullName === 'blink_objects';

	104 }

	105

	106 return {

	107 leakDetectionMetric,

	108 };

	109 });

	110 </script>

OLD	NEW