Adding hooks for gathering total pixels painted and rasterized stats.

Adding hooks for gathering total pixels painted and rasterized stats.

Blocked on: https://bugs.webkit.org/show_bug.cgi?id=98269

BUG=156087
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166911

[hartmanng, 2012-10-03 15:09:23]

Please take a look.

[jamesr1, 2012-10-04 02:09:01]

If you put the paint metrics in ContentLayerChromium you would only have to
instrument one point and would match our histograms.

[brianderson, 2012-10-05 23:51:55]

https://codereview.chromium.org/10982078/diff/2001/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://codereview.chromium.org/10982078/diff/2001/cc/CCRenderingStats.h#newc...
cc/CCRenderingStats.h:19: size_t totalPixelsPainted;
Can you change these to int64_t, so we won't need a follow up patch? Please see
https://bugs.webkit.org/show_bug.cgi?id=98337 and
https://codereview.chromium.org/11028021.

[hartmanng, 2012-10-16 17:05:46]

On 2012/10/04 02:09:01, jamesr1 wrote:
> If you put the paint metrics in ContentLayerChromium you would only have to
> instrument one point and would match our histograms.

Sorry for the delay on this.

I took out the paint metrics from the 2nd point in
cc/bitmap_skpicture_canvas_layer_texture_updater.cc, it seems like that was
double-counting anyway. I looked into moving it into cc/content_layer.cc to make
it consistent with the histograms, but it seems like that wouldn't account for
the painted pixels in the scrollbar (which we want) unless I also plumb it into
cc/scrollbar_layer.cc, so I wasn't sure if you'd still like that approach.

PTAL

[danakj, 2012-10-16 17:20:07]

https://chromiumcodereview.appspot.com/10982078/diff/15001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/15001/cc/rendering_stats...
cc/rendering_stats.cc:20: }
style nit: }<space><space>// namespace cc

[hartmanng, 2012-10-24 21:07:00]

This patch had become a bit stale, so I rebased it, please take another look.

https://chromiumcodereview.appspot.com/10982078/diff/2001/cc/CCRenderingStats.h
File cc/CCRenderingStats.h (right):

https://chromiumcodereview.appspot.com/10982078/diff/2001/cc/CCRenderingStats...
cc/CCRenderingStats.h:19: size_t totalPixelsPainted;
On 2012/10/05 23:51:55, Brian Anderson wrote:
> Can you change these to int64_t, so we won't need a follow up patch? Please
see
> https://bugs.webkit.org/show_bug.cgi?id=98337 and
> https://codereview.chromium.org/11028021.

Done.

https://chromiumcodereview.appspot.com/10982078/diff/15001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/15001/cc/rendering_stats...
cc/rendering_stats.cc:20: }
On 2012/10/16 17:20:08, danakj wrote:
> style nit: }<space><space>// namespace cc

Done.

[jamesr, 2012-10-29 23:38:48]

I think you should ask someone in your office to help guide you through the
basic patch process to make sure you've got basic style things like indentation,
copyright headers, types correct before asking for a remote review.

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:1: #include "config.h"
you need a copyright header

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:2: #include "cc/rendering_stats.h"
newline before this #include, please

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:8: , numFramesSentToScreen(0)
bad indentation - if you want WebKit style, line the comma up with the colon

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:18: {
bad indentation

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats.h
File cc/rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.h:19: int64_t totalPixelsPainted;
why is this int64_t instead of size_t?  Either use size_t to be consistent with
the rest of the struct or use int64 from base/basictypes.h to be consistent with
the rest of the chromium codebase

[Ian Vollick, 2012-10-30 19:58:36]

Thanks for switching everything to int64. I have just a few nits.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:22: , numMainThreadScrolls(0)
This is correct WebKit style, but I'd go for chromium style since we'll have to
eventually, anyhow. The text of the initializers should all line up as you have
them, but commas go at the end of the preceding line.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:24: }
I think chromium style would have the curlies on the same line separated by a
space.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats.h
File cc/rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.h:8: #include <base/basictypes.h>
Please use quotes instead of angle brackets whenever you include a chromium
header.

