Add cheapness predictor success histogram

cc: Add cheapness predictor success histogram

For picture piles that are fairly cheap to rasterize, we want to do the step on the same thread (instead of dispatching it to a different thread). As a first step, this change adds an accuracy metric for the predictor. Three new histograms are added: CheapPredictorAccuracy measures overall success rate of the predictor; CheapPredictorSafelyWrong counts the number of times the prediction was "this is a slow raster", but in reality it was fast; CheapPredictorBadlyWrong counts the number of times the prediction was "this is a fast raster", but in reality it was slow.

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

[brianderson, 2013-02-04 22:51:02]

Would it makes sense to have to accuracy measurements, one for when we predict a
tile is cheap and one for when we predict a tile is expensive?

That way we get feedback about which way we are incorrectly predicting.

[nduca, 2013-02-04 23:16:52]

+1 to directionality

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

https://codereview.chromium.org/12192025/diff/1/cc/tile_manager.cc#newcode800
cc/tile_manager.cc:800: base::TimeTicks end_time = base::TimeTicks::Now();
now that i look at this, didn't we already collect the start and end time inside
:797 already?

[vmpstr, 2013-02-04 23:32:44]

On 2013/02/04 23:16:52, nduca wrote:
> +1 to directionality
> 
> https://codereview.chromium.org/12192025/diff/1/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
> https://codereview.chromium.org/12192025/diff/1/cc/tile_manager.cc#newcode800
> cc/tile_manager.cc:800: base::TimeTicks end_time = base::TimeTicks::Now();
> now that i look at this, didn't we already collect the start and end time
inside
> :797 already?

I moved it out of there in https://codereview.chromium.org/12191006/, it's now
more consistent with ImageDecodeTask and GatherPixelsRefs. Those two do the
timings in tile manager.

[vmpstr, 2013-02-05 00:10:58]

Added IsCheap/IsNotCheap histograms. ptal

[nduca, 2013-02-05 00:22:25]

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode811
cc/tile_manager.cc:811: picture_pile->IsCheapInRect (rect, contents_scale),
might pull out the calculation into local vars so its clearer whats being
passed.

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode818
cc/tile_manager.cc:818: void TileManager::RecordCheapnessPredictorCorrectness(
RecordCheapnessPredictorResults?

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode823
cc/tile_manager.cc:823: UMA_HISTOGRAM_BOOLEAN(
i think these two should actually e

if (is_predicted_cheap && !is_actually_cheap)
   Renderer4.CheapPredictorWasBadlyWrong++
else (!is_predicted_cheap && is_actually_cheap)
   Renderer4.CheapPredictorWasOverconservative++

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode832
cc/tile_manager.cc:832: 
are histograms the right thing to track here? The end state is that we want to
know "how many times did we estimate, and what % were right, what % were
overconservative, and what % were badly wrong?

To do that, I think you need a few more numbers than you're tracking here, like
# of times wrong.

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode834
cc/tile_manager.cc:834: if ((is_predicted_cheap && is_actually_cheap) ||
isn't this just 

is_predicted_cheap == is_actually_cheap?

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode839
cc/tile_manager.cc:839: "Renderer4.CheapPredictorOverallAccuracy",
same comment about histograms being the right tool

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.h
File cc/tile_manager.h (right):

https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.h#newcode174
cc/tile_manager.h:174: static void RecordCheapnessPredictorCorrectness(
indentation, follow chrome style guide

foo(bar,
    baz)

or
  foo(
      bar, baz)

In this case, probably the frist

[vmpstr, 2013-02-05 17:51:59]

On 2013/02/05 00:22:25, nduca wrote:
> https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode811
> cc/tile_manager.cc:811: picture_pile->IsCheapInRect (rect, contents_scale),
> might pull out the calculation into local vars so its clearer whats being
> passed.

Agreed, made local variables

> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode818
> cc/tile_manager.cc:818: void TileManager::RecordCheapnessPredictorCorrectness(
> RecordCheapnessPredictorResults?
> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode823
> cc/tile_manager.cc:823: UMA_HISTOGRAM_BOOLEAN(
> i think these two should actually e
> 
> if (is_predicted_cheap && !is_actually_cheap)
>    Renderer4.CheapPredictorWasBadlyWrong++
> else (!is_predicted_cheap && is_actually_cheap)
>    Renderer4.CheapPredictorWasOverconservative++
> 

I changed this (ptal), I've also updated the description to reflect what I'm
doing

>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode832
> cc/tile_manager.cc:832: 
> are histograms the right thing to track here? The end state is that we want to
> know "how many times did we estimate, and what % were right, what % were
> overconservative, and what % were badly wrong?
> 
> To do that, I think you need a few more numbers than you're tracking here,
like
> # of times wrong.
> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode834
> cc/tile_manager.cc:834: if ((is_predicted_cheap && is_actually_cheap) ||
> isn't this just 
> 
> is_predicted_cheap == is_actually_cheap?

Good point :)

> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.cc#newcode839
> cc/tile_manager.cc:839: "Renderer4.CheapPredictorOverallAccuracy",
> same comment about histograms being the right tool
> 
> https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.h
> File cc/tile_manager.h (right):
> 
>
https://codereview.chromium.org/12192025/diff/2002/cc/tile_manager.h#newcode174
> cc/tile_manager.h:174: static void RecordCheapnessPredictorCorrectness(
> indentation, follow chrome style guide
> 
> foo(bar,
>     baz)
> 
> or
>   foo(
>       bar, baz)
> 
> In this case, probably the frist

I've changed the style throughout

ptal

[nduca, 2013-02-05 20:02:33]

lgtm nice

[vmpstr, 2013-02-05 22:34:57]

I'd appreciate another look at this. I rebased on the latest patch from
12194015. The cl now includes everything that needs to go in for it to work

[nduca, 2013-02-05 22:45:56]

still lgtm

[commit-bot: I haz the power, 2013-02-05 22:53:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12192025/16001

[commit-bot: I haz the power, 2013-02-06 02:14:00]

Change committed as 180863

[Tom Hudson, 2013-02-06 12:24:57]

https://codereview.chromium.org/12192025/diff/16001/cc/picture_pile_impl.cc
File cc/picture_pile_impl.cc (right):

https://codereview.chromium.org/12192025/diff/16001/cc/picture_pile_impl.cc#n...
cc/picture_pile_impl.cc:188: if (!(*i)->IsCheapInRect(layer_rect))
This layer_rect is the content_rect, inverse scaled by contents_scale.

The Picture in the iterator also has a layer_rect_, which often seems to be the
same in practice (and you check to make sure they intersect).

Shouldn't we be worried about the tile bounds somewhere?

[vmpstr, 2013-02-06 17:41:03]

On 2013/02/06 12:24:57, Tom Hudson wrote:
> https://codereview.chromium.org/12192025/diff/16001/cc/picture_pile_impl.cc
> File cc/picture_pile_impl.cc (right):
> 
>
https://codereview.chromium.org/12192025/diff/16001/cc/picture_pile_impl.cc#n...
> cc/picture_pile_impl.cc:188: if (!(*i)->IsCheapInRect(layer_rect))
> This layer_rect is the content_rect, inverse scaled by contents_scale.
> 
> The Picture in the iterator also has a layer_rect_, which often seems to be
the
> same in practice (and you check to make sure they intersect).
> 
> Shouldn't we be worried about the tile bounds somewhere?

PicturePileImpl::IsCheapInRect is actually from
https://codereview.chromium.org/12194015 patch set 6; This change was just made
on top of it. The implementation follows GatherPixelRefs/Raster logic in how it
deals with content_rect/layer_rect, so I think its OK?