cc:: Add RenderingStatsInstrumentation to manage collection of RenderingStats

cc:: Add RenderingStatsInstrumentation to manage collection of RenderingStats

This change adds the class RenderingStatsInstrumentation that manages conditional
saving and thread-specific access to a private RenderingStats instance.
An instance of RenderingStatsRecorder is created on LayerTreeHost, which
passes references to LayerTreeHostImpl and TileManager. Access to reading
and writing on the internal RenderingStats instance is guarded by a lock.

All rendering stats saving in LayerTreeHost, Single-/ThreadProxy,
LayerTreeHostImpl and TileManager has been switched to use the
RenderingStatsInstrumentation. Stats collection within Layer::update() still
follows the original structure to keep this change small.

BUG=181319
NOTRY=true

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

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

[egraether, 2013-03-09 00:40:54]

This is a first draft and I'm not even sure if this works safely as I have
little experience with concurrency (which was a main reason for me to do this).

Can I share a pointer between threads this way or is splitting into
RenderingStatsRecorder/Impl and syncing them with a blocking call necessary?

[egraether, 2013-03-12 19:31:42]

Dana, can you give this a review please?

I discussed this patch with Nat in an 1:1. He agreed on the approach of having
the same instance of RenderingStatsRecorder shared among threads with locked
read/write access to a RenderingStats object. He mentioned that in-lining
methods by putting them into the header could speed up performance. We did not
discuss the implementation of the class in detail.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:267: LayerTreeHostImpl(const LayerTreeSettings&,
LayerTreeHostImplClient*, Proxy*, RenderingStatsRecorder*);
Passing to the constructor needs more changes in the tests. I already updated
the tests to show how this looks. Alternatively the recorder could also be
accessed via LayerTreeHostImplClient or Proxy, but I don't think that's cleaner.

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorder.h
File cc/rendering_stats_recorder.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.h:16: class CC_EXPORT RenderingStatsRecorder {
Nat suggested to name the class RenderingStatsInstrumentation to follow the
naming of the newly added devtools_instrumentation. I prefer recorder, but I'm
fine to change it (I only want to change it once though).

[nduca, 2013-03-13 05:53:59]

lgtm at a high level. Dana, wdyt?

[danakj, 2013-03-13 17:25:41]

I like it. You'll need to rebase and chrome-stylify your changes in LTH (and
LTHI if you rebase on top of https://codereview.chromium.org/12648005/ which is
going into the CQ now).

