Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415963002:
[V8][Metric] Initial implementation of the RuntimeStatsMetric (Closed)
Description[V8][Metric] Initial implementation of the RuntimeStatsMetric
This CL implements a runtime stats metric that extracts the runtime
stats info from the V8ThreadSlices up to interactive time.
Then these values are grouped by 'type'. Each type is a single value
histogram. Histograms are repeated for count and duration.
Each 'Type' histogram has a RelatedValueMap that describes the
individual runtime stats used to calculate that 'type' data.
BUG=catapult:#2938
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/55c4800f013cba6cb2a4b77e8c49b643d56a8873
Patch Set 1 #
Total comments: 40
Patch Set 2 : Address reviewers comments #
Total comments: 30
Patch Set 3 : Add tests and address most of the comments #Patch Set 4 : Add custom boundaries to the histograms #Patch Set 5 : Change the number of bins to 50 #Patch Set 6 : Rebase #
Messages
Total messages: 38 (18 generated)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fmeawad@chromium.org changed reviewers: + benjhayden@chromium.org, eakuefner@chromium.org, lpy@chromium.org
PTAL
https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:38: reset() { I'm uncomfortable with this. Is reset used anywhere? Do we actually need it? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:44: class RuntimeStatsGroupedEntry extends RuntimeStatsEntry { Maybe you should call this RuntimeGroup and then call RuntimeGroups RuntimeGroupBuilder? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:94: slices.forEach(function(slice) { You can write this as an arrow to avoid passing |this| explicitly. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:102: Object.getOwnPropertyNames(runtimeCallStats).forEach( Can you use an ES6 map here instead? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:103: function(runtimeCallStatName) { Same here https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:23: // There should be a single histogram This is a little ambiguous; do you mean that we expect there to be at least one TTI histogram? Exactly one? Can you replace this comment with a conditional and an exception?
Sorry, bunch of nits. :-) https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:15: constructor(name, count, time) { What are the units of time? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:33: accumulate(count, time) { Did you want to make this addSample() to two Histograms in this patch? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:57: if (value !== undefined) Please use braces. This is a "multi-line block". https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:94: slices.forEach(function(slice) { On 2016/10/13 at 19:39:26, eakuefner wrote: > You can write this as an arrow to avoid passing |this| explicitly. Better, for (var slice of slices) https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:102: Object.getOwnPropertyNames(runtimeCallStats).forEach( On 2016/10/13 at 19:39:26, eakuefner wrote: > Can you use an ES6 map here instead? Ah, it looks like this comes from the trace, which can only serialize dicts. You should be able to use tr.b.iterItems with an arrow function instead, though. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:20: tr.metrics.sh.loadingMetric(values, model); Ugh, this is why we need LoadExpectation v1.0. My fault, not yours. :-) https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:21: var ttiEntries = [...values].filter( values.getValuesNamed('timeToFirstInteractive') https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:23: // There should be a single histogram On 2016/10/13 at 19:39:26, eakuefner wrote: > This is a little ambiguous; do you mean that we expect there to be at least one TTI histogram? Exactly one? Can you replace this comment with a conditional and an exception? Better, use tr.b.getOnlyElement(). https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:29: (bin) => bin.diagnosticMaps.length > 0); Don't need parens around only parameter for arrow functions. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: var diagnostic = binsWithSampleDiagnosticMaps[0] tr.b.getOnlyElement() for both of these [0], so you'll know if they ever modify loadingMetric to produce multiple samples. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:36: var slices = [...model.getDescendantEvents()].filter(function(event) { You could use an arrow function here. event => (event instanceof ... https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:44: runtimeGroups.groupedEntries.forEach(function(groupedEntry) { for (var groupedEntry of runtimeGroups.groupedEntries) https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:61: for (var entry of groupedEntry.values) { Can you merge this loop with the other one? https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/ui/extras/v... File tracing/tracing/ui/extras/v8/runtime_call_stats_table.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/ui/extras/v... tracing/tracing/ui/extras/v8/runtime_call_stats_table.html:39: function buildRows_(runtimeGroups) { It looks like this entire method can be removed if you set table.subRowsPropertyName = 'values';
https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:102: Object.getOwnPropertyNames(runtimeCallStats).forEach( On 2016/10/13 at 21:18:30, benjhayden wrote: > On 2016/10/13 at 19:39:26, eakuefner wrote: > > Can you use an ES6 map here instead? > > Ah, it looks like this comes from the trace, which can only serialize dicts. > > You should be able to use tr.b.iterItems with an arrow function instead, though. Thanks for making this comment; I spoke to Fadi offline about this earlier and we agreed that that was the correct thing to do.
I addressed the comments, but I am still adding a couple of test files. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/10/13 19:39:26, eakuefner wrote: > nit: no (c) Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:15: constructor(name, count, time) { On 2016/10/13 21:18:30, benjhayden wrote: > What are the units of time? Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:33: accumulate(count, time) { On 2016/10/13 21:18:30, benjhayden wrote: > Did you want to make this addSample() to two Histograms in this patch? I will add it in a follow up CL https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:38: reset() { On 2016/10/13 19:39:26, eakuefner wrote: > I'm uncomfortable with this. Is reset used anywhere? Do we actually need it? Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:44: class RuntimeStatsGroupedEntry extends RuntimeStatsEntry { On 2016/10/13 19:39:25, eakuefner wrote: > Maybe you should call this RuntimeGroup and then call RuntimeGroups > RuntimeGroupBuilder? Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:57: if (value !== undefined) On 2016/10/13 21:18:30, benjhayden wrote: > Please use braces. This is a "multi-line block". > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:94: slices.forEach(function(slice) { On 2016/10/13 21:18:30, benjhayden wrote: > On 2016/10/13 at 19:39:26, eakuefner wrote: > > You can write this as an arrow to avoid passing |this| explicitly. > > Better, for (var slice of slices) Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:102: Object.getOwnPropertyNames(runtimeCallStats).forEach( On 2016/10/13 21:18:30, benjhayden wrote: > On 2016/10/13 at 19:39:26, eakuefner wrote: > > Can you use an ES6 map here instead? > > Ah, it looks like this comes from the trace, which can only serialize dicts. > > You should be able to use tr.b.iterItems with an arrow function instead, though. Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/extras/v8/r... tracing/tracing/extras/v8/runtime_stats_entry.html:103: function(runtimeCallStatName) { On 2016/10/13 19:39:26, eakuefner wrote: > Same here Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:20: tr.metrics.sh.loadingMetric(values, model); On 2016/10/13 21:18:31, benjhayden wrote: > Ugh, this is why we need LoadExpectation v1.0. > My fault, not yours. :-) Acknowledged. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:21: var ttiEntries = [...values].filter( On 2016/10/13 21:18:31, benjhayden wrote: > values.getValuesNamed('timeToFirstInteractive') Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:23: // There should be a single histogram On 2016/10/13 21:18:31, benjhayden wrote: > On 2016/10/13 at 19:39:26, eakuefner wrote: > > This is a little ambiguous; do you mean that we expect there to be at least > one TTI histogram? Exactly one? Can you replace this comment with a conditional > and an exception? > > Better, use tr.b.getOnlyElement(). Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:29: (bin) => bin.diagnosticMaps.length > 0); On 2016/10/13 21:18:31, benjhayden wrote: > Don't need parens around only parameter for arrow functions. Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: var diagnostic = binsWithSampleDiagnosticMaps[0] On 2016/10/13 21:18:30, benjhayden wrote: > tr.b.getOnlyElement() for both of these [0], so you'll know if they ever modify > loadingMetric to produce multiple samples. Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:36: var slices = [...model.getDescendantEvents()].filter(function(event) { On 2016/10/13 21:18:31, benjhayden wrote: > You could use an arrow function here. > event => (event instanceof ... Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:44: runtimeGroups.groupedEntries.forEach(function(groupedEntry) { On 2016/10/13 21:18:31, benjhayden wrote: > for (var groupedEntry of runtimeGroups.groupedEntries) Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/runtime_stats_metric.html:61: for (var entry of groupedEntry.values) { On 2016/10/13 21:18:31, benjhayden wrote: > Can you merge this loop with the other one? Done. https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/ui/extras/v... File tracing/tracing/ui/extras/v8/runtime_call_stats_table.html (right): https://codereview.chromium.org/2415963002/diff/1/tracing/tracing/ui/extras/v... tracing/tracing/ui/extras/v8/runtime_call_stats_table.html:39: function buildRows_(runtimeGroups) { On 2016/10/13 21:18:31, benjhayden wrote: > It looks like this entire method can be removed if you set > table.subRowsPropertyName = 'values'; Done.
I think we're almost there; I'd suggest a few more tweaks. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:18: // Time in us. Please write it out: Time in microseconds. Also, you should probably move this to a JSDoc comment instead. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:34: accumulate(count, time) { maybe call this addSample instead? https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:48: return this.regex_ && !(!name.match(this.regex_)); Why do you write !!? This suggests that the truthiness/falsiness of name.match(...) will be usable to begin with. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:55: } nit: else should be on this line. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:23: // The metric can handle traces that have a single navigation only. The word order here makes it sound like you are saying it's acceptable for traces to have a single navigation, rather than mandatory. If the latter, I'd rephrase as: // This metric requires traces to contain only one navigation. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:26: bin => bin.diagnosticMaps.length > 0); nit: please make your hanging indents 4 spaces and not 2, unless you're defining a block. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:35: (event instanceof tr.e.v8.V8ThreadSlice) && event.start <= nit: this isn't a block even though it's an arrow body so it should still be indented 4sp. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:46: '.duration', Please don't use dots in histogram names. Prefer colon (:) instead.
a few nits and questions then lgtm https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:63: reset(entry) { Is this used anywhere? https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:99: if (runtimeCallStats !== undefined) { if (runtimeCallStats === undefined) return; so you can dedent the rest of the method. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:43: var countSamples = new tr.v.d.RelatedValueMap(); RelatedValueMaps are displayed as a series of <a> links. It looks like runtimeGroup.count/duration are sums of the counts/durations in the xSampleHistograms, right? Would it make sense to display the diagnostic as a stacked bar chart? That would let users visually compare the component xSampleHistograms' sums as a breakdown. If so, then just say RelatedHistogramBreakdown here instead of RelatedValueMap, they have the same interface. :-) https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:47: tr.b.Unit.byName.timeDurationInMs_smallerIsBetter); It looks like you're using the default bin boundaries. I assume you've looked at the histograms for a few traces and the bin boundaries look reasonable? All of the samples aren't in the underflow bin or anything like that? Same goes for all of the histograms that use the default bin boundaries. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:48: durationSampleHistogram.addSample(entry.time / 1000.0); Please use tr.b.convertUnit(entry.time, tr.b.UnitScale.Metric.MICRO, tr.b.UnitScale.Metric.MILLI) instead of dividing by a magic number 1000.0.
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: return diagnostic.value.interactive; It's worth noting that there maybe no single TTI per model. In fact, a model can have mutiple TTI for each renderer process.
I believe all concerns addressed, PTAL. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... File tracing/tracing/extras/v8/runtime_stats_entry.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:18: // Time in us. On 2016/10/14 17:06:08, eakuefner wrote: > Please write it out: Time in microseconds. Also, you should probably move this > to a JSDoc comment instead. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:34: accumulate(count, time) { On 2016/10/14 17:06:08, eakuefner wrote: > maybe call this addSample instead? Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:48: return this.regex_ && !(!name.match(this.regex_)); On 2016/10/14 17:06:08, eakuefner wrote: > Why do you write !!? This suggests that the truthiness/falsiness of > name.match(...) will be usable to begin with. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:55: } On 2016/10/14 17:06:08, eakuefner wrote: > nit: else should be on this line. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:63: reset(entry) { On 2016/10/14 20:11:22, benjhayden wrote: > Is this used anywhere? Removed https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/extras/... tracing/tracing/extras/v8/runtime_stats_entry.html:99: if (runtimeCallStats !== undefined) { On 2016/10/14 20:11:22, benjhayden wrote: > if (runtimeCallStats === undefined) return; > so you can dedent the rest of the method. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:23: // The metric can handle traces that have a single navigation only. On 2016/10/14 17:06:09, eakuefner wrote: > The word order here makes it sound like you are saying it's acceptable for > traces to have a single navigation, rather than mandatory. If the latter, I'd > rephrase as: > > // This metric requires traces to contain only one navigation. Done. I was trying to phrase it such that it can be added later. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:26: bin => bin.diagnosticMaps.length > 0); On 2016/10/14 17:06:09, eakuefner wrote: > nit: please make your hanging indents 4 spaces and not 2, unless you're defining > a block. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: return diagnostic.value.interactive; On 2016/10/18 23:11:03, nednguyen wrote: > It's worth noting that there maybe no single TTI per model. In fact, a model can > have mutiple TTI for each renderer process. Yes, I am aware of that. I have added a comment at the top "// This metric requires traces to contain only one navigation." Since this metric will be used mostly in benchmarks at first, I think it is a valid assumption for now. We can later expand it but I am not sure how yet. (I.e. what does it mean, do we have multiple loads, do we report each load individually or aggregate, ...), maybe we should report the first one. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:35: (event instanceof tr.e.v8.V8ThreadSlice) && event.start <= On 2016/10/14 17:06:09, eakuefner wrote: > nit: this isn't a block even though it's an arrow body so it should still be > indented 4sp. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:43: var countSamples = new tr.v.d.RelatedValueMap(); On 2016/10/14 20:11:22, benjhayden wrote: > RelatedValueMaps are displayed as a series of <a> links. > It looks like runtimeGroup.count/duration are sums of the counts/durations in > the xSampleHistograms, right? > Would it make sense to display the diagnostic as a stacked bar chart? That would > let users visually compare the component xSampleHistograms' sums as a breakdown. > If so, then just say RelatedHistogramBreakdown here instead of RelatedValueMap, > they have the same interface. :-) Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:46: '.duration', On 2016/10/14 17:06:09, eakuefner wrote: > Please don't use dots in histogram names. Prefer colon (:) instead. Done. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:47: tr.b.Unit.byName.timeDurationInMs_smallerIsBetter); On 2016/10/14 20:11:22, benjhayden wrote: > It looks like you're using the default bin boundaries. I assume you've looked at > the histograms for a few traces and the bin boundaries look reasonable? All of > the samples aren't in the underflow bin or anything like that? > Same goes for all of the histograms that use the default bin boundaries. I have added some boundaries. https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:48: durationSampleHistogram.addSample(entry.time / 1000.0); On 2016/10/14 20:11:22, benjhayden wrote: > Please use tr.b.convertUnit(entry.time, tr.b.UnitScale.Metric.MICRO, > tr.b.UnitScale.Metric.MILLI) instead of dividing by a magic number 1000.0. Done.
fmeawad@chromium.org changed reviewers: + cbruni@chromium.org
+cbruni
LGTM on tracing/extras/v8 and tracing/ui/extras/v8
lgtm
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: return diagnostic.value.interactive; On 2016/10/19 22:04:18, fmeawad wrote: > On 2016/10/18 23:11:03, nednguyen wrote: > > It's worth noting that there maybe no single TTI per model. In fact, a model > can > > have mutiple TTI for each renderer process. > > Yes, I am aware of that. I have added a comment at the top > "// This metric requires traces to contain only one navigation." > Since this metric will be used mostly in benchmarks at first, I think it is a > valid assumption for now. We can later expand it but I am not sure how yet. > (I.e. what does it mean, do we have multiple loads, do we report each load > individually or aggregate, ...), maybe we should report the first one. This will make it hard to later improve the telemetry benchmark cases, i.e: measuring call time stats when there are multiple tabs, multiple navigations, ... Reporting the first one is not a great solution, because we lose data & coverage by doing that. For example, in a browsing case of user go to google.com/search?q=pizza+in+NY, click on a link to page Y, going back to search landing page, how do we make sure that our runtime call stats can capture the warm run of v8 in the 3rd navigation? Since tbmV2 support histogram, I think the easiest model here is to add all the call time stats of same kind to a same histogram. so s.t like for (nav in all navigations) { extract call time stats X in [nav.start, nav.TTI] add call time stats to histogram X }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:30: return diagnostic.value.interactive; On 2016/10/20 15:06:59, nednguyen wrote: > On 2016/10/19 22:04:18, fmeawad wrote: > > On 2016/10/18 23:11:03, nednguyen wrote: > > > It's worth noting that there maybe no single TTI per model. In fact, a model > > can > > > have mutiple TTI for each renderer process. > > > > Yes, I am aware of that. I have added a comment at the top > > "// This metric requires traces to contain only one navigation." > > Since this metric will be used mostly in benchmarks at first, I think it is a > > valid assumption for now. We can later expand it but I am not sure how yet. > > (I.e. what does it mean, do we have multiple loads, do we report each load > > individually or aggregate, ...), maybe we should report the first one. > > This will make it hard to later improve the telemetry benchmark cases, i.e: > measuring call time stats when there are multiple tabs, multiple navigations, > ... > > Reporting the first one is not a great solution, because we lose data & coverage > by doing that. For example, in a browsing case of user go to > google.com/search?q=pizza+in+NY, click on a link to page Y, going back to search > landing page, how do we make sure that our runtime call stats can capture the > warm run of v8 in the 3rd navigation? > > > Since tbmV2 support histogram, I think the easiest model here is to add all the > call time stats of same kind to a same histogram. so s.t like > > for (nav in all navigations) { > extract call time stats X in [nav.start, nav.TTI] > add call time stats to histogram X > } > Since the result is already pushed in a Histogram, I think making that improvement in a later CL should not be a problem. Since I am using the absolute interactive time and not the normal TTI definition, we need to define when to reset the interactive timer. I.e. define the start and end points of a navigation. But I do have another concern, I think in some of these use cases, we will probably need to store the runtime stats in a different histogram for each navigation because we would want to monitor regression on each navigation individually given that they have different characteristics.
On 2016/10/20 15:15:16, fmeawad wrote: > https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2415963002/diff/20001/tracing/tracing/metrics... > tracing/tracing/metrics/v8/runtime_stats_metric.html:30: return > diagnostic.value.interactive; > On 2016/10/20 15:06:59, nednguyen wrote: > > On 2016/10/19 22:04:18, fmeawad wrote: > > > On 2016/10/18 23:11:03, nednguyen wrote: > > > > It's worth noting that there maybe no single TTI per model. In fact, a > model > > > can > > > > have mutiple TTI for each renderer process. > > > > > > Yes, I am aware of that. I have added a comment at the top > > > "// This metric requires traces to contain only one navigation." > > > Since this metric will be used mostly in benchmarks at first, I think it is > a > > > valid assumption for now. We can later expand it but I am not sure how yet. > > > (I.e. what does it mean, do we have multiple loads, do we report each load > > > individually or aggregate, ...), maybe we should report the first one. > > > > This will make it hard to later improve the telemetry benchmark cases, i.e: > > measuring call time stats when there are multiple tabs, multiple navigations, > > ... > > > > Reporting the first one is not a great solution, because we lose data & > coverage > > by doing that. For example, in a browsing case of user go to > > google.com/search?q=pizza+in+NY, click on a link to page Y, going back to > search > > landing page, how do we make sure that our runtime call stats can capture the > > warm run of v8 in the 3rd navigation? > > > > > > Since tbmV2 support histogram, I think the easiest model here is to add all > the > > call time stats of same kind to a same histogram. so s.t like > > > > for (nav in all navigations) { > > extract call time stats X in [nav.start, nav.TTI] > > add call time stats to histogram X > > } > > > > Since the result is already pushed in a Histogram, I think making that > improvement in a later CL should not be a problem. > Since I am using the absolute interactive time and not the normal TTI > definition, we need to define when to reset the interactive timer. > I.e. define the start and end points of a navigation. > > But I do have another concern, I think in some of these use cases, we will > probably need to store the runtime stats in a different histogram for each > navigation because we would want to monitor regression on each navigation > individually given that they have different characteristics. lgtm This is good to land as-is Talked offline, we gonna address the multi processes issue in subsequent CL since the current benchmark cases are simple. In particular, the change we gonna make next is to have a generic API that compute the runtime call stats for a triplet of (renderer pid, start_time, end_time). This generaic API + rail stages detecting library will provide the path way to do things like: runtime call stats during scroll, animation,... in the future.
lgtm
The CQ bit was checked by fmeawad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, lpy@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2415963002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [V8][Metric] Initial implementation of the RuntimeStatsMetric This CL implements a runtime stats metric that extracts the runtime stats info from the V8ThreadSlices up to interactive time. Then these values are grouped by 'type'. Each type is a single value histogram. Histograms are repeated for count and duration. Each 'Type' histogram has a RelatedValueMap that describes the individual runtime stats used to calculate that 'type' data. BUG=catapult:#2938 ========== to ========== [V8][Metric] Initial implementation of the RuntimeStatsMetric This CL implements a runtime stats metric that extracts the runtime stats info from the V8ThreadSlices up to interactive time. Then these values are grouped by 'type'. Each type is a single value histogram. Histograms are repeated for count and duration. Each 'Type' histogram has a RelatedValueMap that describes the individual runtime stats used to calculate that 'type' data. BUG=catapult:#2938 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://chromiumcodereview.appspot.com/2439953002/ by aiolos@chromium.org. The reason for reverting is: This is causing test failures on the CQ and locally. Link to bot with CQ failures: https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li... From a local run on linux: TypeError: tr.v.ValueSet is not a constructor at Object.testFn_ (http://127.0.0.1:8003/tracing/metrics/v8/runtime_stats_metric_test.html:179:18) at Object.run (http://127.0.0.1:8003/tracing/base/unittest/test_case.html:79:19) at Object.<anonymous> (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:218:52) at Object.runCurrentTestCase_ (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:216:14) at Object.runOneTestCase_ (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:169:12).
Message was sent while issue was closed.
On 2016/10/20 at 22:05:06, aiolos wrote: > A revert of this CL (patchset #6 id:100001) has been created in https://chromiumcodereview.appspot.com/2439953002/ by aiolos@chromium.org. > > The reason for reverting is: This is causing test failures on the CQ and locally. > > Link to bot with CQ failures: > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li... > > From a local run on linux: > > TypeError: tr.v.ValueSet is not a constructor > at Object.testFn_ (http://127.0.0.1:8003/tracing/metrics/v8/runtime_stats_metric_test.html:179:18) > at Object.run (http://127.0.0.1:8003/tracing/base/unittest/test_case.html:79:19) > at Object.<anonymous> (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:218:52) > at Object.runCurrentTestCase_ (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:216:14) > at Object.runOneTestCase_ (http://127.0.0.1:8003/tracing/base/unittest/test_runner.html:169:12). Remarkably, this raced with https://codereview.chromium.org/2424933003 causing this failure not to be caught by the CQ. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
