Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639993002:
Adds runtimeStatsTotalMetric to collect v8 metrics and bucket them based on UE. (Closed)
DescriptionAdds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction.
RuntimeStats metric measures various v8 metrics till the time to interact to
measure the startup time of the web pages. This cl extends this metric by adding
runtimeStatsTotalMetric which measures the v8 metrics for the total duration
of the page and buckets them based on user expectations.
BUG=chromium:686250
Review-Url: https://codereview.chromium.org/2639993002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/3a702528b20ff1d13d8abe3ee37a8bf2772f64d2
Patch Set 1 #Patch Set 2 : Fixed naming of the metric. #
Total comments: 4
Patch Set 3 : Address comments from fadi. #Patch Set 4 : Filter out subcategories of V8 stats and bucket them on UE. #
Total comments: 12
Patch Set 5 : Addressed review comments. #Patch Set 6 : Update with the crbug to track removal of runtimeStatsMetric. #
Total comments: 8
Patch Set 7 : Added tests. #Patch Set 8 : Renamed a histogram. #
Total comments: 4
Patch Set 9 : Iterate over expectations instead of segments. #
Total comments: 6
Patch Set 10 : Addressed nits. #Patch Set 11 : Fixed to add only one sample for each UE. #
Total comments: 7
Patch Set 12 : Fixed Nits. #Patch Set 13 : Rebased the patch. #
Total comments: 1
Messages
Total messages: 60 (28 generated)
Description was changed from ========== Adds runtimeStatsInteractiveMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsInteractiveMetric which measures the v8 metrics for the total duration of the page. ========== to ========== Adds runtimeStatsInteractiveMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsInteractiveMetric which measures the v8 metrics for the total duration of the page. ==========
Description was changed from ========== Adds runtimeStatsInteractiveMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsInteractiveMetric which measures the v8 metrics for the total duration of the page. ========== to ========== Adds runtimeStatsInteractiveMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsInteractiveMetric which measures the v8 metrics for the total duration of the page. ==========
The CQ bit was checked by mythria@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.
mythria@chromium.org changed reviewers: + cbruni@chromium.org, fmeawad@chromium.org
Hi Fadi and Camillo, I added a new metric that is similar to the runtimeStatsMetric but does not filter out the samples that appear after the time to interact. This metric will be used to measure RuntimeStats on benchmarks that also have some interaction like scrolling. This is required to enable runtimeStats on browsing benchmarks in this cl: https://codereview.chromium.org/2639213002/ PTAL. Thanks, Mythri
fmeawad@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com
I am actually surprised that you are getting different results between the 2 metrics. I tried the original metric on speedometer for example and it seems to work fine. This is because the main thread is usually very busy that interactiveTime is reached when the page finishes loading. Also I do not think that creating a new metric is the way to go, instead I think we should generate multiple histograms from the same metric up to different points in time. (Adding Ethan and Ned for that) https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:85: if (endTime !== -1) { I think -ve times are feasible, although it is unlikely to be exactly -1 I would say that this is unsafe. https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:133: function runtimeStatsInteractiveMetric(values, model) { Calling the new metric "interactive" is confusing, since the original one is calculated either up to the max of (interactiveTime, domContentLoaded). The names should be swapped while making the one up to interactive more explicit. But that would mean changing the benchmark as well: https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?rcl=0&l=295 Maybe pick a new name that is more descriptive to the new use only
On 2017/01/19 at 00:01:47, fmeawad wrote: > I am actually surprised that you are getting different results between the 2 metrics. I tried the original metric on speedometer for example and it seems to work fine. This is because the main thread is usually very busy that interactiveTime is reached when the page finishes loading. The main difference will be on the interactive pages (we the infinite scroll pages for instance), and I agree, we probably won't see much of a change on speedometer. > Also I do not think that creating a new metric is the way to go, instead I think we should generate multiple histograms from the same metric up to different points in time. (Adding Ethan and Ned for that) I would definitely prefer that over having to manually specify ranges and accidentally not measure something. Maybe we can use the interactiveMetric as a temporary measure until a more general solution is ready?
Thanks Fadi. I addressed your comments. https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:85: if (endTime !== -1) { On 2017/01/19 00:01:47, fmeawad wrote: > I think -ve times are feasible, although it is unlikely to be exactly -1 I would > say that this is unsafe. Thanks Fadi. I moved the computation of slice into the runtimeStats function. So now, this function just computes RuntimeStats on the slices provided to it. https://codereview.chromium.org/2639993002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:133: function runtimeStatsInteractiveMetric(values, model) { On 2017/01/19 00:01:47, fmeawad wrote: > Calling the new metric "interactive" is confusing, since the original one is > calculated either up to the max of (interactiveTime, domContentLoaded). > > The names should be swapped while making the one up to interactive more > explicit. But that would mean changing the benchmark as well: > https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?rcl=0&l=295 > > Maybe pick a new name that is more descriptive to the new use only I changes interactive to total to indicate that we don't filer out any slices. Any other suggestions most welcome. If it does not cause too much confusion, how about renaming: runtimeStatsMetric ==> runtimeStatsStartupMetric runtimeStatsInteractiveMetric ==> runtimeStatsMetric.
lgtm
lgtm
Description was changed from ========== Adds runtimeStatsInteractiveMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsInteractiveMetric which measures the v8 metrics for the total duration of the page. ========== to ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page. ==========
On 2017/01/19 at 19:01:28, Camillo Bruni wrote: > lgtm I updated the metric name in the commit message.
Description was changed from ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page. ========== to ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page and buckets them based on user expectations. ==========
I added support to filter out sub-categories in v8 and to bucket the runtimeStats based on UE. I have retained the original metric that measures startup as is for now. We can replace that, once the new metric is stable. PTAL. Thanks, Mythri
nednguyen@google.com changed reviewers: + benjhayden@chromium.org - nednguyen@google.com
On 2017/01/31 01:13:31, mythria wrote: > I added support to filter out sub-categories in v8 and to bucket the > runtimeStats based on UE. I have retained the original metric that measures > startup as is for now. We can replace that, once the new metric is stable. > PTAL. > > Thanks, > Mythri I defer reviewing this to Ben & Tim
nednguyen@google.com changed reviewers: + tdresser@chromium.org
Thanks! This looks pretty good. I just have some nits, a question, and a performance suggestion. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:120: // when the runtimeStatesMetricTotal is stable. s/States/Stats/ ? Any idea when that function will be stable? Are there concrete plans to change it? A design doc? https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:130: function computeRuntimeStatsBucketOnUE(values, slices, histogramNamePrefix, s/values/histograms/g https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:187: var slices = [...model.getDescendantEvents()].filter(event => It looks like you're iterating over every event in the model as many times as there are Segments. That can be quite slow. Can you split this filter into two parts? The first part can happen before the loop over the segments and capture all V8ThreadSlices into an array named something like |v8ThreadSlices|; the second part must run here and filter by segment. The second part can use segment.range.filterArray(v8ThreadSlices, event => event.start) filterArray() requires that v8ThreadSlices is sorted. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:194: var slices = [...model.getDescendantEvents()].filter(event => This can move before the loop over the segments and be re-used in the loop over the segments.
eakuefner@chromium.org changed reviewers: - eakuefner@chromium.org
I also defer reviewing this to Ben & Tim.
https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:171: for (var expectation of segment.expectations) { This has an implicit priority for metrics, since for each segment, we report to a single histogramName. We should justify our prioritization here.
Thanks for your reviews. I am sorry for the delay. I was at BlinkOn and travelling, so couldn't address the comments earlier. PTAL. Thanks, Mythri. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:120: // when the runtimeStatesMetricTotal is stable. On 2017/01/31 17:55:42, benjhayden wrote: > s/States/Stats/ ? > > Any idea when that function will be stable? Are there concrete plans to change > it? A design doc? I am planning to add browse benchmarks based on the new metric we are adding. The plan is to wait for a couple of weeks to see that the benchmark is stable. Once the browse benchmarks are running well, then we can switch the existing startup benchmarks also to use the runtimeStatsMetricTotal and remove the runtimeStatsMetric. I will add a crbug to track this work. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:130: function computeRuntimeStatsBucketOnUE(values, slices, histogramNamePrefix, On 2017/01/31 17:55:42, benjhayden wrote: > s/values/histograms/g Done. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:171: for (var expectation of segment.expectations) { On 2017/02/03 00:36:45, tdresser wrote: > This has an implicit priority for metrics, since for each segment, we report to > a single histogramName. We should justify our prioritization here. Thanks. This was a bug in the implementation. The idea was basically to add samples to all expectations that this segment belongs to, so that any noise because of differences in the extent of overlaps. I fixed this bug. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:187: var slices = [...model.getDescendantEvents()].filter(event => On 2017/01/31 17:55:42, benjhayden wrote: > It looks like you're iterating over every event in the model as many times as > there are Segments. That can be quite slow. > Can you split this filter into two parts? The first part can happen before the > loop over the segments and capture all V8ThreadSlices into an array named > something like |v8ThreadSlices|; the second part must run here and filter by > segment. > The second part can use segment.range.filterArray(v8ThreadSlices, event => > event.start) > filterArray() requires that v8ThreadSlices is sorted. Thanks for the detailed comment. I fixed it. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:194: var slices = [...model.getDescendantEvents()].filter(event => On 2017/01/31 17:55:42, benjhayden wrote: > This can move before the loop over the segments and be re-used in the loop over > the segments. Done.
Can you add tests? https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:171: for (var expectation of segment.expectations) { On 2017/02/03 11:48:39, mythria wrote: > On 2017/02/03 00:36:45, tdresser wrote: > > This has an implicit priority for metrics, since for each segment, we report > to > > a single histogramName. We should justify our prioritization here. > > Thanks. This was a bug in the implementation. The idea was basically to add > samples to all expectations that this segment belongs to, so that any noise > because of differences in the extent of overlaps. I fixed this bug. Lets make sure that each segment is added to a single bucket, the numbers become confusing if we count the overlap
Thanks for the reviews. I added tests for the new metric. I haven't changed the way we account for the v8 slices when there is a overlap between the expectations. After discussing with Camillo, we thought it might be a bit less noisy if we duplicate the values in the overlapped interval in each of the expectations in that interval. I renamed the histogram from 'Total' to 'Any' so it is less confusing. Could you please take a look? Thanks, Mythri. https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/v8/runtime_stats_metric.html:171: for (var expectation of segment.expectations) { On 2017/02/03 17:50:13, fmeawad wrote: > On 2017/02/03 11:48:39, mythria wrote: > > On 2017/02/03 00:36:45, tdresser wrote: > > > This has an implicit priority for metrics, since for each segment, we report > > to > > > a single histogramName. We should justify our prioritization here. > > > > Thanks. This was a bug in the implementation. The idea was basically to add > > samples to all expectations that this segment belongs to, so that any noise > > because of differences in the extent of overlaps. I fixed this bug. > > Lets make sure that each segment is added to a single bucket, the numbers become > confusing if we count the overlap I took a quick look at the % of overlap for pages from the browse benchmark. On most pages there is no overlap, but there are some pages where the overlap is significant. For ex: on cnn, flipboard, youtube it is 8-10% and on twitter ,reddit it is close to 17-20%. After discussing with Camillo, we thought it is better to duplicate the values in each of the buckets to avoid the noise due to difference in overlap. I renamed the histogram from Total to Any to avoid confusion that the numbers in the UE buckets should add up to the Total.
On 2017/02/07 15:26:07, mythria wrote: > Thanks for the reviews. I added tests for the new metric. I haven't changed the > way we account for the v8 slices when there is a overlap between the > expectations. After discussing with Camillo, we thought it might be a bit less > noisy if we duplicate the values in the overlapped interval in each of the > expectations in that interval. I renamed the histogram from 'Total' to 'Any' so > it is less confusing. Could you please take a look? > > Thanks, > Mythri. > > https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2639993002/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/v8/runtime_stats_metric.html:171: for (var expectation > of segment.expectations) { > On 2017/02/03 17:50:13, fmeawad wrote: > > On 2017/02/03 11:48:39, mythria wrote: > > > On 2017/02/03 00:36:45, tdresser wrote: > > > > This has an implicit priority for metrics, since for each segment, we > report > > > to > > > > a single histogramName. We should justify our prioritization here. > > > > > > Thanks. This was a bug in the implementation. The idea was basically to add > > > samples to all expectations that this segment belongs to, so that any noise > > > because of differences in the extent of overlaps. I fixed this bug. > > > > Lets make sure that each segment is added to a single bucket, the numbers > become > > confusing if we count the overlap > > I took a quick look at the % of overlap for pages from the browse benchmark. On > most pages there is no overlap, but there are some pages where the overlap is > significant. For ex: on cnn, flipboard, youtube it is 8-10% and on twitter > ,reddit it is close to 17-20%. After discussing with Camillo, we thought it is > better to duplicate the values in each of the buckets to avoid the noise due to > difference in overlap. I renamed the histogram from Total to Any to avoid > confusion that the numbers in the UE buckets should add up to the Total. I would like Ned to weigh in on that decision and also Ben. I am OK with it but I am not a fan. If we adopt that decision here we will adopt everywhere else, as I think this metric is just the start.
fmeawad@chromium.org changed reviewers: + nednguyen@google.com
Ned, can you PTAL on the decision of counting the overlap as part of both UEs?
Double counting is not a problem since we are not summing up the data from UE in any form. Only potential problem is duplicate alerts but the dashboard should be able to deal with that already. lgtm https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:198: histogramName = 'Startup'; there should be no startup for v8?
It looks like you don't need Segments, after all, then. :-) Sorry some of these comments are for an old patch, but they still apply. https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:120: // TODO(crbug.com/688342): Remove this metric and use runtimeStatsMetricTotal s/MetricTotal/TotalMetric/ https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:121: // instead when the runtimeStatsMetricTotal is stable. s/MetricTotal/TotalMetric/ https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:200: tr.metrics.MetricRegistry.register(runtimeStatsTotalMetric); charliea: Should we recommend 1 metric per file, like we recommend 1 class per file? https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:204: runtimeStatsMetric, runtimeStatsTotalMetric Please put these on separate lines, with a trailing comma for consistency: return { runtimeStatsMetric, runtimeStatsTotalMetric, }; https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:202: computeRuntimeStatsBucketOnUE(histograms, slices, histogramName, If you're going to double-count like this, then there's no reason to use segments. You can just iterate over model.userModel.expectations directly instead of iterating over segments.
mythria@chromium.org changed reviewers: + charliea@chromium.org
Thanks for the reviews. I addressed all of the comments except the one about moving the metric to a new file. PTAL. Also adding charliea@. charliea@ : For the context: I am adding a new metric to measure runtimeStats from v8. There is an existing metric (runtimeStatsMetric) that measures runtimeStats till the time to interact. I am adding a new metric (runtimeStatsTotalMetric) to measure the runtimeStats over the entire duration of the page and bucket them using UEs. In future, we want to replace the existing one (runtimeStatsMetric) with the new one (runtimeStatsTotalMetric). The question from benjhayden is: Would it be better to add the new metric in a separate file so that we have 1 metric per file. (https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric...) Thanks, Mythri https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:120: // TODO(crbug.com/688342): Remove this metric and use runtimeStatsMetricTotal On 2017/02/07 23:08:19, benjhayden wrote: > s/MetricTotal/TotalMetric/ Thanks. Done. https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:121: // instead when the runtimeStatsMetricTotal is stable. On 2017/02/07 23:08:19, benjhayden wrote: > s/MetricTotal/TotalMetric/ Thanks. Done. https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:200: tr.metrics.MetricRegistry.register(runtimeStatsTotalMetric); On 2017/02/07 23:08:19, benjhayden wrote: > charliea: Should we recommend 1 metric per file, like we recommend 1 class per > file? If it is better to move the new metric to a different file, I will move it to a new file. https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:204: runtimeStatsMetric, runtimeStatsTotalMetric On 2017/02/07 23:08:19, benjhayden wrote: > Please put these on separate lines, with a trailing comma for consistency: > return { > runtimeStatsMetric, > runtimeStatsTotalMetric, > }; Done. https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:198: histogramName = 'Startup'; On 2017/02/07 17:52:01, nednguyen wrote: > there should be no startup for v8? I see some V8 samples in startup bucket. They are very minor % of the overall samples. If it isn't expected I can investigate further. https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:202: computeRuntimeStatsBucketOnUE(histograms, slices, histogramName, On 2017/02/07 23:08:19, benjhayden wrote: > If you're going to double-count like this, then there's no reason to use > segments. You can just iterate over model.userModel.expectations directly > instead of iterating over segments. Thanks. I updated the metric to iterate over expectations instead. Done.
Description was changed from ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page and buckets them based on user expectations. ========== to ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page and buckets them based on user expectations. BUG=chromium:686250 ==========
2 nits, an FYI, lgtm, please wait to see what charliea says Thanks! https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:168: for (var expectation of model.userModel.expectations) { It would be good to keep the comment that was here about double-counting when expectations overlap so that we can see that it's deliberate. https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:172: continue; Please put this on the previous line. https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:173: // filterArray filters the array that intersects the range inclusively. No changes here necessary for now, this is FYI. Ideally, filterArray would support this use-case so that you wouldn't need to do this. When I added the Inclusive and Exclusive versions of many of Range's methods, I considered supporting half-open intervals like this but it was deemed unnecessary complexity that might impact performance. https://github.com/catapult-project/catapult/issues/3196
On 2017/02/08 10:17:20, mythria wrote: > Thanks for the reviews. I addressed all of the comments except the one about > moving the metric to a new file. PTAL. Also adding charliea@. > > charliea@ : > For the context: I am adding a new metric to measure runtimeStats from v8. There > is an existing metric (runtimeStatsMetric) that measures runtimeStats till the > time to interact. I am adding a new metric (runtimeStatsTotalMetric) to measure > the runtimeStats over the entire duration of the page and bucket them using UEs. > In future, we want to replace the existing one > (runtimeStatsMetric) with the new one (runtimeStatsTotalMetric). > > The question from benjhayden is: Would it be better to add the new metric in a > separate file so that we have 1 metric per file. > (https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric...) > > Thanks, > Mythri > > https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:120: // > TODO(crbug.com/688342): Remove this metric and use runtimeStatsMetricTotal > On 2017/02/07 23:08:19, benjhayden wrote: > > s/MetricTotal/TotalMetric/ > > Thanks. Done. > > https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:121: // instead when the > runtimeStatsMetricTotal is stable. > On 2017/02/07 23:08:19, benjhayden wrote: > > s/MetricTotal/TotalMetric/ > > Thanks. Done. > > https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:200: > tr.metrics.MetricRegistry.register(runtimeStatsTotalMetric); > On 2017/02/07 23:08:19, benjhayden wrote: > > charliea: Should we recommend 1 metric per file, like we recommend 1 class per > > file? > > If it is better to move the new metric to a different file, I will move it to a > new file. > > https://codereview.chromium.org/2639993002/diff/100001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:204: runtimeStatsMetric, > runtimeStatsTotalMetric > On 2017/02/07 23:08:19, benjhayden wrote: > > Please put these on separate lines, with a trailing comma for consistency: > > return { > > runtimeStatsMetric, > > runtimeStatsTotalMetric, > > }; > > Done. > > https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:198: histogramName = > 'Startup'; > On 2017/02/07 17:52:01, nednguyen wrote: > > there should be no startup for v8? > > I see some V8 samples in startup bucket. They are very minor % of the overall > samples. If it isn't expected I can investigate further. > > https://codereview.chromium.org/2639993002/diff/140001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:202: > computeRuntimeStatsBucketOnUE(histograms, slices, histogramName, > On 2017/02/07 23:08:19, benjhayden wrote: > > If you're going to double-count like this, then there's no reason to use > > segments. You can just iterate over model.userModel.expectations directly > > instead of iterating over segments. > > Thanks. I updated the metric to iterate over expectations instead. Done. lgtm w/ nits I think that, given that they share a decent amount of code and one will eventually replace the other, this is a rare instance where multiple metrics living in the same file is probably okay, as long as the plan is that runtimeStatsTotal is either renamed runtimeStats after runtimeStats is killed or the whole file is renamed runtime_stats_total.html.
Thanks for your reviews. Once the new metric lands and we start measuring it on benchmarks, we will just ensure the startup runtimeStats (runtimeStats metric) can be computed by looking at appropriate UE buckets. Once we ensure the metrics are consistent I will remove the runtimeStatsMetric and rename the runtimeStatsTotalMetric to runtimeStatsMetric. Thanks, Mythri https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:168: for (var expectation of model.userModel.expectations) { On 2017/02/08 20:11:26, benjhayden wrote: > It would be good to keep the comment that was here about double-counting when > expectations overlap so that we can see that it's deliberate. Thanks. Done. https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:172: continue; On 2017/02/08 20:11:26, benjhayden wrote: > Please put this on the previous line. > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... Thanks. Done. https://codereview.chromium.org/2639993002/diff/160001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:173: // filterArray filters the array that intersects the range inclusively. On 2017/02/08 20:11:26, benjhayden wrote: > No changes here necessary for now, this is FYI. > > Ideally, filterArray would support this use-case so that you wouldn't need to do > this. > When I added the Inclusive and Exclusive versions of many of Range's methods, I > considered supporting half-open intervals like this but it was deemed > unnecessary complexity that might impact performance. > https://github.com/catapult-project/catapult/issues/3196 Thanks.
Fadi and Ben, I am sorry, I did not notice earlier that I was adding multiple samples to the histogram and hence the final metric that would be reported would be the average time spent in each of the UE intervals and not the sum of the time spent in all intervals. What we need to track is the aggregate time spent in each UE. I did not notice this till I added actual benchmark and looked at the results closely. Could you please look at the changes in the latest patch. I am sorry for the several iterations over this cl. Thanks, Mythri https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:182: name); We just want to add one sample per each UE, basically the sum of all slices for that UE. I am sorry, I did not realize that what I did earlier will add multiple samples to the histogram and hence the metrics we may track would not be the sum of all samples in each UE, but would be the average of the UEs in each bucket.
No worries, metrics are hard! 2 more nits, another fyi, still lgtm :-) https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:152: var v8SlicesBucketOnUEMap = {}; Please use ES6 Maps instead of dictionaries. https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:167: if (!expectation.range.intersectsRangeExclusive(lastSlice.range)) Please use curly braces here. https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:182: name); On 2017/02/09 at 17:21:39, mythria wrote: > We just want to add one sample per each UE, basically the sum of all slices for that UE. I am sorry, I did not realize that what I did earlier will add multiple samples to the histogram and hence the metrics we may track would not be the sum of all samples in each UE, but would be the average of the UEs in each bucket. Just to clarify, this doesn't add one sample per UserExpectation object, this code adds one sample per unique *stageTitle* (R/A/I/L). Both can be valid metric formulations, and it can also be valid to add one histogram sample per v8 sample. Which formulation you want is up to you, the metric designer. If you're not sure, you can try the metric this way, see how it works out, and then revisit this design decision. Histograms can compute both average and sum statistics, so the differences between the 3 formulations are basically only relevant when you consider how Histograms will be merged across story repetitions and stories. This merging currently only happens in results.html, but it will be happening in the Histogram Pipeline soon, which will affect the data on the dashboard. Once historic data is on the dashboard, you can't easily recompute it, so if you need to reformulate a metric down the road, you might lose historical continuity in the charts on the dashboard. This is all just FYI, and an apology for not knowing how to explain it very well on how-to-write-metrics.md. :-)
On 2017/02/10 17:59:25, benjhayden wrote: > No worries, metrics are hard! > > 2 more nits, another fyi, still lgtm :-) > > https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:152: var > v8SlicesBucketOnUEMap = {}; > Please use ES6 Maps instead of dictionaries. > > https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:167: if > (!expectation.range.intersectsRangeExclusive(lastSlice.range)) > Please use curly braces here. > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... > > https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:182: name); > On 2017/02/09 at 17:21:39, mythria wrote: > > We just want to add one sample per each UE, basically the sum of all slices > for that UE. I am sorry, I did not realize that what I did earlier will add > multiple samples to the histogram and hence the metrics we may track would not > be the sum of all samples in each UE, but would be the average of the UEs in > each bucket. > > Just to clarify, this doesn't add one sample per UserExpectation object, this > code adds one sample per unique *stageTitle* (R/A/I/L). > Both can be valid metric formulations, and it can also be valid to add one > histogram sample per v8 sample. > Which formulation you want is up to you, the metric designer. If you're not > sure, you can try the metric this way, see how it works out, and then revisit > this design decision. > Histograms can compute both average and sum statistics, so the differences > between the 3 formulations are basically only relevant when you consider how > Histograms will be merged across story repetitions and stories. This merging > currently only happens in results.html, but it will be happening in the > Histogram Pipeline soon, which will affect the data on the dashboard. Once > historic data is on the dashboard, you can't easily recompute it, so if you need > to reformulate a metric down the road, you might lose historical continuity in > the charts on the dashboard. > > This is all just FYI, and an apology for not knowing how to explain it very well > on how-to-write-metrics.md. :-) Can you send me the benchmark patch to patch on top of this one to see if we are getting the expected output? (and it would be great if you can upload a results2.html output for this metric for inspection (maybe with and without --future :) )
Thanks for the reviews. I fixed both nits. Also, thanks for the detailed explanation. Fadi, Here is the data for the runtime stats metric for the system health's browse story set: https://mythria.users.x20web.corp.google.com/www/results_rcs.html This is the cl that uses this metric for the v8.runtimestats.browse benchmark: https://codereview.chromium.org/2639213002/ This is the link that has the data from this benchmark. This contains both runtimeCallStats and GC metrics: https://mythria.users.x20web.corp.google.com/www/results_rcs_gc.html PTAL. Thanks, Mythri https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:152: var v8SlicesBucketOnUEMap = {}; On 2017/02/10 17:59:25, benjhayden wrote: > Please use ES6 Maps instead of dictionaries. Done. https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:167: if (!expectation.range.intersectsRangeExclusive(lastSlice.range)) On 2017/02/10 17:59:25, benjhayden wrote: > Please use curly braces here. > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... Thanks. Done. https://codereview.chromium.org/2639993002/diff/200001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:182: name); On 2017/02/10 17:59:25, benjhayden wrote: > On 2017/02/09 at 17:21:39, mythria wrote: > > We just want to add one sample per each UE, basically the sum of all slices > for that UE. I am sorry, I did not realize that what I did earlier will add > multiple samples to the histogram and hence the metrics we may track would not > be the sum of all samples in each UE, but would be the average of the UEs in > each bucket. > > Just to clarify, this doesn't add one sample per UserExpectation object, this > code adds one sample per unique *stageTitle* (R/A/I/L). > Both can be valid metric formulations, and it can also be valid to add one > histogram sample per v8 sample. > Which formulation you want is up to you, the metric designer. If you're not > sure, you can try the metric this way, see how it works out, and then revisit > this design decision. > Histograms can compute both average and sum statistics, so the differences > between the 3 formulations are basically only relevant when you consider how > Histograms will be merged across story repetitions and stories. This merging > currently only happens in results.html, but it will be happening in the > Histogram Pipeline soon, which will affect the data on the dashboard. Once > historic data is on the dashboard, you can't easily recompute it, so if you need > to reformulate a metric down the road, you might lose historical continuity in > the charts on the dashboard. > > This is all just FYI, and an apology for not knowing how to explain it very well > on how-to-write-metrics.md. :-) Thanks for the detailed explanation.
The CQ bit was checked by mythria@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 Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...) Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by mythria@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.
https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... tracing/tracing/metrics/v8/runtime_stats_metric.html:187: computeRuntimeStatsBucketOnUE(histograms, v8ThreadSlices, 'Any'); Fadi and I were looking at results_rcs_gc.html and we noticed that there are quite a lot of Histograms. TBMv2 has facilities to help organize Histograms so that users don't have to deal with all of them at once. You'll notice the RelatedHistogramBreakdown diagnostics in runtimeStatsMetric. You probably don't want to use RelatedHistogramBreakdowns here since "breakdown" implies exclusivity, whereas UEs can overlap here. However, you can use RelatedHistogramMaps to encode relationships so that the 'Any' Histograms are always visible in results.html at the top level, and the stage-specific Histograms are only visible as diagnostics. Do you want to schedule a VC? Or try to add the RelatedHistogramMaps and see if you have any questions?
On 2017/02/13 at 20:40:30, benjhayden wrote: > https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... > tracing/tracing/metrics/v8/runtime_stats_metric.html:187: computeRuntimeStatsBucketOnUE(histograms, v8ThreadSlices, 'Any'); > Fadi and I were looking at results_rcs_gc.html and we noticed that there are quite a lot of Histograms. > TBMv2 has facilities to help organize Histograms so that users don't have to deal with all of them at once. > You'll notice the RelatedHistogramBreakdown diagnostics in runtimeStatsMetric. > You probably don't want to use RelatedHistogramBreakdowns here since "breakdown" implies exclusivity, whereas UEs can overlap here. > However, you can use RelatedHistogramMaps to encode relationships so that the 'Any' Histograms are always visible in results.html at the top level, and the stage-specific Histograms are only visible as diagnostics. > Do you want to schedule a VC? Or try to add the RelatedHistogramMaps and see if you have any questions? As just discussed over VC, it's ok to land this CL without the RelatedHistogramMaps and add them in a followup. However, before enabling the benchmark, you might want to use customizeSummaryOptions() to control which statistics will be uploaded to the dashboard. That's a method on Histogram that most other metrics use, so you can see how they use it.
On 2017/02/14 17:20:53, benjhayden wrote: > On 2017/02/13 at 20:40:30, benjhayden wrote: > > > https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... > > File tracing/tracing/metrics/v8/runtime_stats_metric.html (right): > > > > > https://codereview.chromium.org/2639993002/diff/240001/tracing/tracing/metric... > > tracing/tracing/metrics/v8/runtime_stats_metric.html:187: > computeRuntimeStatsBucketOnUE(histograms, v8ThreadSlices, 'Any'); > > Fadi and I were looking at results_rcs_gc.html and we noticed that there are > quite a lot of Histograms. > > TBMv2 has facilities to help organize Histograms so that users don't have to > deal with all of them at once. > > You'll notice the RelatedHistogramBreakdown diagnostics in runtimeStatsMetric. > > You probably don't want to use RelatedHistogramBreakdowns here since > "breakdown" implies exclusivity, whereas UEs can overlap here. > > However, you can use RelatedHistogramMaps to encode relationships so that the > 'Any' Histograms are always visible in results.html at the top level, and the > stage-specific Histograms are only visible as diagnostics. > > Do you want to schedule a VC? Or try to add the RelatedHistogramMaps and see > if you have any questions? > > As just discussed over VC, it's ok to land this CL without the > RelatedHistogramMaps and add them in a followup. > > However, before enabling the benchmark, you might want to use > customizeSummaryOptions() to control which statistics will be uploaded to the > dashboard. That's a method on Histogram that most other metrics use, so you can > see how they use it. Thanks Ben. customizeSummaryOptions is already used in functions createDurationHistogram_ and createCountHistogram_. I use these functions to create histograms. I did not pay attention to it earlier. Since we have only one sample we disable everything apart from avg. I will land this cl now and add the RelatedHistogramMaps in a followup cl. Thanks, Mythri
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org, cbruni@chromium.org, nednguyen@google.com, charliea@chromium.org, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2639993002/#ps240001 (title: "Rebased the patch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487094303359620,
"parent_rev": "53d6eed4b031ff4c93eee193da59de1eabf418f5", "commit_rev":
"3a702528b20ff1d13d8abe3ee37a8bf2772f64d2"}
Message was sent while issue was closed.
Description was changed from ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page and buckets them based on user expectations. BUG=chromium:686250 ========== to ========== Adds runtimeStatsTotalMetric to collect v8 metrics on pages with interaction. RuntimeStats metric measures various v8 metrics till the time to interact to measure the startup time of the web pages. This cl extends this metric by adding runtimeStatsTotalMetric which measures the v8 metrics for the total duration of the page and buckets them based on user expectations. BUG=chromium:686250 Review-Url: https://codereview.chromium.org/2639993002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
