cc: Add rasterize and record micro benchmark.

cc: Add rasterize and record micro benchmark.

This patch adds a rasterize and record micro benchmark.
It depends on

https://codereview.chromium.org/67563002/

R=enne@chromium.org, nduca@chromium.org, ernstm@chromium.org

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

[vmpstr, 2013-11-09 01:05:52]

Please take a look.

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
cc/debug/rasterize_and_record_benchmark_impl.cc:75:
tiling->CreateAllTilesForTesting();
This was the only way I could figure out how to create all tilings. This isn't
really release/production code (so for test fits), but I don't really like
calling this function since it kinda is the same binary as chrome.

Suggestions are more than welcome.

[enne (OOO), 2013-11-11 19:47:13]

Sorry for  my confusion, but

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
cc/debug/rasterize_and_record_benchmark_impl.cc:68: void
RasterizeAndRecordBenchmarkImpl::RunOnLayer(PictureLayerImpl* layer) {
I don't understand why this has to run on the compositor thread.  It seems like
it'd be a lot simpler if you just ran it all on the main thread.

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
cc/debug/rasterize_and_record_benchmark_impl.cc:75:
tiling->CreateAllTilesForTesting();
On 2013/11/09 01:05:52, vmpstr wrote:
> This was the only way I could figure out how to create all tilings. This isn't
> really release/production code (so for test fits), but I don't really like
> calling this function since it kinda is the same binary as chrome.
> 
> Suggestions are more than welcome.

I'm not sure why you need tiles here at all.  The iterator should still be able
to return you content rects even when there aren't tiles, yeah?

[vmpstr, 2013-11-11 22:38:37]

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
cc/debug/rasterize_and_record_benchmark_impl.cc:68: void
RasterizeAndRecordBenchmarkImpl::RunOnLayer(PictureLayerImpl* layer) {
On 2013/11/11 19:47:14, enne wrote:
> I don't understand why this has to run on the compositor thread.  It seems
like
> it'd be a lot simpler if you just ran it all on the main thread.

So I tried putting this on the main thread (which technically should work just
fine). However, I ran into a couple of issues:

- I can't get the ideal content scale / device scale: contents_scale_x for
PictureLayer seems to return 1 on any device I try. PictureLayerImpl returns the
correct thing.
- I can't use PictureLayerTiling set since it takes the PictureLayerImpl as a
client (but I can work around by using tiling data).

After discussion with ernstm, it seems that it would be better if we use the
correct scale... so sticking to compositor thread seems to be better (also we
can then use the tiling set coverage iterator, which should eliminate any
off-by-one errors that might creep up in tiling data iteration).

Let me know what your thoughts are.

https://codereview.chromium.org/66213007/diff/1/cc/debug/rasterize_and_record...
cc/debug/rasterize_and_record_benchmark_impl.cc:75:
tiling->CreateAllTilesForTesting();
On 2013/11/11 19:47:14, enne wrote:
> On 2013/11/09 01:05:52, vmpstr wrote:
> > This was the only way I could figure out how to create all tilings. This
isn't
> > really release/production code (so for test fits), but I don't really like
> > calling this function since it kinda is the same binary as chrome.
> > 
> > Suggestions are more than welcome.
> 
> I'm not sure why you need tiles here at all.  The iterator should still be
able
> to return you content rects even when there aren't tiles, yeah?

Geometry rect (from the coverage iterator) returns a clipped-to-viewport
post-scale rect. I could reconstruct the original pre-scale rect, but I can't
really undo the clip. That and I would have to expose picture_pile from
PictureLayerImpl, which I've tried to avoid (now I read it off the tile). This
seemed easier, but am I willing to change this if you insist.

[vmpstr, 2013-11-12 23:54:44]

This is updated as per discussion on the plumbing code review.

[ernstm, 2013-11-13 01:01:48]

Nice! I've compared the new version to the old version. For the same repeat
count, the variance of both versions is in the same ballpark. The new version
finishes much faster on desktop, because conservative stop-wait-times are not
required.

The only thing that concerns me is an extreme outlier in rasterize time on
Twitter.com (190 ms instead of the expected 10 ms) showing up in one of 30 runs.
Outliers are common, but that order of magnitude is unusual. I suggest we run
the new version on the bot in parallel to the old version in order to gather
more data before we replace the current benchmark.

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
File tools/perf/measurements/new_rasterize_and_record.py (right):

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
tools/perf/measurements/new_rasterize_and_record.py:30:
time.sleep(float(self.options.start_wait_time))
We can wait for document.readyState=='complete' and then sleep for a bit until
the content stabilizes like this:

try:
  tab.WaitForJavaScriptExpression("document.readyState == 'complete'", 10)
except TimeoutException:
  pass
time.sleep(float(self.options.start_wait_time))

(Wordpress sometimes times out on the expression. In that case we should just
start the benchmark regardless)

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
tools/perf/measurements/new_rasterize_and_record.py:42: "record_repeat_count":
1000,
I suggest using 100 as the default value for record and rasterize repeat. 100
gives sufficiently low variance (tested on Linux; the old benchmark used 20).
1000 would be nice to have, but I'm concerned that runtimes on Android could get
very long.

Please add command line arguments --rasterize-repeat and --record-repeat to
adjust those values.

[enne (OOO), 2013-11-14 00:24:05]

lgtm

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.cc (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.cc:109: SkTileGridPicture::TileGridInfo
tile_grid_info;
Maybe add this as a static accessor on cc::Picture?

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.h (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.h:59:
base::WeakPtrFactory<RasterizeAndRecordBenchmark> weak_ptr_factory_;
It's a little sad to me that each derived benchmark has to do work to be
threadsafe (rather than being safe by default because the manager handles it),
but maybe this is good enough for now and if we get a bunch more benchmarks we
can refactor later.

[vmpstr, 2013-11-14 19:38:23]

PTAL

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.cc (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.cc:109: SkTileGridPicture::TileGridInfo
tile_grid_info;
On 2013/11/14 00:24:05, enne wrote:
> Maybe add this as a static accessor on cc::Picture?

Unfortunately we don't store tile_grid_info on the picture. We only have it on a
picture pile base, but I kind of feel that it's not really appropriate to get it
from there, since this is a test for a picture record and really has nothing to
do with a picture pile.

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.h (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.h:59:
base::WeakPtrFactory<RasterizeAndRecordBenchmark> weak_ptr_factory_;
On 2013/11/14 00:24:05, enne wrote:
> It's a little sad to me that each derived benchmark has to do work to be
> threadsafe (rather than being safe by default because the manager handles it),
> but maybe this is good enough for now and if we get a bunch more benchmarks we
> can refactor later.

I'll see if I can make this better in a follow-up

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
File tools/perf/measurements/new_rasterize_and_record.py (right):

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
tools/perf/measurements/new_rasterize_and_record.py:30:
time.sleep(float(self.options.start_wait_time))
On 2013/11/13 01:01:49, Manfred Ernst wrote:
> We can wait for document.readyState=='complete' and then sleep for a bit until
> the content stabilizes like this:
> 
> try:
>   tab.WaitForJavaScriptExpression("document.readyState == 'complete'", 10)
> except TimeoutException:
>   pass
> time.sleep(float(self.options.start_wait_time))
> 
> (Wordpress sometimes times out on the expression. In that case we should just
> start the benchmark regardless)

Done.

https://codereview.chromium.org/66213007/diff/70001/tools/perf/measurements/n...
tools/perf/measurements/new_rasterize_and_record.py:42: "record_repeat_count":
1000,
On 2013/11/13 01:01:49, Manfred Ernst wrote:
> I suggest using 100 as the default value for record and rasterize repeat. 100
> gives sufficiently low variance (tested on Linux; the old benchmark used 20).
> 1000 would be nice to have, but I'm concerned that runtimes on Android could
get
> very long.
> 
> Please add command line arguments --rasterize-repeat and --record-repeat to
> adjust those values.

Done.

[enne (OOO), 2013-11-14 19:44:58]

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.cc (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.cc:109: SkTileGridPicture::TileGridInfo
tile_grid_info;
On 2013/11/14 19:38:24, vmpstr wrote:
> On 2013/11/14 00:24:05, enne wrote:
> > Maybe add this as a static accessor on cc::Picture?
> 
> Unfortunately we don't store tile_grid_info on the picture. We only have it on
a
> picture pile base, but I kind of feel that it's not really appropriate to get
it
> from there, since this is a test for a picture record and really has nothing
to
> do with a picture pile.

They could both get it from a static accessor on cc::Picture?

[vmpstr, 2013-11-14 20:04:35]

PTAL

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
File cc/debug/rasterize_and_record_benchmark.cc (right):

https://codereview.chromium.org/66213007/diff/70001/cc/debug/rasterize_and_re...
cc/debug/rasterize_and_record_benchmark.cc:109: SkTileGridPicture::TileGridInfo
tile_grid_info;
On 2013/11/14 19:44:59, enne wrote:
> On 2013/11/14 19:38:24, vmpstr wrote:
> > On 2013/11/14 00:24:05, enne wrote:
> > > Maybe add this as a static accessor on cc::Picture?
> > 
> > Unfortunately we don't store tile_grid_info on the picture. We only have it
on
> a
> > picture pile base, but I kind of feel that it's not really appropriate to
get
> it
> > from there, since this is a test for a picture record and really has nothing
> to
> > do with a picture pile.
> 
> They could both get it from a static accessor on cc::Picture?

Done. (added it to PicturePileBase since that's where the source of the
calculation is, but I can move it as well)

[enne (OOO), 2013-11-14 20:14:18]

Thanks for that cleanup.  :)

[ernstm, 2013-11-14 21:06:52]

lgtm

[Dominik Grewe, 2013-11-15 18:13:25]

Sorry for just jumping in with a random question:
In the current rasterize_and_record benchmark we call
chrome.gpuBenchmarking.setRasterizeOnlyVisibleContent() to restrict rendering to
the visible content. Do we need to do the same here or is this somehow implicit?

[vmpstr, 2013-11-15 18:15:34]

On 2013/11/15 18:13:25, Dominik Grewe wrote:
> Sorry for just jumping in with a random question:
> In the current rasterize_and_record benchmark we call
> chrome.gpuBenchmarking.setRasterizeOnlyVisibleContent() to restrict rendering
to
> the visible content. Do we need to do the same here or is this somehow
implicit?

That's a part of the rasterize_and_record_benchmark.cc. We only rasterize items
in the visible_content_rect. Since we don't rely on the raster worker pool or
any "normal" operations, this should be equivalent

[Dominik Grewe, 2013-11-18 14:46:09]

On 2013/11/15 18:15:34, vmpstr wrote:
> On 2013/11/15 18:13:25, Dominik Grewe wrote:
> > Sorry for just jumping in with a random question:
> > In the current rasterize_and_record benchmark we call
> > chrome.gpuBenchmarking.setRasterizeOnlyVisibleContent() to restrict
rendering
> to
> > the visible content. Do we need to do the same here or is this somehow
> implicit?
> 
> That's a part of the rasterize_and_record_benchmark.cc. We only rasterize
items
> in the visible_content_rect. Since we don't rely on the raster worker pool or
> any "normal" operations, this should be equivalent

Cool, thanks!

[commit-bot: I haz the power, 2013-11-18 17:55:27]

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

[commit-bot: I haz the power, 2013-11-18 20:32:17]

Change committed as 235805