tracing/tracing/metrics/estimated_input_latency_metric.html - Issue 2323533003: [Not for landing - CL being split] Add Estimated Input Latency - EQT 90th Percentile definition

Side by Side Diff: tracing/tracing/metrics/estimated_input_latency_metric.html

Issue 2323533003: [Not for landing - CL being split] Add Estimated Input Latency - EQT 90th Percentile definition

Patch Set: Review comments + rebase on master Created 4 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/estimated_input_latency_metric_test.html » ('j') | tracing/tracing/metrics/estimated_input_latency_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 (c) 2015 The Chromium Authors. All rights reserved.
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: this should be: Copyright 2016 The Chromium nit: this should be: Copyright 2016 The Chromium Authors. All rights reserved. The new copyright doesn't require a (c), and should reflect the current year. dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: this should be: > > Copyright 2016 The Chromium Authors. All rights reserved. > > The new copyright doesn't require a (c), and should reflect the current year. > Done.
	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/category_util.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/model/helpers/chrome_model_helper.html">

	12 <link rel="import" href="/tracing/model/user_model/load_expectation.html">

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

	14

	15 <script>

	16 'use strict';

	17

	18 tr.exportTo('tr.metrics', function() {

	19

	20 var TOPLEVEL_CATEGORY_NAME = 'toplevel';

	21

	22 // TODO(dproy): Remove after we close #2784

	23 function hasCategoryAndName(event, category, title) {

	24 return event.title === title && event.category &&

	25 tr.b.getCategoryParts(event.category).indexOf(category) !== -1;
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: line continuations are indented by 4 characte nit: line continuations are indented by 4 characters, not 2 dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > nit: line continuations are indented by 4 characters, not 2 Done.
	26 }

	27

	28 function runConditionallyOnInnermostDescendants(slice, predicate, cb) {
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: what is an innermost descendant? Are these li nit: what is an innermost descendant? Are these like leaves of the tree? dproy 2016/09/20 01:11:43 This function is a little unfortunately named but Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: what is an innermost descendant? Are these like leaves of the tree? This function is a little unfortunately named but I couldn't find a better word. It works like this: The callback cb is executed on this slice only if the predicate is true on this slice and the predicate is not true on any descendant. Then recurse this on all the children. What this function is used for in this CL is to find the innermost toplevel slices. We have some slices containing the category toplevel and we want to act on those, but if a toplevel slice contains other toplevel tasks I want to ignore the outer toplevel task and only want to act on the inner toplevel slices. I added a js-doc comment for this function. benjhayden 2016/09/20 05:52:35 Thanks for the explanation! Would it be possible t Show quoted text On 2016/09/20 at 01:11:43, dproy1 wrote: > On 2016/09/19 14:35:33, charliea wrote: > > nit: what is an innermost descendant? Are these like leaves of the tree? > > This function is a little unfortunately named but I couldn't find a better word. It works like this: The callback cb is executed on this slice only if the predicate is true on this slice and the predicate is not true on any descendant. Then recurse this on all the children. > > What this function is used for in this CL is to find the innermost toplevel slices. We have some slices containing the category toplevel and we want to act on those, but if a toplevel slice contains other toplevel tasks I want to ignore the outer toplevel task and only want to act on the inner toplevel slices. > > I added a js-doc comment for this function. Thanks for the explanation! Would it be possible to simplify this code by modifying Chrome's tracing macros? Is there a conceivable future where there's a single layer of "toplevel" slices, or a few layers of "roof" and "penthouse" slices that would be easier to find and distinguish between? If so, can you follow-up with fmeawad about discussing this at the instrumentation weekly? I won't block landing this function, but I think it would be better if this function and the whole idea of innermost descendant were not needed. dproy 2016/09/20 15:21:06 One way to get around this would be to just look f Show quoted text On 2016/09/20 05:52:35, benjhayden wrote: > On 2016/09/20 at 01:11:43, dproy1 wrote: > > On 2016/09/19 14:35:33, charliea wrote: > > > nit: what is an innermost descendant? Are these like leaves of the tree? > > > > This function is a little unfortunately named but I couldn't find a better > word. It works like this: The callback cb is executed on this slice only if the > predicate is true on this slice and the predicate is not true on any descendant. > Then recurse this on all the children. > > > > What this function is used for in this CL is to find the innermost toplevel > slices. We have some slices containing the category toplevel and we want to act > on those, but if a toplevel slice contains other toplevel tasks I want to ignore > the outer toplevel task and only want to act on the inner toplevel slices. > > > > I added a js-doc comment for this function. > > Thanks for the explanation! > Would it be possible to simplify this code by modifying Chrome's tracing macros? > Is there a conceivable future where there's a single layer of "toplevel" slices, > or a few layers of "roof" and "penthouse" slices that would be easier to find > and distinguish between? If so, can you follow-up with fmeawad about discussing > this at the instrumentation weekly? > I won't block landing this function, but I think it would be better if this > function and the whole idea of innermost descendant were not needed. One way to get around this would be to just look for "TaskQueueManager::ProcessTaskFromWorkQueue" events (which is what TTI does: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...) but the pedantic inside me thinks that is not 100% correct since there are still a few message loop cycles that are not blink scheduler cycles, and that approach will miss those tasks. skyostil's comment here (https://github.com/GoogleChrome/lighthouse/issues/489) also made me think that it's more futureproof to not hardcode the slice name in the code. But yes, we should probably discuss this more to see if we can do better.
	29 var succeededOnSomeDescendant = false;

	30 for (var child of slice.subSlices) {

	31 var succeededOnThisChild = runConditionallyOnInnermostDescendants(

	32 child, predicate, cb);
	charliea (OOO until 10-5) 2016/09/19 14:35:33 same (line continuations) same (line continuations) dproy 2016/09/20 01:11:43 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > same (line continuations) Done.
	33 succeededOnSomeDescendant =

	34 succeededOnThisChild \|\| succeededOnSomeDescendant;
	charliea (OOO until 10-5) 2016/09/19 14:35:33 same (line continuations) same (line continuations) dproy 2016/09/20 01:11:43 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > same (line continuations) Done.
	35 }

	36

	37 if (succeededOnSomeDescendant) {

	38 return true;
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: no need for braces around a single line branc nit: no need for braces around a single line branch dproy 2016/09/20 01:11:43 Does our style guide forbid this? I really dislike Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: no need for braces around a single line branch Does our style guide forbid this? I really dislike not putting braces around if blocks even if it's one line because I switch often between writing python and js and it becomes really easy for me to just add another indented statement below without remembering the wrap the whole block with braces, leading to many accumulated hours of debugging frustration. Related SO thread: http://stackoverflow.com/a/2125078/2367211 Of course, I'm all for consistent styling across codebase too, so if you feel this makes the codebase too inconsistent, I'll remove the braces. benjhayden 2016/09/20 05:52:35 For what it's worth, Ethan and I also wish that th Show quoted text On 2016/09/20 at 01:11:43, dproy1 wrote: > On 2016/09/19 14:35:33, charliea wrote: > > nit: no need for braces around a single line branch > > Does our style guide forbid this? I really dislike not putting braces around if blocks even if it's one line because I switch often between writing python and js and it becomes really easy for me to just add another indented statement below without remembering the wrap the whole block with braces, leading to many accumulated hours of debugging frustration. Related SO thread: http://stackoverflow.com/a/2125078/2367211 > > Of course, I'm all for consistent styling across codebase too, so if you feel this makes the codebase too inconsistent, I'll remove the braces. For what it's worth, Ethan and I also wish that the style guide required putting braces around single-line blocks, but this CL isn't the time or place to hash it out. I'll start a bug, but it's probably best to go with the flow for now and remove the braces.
	39 }

	40

	41 if (!predicate(slice)) {

	42 return false;
	charliea (OOO until 10-5) 2016/09/19 14:35:33 same (single line branch) same (single line branch)
	43 }

	44

	45 cb(slice);

	46 return true;

	47 }

	48

	49 function forEachInnermostTopLevelSlices(thread, cb) {

	50 var topLevelPredicate = slice => tr.b.getCategoryParts(slice.category)
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: I don't think we ever break at the ., but ins nit: I don't think we ever break at the ., but instead prefer to break inside the parentheses (like Python) my vote would be for something like: var topLevelPredicate = slice => tr.b.getCategoryParts(slice.category).includes( TOPLEVEL_CATEGORY_NAME) dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > nit: I don't think we ever break at the ., but instead prefer to break inside > the parentheses (like Python) > > my vote would be for something like: > > var topLevelPredicate = slice => > tr.b.getCategoryParts(slice.category).includes( > TOPLEVEL_CATEGORY_NAME) Done.
	51 .includes(TOPLEVEL_CATEGORY_NAME);

	52

	53 for (var slice of thread.sliceGroup.topLevelSlices) {
	charliea (OOO until 10-5) 2016/09/19 14:35:33 same (no need for braces) same (no need for braces)
	54 runConditionallyOnInnermostDescendants(slice, topLevelPredicate, cb);

	55 }

	56 }

	57

	58 function getAllInteractiveTimestampsSorted(model) {

	59 // TODO(dproy): When LoadExpectation v.1.0 is released,

	60 // update this function to use the new LoadExpectation rather

	61 // than calling loading_metric.html.

	62

	63 var values = new tr.v.ValueSet();

	64 tr.metrics.sh.loadingMetric(values, model);

	65 var ttiValues = values.getValuesNamed('timeToFirstInteractive');
	charliea (OOO until 10-5) 2016/09/19 14:35:32 wrong indentation? wrong indentation? charliea (OOO until 10-5) 2016/09/19 14:35:32 wrong indentation? wrong indentation? dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > wrong indentation? Done.
	66 var interactiveTsList = [];

	67 for (var bin of tr.b.getOnlyElement(ttiValues).allBins) {

	68 for (var diagnostics of bin.diagnosticMaps) {

	69 var info = diagnostics.get('Navigation infos');

	70 interactiveTsList.push(info.value.interactive);

	71 }

	72 }

	73 return interactiveTsList.sort((x, y) => x - y);

	74 }

	75

	76 function getAllNavStartTimesSorted(rendererHelper) {

	77 var list = [];

	78 for (var ev of rendererHelper.mainThread.sliceGroup.childEvents()) {

	79 if (!hasCategoryAndName(ev, 'blink.user_timing', 'navigationStart'))

	80 continue;

	81 list.push(ev.start);

	82 }

	83 return list.sort((x, y) => x - y);

	84 }

	85

	86 // A task window is defined as time from TTI until either

	87 // 1. beginning of next navigationStart event, or

	88 // 2. end of traces

	89 function getTaskWindows(interactiveTsList, navStartTimeList, endOfTraces) {

	90 var curNavStartTimeIndex = 0;

	91 var endOfLastWindow = -Infinity;

	92 var taskWindows = [];

	93 for (var curTTI of interactiveTsList) {

	94 var curNavStartTime = navStartTimeList[curNavStartTimeIndex];

	95 while (curNavStartTime !== undefined && curNavStartTime < curTTI) {

	96 // There are possibly multiple navigationStart timestamps between

	97 // two interactive timestamps - the previous page load could

	98 // never reach interactive status.

	99 curNavStartTimeIndex++;

	100 curNavStartTime = navStartTimeList[curNavStartTimeIndex];

	101 }

	102

	103 if (curNavStartTime === endOfLastWindow) {

	104 // This is a violation of core assumption.
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: not need for braces nit: not need for braces
	105 // TODO: When two pages share a render process, we can possibly

	106 // have two interactive time stamps between two navigation events.

	107 // If both interactive timestamps are reported, it is not clear how

	108 // to define estimated input latency.

	109 throw new Error("Two TTI timestamps with no navigation between them");

	110 }

	111

	112 if (curNavStartTime === undefined) {

	113 taskWindows.push({start: curTTI, end: endOfTraces});

	114 endOfLastWindow = endOfTraces;

	115 continue;

	116 }

	117

	118 taskWindows.push({start: curTTI, end: curNavStartTime});

	119 endOfLastWindow = curNavStartTime;

	120 }

	121 return taskWindows;

	122 }

	123

	124 /**

	125 * Note: Taken from
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: the first line of a jsdoc should be what the nit: the first line of a jsdoc should be what the function does. I'd definitely say move "Calculate duration at specified percentiles..." as the first line so that people don't have to scan further to see what the purpose is. That means the jsdoc should look something like: /** * Calculate duration at specified percentiles for given population of * durations. * * If one of the durations overlaps the end of the window, the full duration * should bein the duration array, ... * * Note: taken from goo.gl/12KedaW. / dproy* 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: the first line of a jsdoc should be what the function does. I'd definitely > say move "Calculate duration at specified percentiles..." as the first line so > that people don't have to scan further to see what the purpose is. That means > the jsdoc should look something like: > > /** > * Calculate duration at specified percentiles for given population of > * durations. > * > * If one of the durations overlaps the end of the window, the full duration > * should bein the duration array, ... > * > * Note: taken from goo.gl/12KedaW. > */ Done.
	126 * https://github.com/GoogleChrome/lighthouse/blob/a5bbe2338fa474c94bb87584940 8704c81fec3df/lighthouse-core/lib/traces/tracing-processor.js#L121
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: please create bit.ly or goo.gl shortlink and nit: please create bit.ly or goo.gl shortlink and use that so that the line isn't too long dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: please create bit.ly or goo.gl shortlink and use that so that the line > isn't too long Done.
	127 *

	128 * Calculate duration at specified percentiles for given population of
	charliea (OOO until 10-5) 2016/09/19 14:35:32 I'm not sure I understand what duration we're calc I'm not sure I understand what duration we're calculating or what a population of durations is. dproy 2016/09/20 01:11:42 Clarified. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > I'm not sure I understand what duration we're calculating or what a population > of durations is. Clarified.
	129 * durations.

	130 * If one of the durations overlaps the end of the window, the full
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: if you want different paragraphs within a jsd nit: if you want different paragraphs within a jsdoc, please add a blank line (just ' ') between them. If you don't want different paragraphs, move "If one of..." to the end of the previous line. dproy* 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > nit: if you want different paragraphs within a jsdoc, please add a blank line > (just ' *') between them. If you don't want different paragraphs, move "If one > of..." to the end of the previous line. Done.
	131 * duration should be in the duration array, but the length not included

	132 * within the window should be given as `clippedLength`. For instance, if a

	133 * 50ms duration occurs 10ms before the end of the window, `50` should be in

	134 * the `durations` array, and `clippedLength` should be set to 40.

	135 * @see https://docs.google.com/document/d/18gvP-CBA2BiBpi3Rz1I1ISciKGhniTSZ9T Y0XCnXS7E/preview
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: line is too long. Please create a shortlink t nit: line is too long. Please create a shortlink to the doc dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: line is too long. Please create a shortlink to the doc Done.
	136 */

	137 function calculateEILRiskPercentiles(

	138 durations, totalTime, percentiles, clippedLength) {
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: line continuations get four indents nit: line continuations get four indents dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: line continuations get four indents Done.
	139 clippedLength = clippedLength \|\| 0;

	140

	141 var busyTime = tr.b.Statistics.sum(durations);

	142 busyTime -= clippedLength;

	143

	144 // Start with idle time already compvare.
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: compvare => compare nit: compvare => compare benjhayden 2016/09/19 20:00:48 Actually, I think this is supposed to be "complete Show quoted text On 2016/09/19 at 14:35:33, charliea wrote: > nit: compvare => compare Actually, I think this is supposed to be "complete". He had replaced all "let" with "var" :-) dproy 2016/09/20 01:11:42 Yes this was indeed complete. s/let/var got me aga Show quoted text On 2016/09/19 20:00:48, benjhayden wrote: > On 2016/09/19 at 14:35:33, charliea wrote: > > nit: compvare => compare > > Actually, I think this is supposed to be "complete". He had replaced all "let" > with "var" :-) Yes this was indeed complete. s/let/var got me again - I need to find/write an AST based text substitution package.
	145 var completedTime = totalTime - busyTime;

	146 var duration = 0;

	147 var cdfTime = completedTime;
	charliea (OOO until 10-5) 2016/09/19 14:35:33 I can kind of vaguely guess what cdfTime is, but I I can kind of vaguely guess what cdfTime is, but I don't think it's a fair assumption to assume everyone reading the code will have the same knowledge of statistics. It probably warrants a comment. Also, I'm not sure "cdfTime" is a great variable name here. Even if you write out the abbreviation, "cumulativeDistributionFunctionTime"... what the heck is that? I know that the cumulative distribution function gives the probability that the function will take on a value less than X, but what does that make a "cdfTime"? All of these questions make me think that a better variable name is in order. dproy 2016/09/20 01:11:42 I didn't write this code, and I'm finding it very Show quoted text On 2016/09/19 14:35:33, charliea wrote: > I can kind of vaguely guess what cdfTime is, but I don't think it's a fair > assumption to assume everyone reading the code will have the same knowledge of > statistics. It probably warrants a comment. > > Also, I'm not sure "cdfTime" is a great variable name here. Even if you write > out the abbreviation, "cumulativeDistributionFunctionTime"... what the heck is > that? I know that the cumulative distribution function gives the probability > that the function will take on a value less than X, but what does that make a > "cdfTime"? All of these questions make me think that a better variable name is > in order. I didn't write this code, and I'm finding it very hard to find a name that captures what this variable actually is. Very vaguely, it's cdf multiplied by total time at the end of each iteration, but (1) that's not even strictly true because it's more like "projected cdfTime at the end of this iteration if we had processed all the tasks with same length of the current task", and (2) cdfTimesTime is hardly an improvement. I added copious amounts of comments before its definition, but the best way to understand this variable is just to pretend it's called "x" or something opaque, and see its value change through the loop iteration. If we know we will stick with this definition of EIL in the long run it may be worth it to revisit this function and write it in a slightly more easier-to-understand way, but I'm reluctant to invest the time at the moment. The tests are quite comprehensive and prove beyond reasonable doubt to me that it does what it's supposed to do.
	148 var results = new Map();

	149

	150 var durationIndex = -1;

	151 var remainingCount = durations.length + 1;

	152 if (clippedLength > 0) {

	153 // If there was a clipped duration, one less in count
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: no need for braces nit: no need for braces
	154 // since one hasn't started yet.

	155 remainingCount--;

	156 }

	157

	158 // Find percentiles of interest, in order.

	159 for (var percentile of percentiles) {

	160 // Loop over durations, calculating a CDF value for each until it is above

	161 // the target percentile.

	162 var percentileTime = percentile * totalTime;

	163 while (cdfTime < percentileTime && durationIndex < durations.length - 1) {

	164 completedTime += duration;

	165 remainingCount += (duration >= 0 ? -1 : 1);

	166

	167 if (clippedLength > 0 && clippedLength < durations[durationIndex + 1]) {

	168 duration = -clippedLength;

	169 clippedLength = 0;

	170 } else {

	171 durationIndex++;

	172 duration = durations[durationIndex];

	173 }

	174

	175 // Calculate value of CDF (multiplied by totalTime) for the end of

	176 // this duration.

	177 cdfTime = completedTime + Math.abs(duration) * remainingCount;

	178 }

	179

	180 // Negative results are within idle time (0ms wait by definition),

	181 // so clamp at zero.

	182 results.set(

	183 percentile,
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: line continuations get four indents nit: line continuations get four indents dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > nit: line continuations get four indents Done.
	184 Math.max(0, (percentileTime - completedTime) / remainingCount));

	185 }

	186

	187 return results;

	188 }

	189

	190 /**

	191 * Note: This is adapted from

	192 * https://github.com/GoogleChrome/lighthouse/blob/a5bbe2338fa474c94bb87584940 8704c81fec3df/lighthouse-core/lib/traces/tracing-processor.js#L185
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: shortlink. nit: shortlink. dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: shortlink. Done.
	193 */

	194 function getEQTPercentilesForWindow(
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: what the heck is an EQT? nit: what the heck is an EQT? dproy 2016/09/20 01:11:42 Added comments Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: what the heck is an EQT? Added comments
	195 percentiles, rendererHelper, startTime, endTime) {

	196 var totalTime = endTime - startTime;

	197 percentiles.sort((a, b) => a - b);

	198

	199 var durations = [];

	200 var clippedLength = 0;

	201 forEachInnermostTopLevelSlices(rendererHelper.mainThread, slice => {

	202 // Discard slices outside range.

	203 if (slice.end <= startTime \|\| slice.start >= endTime) {

	204 return;

	205 }

	206

	207 // Clip any at edges of range.

	208 var duration = slice.duration;

	209 var sliceStart = slice.start;

	210 if (sliceStart < startTime) {

	211 // Any part of task before window can be discarded.

	212 sliceStart = startTime;

	213 duration = slice.end - sliceStart;

	214

	215 }

	216 if (slice.end > endTime) {

	217 // Any part of task after window must be clipped but accounted for.

	218 clippedLength = duration - (endTime - sliceStart);

	219 }

	220 durations.push(duration);

	221

	222 });

	223 durations.sort((a, b) => a - b);

	224 return calculateEILRiskPercentiles(

	225 durations, totalTime, percentiles, clippedLength);

	226 }

	227

	228 /**

	229 * @param {!tr.v.ValueSet} values

	230 * @param {!tr.model.Model} model

	231 * @param {!Object=} opt_options
	charliea (OOO until 10-5) 2016/09/19 14:35:32 what type of options are available? This might als what type of options are available? This might also be a good place to explain what the estimated input latency metric is. You may know, but people looking at this file a year from now likely will not. benjhayden 2016/09/19 20:00:48 This metric doesn't support any opt_options, so I Show quoted text On 2016/09/19 at 14:35:32, charliea wrote: > what type of options are available? This might also be a good place to explain what the estimated input latency metric is. You may know, but people looking at this file a year from now likely will not. This metric doesn't support any opt_options, so I think it's appropriate for this jsdoc to not explain opt_options.rangeOfInterest, which is currently the only option. I think that the metrics that do support it should document it. dproy 2016/09/20 01:11:42 Let me know if I should remove opt_options altoget Show quoted text On 2016/09/19 20:00:48, benjhayden wrote: > On 2016/09/19 at 14:35:32, charliea wrote: > > what type of options are available? This might also be a good place to explain > what the estimated input latency metric is. You may know, but people looking > at this file a year from now likely will not. > > This metric doesn't support any opt_options, so I think it's appropriate for > this jsdoc to not explain opt_options.rangeOfInterest, which is currently the > only option. I think that the metrics that do support it should document it. Let me know if I should remove opt_options altogether.
	232 */

	233 function estimatedInputLatencyMetric(values, model, opt_options) {

	234
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: unnecessary blank line nit: unnecessary blank line dproy 2016/09/20 01:11:43 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: unnecessary blank line Done.
	235 var chromeHelper = model.getOrCreateHelper(

	236 tr.model.helpers.ChromeModelHelper);

	237 // The largest pid renderer is probably the one we're interested in

	238 var rendererHelper = chromeHelper.rendererWithLargestPid;

	239

	240 var interactiveTimestamps = getAllInteractiveTimestampsSorted(model);

	241

	242 // We're assuming children iframes will never emit navigationStart events

	243 var navStartTimeList = getAllNavStartTimesSorted(rendererHelper);

	244 var taskWindowList = getTaskWindows(

	245 interactiveTimestamps,
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: line continuations nit: line continuations dproy 2016/09/20 01:11:43 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: line continuations Done.
	246 navStartTimeList,

	247 rendererHelper.mainThread.bounds.max);

	248

	249 // Compute the 90th percentile

	250 var eqtTargetPercentiles = [0.9];

	251

	252 var eqt90thPercentile = new tr.v.Histogram(

	253 'Expected Queueing Time 90th Percentile',
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: line continuations nit: line continuations dproy 2016/09/20 01:11:42 Done. Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: line continuations Done.
	254 tr.b.Unit.byName.timeDurationInMs_smallerIsBetter,

	255 tr.v.HistogramBinBoundaries.createExponential(0.1, 1e3, 100));
	charliea (OOO until 10-5) 2016/09/19 14:35:33 nit: I think the best practice is to create these nit: I think the best practice is to create these boundaries as a constant near the top of the namespace scope. It'd also be good to provide some justification as to why .1 and 1000 are reasonable choices: you've likely thought about this more than anyone else, and trying to figure this out in reverse in the future might be painful dproy 2016/09/20 01:11:43 Done: made these top level constants with some com Show quoted text On 2016/09/19 14:35:33, charliea wrote: > nit: I think the best practice is to create these boundaries as a constant near > the top of the namespace scope. It'd also be good to provide some justification > as to why .1 and 1000 are reasonable choices: you've likely thought about this > more than anyone else, and trying to figure this out in reverse in the future > might be painful Done: made these top level constants with some comments.
	256

	257 for (var taskWindow of taskWindowList) {

	258 var eqtPercentiles = getEQTPercentilesForWindow(

	259 eqtTargetPercentiles, rendererHelper, taskWindow.start, taskWindow.end);
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: line continuations nit: line continuations dproy 2016/09/20 01:11:43 Done. Show quoted text On 2016/09/19 14:35:32, charliea wrote: > nit: line continuations Done.
	260 var eqt90th = eqtPercentiles.get(0.9);

	261 if (eqt90th !== undefined) {

	262 eqt90thPercentile.addSample(eqt90th);
	charliea (OOO until 10-5) 2016/09/19 14:35:32 nit: single line branch nit: single line branch
	263 }

	264 }

	265

	266 values.addHistogram(eqt90thPercentile);

	267 }

	268

	269 tr.metrics.MetricRegistry.register(estimatedInputLatencyMetric, {

	270 supportsRangeOfInterest: false

	271 });

	272

	273 return {

	274 estimatedInputLatencyMetric: estimatedInputLatencyMetric,

	275 calculateEILRiskPercentiles: calculateEILRiskPercentiles,

	276 getEQTPercentilesForWindow: getEQTPercentilesForWindow

	277 };

	278 });

	279 </script>

OLD	NEW