Ned, is this work owned or planned for? If so can we give the bug an owner?

+graphic folks: do you have any plan when we can get to this?

I think it makes sense for this to happen once issue 760553 is done? I will find an owner for this task.

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0

commit 5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0
Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Date: Tue Mar 06 00:22:26 2018

Tracing: CPU time computation clean up

tr.model.Thread.getCpuStatsForRange is not used anywhere. Instead, there is a
code for computing CPU time of a thread in a range that is duplicated in two
places, for some reason, and is more useful.  This CL deletes
tr.model.Thread.getCpuStatsForRange and replaces it with the more interesting
version. This is in preparation for migrating thread times metrics to TBMv2.

Bug:  chromium:627461 
Change-Id: I8bfe6813dd8e0c69c352f61fa61f4620e53449d2
Reviewed-on: https://chromium-review.googlesource.com/949771
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Deepanjan Roy <dproy@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>

[modify] https://crrev.com/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0/tracing/tracing/model/thread_test.html
[modify] https://crrev.com/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0/tracing/tracing/model/thread.html
[modify] https://crrev.com/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0/tracing/tracing/extras/chrome/cpu_time_test.html
[modify] https://crrev.com/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0/tracing/tracing/metrics/system_health/cpu_time_metric.html
[modify] https://crrev.com/5c590a9bf20e5f08030c93ecf7c78a3b1dd2e7a0/tracing/tracing/extras/chrome/cpu_time.html

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/718296623c33797a6fe4ea95cca9829c7d02713a

commit 718296623c33797a6fe4ea95cca9829c7d02713a
Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Date: Mon Mar 19 20:22:17 2018

First set of thread times metrics in TBMv2

This implements cores_per_second_{thread group}_thread metrics.
Please see the documentation of the metric in
tracing/tracing/metrics/rendering_metric.html.

An example run for thread_times.tough_scrolling_cases, story
text_10000_pixels_per_second gave the following numbers:

Name                                        thread_times
=========================================================
cores_per_second_all_thread                 0.371 cores/s
cores_per_second_browser_thread             0.033 cores/s
cores_per_second_fast_path_thread           0.251 cores/s
cores_per_second_gpu_thread                 0.125 cores/s
cores_per_second_io_thread                  0.049 cores/s
cores_per_second_other_thread               0.050 cores/s
cores_per_second_raster_thread              0.040 cores/s
cores_per_second_renderer_compositor_thread 0.045 cores/s
cores_per_second_renderer_main_thread       0.030 cores/s

Bug:  chromium:627461 
Change-Id: Ia8b7a4d1cec870b75782266c675ab6446563e8f3
Reviewed-on: https://chromium-review.googlesource.com/952197
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>

[modify] https://crrev.com/718296623c33797a6fe4ea95cca9829c7d02713a/tracing/trace_viewer.gypi
[add] https://crrev.com/718296623c33797a6fe4ea95cca9829c7d02713a/tracing/tracing/metrics/rendering_metric_test.html
[modify] https://crrev.com/718296623c33797a6fe4ea95cca9829c7d02713a/tracing/tracing/model/timed_event.html
[modify] https://crrev.com/718296623c33797a6fe4ea95cca9829c7d02713a/tracing/tracing/metrics/all_metrics.html
[add] https://crrev.com/718296623c33797a6fe4ea95cca9829c7d02713a/tracing/tracing/metrics/rendering_metric.html

+dproy, how does this relate to your CPU time metrics? Should we use the same breakdown structure?

This is a more specialized version of the new cpu time metric. Maybe we should eventually unify the two, but my understanding was it was easier to write a custom metric at the moment. 

The breakdowns + related decisions for this metric is based on the needs of the graphics team, and I'll defer the decision to them. I used process:thread:stage:initiator breakdown in the new cpu time metric, and it's rather overwhelming and not obviously better. 

 

I think that the metric names are a little misleading here: "cores_per_second_all_thread" seems to indicate that the metric is agnostic as to what purpose the CPU is being utilized for. In reality, we're only measuring cores per second for animations. As a rule, the metric names should require as little outside context as possible to understand so that someone browsing the benchmarks from the perf dashboard can select a metric and see the information that they expect.

Also, I think that the idea of "cores per second" already has a more commonly used metric name: CPU utilization. Given that, I'd like to suggest renaming these metrics to animation_cpu_utilization_<threads_category> or something similar.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6aea1cac122502499713bfc182d0454d0f325fa9

commit 6aea1cac122502499713bfc182d0454d0f325fa9
Author: catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Mar 23 14:48:06 2018

Roll src/third_party/catapult/ 734f737c6..c4e9b1332 (14 commits)

