cc: Chromify rendering_stats

cc: Chromify rendering_stats


BUG=

TBR=jamesr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189428

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190140

[alexst (slow to review), 2013-03-20 18:41:25]

chromey chromey

[danakj, 2013-03-20 19:00:43]

LGTM

https://codereview.chromium.org/12780025/diff/1/cc/debug/rendering_stats.h
File cc/debug/rendering_stats.h (right):

https://codereview.chromium.org/12780025/diff/1/cc/debug/rendering_stats.h#ne...
cc/debug/rendering_stats.h:15: // FIXME: Rename these to animationFrameCount and
screenFrameCount,
This bug is WontFix, remove this comment?

https://codereview.chromium.org/12780025/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/12780025/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:510: stats->total_image_gathering_count =
rendering_stats_.total_image_gathering_count;
80 col

https://codereview.chromium.org/12780025/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/12780025/diff/1/cc/trees/layer_tree_host.cc#n...
cc/trees/layer_tree_host.cc:349:
host_impl->SavePaintTime(rendering_stats_.total_paint_time, commit_number());
80 col

[dtu, 2013-03-20 19:08:08]

https://codereview.chromium.org/12780025/diff/1/cc/debug/rendering_stats.h
File cc/debug/rendering_stats.h (right):

https://codereview.chromium.org/12780025/diff/1/cc/debug/rendering_stats.h#ne...
cc/debug/rendering_stats.h:15: // FIXME: Rename these to animationFrameCount and
screenFrameCount,
On 2013/03/20 19:00:43, danakj wrote:
> This bug is WontFix, remove this comment?

Seems like it'd be easy enough to fix now. animation_frame_count,
screen_frame_count.

