Report texture upload time in renderingStats.

Report texture upload time in renderingStats.

BUG=133658
TEST=TEST=Launch Chrome with --enable-gpu-benchmarking, open a tab to a composited page, and type chrome.gpuBenchmarking.renderingStats(). textureUploadCount, and totalTextureUploadTimeInSeconds should be present.


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

[reveman, 2012-08-23 18:15:23]

Hi,

how is something like this for measuring texture upload times and such in the
GPU process? It currently collects data from all non-offscreen command buffers
but I guess we should make it only collect data from the compositor command
buffer.

David

[nduca, 2012-08-23 18:18:46]

Is there a way to make a distinction in this api between surface_ids? E.g.
GetRenderingStatsForSurface? Where the stats structure has surface-specific vars
as well as globals...

[reveman, 2012-08-23 18:32:08]

On 2012/08/23 18:18:46, nduca wrote:
> Is there a way to make a distinction in this api between surface_ids? E.g.
> GetRenderingStatsForSurface? Where the stats structure has surface-specific
vars
> as well as globals...

Sure. Would it be ok to make multiple IPC calls get stats for different surfaces
and return global stats when surface_id=0 ?

[nduca, 2012-08-23 18:50:57]

Wouldn't it be simpler to just do a single ipc call with all the details on it?
E.g. getGpuStats(routing_id, surface_id). That could return a mix of global,
stub-specific and surface-specific stats.

[nduca, 2012-08-23 18:51:27]

https://chromiumcodereview.appspot.com/10868048/diff/1/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://chromiumcodereview.appspot.com/10868048/diff/1/content/renderer/rende...
content/renderer/render_widget.cc:1830:
RenderThreadImpl::current()->GetGpuRenderingStats(stats);
Any reason to not do
RenderThreadIMpl::current()->GetGpuChannel()->GetRenderingStatsForSurface(surface_id())?

That'd avoid edits to render_thread

[ccameron, 2012-08-24 09:16:45]

Some interesting stats to gather (for later) would be
- maximum single texture upload time
- memory usage (total in chrome and for-this-decoder's-context-group) at the
time of the maximum texture upload time
- free memory on the system at that time (on platforms that support this... an
incomplete matrix there)
That could help inform what sorts of cliffs we are walking around.

Memory for each ContextGroup is tracked by a GpuCommandBufferMemoryTracker --
it's a bit hidden, but we could make each GpuCommandBufferStub in a ContextGroup
point to a GpuCommandBufferMemoryTracker, which is also pointed to by the
ContextGroup's GpuCommandBufferMemoryTracker.  Some poking around, but no
seismic changes.  Some significant changes would be needed to get per-decoder
memory usage.

Also of note is that this is all driver-CPU-time.  While that can pretty good
measure of GPU time a well, some drivers just stash the glTexImage2D arguments
in an internal staging buffer, and don't actually execute them until much later.
 In practice, the back-pressure eventually hits the API calling thread
eventually, but with some delay.

[reveman, 2012-08-27 20:41:10]

yes, driver CPU time spent in TexSubImage2D can be very misleading. i'm hoping
that these stats will help us determine platform/drivers where these
measurements are useful.

[nduca, 2012-08-28 19:45:10]

Awesome, LGTM with nits! I'd send this over to OWNERS for them to eviscerate
you. :)

The processCommands time is just a request. No worries if you want to do that
later.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/public/comm...
File content/public/common/gpu_rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/public/comm...
content/public/common/gpu_rendering_stats.h:20: float
globalTotalTextureUploadTimeInSeconds;
Can we use base::TimeTicks instead and convert to seconds at v8 time?

https://chromiumcodereview.appspot.com/10868048/diff/6002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2876: 
while we're at it, can we add tracking for "time spent processing commands"?

[apatrick_chromium, 2012-08-28 20:07:06]

