Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2385253002:
Split up software and gpu raster time UMA stats (Closed)
|
Created:
4 years, 2 months ago by enne (OOO) Modified:
4 years, 2 months ago CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit up software and gpu raster time UMA stats
R=vmpstr@chromium.org
BUG=646032
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/d0a00e25974cfa4ce3a431e31e43068bc3b12ff4
Cr-Commit-Position: refs/heads/master@{#422619}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #Patch Set 3 : Now with fancypants optional #
Total comments: 3
Patch Set 4 : Using histogram suffixes #
Total comments: 1
Messages
Total messages: 23 (13 generated)
Description was changed from ========== Split up software and gpu raster time UMA stats R=vmpstr@chromium.org BUG=646032 ========== to ========== Split up software and gpu raster time UMA stats R=vmpstr@chromium.org BUG=646032 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
https://codereview.chromium.org/2385253002/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2385253002/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:104: if (is_gpu_rasterization_) { Scoped timers, what are you going to do. Certainly not heap allocate them, that's for sure.
lgtm https://codereview.chromium.org/2385253002/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2385253002/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:44: "Compositing.%s.RasterTask.RasterUs", This is also being monitored by some system, can you note that in the TODO that we need to adjust the monitor as well? https://codereview.chromium.org/2385253002/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:104: if (is_gpu_rasterization_) { On 2016/10/03 17:11:36, enne wrote: > Scoped timers, what are you going to do. Certainly not heap allocate them, > that's for sure. Maybe something like base::Optional<ScopedGpuRasterTaskTimer> gpu_timer; base::Optional<ScopedSoftwareRasterTaskTimer> software_timer; ScopedUMAHistogramTimerBase* raster_timer = nullptr; if (is_gpu_rasterization_) { gpu_timer.emplace(); raster_timer = &*gpu_timer; } else { software_timer.emplace(); raster_timer = &*software_timer; } timer.SetArea(area); raster_timer->SetArea(area); raster_buffer_->Playback(...); Is this cleaner? Not really. :D
enne@chromium.org changed reviewers: + chrishtr@chromium.org, isherman@chromium.org
vmpstr: take another look? I made it fancier chrishtr: FYI for paint-dev raster metrics (just splitting this up) isherman: tools/metrics/histograms.xml owners
On 2016/10/03 at 18:30:11, enne wrote: > vmpstr: take another look? I made it fancier > chrishtr: FYI for paint-dev raster metrics (just splitting this up) > isherman: tools/metrics/histograms.xml owners ack. Thanks for the note.
lgtm thanks!
The CQ bit was checked by enne@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.
LGTM % nits: https://codereview.chromium.org/2385253002/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2385253002/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:42: // data from the other two raster task timers. nit: Mebbe provide a rough milestone target? https://codereview.chromium.org/2385253002/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:51: "Compositing.%s.RasterTask.SoftwareRasterPixelsPerMs"); Optional nit: You might want to name these as "RasterUs.Software", etc., so that "RasterUs.Software" and "RasterUs.Gpu" end up sorted next to each other. Also, if you don't like this suggestion, you might still want to separate "Software" and "RasterUs" with a dot. https://codereview.chromium.org/2385253002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2385253002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6725: +</histogram> One advantage of making ".Software" and ".Gpu" be the last bits of the histogram names is that you can use a histogram_suffixes element to reduce the amount of repetition in this file.
Thanks for the suggestion to use suffixes. That's much cleaner! https://codereview.chromium.org/2385253002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2385253002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:107583: + <affected-histogram name="Compositing.Browser.RasterTask.RasterPixelsPerMs"/> The pretty printer really wanted to put this one first. <_<
The CQ bit was checked by enne@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.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2385253002/#ps60001 (title: "Using histogram suffixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Split up software and gpu raster time UMA stats R=vmpstr@chromium.org BUG=646032 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Split up software and gpu raster time UMA stats R=vmpstr@chromium.org BUG=646032 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/d0a00e25974cfa4ce3a431e31e43068bc3b12ff4 Cr-Commit-Position: refs/heads/master@{#422619} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d0a00e25974cfa4ce3a431e31e43068bc3b12ff4 Cr-Commit-Position: refs/heads/master@{#422619} |