You need to move the autolocks in the recorder class to also protect the flag,
since the variable can be set on one thread and read on another. Not locking
providers non-deterministic behaviour.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:425: RenderingStatsRecorder*
LayerTreeHost::renderingStatsRecorder() const
rename rendering_stats_recorder()

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:427: return m_renderingStatsRecorder.get();
move this function to the header

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:744: // TODO: Switch Layer::update() to use
RenderingStatsRecorder
stick you ldap in brackets after the TODO - ie TODO(egraether):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:27: #include "cc/rendering_stats_recorder.h"
forward declare the class here, include the .h from layer_tree_host.cc

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:513: layersDrawn++;
++layersDrawn is best practice

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:808:
m_renderingStatsRecorder->SetScreenAndDroppedFrameCount(m_fpsCounter->currentFrameNumber(),
m_fpsCounter->droppedFrameCount());
"SetScreen" -> "SetFrameNumber" ? What's "Screen" in this context?

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1078: if (m_settings.implSidePainting)
keep the {}

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
File cc/rendering_stats_recorder.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:10: scoped_ptr<RenderingStatsRecorder>
RenderingStatsRecorder::create() {
Create()

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:24: return record_rendering_stats_;
why no lock?

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:29: record_rendering_stats_ =
record_rendering_stats;
autolock here

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:33: if (record_rendering_stats_)
autolock here

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:46: if (!record_rendering_stats_)
move the lock to the top of these functions, so you lock access to the flag too.

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorder.h
File cc/rendering_stats_recorder.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.h:16: class CC_EXPORT RenderingStatsRecorder {
On 2013/03/12 19:31:42, egraether wrote:
> Nat suggested to name the class RenderingStatsInstrumentation to follow the
> naming of the newly added devtools_instrumentation. I prefer recorder, but I'm
> fine to change it (I only want to change it once though).

I'm okay with either, so I think go with Nat's preference.

[egraether, 2013-03-13 19:13:05]

> You need to move the autolocks in the recorder class to also protect the flag,
> since the variable can be set on one thread and read on another. Not locking
> provides non-deterministic behaviour.

I was not sure if the behavior is also non-deterministic when the value is a
boolean, so I read through some discussions online and there is also a
developers
reference on chromium:
http://www.chromium.org/developers/lock-and-condition-variable

I learned that the read and write operations should be atomic (no guarantee),
but
threads can cache values and miss a change. There is a few ways of avoiding
this:
- volatile: value is reloaded from memory every time used
- atomic types: make sure that operations on types are synched
- lock: prevents access to a value at the same time and also handles synching

The general agreement is that using a lock is the best approach, because it's
cheaper
than most developers think, guarantees that the value is updated everywhere and
keeps
the code maintainable.

So I will change to using the lock for the record_rendering_stats_ flag, but I'm
still
concerned that this is not really optimal. The flag is turned off 99.9% of times
when
Chrome is used and there are quite a lot of calls to the
rendering_stats_recorder,
especially spread across threads in impl-side painting. At the moment the only
time
where the flag changes during run-time is when using continuous painting, which
is also
hardly ever the case. So I'm just wondering if we can architect this any better.

For example we could have a second implementation that has no functionality, and
when
turning the flag on we switch them. This would probably involve virtual
inheritance
though, and the switching would need to be thread safe too. The switching could
be a
blocking call. I don't know.

Any better ideas or are my concerns pointless?

[egraether, 2013-03-13 19:19:27]

I also learned to not mess with the formating. Again:

> You need to move the autolocks in the recorder class to also protect the flag,
> since the variable can be set on one thread and read on another. Not locking
> providers non-deterministic behaviour.

I was not sure if the behavior is also non-deterministic when the value is a
boolean, so I read through some discussions online and there is also a
developers reference on chromium:
http://www.chromium.org/developers/lock-and-condition-variable

I learned that the read and write operations should be atomic (no guarantee),
but threads can cache values and miss a change. There is a few ways of avoiding
this:
- volatile: value is reloaded from memory every time used
- atomic types: make sure that operations on types are synched
- lock: prevents access to a value at the same time and also handles synching

The general agreement is that using a lock is the best approach, because it's
cheaper than most developers think, guarantees that the value is updated
everywhere and keeps the code maintainable.

So I will change to using the lock for the record_rendering_stats_ flag, but I'm
still concerned that this is not really optimal. The flag is turned off 99.9% of
times when Chrome is used and there are quite a lot of calls to the
rendering_stats_recorder, especially spread across threads in impl-side
painting. At the moment the only time where the flag changes during run-time is
when using continuous painting, which is also hardly ever the case. So I'm just
wondering if we can architect this any better.

For example we could have a second implementation that has no functionality, and
when turning the flag on we switch them. This would probably involve virtual
inheritance though, and the switching would need to be thread safe too. The
switching could be a blocking call. I don't know.

Any better ideas or are my concerns pointless?

[danakj, 2013-03-13 21:08:28]

On 2013/03/13 19:19:27, egraether wrote:
> I also learned to not mess with the formating. Again:
> 
> > You need to move the autolocks in the recorder class to also protect the
flag,
> > since the variable can be set on one thread and read on another. Not locking
> > providers non-deterministic behaviour.
> 
> I was not sure if the behavior is also non-deterministic when the value is a
> boolean, so I read through some discussions online and there is also a
> developers reference on chromium:
> http://www.chromium.org/developers/lock-and-condition-variable
> 
> I learned that the read and write operations should be atomic (no guarantee),
> but threads can cache values and miss a change. There is a few ways of
avoiding
> this:
> - volatile: value is reloaded from memory every time used
> - atomic types: make sure that operations on types are synched
> - lock: prevents access to a value at the same time and also handles synching
> 
> The general agreement is that using a lock is the best approach, because it's
> cheaper than most developers think, guarantees that the value is updated
> everywhere and keeps the code maintainable.
> 
> So I will change to using the lock for the record_rendering_stats_ flag, but
I'm
> still concerned that this is not really optimal. The flag is turned off 99.9%
of
> times when Chrome is used and there are quite a lot of calls to the
> rendering_stats_recorder, especially spread across threads in impl-side
> painting. At the moment the only time where the flag changes during run-time
is
> when using continuous painting, which is also hardly ever the case. So I'm
just
> wondering if we can architect this any better.
> 
> For example we could have a second implementation that has no functionality,
and
> when turning the flag on we switch them. This would probably involve virtual
> inheritance though, and the switching would need to be thread safe too. The
> switching could be a blocking call. I don't know.
> 
> Any better ideas or are my concerns pointless?

I think if you can run it in a profiler (valgrind's callgrind tool) and see the
result as being costly (with a viewer like kcachegrind) then you'd have data to
support doing something more tricky than a lock. I suspect the overhead of
things like base::Bind callbacks will be as much or more than the lock so it is
probably fine, but I don't have data to say that.

Generally I think we do things simple and come back later to optimize based on
data that it's slow. So I'd keep it simple unless you want to collect that data
now.

[nduca, 2013-03-13 21:16:47]

The  bool can and should be outside the lock.

[nduca, 2013-03-13 21:17:31]

You may want to use base::tricky to make it correct though... look at
base/debug/trace_event_impl and look where we assign 1 to the category enabled
pointers. We should use something like barrier write or something like that.

[egraether, 2013-03-14 18:57:40]

On 2013/03/13 21:17:31, nduca wrote:
> You may want to use base::tricky to make it correct though... look at
> base/debug/trace_event_impl and look where we assign 1 to the category enabled
> pointers. We should use something like barrier write or something like that.

Nat and me discussed this again yesterday. We also looked into how this is
handled in tracing, which also does the check for recording without locking.

According to Nat, we should not run into the theoretical problem that a flag
change doesn't propagate to other threads. And even if the other threads don't
get the update immediately, it's not crucial for this functionality.

[egraether, 2013-03-14 18:58:07]

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:425: RenderingStatsRecorder*
LayerTreeHost::renderingStatsRecorder() const
On 2013/03/13 17:25:41, danakj wrote:
> rename rendering_stats_recorder()

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:427: return m_renderingStatsRecorder.get();
On 2013/03/13 17:25:41, danakj wrote:
> move this function to the header

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:744: // TODO: Switch Layer::update() to use
RenderingStatsRecorder
On 2013/03/13 17:25:41, danakj wrote:
> stick you ldap in brackets after the TODO - ie TODO(egraether):

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:27: #include "cc/rendering_stats_recorder.h"
On 2013/03/13 17:25:41, danakj wrote:
> forward declare the class here, include the .h from layer_tree_host.cc

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:513: layersDrawn++;
On 2013/03/13 17:25:41, danakj wrote:
> ++layersDrawn is best practice

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:808:
m_renderingStatsRecorder->SetScreenAndDroppedFrameCount(m_fpsCounter->currentFrameNumber(),
m_fpsCounter->droppedFrameCount());
On 2013/03/13 17:25:41, danakj wrote:
> "SetScreen" -> "SetFrameNumber" ? What's "Screen" in this context?

I split this weird double call into separate calls.

There's an open bug to rename the members numAnimationFrame and
numFramesSentToScreen of RenderingStats to animationFrameCount and
screenFrameCount (crbug.com/138641). So I used the better naming in the new
instrumentation class.

https://codereview.chromium.org/12519006/diff/6001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1078: if (m_settings.implSidePainting)
On 2013/03/13 17:25:41, danakj wrote:
> keep the {}

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
File cc/rendering_stats_recorder.cc (right):

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.cc:10: scoped_ptr<RenderingStatsRecorder>
RenderingStatsRecorder::create() {
On 2013/03/13 17:25:41, danakj wrote:
> Create()

Done.

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorder.h
File cc/rendering_stats_recorder.h (right):

https://codereview.chromium.org/12519006/diff/6001/cc/rendering_stats_recorde...
cc/rendering_stats_recorder.h:16: class CC_EXPORT RenderingStatsRecorder {
On 2013/03/13 17:25:41, danakj wrote:
> On 2013/03/12 19:31:42, egraether wrote:
> > Nat suggested to name the class RenderingStatsInstrumentation to follow the
> > naming of the newly added devtools_instrumentation. I prefer recorder, but
I'm
> > fine to change it (I only want to change it once though).
> 
> I'm okay with either, so I think go with Nat's preference.

Done.

[danakj, 2013-03-14 19:16:23]

On 2013/03/14 18:57:40, egraether wrote:
> On 2013/03/13 21:17:31, nduca wrote:
> > You may want to use base::tricky to make it correct though... look at
> > base/debug/trace_event_impl and look where we assign 1 to the category
enabled
> > pointers. We should use something like barrier write or something like that.
> 
> Nat and me discussed this again yesterday. We also looked into how this is
> handled in tracing, which also does the check for recording without locking.
> 
> According to Nat, we should not run into the theoretical problem that a flag
> change doesn't propagate to other threads. And even if the other threads don't
> get the update immediately, it's not crucial for this functionality.

OK

[danakj, 2013-03-15 17:28:48]

LGTM this is a vast improvement, thanks for it!

https://codereview.chromium.org/12519006/diff/36001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12519006/diff/36001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:495:
rendering_stats_instrumentation_->set_record_rendering_stats(
The general pattern is to change states, then call setNeedsCommit() last.

[egraether, 2013-03-15 21:14:26]

James, can you have a look at RenderWidget please?

https://codereview.chromium.org/12519006/diff/36001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12519006/diff/36001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:495:
rendering_stats_instrumentation_->set_record_rendering_stats(
On 2013/03/15 17:28:48, danakj wrote:
> The general pattern is to change states, then call setNeedsCommit() last.

Done.

[jamesr, 2013-03-18 01:04:37]

lgtm

[commit-bot: I haz the power, 2013-03-18 17:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/52001

[commit-bot: I haz the power, 2013-03-18 19:53:04]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-03-18 20:56:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/10031

[commit-bot: I haz the power, 2013-03-18 23:13:18]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-19 15:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/66001

[commit-bot: I haz the power, 2013-03-19 21:40:55]

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 #2 FAILED at 162.
  Hunk #3 succeeded at 495 (offset -6 lines).
  Hunk #4 succeeded at 721 (offset -9 lines).
  Hunk #5 succeeded at 747 (offset -9 lines).
  Hunk #6 succeeded at 767 (offset -9 lines).
  Hunk #7 succeeded at 844 (offset 2 lines).
  Hunk #8 succeeded at 955 (offset -4 lines).
  Hunk #9 succeeded at 979 (offset -4 lines).
  Hunk #10 succeeded at 1083 (offset -4 lines).
  1 out of 10 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
153a240f1b703026f217145205902de16e59cece..1f457348b2ab116abc866d6c5a5b08cb4292f4c1
100644
--- a/cc/resources/tile_manager.cc
+++ b/cc/resources/tile_manager.cc
@@ -152,7 +152,8 @@ TileManager::TileManager(
     size_t num_raster_threads,
     bool use_cheapness_estimator,
     bool use_color_estimator,
-    bool prediction_benchmarking)
+    bool prediction_benchmarking,
+    RenderingStatsInstrumentation* rendering_stats_instrumentation)
     : client_(client),
       resource_pool_(ResourcePool::Create(resource_provider)),
       raster_worker_pool_(RasterWorkerPool::Create(this, num_raster_threads)),
@@ -161,14 +162,14 @@ TileManager::TileManager(
       bytes_pending_upload_(0),
       has_performed_uploads_since_last_flush_(false),
       ever_exceeded_memory_budget_(false),
-      record_rendering_stats_(false),
       use_cheapness_estimator_(use_cheapness_estimator),
       use_color_estimator_(use_color_estimator),
       did_schedule_cheap_tasks_(false),
       allow_cheap_tasks_(true),
       prediction_benchmarking_(prediction_benchmarking),
       pending_tasks_(0),
-      max_pending_tasks_(kMaxNumPendingTasksPerThread * num_raster_threads) {
+      max_pending_tasks_(kMaxNumPendingTasksPerThread * num_raster_threads),
+      rendering_stats_instrumentation_(rendering_stats_instrumentation) {
   for (int i = 0; i < NUM_STATES; ++i) {
     for (int j = 0; j < NUM_TREES; ++j) {
       for (int k = 0; k < NUM_BINS; ++k)
@@ -500,24 +501,6 @@ scoped_ptr<base::Value>
TileManager::GetMemoryRequirementsAsValue() const {
   return requirements.PassAs<base::Value>();
 }
 
-void TileManager::SetRecordRenderingStats(bool record_rendering_stats) {
-  if (record_rendering_stats_ == record_rendering_stats)
-    return;
-
-  record_rendering_stats_ = record_rendering_stats;
-  raster_worker_pool_->SetRecordRenderingStats(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;
-}
-
 bool TileManager::HasPendingWorkScheduled(WhichTree tree) const {
   // Always true when ManageTiles() call is pending.
   if (manage_tiles_pending_)
@@ -747,19 +730,16 @@ void TileManager::GatherPixelRefsForTile(Tile* tile) {
   TRACE_EVENT0("cc", "TileManager::GatherPixelRefsForTile");
   ManagedTileState& managed_tile_state = tile->managed_state();
   if (managed_tile_state.need_to_gather_pixel_refs) {
-    base::TimeTicks gather_begin_time;
-    if (record_rendering_stats_)
-      gather_begin_time = base::TimeTicks::HighResNow();
+    base::TimeTicks start_time =
+        rendering_stats_instrumentation_->StartRecording();
     tile->picture_pile()->GatherPixelRefs(
         tile->content_rect_,
         tile->contents_scale_,
         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 +=
-          base::TimeTicks::HighResNow() - gather_begin_time;
-    }
+    base::TimeDelta duration =
+        rendering_stats_instrumentation_->EndRecording(start_time);
+    rendering_stats_instrumentation_->AddImageGathering(duration);
   }
 }
 
@@ -776,7 +756,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_instrumentation_->IncrementDeferredImageCacheHitCount();
       pending_pixel_refs.erase(it++);
     } else {
       if (pending_tasks_ >= max_pending_tasks_)
@@ -796,7 +776,9 @@ void TileManager::DispatchOneImageDecodeTask(
   pending_decode_tasks_[pixel_ref_id] = pixel_ref;
 
   raster_worker_pool_->PostTaskAndReply(
-      base::Bind(&TileManager::RunImageDecodeTask, pixel_ref),
+      base::Bind(&TileManager::RunImageDecodeTask,
+                 pixel_ref,
+                 rendering_stats_instrumentation_),
       base::Bind(&TileManager::OnImageDecodeTaskCompleted,
                  base::Unretained(this),
                  tile,
@@ -860,7 +842,8 @@ void TileManager::DispatchOneRasterTask(scoped_refptr<Tile>
tile) {
                  buffer,
                  tile->content_rect(),
                  tile->contents_scale(),
-                 GetRasterTaskMetadata(*tile)),
+                 GetRasterTaskMetadata(*tile),
+                 rendering_stats_instrumentation_),
       base::Bind(&TileManager::OnRasterTaskCompleted,
                  base::Unretained(this),
                  tile,
@@ -976,12 +959,13 @@ void TileManager::DidTileTreeBinChange(Tile* tile,
 }
 
 // static
-void TileManager::RunRasterTask(uint8* buffer,
-                                const gfx::Rect& rect,
-                                float contents_scale,
-                                const RasterTaskMetadata& metadata,
-                                PicturePileImpl* picture_pile,
-                                RenderingStats* stats) {
+void TileManager::RunRasterTask(
+    uint8* buffer,
+    const gfx::Rect& rect,
+    float contents_scale,
+    const RasterTaskMetadata& metadata,
+    RenderingStatsInstrumentation* stats_instrumentation,
+    PicturePileImpl* picture_pile) {
   TRACE_EVENT2(
       "cc", "TileManager::RunRasterTask",
       "is_on_pending_tree",
@@ -999,22 +983,18 @@ void TileManager::RunRasterTask(uint8* buffer,
   SkDevice device(bitmap);
   SkCanvas canvas(&device);
 
-  base::TimeTicks begin_time;
-  if (stats)
-    begin_time = base::TimeTicks::HighResNow();
+  base::TimeTicks start_time = stats_instrumentation->StartRecording();
 
   int64 total_pixels_rasterized = 0;
   picture_pile->Raster(&canvas, rect, contents_scale,
                        &total_pixels_rasterized);
 
-  if (stats) {
-    stats->totalPixelsRasterized += total_pixels_rasterized;
+  base::TimeDelta duration = stats_instrumentation->EndRecording(start_time);
 
-    base::TimeTicks end_time = base::TimeTicks::HighResNow();
-    base::TimeDelta duration = end_time - begin_time;
-    stats->totalRasterizeTime += duration;
-    if (metadata.is_tile_in_pending_tree_now_bin)
-      stats->totalRasterizeTimeForNowBinsOnPendingTree += duration;
+  if (stats_instrumentation->record_rendering_stats()) {
+    stats_instrumentation->AddRaster(duration,
+                                     total_pixels_rasterized,
+                                     metadata.is_tile_in_pending_tree_now_bin);
 
     UMA_HISTOGRAM_CUSTOM_COUNTS("Renderer4.PictureRasterTimeMS",
                                 duration.InMilliseconds(),
@@ -1107,18 +1087,14 @@ void TileManager::RecordSolidColorPredictorResults(
 }
 
 // static
-void TileManager::RunImageDecodeTask(skia::LazyPixelRef* pixel_ref,
-                                     RenderingStats* stats) {
+void TileManager::RunImageDecodeTask(
+    skia::LazyPixelRef* pixel_ref,
+    RenderingStatsInstrumentation* stats_instrumentation) {
   TRACE_EVENT0("cc", "TileManager::RunImageDecodeTask");
-  base::TimeTicks decode_begin_time;
-  if (stats)
-    decode_begin_time = base::TimeTicks::HighResNow();
+  base::TimeTicks start_time = stats_instrumentation->StartRecording();
   pixel_ref->Decode();
-  if (stats) {
-    stats->totalDeferredImageDecodeCount++;
-    stats->totalDeferredImageDecodeTime +=
-        base::TimeTicks::HighResNow() - decode_begin_time;
-  }
+  base::TimeDelta duration = stats_instrumentation->EndRecording(start_time);
+  stats_instrumentation->AddDeferredImageDecode(duration);
 }
 
 }  // namespace cc

[commit-bot: I haz the power, 2013-03-20 01:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/84001

[commit-bot: I haz the power, 2013-03-20 02:30:26]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-20 02:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/84001

[commit-bot: I haz the power, 2013-03-20 02:40:53]

Failed to apply patch for cc/trees/layer_tree_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host_impl.cc
  Hunk #3 FAILED at 173.
  1 out of 14 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host_impl.cc.rej

Patch:       cc/trees/layer_tree_host_impl.cc
Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index
6d1f6e98078f0f3f5c82a1f88df3aa4901927288..355529aa791fced3aa18afc54a221ac2bb844f77
100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -19,7 +19,7 @@
 #include "cc/debug/frame_rate_counter.h"
 #include "cc/debug/overdraw_metrics.h"
 #include "cc/debug/paint_time_counter.h"
-#include "cc/debug/rendering_stats.h"
+#include "cc/debug/rendering_stats_instrumentation.h"
 #include "cc/input/page_scale_animation.h"
 #include "cc/input/top_controls_manager.h"
 #include "cc/layers/append_quads_data.h"
@@ -141,13 +141,20 @@ LayerTreeHostImpl::FrameData::~FrameData() {}
 scoped_ptr<LayerTreeHostImpl> LayerTreeHostImpl::Create(
     const LayerTreeSettings& settings,
     LayerTreeHostImplClient* client,
-    Proxy* proxy) {
-  return make_scoped_ptr(new LayerTreeHostImpl(settings, client, proxy));
+    Proxy* proxy,
+    RenderingStatsInstrumentation* rendering_stats_instrumentation) {
+  return make_scoped_ptr(
+      new LayerTreeHostImpl(settings,
+                            client,
+                            proxy,
+                            rendering_stats_instrumentation));
 }
 
-LayerTreeHostImpl::LayerTreeHostImpl(const LayerTreeSettings& settings,
-                                     LayerTreeHostImplClient* client,
-                                     Proxy* proxy)
+LayerTreeHostImpl::LayerTreeHostImpl(
+    const LayerTreeSettings& settings,
+    LayerTreeHostImplClient* client,
+    Proxy* proxy,
+    RenderingStatsInstrumentation* rendering_stats_instrumentation)
     : client_(client),
       proxy_(proxy),
       did_lock_scrolling_layer_(false),
@@ -166,14 +173,11 @@ LayerTreeHostImpl::LayerTreeHostImpl(const
LayerTreeSettings& settings,
       paint_time_counter_(PaintTimeCounter::Create()),
       memory_history_(MemoryHistory::Create()),
       debug_rect_history_(DebugRectHistory::Create()),
-      num_impl_thread_scrolls_(0),
-      num_main_thread_scrolls_(0),
-      cumulative_num_layers_drawn_(0),
-      cumulative_num_missing_tiles_(0),
       last_sent_memory_visible_bytes_(0),
       last_sent_memory_visible_and_nearby_bytes_(0),
       last_sent_memory_use_bytes_(0),
-      animation_registrar_(AnimationRegistrar::create()) {
+      animation_registrar_(AnimationRegistrar::create()),
+      rendering_stats_instrumentation_(rendering_stats_instrumentation) {
   DCHECK(proxy_->IsImplThread());
   DidVisibilityChange(this, visible_);
 
@@ -541,6 +545,8 @@ bool LayerTreeHostImpl::CalculateRenderPasses(FrameData*
frame) {
   // texture suddenly appearing in the future.
   bool draw_frame = true;
 
+  int layers_drawn = 0;
+
   LayerIteratorType end =
       LayerIteratorType::end(frame->render_surface_layer_list);
   for (LayerIteratorType it =
@@ -609,14 +615,15 @@ bool LayerTreeHostImpl::CalculateRenderPasses(FrameData*
frame) {
                             &append_quads_data);
       }
 
-      ++cumulative_num_layers_drawn_;
+      ++layers_drawn;
     }
 
     if (append_quads_data.hadOcclusionFromOutsideTargetSurface)
       target_render_pass->has_occlusion_from_outside_target_surface = true;
 
     if (append_quads_data.numMissingTiles) {
-      cumulative_num_missing_tiles_ += append_quads_data.numMissingTiles;
+      rendering_stats_instrumentation_->AddMissingTiles(
+          append_quads_data.numMissingTiles);
       bool layer_has_animating_transform =
           it->screen_space_transform_is_animating() ||
           it->draw_transform_is_animating();
@@ -630,6 +637,8 @@ bool LayerTreeHostImpl::CalculateRenderPasses(FrameData*
frame) {
     occlusion_tracker.LeaveLayer(it);
   }
 
+  rendering_stats_instrumentation_->AddLayersDrawn(layers_drawn);
+
 #ifndef NDEBUG
   for (size_t i = 0; i < frame->render_passes.size(); ++i) {
     for (size_t j = 0; j < frame->render_passes[i]->quad_list.size(); ++j)
@@ -945,6 +954,11 @@ void LayerTreeHostImpl::DrawLayers(FrameData* frame,
 
   fps_counter_->SaveTimeStamp(frame_begin_time);
 
+  rendering_stats_instrumentation_->SetScreenFrameCount(
+      fps_counter_->current_frame_number());
+  rendering_stats_instrumentation_->SetDroppedFrameCount(
+      fps_counter_->dropped_frame_count());
+
   if (tile_manager_) {
     memory_history_->SaveEntry(
         tile_manager_->memory_stats_from_last_assign());
@@ -1175,8 +1189,8 @@ void LayerTreeHostImpl::ActivatePendingTree() {
   client_->RenewTreePriority();
 
   if (tile_manager_ && debug_state_.continuous_painting) {
-    RenderingStats stats;
-    tile_manager_->GetRenderingStats(&stats);
+    RenderingStats stats =
+        rendering_stats_instrumentation_->GetRenderingStats();
     paint_time_counter_->SaveRasterizeTime(
         stats.totalRasterizeTimeForNowBinsOnPendingTree,
         active_tree_->source_frame_number());
@@ -1236,8 +1250,8 @@ bool LayerTreeHostImpl::InitializeRenderer(
                                         settings_.numRasterThreads,
                                         settings_.useCheapnessEstimator,
                                         settings_.useColorEstimator,
-                                        settings_.predictionBenchmarking));
-   
tile_manager_->SetRecordRenderingStats(debug_state_.RecordRenderingStats());
+                                        settings_.predictionBenchmarking,
+                                        rendering_stats_instrumentation_));
   }
 
   if (output_surface->capabilities().has_parent_compositor) {
@@ -1362,7 +1376,7 @@ InputHandlerClient::ScrollStatus
LayerTreeHostImpl::ScrollBegin(
     // thread.
     ScrollStatus status = layer_impl->TryScroll(device_viewport_point, type);
     if (status == ScrollOnMainThread) {
-      num_main_thread_scrolls_++;
+      rendering_stats_instrumentation_->IncrementMainThreadScrolls();
       UMA_HISTOGRAM_BOOLEAN("TryScroll.SlowScroll", true);
       active_tree()->DidBeginScroll();
       return ScrollOnMainThread;
@@ -1376,7 +1390,7 @@ InputHandlerClient::ScrollStatus
LayerTreeHostImpl::ScrollBegin(
 
     // If any layer wants to divert the scroll event to the main thread, abort.
     if (status == ScrollOnMainThread) {
-      num_main_thread_scrolls_++;
+      rendering_stats_instrumentation_->IncrementMainThreadScrolls();
       UMA_HISTOGRAM_BOOLEAN("TryScroll.SlowScroll", true);
       active_tree()->DidBeginScroll();
       return ScrollOnMainThread;
@@ -1400,7 +1414,7 @@ InputHandlerClient::ScrollStatus
LayerTreeHostImpl::ScrollBegin(
         potentially_scrolling_layer_impl);
     should_bubble_scrolls_ = (type != NonBubblingGesture);
     wheel_scrolling_ = (type == Wheel);
-    num_impl_thread_scrolls_++;
+    rendering_stats_instrumentation_->IncrementImplThreadScrolls();
     client_->RenewTreePriority();
     UMA_HISTOGRAM_BOOLEAN("TryScroll.SlowScroll", false);
     active_tree()->DidBeginScroll();
@@ -1796,18 +1810,6 @@ int LayerTreeHostImpl::SourceAnimationFrameNumber() const
{
   return fps_counter_->current_frame_number();
 }
 
-void LayerTreeHostImpl::CollectRenderingStats(RenderingStats* stats) const {
-  stats->numFramesSentToScreen = fps_counter_->current_frame_number();
-  stats->droppedFrameCount = fps_counter_->dropped_frame_count();
-  stats->numImplThreadScrolls = num_impl_thread_scrolls_;
-  stats->numMainThreadScrolls = num_main_thread_scrolls_;
-  stats->numLayersDrawn = cumulative_num_layers_drawn_;
-  stats->numMissingTiles = cumulative_num_missing_tiles_;
-
-  if (tile_manager_)
-    tile_manager_->GetRenderingStats(stats);
-}
-
 void LayerTreeHostImpl::SendManagedMemoryStats(
     size_t memory_visible_bytes,
     size_t memory_visible_and_nearby_bytes,
@@ -1962,9 +1964,6 @@ void LayerTreeHostImpl::SetDebugState(const
LayerTreeDebugState& debug_state) {
     paint_time_counter_->ClearHistory();
 
   debug_state_ = debug_state;
-
-  if (tile_manager_)
-   
tile_manager_->SetRecordRenderingStats(debug_state_.RecordRenderingStats());
 }
 
 void LayerTreeHostImpl::SavePaintTime(const base::TimeDelta& total_paint_time,

[commit-bot: I haz the power, 2013-03-20 16:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/93001

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

Failed to apply patch for cc/trees/layer_tree_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host.cc
  Hunk #10 succeeded at 856 with fuzz 2.
  Hunk #11 FAILED at 871.
  1 out of 11 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host.cc.rej

Patch:       cc/trees/layer_tree_host.cc
Index: cc/trees/layer_tree_host.cc
diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index
cb195db1f95a759f9ba1842d21da18068fc7ceca..cb6c92c4e787dd85b0e50fe282a8f857742d9161
100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -15,6 +15,7 @@
 #include "cc/base/math_util.h"
 #include "cc/base/thread.h"
 #include "cc/debug/overdraw_metrics.h"
+#include "cc/debug/rendering_stats_instrumentation.h"
 #include "cc/input/pinch_zoom_scrollbar.h"
 #include "cc/input/pinch_zoom_scrollbar_geometry.h"
 #include "cc/input/pinch_zoom_scrollbar_painter.h"
@@ -77,7 +78,7 @@ LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client,
       needs_filter_context_(false),
       client_(client),
       commit_number_(0),
-      rendering_stats_(),
+     
rendering_stats_instrumentation_(RenderingStatsInstrumentation::Create()),
       renderer_initialized_(false),
       output_surface_lost_(false),
       num_failed_recreate_attempts_(0),
@@ -95,6 +96,9 @@ LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client,
   if (settings_.acceleratedAnimationEnabled)
     animation_registrar_ = AnimationRegistrar::Create();
   s_num_layer_tree_instances++;
+
+  rendering_stats_instrumentation_->set_record_rendering_stats(
+      debug_state_.RecordRenderingStats());
 }
 
 bool LayerTreeHost::Initialize(scoped_ptr<Thread> impl_thread) {
@@ -230,7 +234,7 @@ void LayerTreeHost::UpdateAnimations(base::TimeTicks
frame_begin_time) {
   AnimateLayers(frame_begin_time);
   animating_ = false;
 
-  rendering_stats_.numAnimationFrames++;
+  rendering_stats_instrumentation_->IncrementAnimationFrameCount();
 }
 
 void LayerTreeHost::DidStopFlinging() {
@@ -345,8 +349,12 @@ void
LayerTreeHost::FinishCommitOnImplThread(LayerTreeHostImpl* host_impl) {
     sync_tree->DidBecomeActive();
   }
 
-  if (debug_state_.continuous_painting)
-    host_impl->SavePaintTime(rendering_stats_.totalPaintTime, commit_number());
+  if (debug_state_.continuous_painting) {
+    host_impl->SavePaintTime(
+      rendering_stats_instrumentation_->GetRenderingStats().totalPaintTime,
+      commit_number()
+    );
+  }
 
   commit_number_++;
 }
@@ -443,7 +451,10 @@ scoped_ptr<LayerTreeHostImpl>
LayerTreeHost::CreateLayerTreeHostImpl(
     LayerTreeHostImplClient* client) {
   DCHECK(proxy_->IsImplThread());
   scoped_ptr<LayerTreeHostImpl> host_impl =
-      LayerTreeHostImpl::Create(settings_, client, proxy_.get());
+      LayerTreeHostImpl::Create(settings_,
+                                client,
+                                proxy_.get(),
+                                rendering_stats_instrumentation_.get());
   if (settings_.calculateTopControlsPosition &&
       host_impl->top_controls_manager()) {
     top_controls_manager_weak_ptr_ =
@@ -482,8 +493,7 @@ void LayerTreeHost::DidDeferCommit() {}
 
 void LayerTreeHost::CollectRenderingStats(RenderingStats* stats) const {
   CHECK(debug_state_.RecordRenderingStats());
-  *stats = rendering_stats_;
-  proxy_->CollectRenderingStats(stats);
+  *stats = rendering_stats_instrumentation_->GetRenderingStats();
 }
 
 const RendererCapabilities& LayerTreeHost::GetRendererCapabilities() const {
@@ -557,6 +567,10 @@ void LayerTreeHost::SetDebugState(const
LayerTreeDebugState& debug_state) {
     return;
 
   debug_state_ = new_debug_state;
+
+  rendering_stats_instrumentation_->set_record_rendering_stats(
+      debug_state_.RecordRenderingStats());
+
   SetNeedsCommit();
 }
 
@@ -796,15 +810,13 @@ size_t LayerTreeHost::CalculateMemoryForRenderSurfaces(
 }
 
 bool LayerTreeHost::PaintMasksForRenderSurface(Layer* render_surface_layer,
-                                               ResourceUpdateQueue* queue) {
+                                               ResourceUpdateQueue* queue,
+                                               RenderingStats* stats) {
   // Note: Masks and replicas only exist for layers that own render surfaces.
If
   // we reach this point in code, we already know that at least something will
   // be drawn into this render surface, so the mask and replica should be
   // painted.
 
-  RenderingStats* stats =
-      debug_state_.RecordRenderingStats() ? &rendering_stats_ : NULL;
-
   bool need_more_updates = false;
   Layer* mask_layer = render_surface_layer->mask_layer();
   if (mask_layer) {
@@ -844,8 +856,10 @@ bool LayerTreeHost::PaintLayerContents(
   PrioritizeTextures(render_surface_layer_list,
                      occlusion_tracker.overdraw_metrics());
 
-  RenderingStats* stats = debug_state_.RecordRenderingStats() ?
-                          &rendering_stats_ : NULL;
+  // TODO(egraether): Use RenderingStatsInstrumentation in Layer::update()
+  RenderingStats stats;
+  RenderingStats* stats_ptr =
+      debug_state_.RecordRenderingStats() ? &stats : NULL;
 
   LayerIteratorType end = LayerIteratorType::end(&render_surface_layer_list);
   for (LayerIteratorType it =
@@ -857,16 +871,18 @@ bool LayerTreeHost::PaintLayerContents(
     if (it.representsTargetRenderSurface()) {
       DCHECK(it->render_surface()->draw_opacity() ||
              it->render_surface()->draw_opacity_is_animating());
-      need_more_updates |= PaintMasksForRenderSurface(*it, queue);
+      need_more_updates |= PaintMasksForRenderSurface(*it, queue, stats_ptr);
     } else if (it.representsItself()) {
       DCHECK(!it->bounds().IsEmpty());
-      it->Update(queue, &occlusion_tracker, stats);
+      it->Update(queue, &occlusion_tracker, stats_ptr);
       need_more_updates |= it->NeedMoreUpdates();
     }
 
     occlusion_tracker.LeaveLayer(it);
   }
 
+  rendering_stats_instrumentation_->AddStats(stats);
+
   occlusion_tracker.overdraw_metrics()->RecordMetrics(this);
 
   return need_more_updates;

[commit-bot: I haz the power, 2013-03-20 23:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/45002

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

Change committed as 189475

[tapted, 2013-03-21 02:27:22]

https://chromiumcodereview.appspot.com/12519006/diff/45002/cc/test/fake_layer...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://chromiumcodereview.appspot.com/12519006/diff/45002/cc/test/fake_layer...
cc/test/fake_layer_tree_host_impl.cc:10: :
rendering_stats_instrumentation_(RenderingStatsInstrumentation::Create()),
sheriff drive-by: I'm .. pretty sure the compiler will reorder these so the base
class will be initialized first, with an invalid pointer.

[egraether, 2013-03-21 02:54:41]

On 2013/03/21 02:27:22, tapted wrote:
>
https://chromiumcodereview.appspot.com/12519006/diff/45002/cc/test/fake_layer...
> File cc/test/fake_layer_tree_host_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12519006/diff/45002/cc/test/fake_layer...
> cc/test/fake_layer_tree_host_impl.cc:10: :
> rendering_stats_instrumentation_(RenderingStatsInstrumentation::Create()),
> sheriff drive-by: I'm .. pretty sure the compiler will reorder these so the
base
> class will be initialized first, with an invalid pointer.

Sounds plausible, thanks for your help!

[egraether, 2013-03-21 04:01:02]

Dana, can you have a look at the test fix please?

https://codereview.chromium.org/12519006/diff/118001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_host_impl.h (right):

https://codereview.chromium.org/12519006/diff/118001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_host_impl.h:31: FakeRenderingStatsInstrumentation
stats_instrumentation_;
Solved this similar to FakeLayerTreeHostImplClient above.

[danakj, 2013-03-21 04:21:59]

LGTM+cq

https://codereview.chromium.org/12519006/diff/118001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_host_impl.h (right):

https://codereview.chromium.org/12519006/diff/118001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_host_impl.h:31: FakeRenderingStatsInstrumentation
stats_instrumentation_;
On 2013/03/21 04:01:02, egraether wrote:
> Solved this similar to FakeLayerTreeHostImplClient above.

Hm, this is a little sketchy, in that the FRSI object won't be initialized yet,
when its pointer is given to the LTHI constructor.. I think. But the LTHI
constructor doesn't use it - similar to the client, so it seems okay for now. At
least it's as right or wrong as what's already happening, and shouldn't cause a
problem unless we start using it from the constructor.

We should probably do a followup that fixes this and client to be initialized
first, at some point.

[commit-bot: I haz the power, 2013-03-21 04:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/118001

[commit-bot: I haz the power, 2013-03-21 04:25:37]

Failed to apply patch for cc/trees/layer_tree_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host.cc
  Hunk #3 succeeded at 96 with fuzz 1.
  Hunk #6 FAILED at 451.
  1 out of 11 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host.cc.rej

Patch:       cc/trees/layer_tree_host.cc
Index: cc/trees/layer_tree_host.cc
diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index
769bac82318cbc96841a0d35dfa150de9f81ab38..6c6eb5aa269264cfb18676c62579490a27c0de7f
100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -15,6 +15,7 @@
 #include "cc/base/math_util.h"
 #include "cc/base/thread.h"
 #include "cc/debug/overdraw_metrics.h"
+#include "cc/debug/rendering_stats_instrumentation.h"
 #include "cc/input/pinch_zoom_scrollbar.h"
 #include "cc/input/pinch_zoom_scrollbar_geometry.h"
 #include "cc/input/pinch_zoom_scrollbar_painter.h"
@@ -77,7 +78,7 @@ LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client,
       needs_filter_context_(false),
       client_(client),
       commit_number_(0),
-      rendering_stats_(),
+     
rendering_stats_instrumentation_(RenderingStatsInstrumentation::Create()),
       renderer_initialized_(false),
       output_surface_lost_(false),
       num_failed_recreate_attempts_(0),
@@ -95,6 +96,9 @@ LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client,
   if (settings_.acceleratedAnimationEnabled)
     animation_registrar_ = AnimationRegistrar::Create();
   s_num_layer_tree_instances++;
+
+  rendering_stats_instrumentation_->set_record_rendering_stats(
+      debug_state_.RecordRenderingStats());
 }
 
 bool LayerTreeHost::Initialize(scoped_ptr<Thread> impl_thread) {
@@ -230,7 +234,7 @@ void LayerTreeHost::UpdateAnimations(base::TimeTicks
frame_begin_time) {
   AnimateLayers(frame_begin_time);
   animating_ = false;
 
-  rendering_stats_.numAnimationFrames++;
+  rendering_stats_instrumentation_->IncrementAnimationFrameCount();
 }
 
 void LayerTreeHost::DidStopFlinging() {
@@ -345,8 +349,12 @@ void
LayerTreeHost::FinishCommitOnImplThread(LayerTreeHostImpl* host_impl) {
     sync_tree->DidBecomeActive();
   }
 
-  if (debug_state_.continuous_painting)
-    host_impl->SavePaintTime(rendering_stats_.totalPaintTime, commit_number());
+  if (debug_state_.continuous_painting) {
+    host_impl->SavePaintTime(
+      rendering_stats_instrumentation_->GetRenderingStats().totalPaintTime,
+      commit_number()
+    );
+  }
 
   commit_number_++;
 }
@@ -443,7 +451,10 @@ scoped_ptr<LayerTreeHostImpl>
LayerTreeHost::CreateLayerTreeHostImpl(
     LayerTreeHostImplClient* client) {
   DCHECK(proxy_->IsImplThread());
   scoped_ptr<LayerTreeHostImpl> host_impl =
-      LayerTreeHostImpl::Create(settings_, client, proxy_.get());
+      LayerTreeHostImpl::Create(settings_,
+                                client,
+                                proxy_.get(),
+                                rendering_stats_instrumentation_.get());
   if (settings_.calculateTopControlsPosition &&
       host_impl->top_controls_manager()) {
     top_controls_manager_weak_ptr_ =
@@ -482,8 +493,7 @@ void LayerTreeHost::DidDeferCommit() {}
 
 void LayerTreeHost::CollectRenderingStats(RenderingStats* stats) const {
   CHECK(debug_state_.RecordRenderingStats());
-  *stats = rendering_stats_;
-  proxy_->CollectRenderingStats(stats);
+  *stats = rendering_stats_instrumentation_->GetRenderingStats();
 }
 
 const RendererCapabilities& LayerTreeHost::GetRendererCapabilities() const {
@@ -557,6 +567,10 @@ void LayerTreeHost::SetDebugState(const
LayerTreeDebugState& debug_state) {
     return;
 
   debug_state_ = new_debug_state;
+
+  rendering_stats_instrumentation_->set_record_rendering_stats(
+      debug_state_.RecordRenderingStats());
+
   SetNeedsCommit();
 }
 
@@ -796,15 +810,13 @@ size_t LayerTreeHost::CalculateMemoryForRenderSurfaces(
 }
 
 bool LayerTreeHost::PaintMasksForRenderSurface(Layer* render_surface_layer,
-                                               ResourceUpdateQueue* queue) {
+                                               ResourceUpdateQueue* queue,
+                                               RenderingStats* stats) {
   // Note: Masks and replicas only exist for layers that own render surfaces.
If
   // we reach this point in code, we already know that at least something will
   // be drawn into this render surface, so the mask and replica should be
   // painted.
 
-  RenderingStats* stats =
-      debug_state_.RecordRenderingStats() ? &rendering_stats_ : NULL;
-
   bool need_more_updates = false;
   Layer* mask_layer = render_surface_layer->mask_layer();
   if (mask_layer) {
@@ -844,8 +856,10 @@ bool LayerTreeHost::PaintLayerContents(
   PrioritizeTextures(render_surface_layer_list,
                      occlusion_tracker.overdraw_metrics());
 
-  RenderingStats* stats = debug_state_.RecordRenderingStats() ?
-                          &rendering_stats_ : NULL;
+  // TODO(egraether): Use RenderingStatsInstrumentation in Layer::update()
+  RenderingStats stats;
+  RenderingStats* stats_ptr =
+      debug_state_.RecordRenderingStats() ? &stats : NULL;
 
   LayerIteratorType end = LayerIteratorType::End(&render_surface_layer_list);
   for (LayerIteratorType it =
@@ -857,16 +871,18 @@ bool LayerTreeHost::PaintLayerContents(
     if (it.represents_target_render_surface()) {
       DCHECK(it->render_surface()->draw_opacity() ||
              it->render_surface()->draw_opacity_is_animating());
-      need_more_updates |= PaintMasksForRenderSurface(*it, queue);
+      need_more_updates |= PaintMasksForRenderSurface(*it, queue, stats_ptr);
     } else if (it.represents_itself()) {
       DCHECK(!it->bounds().IsEmpty());
-      it->Update(queue, &occlusion_tracker, stats);
+      it->Update(queue, &occlusion_tracker, stats_ptr);
       need_more_updates |= it->NeedMoreUpdates();
     }
 
     occlusion_tracker.LeaveLayer(it);
   }
 
+  rendering_stats_instrumentation_->AddStats(stats);
+
   occlusion_tracker.overdraw_metrics()->RecordMetrics(this);
 
   return need_more_updates;

[commit-bot: I haz the power, 2013-03-21 16:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/12519006/80002

[commit-bot: I haz the power, 2013-03-21 16:38:46]

Change committed as 189621