[danakj, 2012-10-30 20:01:25]

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:24: }
On 2012/10/30 19:58:37, vollick wrote:
> I think chromium style would have the curlies on the same line separated by a
> space.
The leading curly is on the last line of the initializers, the following is on
its own line.

[hartmanng, 2012-10-30 20:10:51]

jamesr: I think this is better, PTAL

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:1: #include "config.h"
On 2012/10/29 23:38:49, jamesr wrote:
> you need a copyright header

Done.

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:2: #include "cc/rendering_stats.h"
On 2012/10/29 23:38:49, jamesr wrote:
> newline before this #include, please

Done.

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:8: , numFramesSentToScreen(0)
On 2012/10/29 23:38:49, jamesr wrote:
> bad indentation - if you want WebKit style, line the comma up with the colon

Done.

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.cc:18: {
On 2012/10/29 23:38:49, jamesr wrote:
> bad indentation

Done.

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats.h
File cc/rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10982078/diff/23001/cc/rendering_stats...
cc/rendering_stats.h:19: int64_t totalPixelsPainted;
On 2012/10/29 23:38:49, jamesr wrote:
> why is this int64_t instead of size_t?  Either use size_t to be consistent
with
> the rest of the struct or use int64 from base/basictypes.h to be consistent
with
> the rest of the chromium codebase

brianderson@ requested that I switch it to 64-bit values, presumably to ensure
we can handle very large numbers of pixels if we leave the benchmark running for
a long period of time.

I went ahead and switched all the size_t and int values here to int64 since I
believe brianderson@ plans to anyway.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:22: , numMainThreadScrolls(0)
On 2012/10/30 19:58:37, vollick wrote:
> This is correct WebKit style, but I'd go for chromium style since we'll have
to
> eventually, anyhow. The text of the initializers should all line up as you
have
> them, but commas go at the end of the preceding line.

Done.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:24: }
On 2012/10/30 19:58:37, vollick wrote:
> I think chromium style would have the curlies on the same line separated by a
> space.

Done.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.cc:24: }
On 2012/10/30 20:01:25, danakj wrote:
> On 2012/10/30 19:58:37, vollick wrote:
> > I think chromium style would have the curlies on the same line separated by
a
> > space.
> The leading curly is on the last line of the initializers, the following is on
> its own line.

Done.

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats.h
File cc/rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10982078/diff/42002/cc/rendering_stats...
cc/rendering_stats.h:8: #include <base/basictypes.h>
On 2012/10/30 19:58:37, vollick wrote:
> Please use quotes instead of angle brackets whenever you include a chromium
> header.

Done.

[jamesr, 2012-10-30 21:40:44]

You've left out the framebuffer-backed SkPicture path - there pixels are
GPU-rasterized, so I'm not sure where they fit in this categorization.  Is that
intentional?

[hartmanng, 2012-10-31 16:56:07]

On 2012/10/30 21:40:44, jamesr wrote:
> You've left out the framebuffer-backed SkPicture path - there pixels are
> GPU-rasterized, so I'm not sure where they fit in this categorization.  Is
that
> intentional?

After discussing with danakj, I believe this is what we want. It looks like the
content_layer_updater changes record the painted pixels, but the GPU-rasterizing
isn't quite what we're looking for. It may be useful to separately record GL
operations or something like that, but I'd have to discuss that with nduca, so
maybe we can leave that for a separate issue.

[nduca, 2012-11-06 04:18:23]

I'm fine ignoring the framebuffer path. Its not long for this world anyway.

http://codereview.chromium.org/10982078/diff/35002/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10982078/diff/35002/content/renderer/render_wi...
content/renderer/render_widget.cc:1891: stats.totalPixelsRasterized +=
software_stats_.totalPixelsRasterized;
are you tracking software mode rasterization time and pixel count on
software_stats? E.g. time spent in RenderWidget::PaintRect?

[nduca, 2012-11-06 21:30:53]

That looks about right

https://codereview.chromium.org/10982078/diff/56001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10982078/diff/56001/content/renderer/render_w...
content/renderer/render_widget.cc:670: base::TimeTicks rasterize_begin_ticks =
base::TimeTicks::Now();
HighResNow

[hartmanng, 2012-11-06 21:53:16]