https://chromium.googlesource.com/catapult.git/+log/734f737c6b57..c4e9b1332818

$ git log 734f737c6..c4e9b1332 --date=short --no-merges --format='%ad %ae %s'
2018-03-23 nednguyen Revert "Add --enable-automation flag in GetFromBrowserOptions()"
2018-03-22 dtu [pinpoint] Set auto_explore == True for "patch jobs".
2018-03-21 ynovikov Remove a note on android_optional_gpu_tests_rel from manual rolls doc.
2018-03-15 dproy Reland "More precise self time calculation"
2018-03-21 nednguyen Ignore fetchts file
2018-03-20 laszio [Telemetry] Support outputing json in benchmark_runner
2018-03-19 kjharland [Dashboard] Whitelist prod Fuchsia Garnet builder.
2018-03-20 benjhayden Fix sorting results.html
2018-03-19 horo Add --enable-automation flag in GetFromBrowserOptions()
2018-03-19 eakuefner [Dashboard] Avoid creating empty rows/histograms/tests if histograms are empty
2018-03-16 dtu [pinpoint] Infer "repository" parameter from configuration.
2018-03-19 chiniforooshan First set of thread times metrics in TBMv2
2018-03-16 erikchen Replace --enable-heap-profiling with --memlog equivalent.
2018-03-19 wangxianzhu Fix visual rect support for cc::DisplayItemList

Created with:
  roll-dep src/third_party/catapult
BUG=chromium:822258, chromium:786572 , chromium:731979 ,chromium:821531,chromium:821521, chromium:819969 ,chromium:822258, chromium:627461 , chromium:822843 


The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=sullivan@chromium.org

Change-Id: I8c0025df6198bf9b1662bb8d78e45741d66f6aff
Reviewed-on: https://chromium-review.googlesource.com/977884
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#545441}
[modify] https://crrev.com/6aea1cac122502499713bfc182d0454d0f325fa9/DEPS

Ehsan: what is the next step for this task? Do you think we can remove the thread_times_*per_frame/per_second metrics in favour of cores_per_second* metrics? If not, should we move over the old metrics into tbmv2?

#14: To be honest, I don't think I'm really the person to answer this question. You and/or vmiura@ co-own the vast majority of thread_times benchmarks, so I'd say it's your judgement call to make. I'm basically fine with any solution that eventually leads to us subsuming the non-TBMv2 version of thread_times completely.

skyostil@ owns thread_times.key_idle_power_cases, and bokan@ and nzolghadr@ own thread_times.tough_scrolling_cases, so maybe it would be useful to get their input.

That was for Ehsan (chiniforooshan@), who added the new cores_per_second* metrics :)

#14: I would say let's wait for crrev.com/c/1117474 to land. That patch changes the animation periods for which we measure the new TBMv2 metrics to be similar to the ones used in smoothness/thread_times benchmarks instead of user-model based ones. After that, I will monitor the two versions, let's say for about 2 weeks, and we can make a decision then. Does it make sense?

Yep, thanks!

My bad, of course I misread Ehsan as Ethan :) 

Agreed with #21: I totally read cores_per_second as meaning CPU utilization instead of an animation-specific measurement. This is particularly important for key_idle_power_cases which test scenarios where no animation is taking place (and thus we should be using very little power).

#20 and #12: Thanks for suggestions. This is still a WIP metric: after crrev.com/c/1117474 lands, the metric will not be animation specific. It will measure CPU utilization during the period that is marked by the test (can be an animation or anything else that the test is interested in). I will rename it to cpu_utilization if we decide to keep the metric later.

The NextAction date has arrived: 2018-07-17

What is the status of this? Can we remove a set of the thread-times metrics?

I've been doing a lot of metrics analysis recently, and having a hard time interpreting the cores_per_second metrics.

Sorry, but the thread_*_cpu_time_per_frame in milliseconds are still my go-to metrics.  Could we keep those?

