Improve correctness of thread_times metrics in rendering benchmark |
||
Issue descriptionThe thread_times metric currently counts the number of frames submitted by the renderer-compositor for reporting the various *_per_frame metrics [1]. There are a couple of issues with this: (a) The |frame_times| metrics reported in the same benchmark counts the number of frames produced by the display-compositor [2] (instead of the frames by the renderer-compositor). So the |frame_times| metric and the various |thread_time_*per_frame| and |tasks_per_*frame| metrics aren't using the same definition for a 'frame'. (b) With site-isolation turned on, the various OOPIFs (there are many pages in the pagesets that have them) will also be submitting at least some frames. thread_times currently counts them all. So the number of frames it counts are more than what the user would actually see, and so the thread-time metrics reported would be better than they actually are. [3] If thread_times uses the display-compositor frames, then it addresses both concerns. [1] https://cs.chromium.org/chromium/src/tools/perf/metrics/timeline.py?l=243 [2] https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/web_perf/metrics/rendering_stats.py?l=119 [3] Running pinpoint to see how big the skew is at the moment: https://pinpoint-dot-chromeperf.appspot.com/job/12e363b7640000
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/a0faa9d4d70b8b2e68d73993a4841db2a93d71b5 commit a0faa9d4d70b8b2e68d73993a4841db2a93d71b5 Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org> Date: Fri Sep 28 21:41:21 2018 Telemetry: cpu_per_frame metrics in TBMv2 This is the last set of metrics that we need to migrate to TBMv2. After landing this CL, I'll start deleting legacy codes. Bug: chromium:760553 Bug: chromium:888551 Bug: chromium:888114 Change-Id: I5bf47cf0d6d693e0a1e793b18686911a1dcec078 Reviewed-on: https://chromium-review.googlesource.com/1244036 Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org> Reviewed-by: Ben Hayden <benjhayden@chromium.org> [modify] https://crrev.com/a0faa9d4d70b8b2e68d73993a4841db2a93d71b5/tracing/tracing/metrics/rendering/frame_time.html [modify] https://crrev.com/a0faa9d4d70b8b2e68d73993a4841db2a93d71b5/tracing/tracing/metrics/rendering/rendering_metric.html [modify] https://crrev.com/a0faa9d4d70b8b2e68d73993a4841db2a93d71b5/tracing/tracing/metrics/rendering/cpu_utilization_test.html [modify] https://crrev.com/a0faa9d4d70b8b2e68d73993a4841db2a93d71b5/tracing/tracing/metrics/rendering/cpu_utilization.html
,
Sep 28
Awesome progress Do you think it will be possible to migrate the old metrics data to new names, or instead change the new names to match the old names after they're removed (for continuity)?
,
Sep 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0ac64f08cdcc2d964a60be45db795505784d931 commit f0ac64f08cdcc2d964a60be45db795505784d931 Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Sat Sep 29 19:17:06 2018 Roll src/third_party/catapult d525ef309fca..98289bcecf60 (5 commits) https://chromium.googlesource.com/catapult.git/+log/d525ef309fca..98289bcecf60 git log d525ef309fca..98289bcecf60 --date=short --no-merges --format='%ad %ae %s' 2018-09-28 simonhatch@chromium.org Dashboard - Add missing index for querying last alert. 2018-09-28 chiniforooshan@chromium.org Telemetry: cpu_per_frame metrics in TBMv2 2018-09-28 manojgupta@google.com benchmark_runner: Emit all stories in a benchmark. 2018-09-28 dtu@chromium.org [pinpoint] Don't set difference_count for try jobs. 2018-09-28 jbudorick@chromium.org devil: remove host arm build rules. Created with: gclient setdep -r src/third_party/catapult@98289bcecf60 The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll 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:890325 ,chromium:760553, chromium:888551 , chromium:888114 , chromium:890041 , chromium:887888 TBR=sullivan@chromium.org Change-Id: I25e06429f53579449a9ebd2d1ba948ff6409f4e7 Reviewed-on: https://chromium-review.googlesource.com/1252266 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#595322} [modify] https://crrev.com/f0ac64f08cdcc2d964a60be45db795505784d931/DEPS
,
Oct 1
The TBMv2 implementation, using display compositor frames, is landed. crbug.com/890757 is for the clean up work. c#3: Sure. I'll try to use old names, whenever possible, for continuity. |
||
►
Sign in to add a comment |
||
Comment 1 by chiniforooshan@chromium.org
, Sep 25