cc: Switch RenderingStats collection in Layer::Update() to RenderingStatsInstrumentation

cc: Switch RenderingStats collection in Layer::Update() to RenderingStatsInstrumentation

This change switches all of the remaining RenderingStats collection in
composited mode to use RenderinStatsInstrumentation.

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

[egraether, 2013-03-23 00:15:40]

Dana, can you review please?

https://codereview.chromium.org/12426024/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/layers/layer.cc#newcode821
cc/layers/layer.cc:821: CHECK(layer_tree_host());
I'm not sure what's best here. Should I just store a reference to
RenderingStatsInstrumentation on Layer? Or can we assume that it's only used
when a LayerTreeHost is set?

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
File cc/resources/bitmap_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
cc/resources/bitmap_content_layer_updater.cc:65: base::TimeDelta(),
No rasterize time saved here, should we change that?

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
File cc/resources/bitmap_skpicture_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
cc/resources/bitmap_skpicture_content_layer_updater.cc:81:
rendering_stats_instrumentation_->AddPaint(duration, 0);
I moved the paint time saving down from above. Should we add pixels painted here
as well?

https://codereview.chromium.org/12426024/diff/1/cc/trees/layer_tree_host_impl.h
File cc/trees/layer_tree_host_impl.h (left):

https://codereview.chromium.org/12426024/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.h:437: int64 num_impl_thread_scrolls_;
I forgot to remove these members in the last patch, they are no longer used.

https://codereview.chromium.org/12426024/diff/1/cc/trees/single_thread_proxy.h
File cc/trees/single_thread_proxy.h (left):

https://codereview.chromium.org/12426024/diff/1/cc/trees/single_thread_proxy....
cc/trees/single_thread_proxy.h:110: base::TimeDelta total_commit_time_;
Forgot to remove these as well.

[danakj, 2013-03-23 01:56:32]

I like!

https://codereview.chromium.org/12426024/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/layers/layer.cc#newcode821
cc/layers/layer.cc:821: CHECK(layer_tree_host());
On 2013/03/23 00:15:40, egraether wrote:
> I'm not sure what's best here. Should I just store a reference to
> RenderingStatsInstrumentation on Layer? Or can we assume that it's only used
> when a LayerTreeHost is set?

We won't update() when there's no LayerTreeHost, so I think you should go
through that.

https://codereview.chromium.org/12426024/diff/1/cc/layers/tiled_layer_unittes...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/layers/tiled_layer_unittes...
cc/layers/tiled_layer_unittest.cc:1629: layer->Update(m_queue.get(), 0);
can you make these 0's into NULL's while you're here?

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
File cc/resources/bitmap_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
cc/resources/bitmap_content_layer_updater.cc:65: base::TimeDelta(),
On 2013/03/23 00:15:40, egraether wrote:
> No rasterize time saved here, should we change that?

It will be equal to the paint time, and I think that was causing a problem
because it was double basically with some metrics. Let's not change behaviour
here. Can you file a bug about this and follow up with hartmanng@ about it?

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
File cc/resources/bitmap_skpicture_content_layer_updater.cc (left):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
cc/resources/bitmap_skpicture_content_layer_updater.cc:36: paint_begin_time =
base::TimeTicks::Now();
Hm, this was called paint time but was recording raster time before actually,
since PaintContentsRect() did rastering instead.. the naming in here..

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
File cc/resources/bitmap_skpicture_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
cc/resources/bitmap_skpicture_content_layer_updater.cc:81:
rendering_stats_instrumentation_->AddPaint(duration, 0);
On 2013/03/23 00:15:40, egraether wrote:
> I moved the paint time saving down from above. Should we add pixels painted
here
> as well?

This is actually not paint time at all, this is already recorded in
ContentLayerUpdater, so it should be removed here.

