cc: Use commit to activation time in repaint HUD

cc: Use commit to activation time in repaint HUD

In an effort to remove rasterize_time from the impl stats, I'm using
commit_to_activate_duration in its place, which should give a better
indication of the commit to activation time (raster + management time).

BUG=

Committed: https://crrev.com/f1775271d3c335de70d5b6b317f64b0d464a6191
Cr-Commit-Position: refs/heads/master@{#302633}

[hendrikw, 2014-10-29 00:06:48]

hendrikw@chromium.org changed reviewers:
+ brianderson@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-10-29 00:07:00]

PTAL, thanks!

[brianderson, 2014-10-29 00:24:15]

I would like to understand the use cases for the old metric, but I think the new
metric is a really good reflection of how slow activation can become a
scheduling bottleneck. We should be displaying it somewhere, if not in the HUD.

From the discussions I overheard, it sounds like this would also better reflect
improvements we would get from multi-thread rasterization?

https://codereview.chromium.org/689503002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/689503002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1874:
paint_time_counter_->SavePaintTime(stats.main_stats.paint_time +
What is main_stats.paint_time?

[hendrikw, 2014-10-29 01:54:57]

On 2014/10/29 00:24:15, brianderson wrote:
> I would like to understand the use cases for the old metric

I agree, anyone we can add to this who might know more.

> What is main_stats.paint_time?

|TiledLayer::UpdateTileTextures()| calls
|BitmapContentLayerUpdater::PrepareToUpdate()|, paint_time is the duration of
|PaintContents()|.

Hmm, I suppose I should rename the PaintTimeCounter class too :/

[vmpstr, 2014-10-29 07:43:05]

vmpstr@chromium.org changed reviewers:
+ ernstm@chromium.org, nduca@chromium.org

[vmpstr, 2014-10-29 07:43:05]

In general I think this is a more interesting number to display, since it
captures how much time we spend from commit to activation, which is basically
the time it takes us to produce a frame. It captures both rasterization, as well
as tile management/draw props updates. 

Additionally, according to hendrikw's quick check, we would be able to clean up
the interface quite a bit if we just get rid of rasterize_time (it takes some
weird plumbing to get that number). 

+nduca +ernstm, does the paint time number get used a lot? That is, is the
actual value a useful metric when displayed in continuous repainting mode or
would a frame time be a good (better?) substitute for that?

[danakj, 2014-10-29 14:37:47]

On 2014/10/29 01:54:57, Hendrik wrote:
> On 2014/10/29 00:24:15, brianderson wrote:
> > I would like to understand the use cases for the old metric
> 
> I agree, anyone we can add to this who might know more.
> 
> > What is main_stats.paint_time?
> 
> |TiledLayer::UpdateTileTextures()| calls
> |BitmapContentLayerUpdater::PrepareToUpdate()|, paint_time is the duration of
> |PaintContents()|.
> 
> Hmm, I suppose I should rename the PaintTimeCounter class too :/

Does Picture* not use this metric? If it's just TiledLayer/LayerUpdater then
it's basically dead code (browser compositor only atm).

[ernstm, 2014-10-29 17:21:19]

I think the paint data in the HUD was meant as some kind of live
rasterize-and-record metric, but Nat would know better how it is used by
developers.

https://codereview.chromium.org/689503002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/689503002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1874:
paint_time_counter_->SavePaintTime(stats.main_stats.paint_time +
On 2014/10/29 00:24:15, brianderson wrote:
> What is main_stats.paint_time?

paint_time was a pre-impl-side painting metric, originally used for the legacy
software mode. The last call site that records it is
cc/resources/bitmap_content_layer_updater.cc:83 which is dead code according to
Dana. We should remove paint_time from rendering_stats and the telemetry side as
well; maybe as a separate patch?

[hendrikw, 2014-10-29 21:26:16]

On 2014/10/29 14:37:47, danakj wrote:
> On 2014/10/29 01:54:57, Hendrik wrote:
> > On 2014/10/29 00:24:15, brianderson wrote:
> > > I would like to understand the use cases for the old metric
> > 
> > I agree, anyone we can add to this who might know more.
> > 
> > > What is main_stats.paint_time?
> > 
> > |TiledLayer::UpdateTileTextures()| calls
> > |BitmapContentLayerUpdater::PrepareToUpdate()|, paint_time is the duration
of
> > |PaintContents()|.
> > 
> > Hmm, I suppose I should rename the PaintTimeCounter class too :/
> 
> Does Picture* not use this metric? If it's just TiledLayer/LayerUpdater then
> it's basically dead code (browser compositor only atm).

That's right, dead(ish).  It is only used in
BitmapContentLayerUpdater::PrepareToUpdate.

[ernstm, 2014-10-29 21:31:57]

> > Does Picture* not use this metric? If it's just TiledLayer/LayerUpdater then
> > it's basically dead code (browser compositor only atm).
> 
> That's right, dead(ish).  It is only used in
> BitmapContentLayerUpdater::PrepareToUpdate.

I'm not even sure if that should be paint_time. It's possible that this is
actually rasterize_time. Can someone more familiar with content layers confirm?
In any case we should remove paint_time from rendering stats.

[danakj, 2014-10-30 15:17:13]

On Wed, Oct 29, 2014 at 5:31 PM, <ernstm@chromium.org> wrote:

> > Does Picture* not use this metric? If it's just TiledLayer/LayerUpdater
>> then
>> > it's basically dead code (browser compositor only atm).
>>
>
>  That's right, dead(ish).  It is only used in
>> BitmapContentLayerUpdater::PrepareToUpdate.
>>
>
> I'm not even sure if that should be paint_time. It's possible that this is
> actually rasterize_time. Can someone more familiar with content layers
> confirm?
> In any case we should remove paint_time from rendering stats.
>

"Painting" with implside painting is just recording.
BitmapContentLayerUpdater does record+raster inside of "painting".

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hendrikw, 2014-10-30 18:59:03]

Hi, PTAL

On Brian's advice, I've also added the draw_duration to the value reported, so
that when things change with Ganesh, we should still get good numbers. I've also
renamed it to "Compositor frame time".

[hendrikw, 2014-10-30 20:52:06]

On 2014/10/30 18:59:03, Hendrik wrote:
> Hi, PTAL
> 
> On Brian's advice, I've also added the draw_duration to the value reported, so
> that when things change with Ganesh, we should still get good numbers. I've
also
> renamed it to "Compositor frame time".

Brian pointed out that we could get multiple draws per activation, we're only
counting the last one. I'm not sure if this is a problem or not, maybe we
shouldn't be including the draw_duration.  According to Nat, what's important is
that the shape of the graph shows when stuff slows down, which it does, and not
the actual numbers.  

We'll also need to change what we report here when we start committing directly
to the active tree.  I'll log a bug to capture this since it's not obvious that
it needs to be fixed when this happens.

[hendrikw, 2014-10-30 21:38:05]

On 2014/10/30 20:52:06, Hendrik wrote:
> On 2014/10/30 18:59:03, Hendrik wrote:
> > Hi, PTAL
> > 
> > On Brian's advice, I've also added the draw_duration to the value reported,
so
> > that when things change with Ganesh, we should still get good numbers. I've
> also
> > renamed it to "Compositor frame time".
> 
> Brian pointed out that we could get multiple draws per activation, we're only
> counting the last one. I'm not sure if this is a problem or not, maybe we
> shouldn't be including the draw_duration.  According to Nat, what's important
is
> that the shape of the graph shows when stuff slows down, which it does, and
not
> the actual numbers.  
> 
> We'll also need to change what we report here when we start committing
directly
> to the active tree.  I'll log a bug to capture this since it's not obvious
that
> it needs to be fixed when this happens.

Hi again, here's the change that depend on this:
https://codereview.chromium.org/683863003/

It's a nice cleanup.

[hendrikw, 2014-10-31 21:17:42]

> Hi again, here's the change that depend on this:
> https://codereview.chromium.org/683863003/
> 
> It's a nice cleanup.

Hi, I've added a comment and a bug crbug.com/429311, that's now blocking
https://code.google.com/p/chromium/issues/detail?id=383157.

PTAL, I'd like to move on this so that I can commit the other CL that depends on
this. THANKS!

[vmpstr, 2014-11-03 19:44:14]

lgtm

[brianderson, 2014-11-03 21:16:37]

lgtm

[hendrikw, 2014-11-04 17:10:27]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-11-04 17:11:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/689503002/80001

[commit-bot: I haz the power, 2014-11-04 18:55:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-04 18:57:08]

Patchset 5 (id:??) landed as
https://crrev.com/f1775271d3c335de70d5b6b317f64b0d464a6191
Cr-Commit-Position: refs/heads/master@{#302633}