Just nits.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
content/common/gpu/client/gpu_channel_host.cc:227: if (!Send(new
GpuChannelMsg_CollectRenderingStats(surface_id, &stats))) {
No need for braces here.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
File content/common/gpu/client/gpu_channel_host.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
content/common/gpu/client/gpu_channel_host.h:140: int surface_id,
content::GpuRenderingStats& stats);
content::GpuRenderingStats* so it's clear this is an output.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
File content/common/gpu/gpu_channel.cc (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
content/common/gpu/gpu_channel.cc:546: #if defined(ENABLE_GPU)
you don't need to check ENABLE_GPU is defined any more. we're in the process of
getting rid of it. it's always defined.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
content/common/gpu/gpu_channel.cc:549: int textureUploadCount =
Google style is texture_upload_count. ditto all the other camel case variables.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/common/gpu/...
content/common/gpu/gpu_messages.h:397:
IPC_SYNC_MESSAGE_CONTROL1_1(GpuChannelMsg_CollectRenderingStats,
Can you put these with the other GpuChannelMsg messages?

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/public/comm...
File content/public/common/gpu_rendering_stats.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/public/comm...
content/public/common/gpu_rendering_stats.h:19: int globalTextureUploadCount;
Google style is global_texture_upload_count.

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/renderer/re...
File content/renderer/render_widget.cc (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/renderer/re...
content/renderer/render_widget.cc:1820: void
RenderWidget::GetRenderingStats(WebKit::WebRenderingStats& stats) const {
WebKit::WebRenderingStats*

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/renderer/re...
content/renderer/render_widget.cc:1828: content::GpuRenderingStats& stats) const
{
Ditto

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/renderer/re...
File content/renderer/render_widget.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/content/renderer/re...
content/renderer/render_widget.h:169: void
GetGpuRenderingStats(content::GpuRenderingStats&) const;
content::GpuRenderingStats* so it is clear it is an output.

https://chromiumcodereview.appspot.com/10868048/diff/6002/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://chromiumcodereview.appspot.com/10868048/diff/6002/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_decoder.h:177: virtual double
GetTotalTextureUploadTimeInSeconds() = 0;
double could be base::TimeDelta

[apatrick_chromium, 2012-08-30 22:20:57]

lgtm

[piman, 2012-08-30 22:44:11]

http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
File content/common/gpu/client/gpu_channel_host.cc (right):

http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
content/common/gpu/client/gpu_channel_host.cc:218: int surface_id,
content::GpuRenderingStats* stats) {
At a high level, why restricting here to "view" command buffers (which have a
non-null surface_id)?
It seems that this would be useful in general for any command buffer (including
"offscreen" ones), by passing their routing id instead.

http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
content/common/gpu/client/gpu_channel_host.cc:223: if (!channel_.get())
Why this? I think it's racy if called on a non-main thread. In case of lost
channel, the Send below will fail anyway.
I would very much like to avoid holding a lock while sending a sync message
(that can be very slow since it's effectively a command buffer finish).

I think we can simply get rid of the autolock and this test and we'll have the
same behavior, correct?

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

http://codereview.chromium.org/10868048/diff/15002/content/renderer/render_wi...
content/renderer/render_widget.cc:1830: surface_id(), stats);
Beware, GetGpuChannel() can be NULL if we haven't created a context yet, or if
we're in the process of recovering from a lost channel.

[reveman, 2012-09-04 21:53:12]

On 2012/08/30 22:44:11, piman wrote:
>
http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
> File content/common/gpu/client/gpu_channel_host.cc (right):
> 
>
http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
> content/common/gpu/client/gpu_channel_host.cc:218: int surface_id,
> content::GpuRenderingStats* stats) {
> At a high level, why restricting here to "view" command buffers (which have a
> non-null surface_id)?
> It seems that this would be useful in general for any command buffer
(including
> "offscreen" ones), by passing their routing id instead.

good point. changed to route_id in latest patch.

> 
>
http://codereview.chromium.org/10868048/diff/15002/content/common/gpu/client/...
> content/common/gpu/client/gpu_channel_host.cc:223: if (!channel_.get())
> Why this? I think it's racy if called on a non-main thread. In case of lost
> channel, the Send below will fail anyway.
> I would very much like to avoid holding a lock while sending a sync message
> (that can be very slow since it's effectively a command buffer finish).

This was really just to be consistent with other functions in this file such as
CreateOffscreenCommandBuffer.

> 
> I think we can simply get rid of the autolock and this test and we'll have the
> same behavior, correct?

changed this latest patch. seems to be working fine.

> 
>
http://codereview.chromium.org/10868048/diff/15002/content/renderer/render_wi...
> File content/renderer/render_widget.cc (right):
> 
>
http://codereview.chromium.org/10868048/diff/15002/content/renderer/render_wi...
> content/renderer/render_widget.cc:1830: surface_id(), stats);
> Beware, GetGpuChannel() can be NULL if we haven't created a context yet, or if
> we're in the process of recovering from a lost channel.

Ok, added a NULL check in latest patch.

Thanks for the review.

[piman, 2012-09-04 22:05:24]

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

http://codereview.chromium.org/10868048/diff/21002/content/renderer/render_wi...
content/renderer/render_widget.cc:1839: return
gpu_channel->CollectRenderingStats(routing_id(), stats);
oh, mmh, this routing_id() is the routing id of the RenderWidget (i.e. the index
of the RW in the RenderThread to route messages from the browser to the
renderer), not the routing id of the context (i.e. the index of the context in
the GpuChannel, to route messages from the renderer to the GPU process).

[reveman, 2012-09-04 23:04:01]

On 2012/09/04 22:05:24, piman wrote:
>
http://codereview.chromium.org/10868048/diff/21002/content/renderer/render_wi...
> File content/renderer/render_widget.cc (right):
> 
>
http://codereview.chromium.org/10868048/diff/21002/content/renderer/render_wi...
> content/renderer/render_widget.cc:1839: return
> gpu_channel->CollectRenderingStats(routing_id(), stats);
> oh, mmh, this routing_id() is the routing id of the RenderWidget (i.e. the
index
> of the RW in the RenderThread to route messages from the browser to the
> renderer), not the routing id of the context (i.e. the index of the context in
> the GpuChannel, to route messages from the renderer to the GPU process).

Oops, that's obviously broken.

Am I missing something or do we need to add webkit API to extract the route_id
from the compositor context?

[nduca, 2012-09-04 23:25:37]

One way to do this would be for RenderView::createOutputSurface, which creates
the context on the main thread, to stash the routing id of the created context
on the RenderView as a member variable, then have RenderView expose a
compositor_context_routing_id member, and make the GetCompositorRenderStats api
go via RenderView types instead of RenderWidget.

Or we could just make this be GetRenderStatsForSurfaceID and be done with it. I
think thats fine as long as you name things right.

[piman, 2012-09-05 00:43:00]

On 2012/09/04 23:25:37, nduca wrote:
> One way to do this would be for RenderView::createOutputSurface, which creates
> the context on the main thread, to stash the routing id of the created context
> on the RenderView as a member variable, then have RenderView expose a
> compositor_context_routing_id member, and make the GetCompositorRenderStats
api
> go via RenderView types instead of RenderWidget.
> 
> Or we could just make this be GetRenderStatsForSurfaceID and be done with it.
I
> think thats fine as long as you name things right.

Yeah, agreed, ok. Stashing the route_id for the compositor context is going to
be a bit racy (you can have >1 view context while it's recreating).
GetRenderStatsForSurfaceID could also be a bit racy if it's run while you're
recreating that context (i.e. you may count stats for both, maybe that's what
you want), but maybe less critically so.

[nduca, 2012-09-05 01:00:46]

@piman, would you be cool with a [properly-named] surface-specific api to
sidestep this and get the measurement infrastructure in place, plus a bug to add
a generalized mechanism for route-specific stats in the future?

[piman, 2012-09-05 01:06:08]

On Tue, Sep 4, 2012 at 6:00 PM, <nduca@chromium.org> wrote:

> @piman, would you be cool with a [properly-named] surface-specific api to
> sidestep this and get the measurement infrastructure in place, plus a bug
> to add
> a generalized mechanism for route-specific stats in the future?
>

That's fine for now. The call looks somewhat ad-hoc (get stats for all
context, plus one magic context), but the alternative currently looks like
a lot of work.
But it'd be nice to clean up. Note that in the Übercompostor future, we
won't have a "view" context in the renderer (since we won't have a
compositing surface), so this call won't find the stats for the context
you're interested in.


>
>
http://codereview.chromium.**org/10868048/<http://codereview.chromium.org/108...
>

[nduca, 2012-09-05 01:37:28]

Agreed that we should make it cleaner. @reveman, would you mind filing a cleanup
bug and self-assigning, taking this patch back to the surface_id approach, and
land? That'll allow us to start tracking texture upload perf which will help
unblock the retina-related bug.

[nduca, 2012-09-05 23:03:11]

piman, look good?

[piman, 2012-09-05 23:05:47]

lgtm

[commit-bot: I haz the power, 2012-09-05 23:48:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10868048/24019

[commit-bot: I haz the power, 2012-09-06 02:04:54]

Change committed as 155099

[jam, 2012-09-07 17:14:23]

this change wasn't properly reviewed (see content/public/OWNERS)

this structure doesn't belong in content/public/common. it should be in
content/common.

can you send a cl soon to fix this?

[nduca, 2012-09-07 17:20:50]

How'd the commit bot let this through? Is the OWNERS file new?

[nduca, 2012-09-07 17:21:43]

Oh, there's not a set noparent on that directory. So any content/OWNER can lgtm,
from the point of view of presubmit.

[jam, 2012-09-07 17:27:42]

On Fri, Sep 7, 2012 at 10:21 AM, <nduca@chromium.org> wrote:

> Oh, there's not a set noparent on that directory. So any content/OWNER can
> lgtm,
> from the point of view of presubmit.
>

yeah, it's not enforced through presubmit, just through top level owners.
but this was forgotten.


>
>
https://chromiumcodereview.**appspot.com/10868048/<https://chromiumcodereview...
>

[nduca, 2012-09-07 17:28:51]

Got it! Sorry about that. I'll throw a fix up now.