Provide way to identify compositor frames submitted from different sources |
|||||
Issue descriptionIn 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.
,
Sep 13
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.
,
Sep 13
Issue 883314 has been merged into this issue.
,
Sep 13
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
,
Sep 13
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.
,
Oct 3
Sadrul, any update on this? We can do the work, we just need to know what to do.
,
Oct 11
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
,
Oct 12
Sadrul's plan sounds good to me.
,
Oct 15
,
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
,
Oct 16
I believe I've landed the necessary changes to chrome side. Ehsan said he would take care of updating the scripts, assigning to him.
,
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
,
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
,
Oct 24
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sadrul@chromium.org
, Sep 13