(same here. it's not clear to me how to interpret cores_per_second_* metrics)

Sorry for the confusion. I should've thought and investigate more about the name. cores_per_second_* metrics are not new; they are exactly the same as existing thread_*_cpu_time_per_second, just implemented in TBMv2. For example:

https://chromeperf.appspot.com/report?sid=ee9aab9f4505007671341d8113587088a52687abd2cf4a9f37ef9721ecd1a4e2

I'll rename cores_per_second metrics to thread_*_cpu_time_per_second_tbmv2, to avoid confusion. The meaning is really simple: they show CPU utilization of different thread groups during the test.

thread_*_cpu_per_frame is just the product of thread_*_cpu_time_per_second and frame_times. Here is an example:

https://chromeperf.appspot.com/report?sid=89b8f0848db4c3c7ffe6fa3bb9b7979f49672cb4a0ee5f34ee9bf993bf2e6217

IMO, it is redundant to keep all 3 metrics when one of them is the product of the other two. But, I'm OK with it. We can discuss this in a separate bug later, since there are other redundant sets of metrics, too, like frame_times and mean_frame_times which are different averages of the same thing.

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/68c00fdca168795dd1fb9c387a2e9c74dde70d7f

commit 68c00fdca168795dd1fb9c387a2e9c74dde70d7f
Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Date: Mon Aug 20 19:09:45 2018

Telemetry: rename metrics as per  crbug.com/627461 

cores_per_second_* metrics should be called thread_*_cpu_per_second_tbmv2
since they are just TBMv2 versions of thread_*_cpu_per_second.

Bug:  chromium:627461 
Change-Id: I69db70b125a9595c7b90e1d026104bdbb287b7f2
Reviewed-on: https://chromium-review.googlesource.com/1181702
Reviewed-by: Ben Hayden <benjhayden@chromium.org>
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>

[modify] https://crrev.com/68c00fdca168795dd1fb9c387a2e9c74dde70d7f/tracing/tracing/metrics/rendering/cpu_utilization_test.html
[modify] https://crrev.com/68c00fdca168795dd1fb9c387a2e9c74dde70d7f/tracing/tracing/metrics/rendering/cpu_utilization.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bcc387a51abaa21aa580d1703ff431c1356c07b4

commit bcc387a51abaa21aa580d1703ff431c1356c07b4
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Aug 22 17:46:38 2018

Roll src/third_party/catapult ba76717a8d18..bbb04a38bbdb (23 commits)

https://chromium.googlesource.com/catapult.git/+log/ba76717a8d18..bbb04a38bbdb


git log ba76717a8d18..bbb04a38bbdb --date=short --no-merges --format='%ad %ae %s'
2018-08-22 wangge@google.com Fix Bug When No APK is present and Add Relevant Test Cases.
2018-08-22 anthonyalridge@google.com Initial application of mann whitney testing.
2018-08-22 benjhayden@chromium.org [chromeperf v2] Simple redux helpers.
2018-08-22 benjhayden@chromium.org Add icons for V2SPA.
2018-08-21 mseaborn@google.com [dashboard] Update docs to mention old issues filed in the Github tracker
2018-08-21 benjhayden@chromium.org Fix minify script for v2spa.
2018-08-21 benjhayden@chromium.org Add some utility functions to V2SPA.
2018-08-21 eakuefner@chromium.org [Tracing] Fix Pylint errors
2018-08-21 amyqiu@google.com Fix search bug in metrics visualization tool
2018-08-21 benjhayden@chromium.org Add ElementBase for V2SPA.
2018-08-21 benjhayden@chromium.org Plumb test case tag maps via test suite descriptors.
2018-08-21 vovoy@chromium.org Add story property: wpr_mode
2018-08-21 wangge@google.com Restructure Long Term Health Tool Output File Structure.
2018-08-20 benjhayden@chromium.org Add Material textarea for V2SPA.
2018-08-20 benjhayden@chromium.org Add checkbox to V2SPA.
2018-08-20 benjhayden@chromium.org Add cp-loading for V2SPA.
2018-08-20 benjhayden@chromium.org Add raised-button to V2SPA.
2018-08-20 chiniforooshan@chromium.org Telemetry: pixel metrics in TBMv2
2018-08-20 simonhatch@chromium.org Dashboard - Add a path for inserting out-of-order diagnostics.
2018-08-20 simonhatch@chromium.org Dashboard - Fix gcs read.
2018-08-20 chiniforooshan@chromium.org Telemetry: rename metrics as per  crbug.com/627461 
2018-08-20 simonhatch@chromium.org Dashboard - Cleanup unused masters and bots
2018-08-20 chiniforooshan@chromium.org Telemetry: break rendering_metric.html


Created with:
  gclient setdep -r src/third_party/catapult@bbb04a38bbdb

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:863390 ,chromium:866423,chromium:862077, chromium:863390 ,chromium:760553, chromium:874856 , chromium:627461 ,chromium:760553
TBR=sullivan@chromium.org

Change-Id: I6f9a52e2e301d0e04b4800a9c57a33000ec51f26
Reviewed-on: https://chromium-review.googlesource.com/1185141
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#585147}
[modify] https://crrev.com/bcc387a51abaa21aa580d1703ff431c1356c07b4/DEPS

Since we have TBMv2 implementation of all thread_times (and smoothness) metrics, I'm going to close this bug.  crbug.com/890757  is for the clean up work.

Yay!

Woho!