New issue
Advanced search Search tips

Issue 883894 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 881891



Sign in to add a comment

Provide way to identify compositor frames submitted from different sources

Project Member Reported by sky@chromium.org, Sep 13

Issue description

In mash both ash and the browser will submit compositor frames (they each have their own compositors). Currently there isn't a good way for telemetry to identify the source, which means the times are aggregated between the two. There needs to be a way for telemetry to identify the source.

Sadrul and myself talked about a couple of ways to do this. I'm passing to Sadrul as he was going to investigate the best approach.
 
Cc: chiniforooshan@chromium.org
Sadrul and I have chatted about this problem in the context of OOPIFs. I think we need a general way to identify types of cc clients and break down metrics by type of client. 
Issue 883314 has been merged into this issue.
The client-name currently used in cc [1] is used primarily for UMAs. These client-names are currently not used for trace-events (afaik). We could argue that trace-events should use the same client-name. There can currently be a single client-name for a process. All of the UMA reports in cc depends on that, since the UMA_ macros cache the histogram (and expect the histogram-name to not change at runtime). So, if we want to allow multiple client-names, then those UMA_ macros need to be replaced:
 . either to use the various base::Uma* functions [2]. This has the added cost of hash + looking for every report, which would be nice to avoid.
 . Update the code to lookup [3] and cache the histograms (either at startup or on-demand) explicitly, and add samples to that as needed.

Alternatively, it would also be possible to instrument ui/compositor with trace-events with names, and use that to compute the frame-times for ash/browser separately.

[1] https://cs.chromium.org/chromium/src/cc/base/histograms.h?l=25
[2] https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.h?l=33
[3] https://cs.chromium.org/chromium/src/base/metrics/histogram.h?l=119
Looks like we are talking about 2 slightly different issues in this bug:

1) c#0: Different sources are submitting compositor frames from the same process: this makes current frame_length metrics almost garbage.

2) c#2: frame_lengths from different sources from different processes are merged together in the same histogram: this still can be a problem, but it's less sever than 1). The problem is e.g. when the sources are supposed to have different FPS, are of different importance, or submit considerably different number of data points.

Just wanted to mention this here to avoid confusion in future discussions.
Sadrul, any update on this? We can do the work, we just need to know what to do.
Cc: sadrul@chromium.org
Owner: sky@chromium.org
ui_frame_times* metrics are currently generated based on the FramePresented trace event in LayerTreeHostImpl [1]. Instead of reporting the trace-event there, if we moved over to report it in ui::Compositor instead [2], then we could include additional info into the trace event from the ui::Compositor (which would need to be given a name when it's created [3], e.g. 'ash' or 'browser'). The code in catapult [4] then would need to be updated to filter the trace-events based on the FramePresented name, as well as that extra parameter.

Ehsan: do you agree this should work?

[1] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=1814
[2] https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?l=406
[3] https://cs.chromium.org/chromium/src/ui/aura/window_tree_host.cc?l=340
[4] https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/metrics/rendering/frame_time.html?l=105
Sadrul's plan sounds good to me.
Status: Started (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 16

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

commit 62d717bc88b3518006aa615c6b6aa926ae0f232e
Author: Scott Violet <sky@chromium.org>
Date: Tue Oct 16 17:07:58 2018

aura: moves "FramePresented" trace to ui::Compositor, and provides context

The trace event macro in LayerTreeHostImpl::DidPresentCompositorFrame() moves
to ui::Compositor::DidPresentCompositorFrame(), and Compositor/WindowTreeHost
get the ability to provide an additional value that is then passed to the
macro. This value will be later used to identify where the frame came from.

BUG= 883894 
TEST=none

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I1341c39029c54228964e433de5aafa1c89099e0b
Reviewed-on: https://chromium-review.googlesource.com/c/1282039
Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600023}
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ash/host/ash_window_tree_host_platform.cc
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/aura/window_tree_host.cc
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/aura/window_tree_host.h
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/aura/window_tree_host_platform.cc
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/aura/window_tree_host_platform.h
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/compositor/compositor.cc
[modify] https://crrev.com/62d717bc88b3518006aa615c6b6aa926ae0f232e/ui/compositor/compositor.h

Cc: sky@chromium.org
Owner: chiniforooshan@chromium.org
I believe I've landed the necessary changes to chrome side. Ehsan said he would take care of updating the scripts, assigning to him.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 17

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

commit fd3370d4ae488b8bc9b67ffd01b65efdd9b4742e
Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Date: Wed Oct 17 13:50:55 2018

aura: filter out frames not submitted from browser

This CL changes the ui_frame_time and ui_percentage_smooth computations
according to crrev.com/c/1282039. crrev.com/c/1282039 indicates the source of
the compositor frame in the trace event argument which enables us to filter out
the ones that are not coming from the browser UI.

Bug:  chromium:883894 
Change-Id: I943ec1e4256696868846a8a1720a6b09c356d76b
Reviewed-on: https://chromium-review.googlesource.com/c/1284271
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>

[modify] https://crrev.com/fd3370d4ae488b8bc9b67ffd01b65efdd9b4742e/tracing/tracing/metrics/rendering/frame_time.html
[modify] https://crrev.com/fd3370d4ae488b8bc9b67ffd01b65efdd9b4742e/tracing/tracing/metrics/rendering/frame_time_test.html

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 17

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

commit d78223532dd4dc471195262fdcdcca227861f1b9
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Wed Oct 17 20:07:40 2018

Roll src/third_party/catapult b273e0cd217d..519565187c85 (4 commits)

https://chromium.googlesource.com/catapult.git/+log/b273e0cd217d..519565187c85


git log b273e0cd217d..519565187c85 --date=short --no-merges --format='%ad %ae %s'
2018-10-17 benjhayden@chromium.org Revert "Migrate trace_viewer.gypi to gni"
2018-10-17 nednguyen@google.com Add support for --repeat flag in typ
2018-10-17 chiniforooshan@chromium.org aura: filter out frames not submitted from browser
2018-10-17 benjhayden@chromium.org Migrate trace_viewer.gypi to gni


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

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:895699 ,chromium:894261, chromium:883894 , chromium:895699 
TBR=sullivan@chromium.org

Change-Id: I42ab97661d390f5ea0e56de3f6196dd4ac19a925
Reviewed-on: https://chromium-review.googlesource.com/c/1286751
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@{#600532}
[modify] https://crrev.com/d78223532dd4dc471195262fdcdcca227861f1b9/DEPS

Status: Fixed (was: Started)

Sign in to add a comment