Please take another look, jamesr

https://codereview.chromium.org/10982078/diff/35002/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10982078/diff/35002/content/renderer/render_w...
content/renderer/render_widget.cc:1891: stats.totalPixelsRasterized +=
software_stats_.totalPixelsRasterized;
On 2012/11/06 04:18:23, nduca wrote:
> are you tracking software mode rasterization time and pixel count on
> software_stats? E.g. time spent in RenderWidget::PaintRect?

I am now.

https://codereview.chromium.org/10982078/diff/56001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10982078/diff/56001/content/renderer/render_w...
content/renderer/render_widget.cc:670: base::TimeTicks rasterize_begin_ticks =
base::TimeTicks::Now();
On 2012/11/06 21:30:53, nduca wrote:
> HighResNow

Done here and for the paint version of this.

[nduca, 2012-11-07 04:24:42]

lgtm but you'll need OWNERS on content/renderer

http://codereview.chromium.org/10982078/diff/60001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10982078/diff/60001/content/renderer/render_wi...
content/renderer/render_widget.cc:669: 
I wouldn't bother with static bool. Just check it directly, e.g. if
(CommandLineHasBlahBlah)

[jamesr, 2012-11-07 06:11:20]

going to HighResNow() on these means taking a mutex (on each call) on systems
without QPC (shitty old windows boxes probably). Sure we're OK with that?

http://codereview.chromium.org/10982078/diff/60001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

http://codereview.chromium.org/10982078/diff/60001/cc/rendering_stats.cc#newc...
cc/rendering_stats.cc:5: #include "config.h"
you don't need this, remove. you only need this if you're including
(transitively) headers from WTF which comes in from some WebKit headers

[nduca, 2012-11-07 07:42:58]

We should make sure the HighResNow is only called with the benchmarking
extension present. Looks like we haven't quite caught all the cases there.
Glenn, can you verify?

[hartmanng, 2012-11-07 14:45:02]

On 2012/11/07 07:42:58, nduca wrote:
> We should make sure the HighResNow is only called with the benchmarking
> extension present. Looks like we haven't quite caught all the cases there.
> Glenn, can you verify?

fixed, PTAL.

http://codereview.chromium.org/10982078/diff/60001/cc/rendering_stats.cc
File cc/rendering_stats.cc (right):

http://codereview.chromium.org/10982078/diff/60001/cc/rendering_stats.cc#newc...
cc/rendering_stats.cc:5: #include "config.h"
On 2012/11/07 06:11:20, jamesr wrote:
> you don't need this, remove. you only need this if you're including
> (transitively) headers from WTF which comes in from some WebKit headers

Done.

http://codereview.chromium.org/10982078/diff/60001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10982078/diff/60001/content/renderer/render_wi...
content/renderer/render_widget.cc:669: 
On 2012/11/07 04:24:42, nduca wrote:
> I wouldn't bother with static bool. Just check it directly, e.g. if
> (CommandLineHasBlahBlah)

Done.

[jamesr, 2012-11-08 23:33:12]

lgtm.  Have one style suggestion, but you don't need to go another review round.

http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
content/renderer/render_widget.cc:738: switches::kEnableGpuBenchmarking))
we're repeating this a lot - can we stash this in a bool just for readability?

[commit-bot: I haz the power, 2012-11-09 14:05:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/10982078/60002

[hartmanng, 2012-11-09 14:07:10]

On 2012/11/08 23:33:12, jamesr wrote:
> lgtm.  Have one style suggestion, but you don't need to go another review
round.
> 
>
http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
> File content/renderer/render_widget.cc (right):
> 
>
http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
> content/renderer/render_widget.cc:738: switches::kEnableGpuBenchmarking))
> we're repeating this a lot - can we stash this in a bool just for readability?


Thanks!

http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10982078/diff/53002/content/renderer/render_wi...
content/renderer/render_widget.cc:738: switches::kEnableGpuBenchmarking))
On 2012/11/08 23:33:12, jamesr wrote:
> we're repeating this a lot - can we stash this in a bool just for readability?

Done.

[commit-bot: I haz the power, 2012-11-09 15:58:46]

Change committed as 166911