cc: add rasterize time to continuous painting graph data in impl-side-painting

This change adds the rasterize time of tiles labeled NOW_BIN of the pending tree
to the continuous painting data when in impl-side-painting.

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

[egraether, 2013-02-05 19:06:32]

I had some troubles here with getting rendering stats from the TileManager after
it was initialized. What do you think of this solution?

[danakj, 2013-02-05 19:20:39]

WDYT of making the recordRenderingStats a debugState flag instead?

WebLayerTreeViewImpl could set it based on the value of the webSettings value
(similar to settings.initialDebugState.showFPSCounter =
webSettings.showFPSCounter).

Then WebLayerTreeViewImpl::setContinuousPaintingEnabled can turn it on/off and
use the intialDebugSettings to tell when it should turn it off.

That'd let us avoid the whole setting || debugstate stuff, which I'd prefer.

[nduca, 2013-02-05 23:09:35]

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

https://codereview.chromium.org/12209022/diff/1/cc/layer_tree_host.cc#newcode326
cc/layer_tree_host.cc:326: if (m_debugState.continuousPainting)
&& !implSidePainting

https://codereview.chromium.org/12209022/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12209022/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1720: {
bool continousChanged = m_debugState.continuousPainting !=
debugState.continuousPainting

https://codereview.chromium.org/12209022/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1727: void LayerTreeHostImpl::savePaintTime(const
base::TimeDelta& totalPaintTime)
in impl side painting && continuous painting mode, call savePaintTime in
::activatePendingTree

https://codereview.chromium.org/12209022/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1735: totalPaintAndRasterizeTime +=
stats.totalRasterizeTime;
stats.totalRasterizeTimeForNowBinsOnPendingTree

[egraether, 2013-02-06 19:29:00]

The new patch only saves the rasterize time of tiles labeled NOW_BIN in the
pending tree. The rasterize time is saved to PaintTimeCounter in a separate
SaveRasterizeTime() call in LayerTreeHostImpl::activatePendingTree(). Paint- and
RasterizeTime is then summed up, because in impl-side-painting we still need to
take PaintTime into account. Does this look right?

[egraether, 2013-02-12 00:15:30]

The new patch saves paint and rasterize times in a struct along with the frame
number they belong to. I'm still saving the times as deltas instead of totals. I
found saving totals over-complicates things, in practice we only need the
deltas.

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:79: if (ring_buffer_.IsFilledIndex(i) &&
Buffer iteration is a mess. I will try to implement a simple iterator class.

[nduca, 2013-02-13 08:12:21]

https://codereview.chromium.org/12209022/diff/6007/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12209022/diff/6007/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:151: , m_paintTimeCounter(PaintTimeCounter::create())
why do you defer the creation of the counter? It seems a bit risky to me

https://codereview.chromium.org/12209022/diff/6007/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12209022/diff/6007/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1748:
m_paintTimeCounter->SavePaintTime(totalPaintTime, frameNumber);
maybe we should use the word commitNumber here and throughout? It looks like its
a typo that we say source_frame_number on the impl side elsewhere

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:42: if (!ignore_next_rasterize_time_)
since this is just for the first entry in the ring buffer, what about just
saying if (num entries in ring buffer  == 1)?

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:43: GetEntryForFrame(frame_number).rasterize_time =
Oh, i just realized....

So! The sequence in walltime is

commit
   painting happens for pending now bins
   didActivateTree

if you save the paintNowBin times at didActivateTree, then you actually are
done. All the now-bin painting has completed and you have the correct number...

Ping me when you're in tomorrow if this doesn't make sense.

https://codereview.chromium.org/12209022/diff/6007/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:76: PaintTimeCounter::Entry&
PaintTimeCounter::GetEntryForFrame(
GetOrCreateEntryForFrame since it adds it if it doesn't exist?

[egraether, 2013-02-13 20:28:07]

https://codereview.chromium.org/12209022/diff/9001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12209022/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:160: , m_paintTimeCounter(PaintTimeCounter::create())
Creating PaintTimeCounter in constructor again.

https://codereview.chromium.org/12209022/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1778: void LayerTreeHostImpl::savePaintTime(const
base::TimeDelta& totalPaintTime, const int& commitNumber)
Changed naming to commitNumber.

https://codereview.chromium.org/12209022/diff/9001/cc/paint_time_counter.cc
File cc/paint_time_counter.cc (right):

https://codereview.chromium.org/12209022/diff/9001/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:31: if (can_save_paint_time_delta) {
These checks that no entry gets saved, when it's not possible to calculate a
correct delta time. It's not possible to check for num entries of the buffer,
since there are no. I renamed the flags to make this more clear.

https://codereview.chromium.org/12209022/diff/9001/cc/paint_time_counter.cc#n...
cc/paint_time_counter.cc:47: DCHECK(commit_number == entry.commit_number);
This checks whether the assumption that rasterize time is saved after paint time
holds true. It's also the only place now where Entry.commit_number is used.

[nduca, 2013-02-13 21:14:39]

What about the observation I made about this being simpler than we thought
because of activation implying all pending painting is done?

[egraether, 2013-02-13 22:01:16]

On 2013/02/13 21:14:39, nduca wrote:
> What about the observation I made about this being simpler than we thought
> because of activation implying all pending painting is done?

Yes, now I just add the rasterize time to the last Entry that saved paint time.
I still do separate calls for saving paint and rasterize time, because paint
time is saved during commit and rasterize time is saved on pending tree
activation.

What I could do is switch back to just saving single paint times instead of this
new Entry struct and add the rasterize time to that. Would that be better?

[nduca, 2013-02-13 22:23:16]

That seems fine. I can't review right now but this seems on track.

[nduca, 2013-02-13 22:38:23]

maybe dana can give you final review. i'll lgtm to get out of the way.

[nduca, 2013-02-13 22:40:36]

Oh, btw, you need to rebase. I landed the now bin stuff last night.

[egraether, 2013-02-13 23:00:50]

On 2013/02/13 22:40:36, nduca wrote:
> Oh, btw, you need to rebase. I landed the now bin stuff last night.

That's already in there.

[danakj, 2013-02-15 18:45:06]

https://codereview.chromium.org/12209022/diff/17001/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12209022/diff/17001/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:221: void savePaintTime(const base::TimeDelta&
totalPaintTime, const int& commitNumber);
no const& for an int parameter please.

https://codereview.chromium.org/12209022/diff/17001/cc/paint_time_counter.h
File cc/paint_time_counter.h (right):

https://codereview.chromium.org/12209022/diff/17001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:30: base::TimeDelta time() const {
nit: total_time()?

https://codereview.chromium.org/12209022/diff/17001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:40: const int& commit_number);
int by value

https://codereview.chromium.org/12209022/diff/17001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:42: const int& commit_number);
int by value

https://codereview.chromium.org/12209022/diff/17001/cc/paint_time_counter.h#n...
cc/paint_time_counter.h:55: bool can_save_paint_time_delta;
these should have _ suffix

[egraether, 2013-02-15 18:58:16]

Fixed style and resolved nits.

[danakj, 2013-02-15 19:18:36]

thanks, LGTM

[commit-bot: I haz the power, 2013-02-15 19:25:29]

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

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

Change committed as 182839