https://codereview.chromium.org/12426024/diff/1/cc/resources/picture.h
File cc/resources/picture.h (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/picture.h#newcode46
cc/resources/picture.h:46: void Record(ContentLayerClient*,
RenderingStatsInstrumentation*,
variables need names

[egraether, 2013-03-25 21:07:43]

I filed a bug and added a notice to the places in BitmapContentLayerUpdater and
BitmapSkPictureContentLayerUpdater where the stats need cleanup.

https://codereview.chromium.org/12426024/diff/1/cc/layers/tiled_layer_unittes...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/layers/tiled_layer_unittes...
cc/layers/tiled_layer_unittest.cc:1629: layer->Update(m_queue.get(), 0);
On 2013/03/23 01:56:32, danakj wrote:
> can you make these 0's into NULL's while you're here?

Done.

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
File cc/resources/bitmap_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
cc/resources/bitmap_content_layer_updater.cc:65: base::TimeDelta(),
On 2013/03/23 01:56:32, danakj wrote:
> On 2013/03/23 00:15:40, egraether wrote:
> > No rasterize time saved here, should we change that?
> 
> It will be equal to the paint time, and I think that was causing a problem
> because it was double basically with some metrics. Let's not change behaviour
> here. Can you file a bug about this and follow up with hartmanng@ about it?

Had a chat with hartmanng, he was not sure what's best to do here and thought
you might know better than him. I'll leave behaviour the same and file a bug.

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
File cc/resources/bitmap_skpicture_content_layer_updater.cc (left):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
cc/resources/bitmap_skpicture_content_layer_updater.cc:36: paint_begin_time =
base::TimeTicks::Now();
On 2013/03/23 01:56:32, danakj wrote:
> Hm, this was called paint time but was recording raster time before actually,
> since PaintContentsRect() did rastering instead.. the naming in here..

I will leave behaviour the same here as well, so I keep this paint time
recording moved down to PaintContentsRect() where I have stats instrumentation.

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
File cc/resources/bitmap_skpicture_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_skpicture...
cc/resources/bitmap_skpicture_content_layer_updater.cc:81:
rendering_stats_instrumentation_->AddPaint(duration, 0);
On 2013/03/23 01:56:32, danakj wrote:
> On 2013/03/23 00:15:40, egraether wrote:
> > I moved the paint time saving down from above. Should we add pixels painted
> here
> > as well?
> 
> This is actually not paint time at all, this is already recorded in
> ContentLayerUpdater, so it should be removed here.

I will leave it to keep behaviour the same.

https://codereview.chromium.org/12426024/diff/1/cc/resources/picture.h
File cc/resources/picture.h (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/picture.h#newcode46
cc/resources/picture.h:46: void Record(ContentLayerClient*,
RenderingStatsInstrumentation*,
On 2013/03/23 01:56:32, danakj wrote:
> variables need names

Done.

[danakj, 2013-03-26 18:47:50]

LGTM

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
File cc/resources/bitmap_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/1/cc/resources/bitmap_content_l...
cc/resources/bitmap_content_layer_updater.cc:65: base::TimeDelta(),
On 2013/03/25 21:07:44, egraether wrote:
> On 2013/03/23 01:56:32, danakj wrote:
> > On 2013/03/23 00:15:40, egraether wrote:
> > > No rasterize time saved here, should we change that?
> > 
> > It will be equal to the paint time, and I think that was causing a problem
> > because it was double basically with some metrics. Let's not change
behaviour
> > here. Can you file a bug about this and follow up with hartmanng@ about it?
> 
> Had a chat with hartmanng, he was not sure what's best to do here and thought
> you might know better than him. I'll leave behaviour the same and file a bug.

Ok, can you add a TODO that points to the crbug in both of these places? Thanks!

https://codereview.chromium.org/12426024/diff/12001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12426024/diff/12001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:821: CHECK(layer_tree_host());
The check is kind of redundant. We'll crash on the layer_tree_host() NULL deref
anyways. And a null deref isn't accessing arbitrary memory, so I don't think you
need the CHECK() here.

https://codereview.chromium.org/12426024/diff/12001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:822: return
layer_tree_host()->rendering_stats_instrumentation();
nit: layer_tree_host_->

[danakj, 2013-03-26 18:48:32]

https://codereview.chromium.org/12426024/diff/12001/cc/resources/bitmap_conte...
File cc/resources/bitmap_content_layer_updater.cc (right):

https://codereview.chromium.org/12426024/diff/12001/cc/resources/bitmap_conte...
cc/resources/bitmap_content_layer_updater.cc:64: // TODO: Clarify if this needs
to be saved here. crbug.com/223693
Oh you did this already :D

[commit-bot: I haz the power, 2013-03-26 19:34:42]

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

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

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-26 20:53:58]

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

[commit-bot: I haz the power, 2013-03-26 20:54:12]

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

Patch:       cc/trees/layer_tree_host_unittest.cc
Index: cc/trees/layer_tree_host_unittest.cc
diff --git a/cc/trees/layer_tree_host_unittest.cc
b/cc/trees/layer_tree_host_unittest.cc
index
5c9c2dac823a360b28e841f372a8465e3c394f74..f42ad1e73b4466a7d4b4fc4b2a82f5392ebd7970
100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -695,9 +695,9 @@ public:
     int paintContentsCount() { return m_paintContentsCount; }
     void resetPaintContentsCount() { m_paintContentsCount = 0; }
 
-    virtual void Update(ResourceUpdateQueue* queue, const OcclusionTracker*
occlusion, RenderingStats* stats) OVERRIDE
+    virtual void Update(ResourceUpdateQueue* queue, const OcclusionTracker*
occlusion) OVERRIDE
     {
-        ContentLayer::Update(queue, occlusion, stats);
+        ContentLayer::Update(queue, occlusion);
         m_paintContentsCount++;
     }
 
@@ -1376,7 +1376,7 @@ class EvictionTestLayer : public Layer {
 public:
     static scoped_refptr<EvictionTestLayer> Create() { return
make_scoped_refptr(new EvictionTestLayer()); }
 
-    virtual void Update(ResourceUpdateQueue*, const OcclusionTracker*,
RenderingStats*) OVERRIDE;
+    virtual void Update(ResourceUpdateQueue*, const OcclusionTracker*)
OVERRIDE;
     virtual bool DrawsContent() const OVERRIDE { return true; }
 
     virtual scoped_ptr<LayerImpl> CreateLayerImpl(LayerTreeImpl* treeImpl)
OVERRIDE;
@@ -1435,7 +1435,7 @@ void EvictionTestLayer::SetTexturePriorities(const
PriorityCalculator&)
     m_texture->set_request_priority(PriorityCalculator::UIPriority(true));
 }
 
-void EvictionTestLayer::Update(ResourceUpdateQueue* queue, const
OcclusionTracker*, RenderingStats*)
+void EvictionTestLayer::Update(ResourceUpdateQueue* queue, const
OcclusionTracker*)
 {
     createTextureIfNeeded();
     if (!m_texture.get())

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

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

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

Failed to trigger a try job on mac
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-26 21:40:18]

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

[commit-bot: I haz the power, 2013-03-26 23:11:30]

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

[commit-bot: I haz the power, 2013-03-26 23:27:03]

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

[commit-bot: I haz the power, 2013-03-27 02:31:08]

Change committed as 190817

[commit-bot: I haz the power, 2013-03-27 19:36:48]

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

[commit-bot: I haz the power, 2013-03-28 01:34:30]

Change committed as 191086