[commit-bot: I haz the power, 2013-03-20 19:42:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/5002

[commit-bot: I haz the power, 2013-03-20 22:12:02]

Change committed as 189428

[tonyg, 2013-03-20 22:46:27]

The naming change broke the scrolling benchmark or the perf waterfall. Could we
please fix the naming ASAP or else roll back in the meantime. There is currently
a lot of redness on the perf waterfall and it would be good not to allow this to
continue adding to it.

Log:
http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/bui...

[jamesr, 2013-03-20 22:53:15]

https://chromiumcodereview.appspot.com/12780025/diff/5002/cc/debug/rendering_...
File cc/debug/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/12780025/diff/5002/cc/debug/rendering_...
cc/debug/rendering_stats.cc:25: enumerator->AddInt64("animation_frame_count",
animation_frame_count);
this enumerator is exposed API to JS extensions like the gpu benchmarking
extension, so this breaks benchmarking

[jamesr, 2013-03-20 22:53:45]

Please revert ASAP.  We'll have to figure out how to stage this with changes to
the gpu benchmarking harness.

[danakj, 2013-03-20 23:05:45]

On 2013/03/20 22:53:45, jamesr wrote:
> Please revert ASAP.  We'll have to figure out how to stage this with changes
to
> the gpu benchmarking harness.

Done.

[alexst (slow to review), 2013-03-21 17:45:02]

After speaking with hartmanng about the implications of these name changes, I
propose the following.

Submit the chromification of rendering stats, minus the enumerator names.

As a followup, these issues need to be addressed:
 * names in src/tools/perf/perf_tools/*_benchmark.py could be changed
accordingly.
 * but only when we have a good way to deal with bots that run older builds of
chrome and rely on the legacy names.

Do we want to block code chromification on the perf_tools changes?

[danakj, 2013-03-21 17:48:19]

I'm fine with it, we don't need to chromify stuff inside string literals.

James wdyt?

[jamesr, 2013-03-21 17:55:54]

On 2013/03/21 17:48:19, danakj wrote:
> I'm fine with it, we don't need to chromify stuff inside string literals.
> 
> James wdyt?

String literals aren't C++ identifiers so they aren't subject to the style rules
for C++ identifiers.

[commit-bot: I haz the power, 2013-03-21 18:30:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/24001

[commit-bot: I haz the power, 2013-03-21 18:30:47]

Failed to apply patch for cc/resources/tile_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/tile_manager.cc
  Hunk #1 FAILED at 505.
  Hunk #2 FAILED at 747.
  Hunk #3 FAILED at 767.
  Hunk #4 FAILED at 1004.
  Hunk #5 FAILED at 1111.
  5 out of 5 hunks FAILED -- saving rejects to file
cc/resources/tile_manager.cc.rej

Patch:       cc/resources/tile_manager.cc
Index: cc/resources/tile_manager.cc
diff --git a/cc/resources/tile_manager.cc b/cc/resources/tile_manager.cc
index
61e112b9193bbedd619130aa5949691a0485aac9..88b7c844c60923e54480d0111793d9a78ce0b738
100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -505,11 +505,12 @@ void TileManager::SetRecordRenderingStats(bool
record_rendering_stats) {
 void TileManager::GetRenderingStats(RenderingStats* stats) {
   CHECK(record_rendering_stats_);
   raster_worker_pool_->GetRenderingStats(stats);
-  stats->totalDeferredImageCacheHitCount =
-      rendering_stats_.totalDeferredImageCacheHitCount;
-  stats->totalImageGatheringCount = rendering_stats_.totalImageGatheringCount;
-  stats->totalImageGatheringTime =
-      rendering_stats_.totalImageGatheringTime;
+  stats->total_deferred_image_cache_hit_count =
+      rendering_stats_.total_deferred_image_cache_hit_count;
+  stats->total_image_gathering_count =
+      rendering_stats_.total_image_gathering_count;
+  stats->total_image_gathering_time =
+      rendering_stats_.total_image_gathering_time;
 }
 
 bool TileManager::HasPendingWorkScheduled(WhichTree tree) const {
@@ -747,8 +748,8 @@ void TileManager::GatherPixelRefsForTile(Tile* tile) {
         managed_tile_state.pending_pixel_refs);
     managed_tile_state.need_to_gather_pixel_refs = false;
     if (record_rendering_stats_) {
-      rendering_stats_.totalImageGatheringCount++;
-      rendering_stats_.totalImageGatheringTime +=
+      rendering_stats_.total_image_gathering_count++;
+      rendering_stats_.total_image_gathering_time +=
           base::TimeTicks::HighResNow() - gather_begin_time;
     }
   }
@@ -767,7 +768,7 @@ void TileManager::DispatchImageDecodeTasksForTile(Tile*
tile) {
     }
     // TODO(qinmin): passing correct image size to PrepareToDecode().
     if ((*it)->PrepareToDecode(skia::LazyPixelRef::PrepareParams())) {
-      rendering_stats_.totalDeferredImageCacheHitCount++;
+      rendering_stats_.total_deferred_image_cache_hit_count++;
       pending_pixel_refs.erase(it++);
     } else {
       if (pending_tasks_ >= max_pending_tasks_)
@@ -1004,13 +1005,13 @@ void TileManager::RunRasterTask(uint8* buffer,
                        &total_pixels_rasterized);
 
   if (stats) {
-    stats->totalPixelsRasterized += total_pixels_rasterized;
+    stats->total_pixels_rasterized += total_pixels_rasterized;
 
     base::TimeTicks end_time = base::TimeTicks::HighResNow();
     base::TimeDelta duration = end_time - begin_time;
-    stats->totalRasterizeTime += duration;
+    stats->total_rasterize_time += duration;
     if (metadata.is_tile_in_pending_tree_now_bin)
-      stats->totalRasterizeTimeForNowBinsOnPendingTree += duration;
+      stats->total_rasterize_time_for_now_bins_on_pending_tree += duration;
 
     UMA_HISTOGRAM_CUSTOM_COUNTS("Renderer4.PictureRasterTimeMS",
                                 duration.InMilliseconds(),
@@ -1111,8 +1112,8 @@ void TileManager::RunImageDecodeTask(skia::LazyPixelRef*
pixel_ref,
     decode_begin_time = base::TimeTicks::HighResNow();
   pixel_ref->Decode();
   if (stats) {
-    stats->totalDeferredImageDecodeCount++;
-    stats->totalDeferredImageDecodeTime +=
+    stats->total_deferred_image_decode_count++;
+    stats->total_deferred_image_decode_time +=
         base::TimeTicks::HighResNow() - decode_begin_time;
   }
 }

[danakj, 2013-03-21 20:09:56]

rebase lgtm

[commit-bot: I haz the power, 2013-03-21 20:11:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/33001

[commit-bot: I haz the power, 2013-03-22 14:15:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/33001

[commit-bot: I haz the power, 2013-03-22 18:41:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/33001

[commit-bot: I haz the power, 2013-03-23 15:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12780025/33001

[commit-bot: I haz the power, 2013-03-24 00:39:03]

Change committed as 190140