Shallow flush after calling beginSetPixels

Shallow flush after calling beginSetPixels

This reduces the latency of our uploads.

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

[epenner, 2013-01-10 04:45:49]

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

https://codereview.chromium.org/11830041/diff/1/cc/tile_manager.cc#newcode553
cc/tile_manager.cc:553:
resource_pool_->resource_provider()->shallowFlushIfSupported();
Could this just go straight into the resource provider?

In general I'm starting to think the upload query and flushes should come out of
the resource provider so we could do things in batches rather than do everything
per-upload (query / flush / etc.)  But maybe the overhead is negligible? It
looks significant on traces, but maybe that's just tracing overhead?

[reveman, 2013-01-10 14:46:58]

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

https://codereview.chromium.org/11830041/diff/1/cc/tile_manager.cc#newcode553
cc/tile_manager.cc:553:
resource_pool_->resource_provider()->shallowFlushIfSupported();
On 2013/01/10 04:45:49, epenner wrote:
> Could this just go straight into the resource provider?
> 
> In general I'm starting to think the upload query and flushes should come out
of
> the resource provider so we could do things in batches rather than do
everything
> per-upload (query / flush / etc.)  But maybe the overhead is negligible? It
> looks significant on traces, but maybe that's just tracing overhead?

I think the tile manager is the right place for this. Batches might be the way
to go but I think that should be controlled by the tile manager rather than
being hidden behind the resource provider interface.

[reveman, 2013-01-10 17:17:46]

lgtm. this should improve the current system and we can look at batching later.

[epenner, 2013-01-10 17:34:42]

lgtm

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

https://codereview.chromium.org/11830041/diff/1/cc/tile_manager.cc#newcode553
cc/tile_manager.cc:553:
resource_pool_->resource_provider()->shallowFlushIfSupported();
On 2013/01/10 14:46:58, David Reveman wrote:
> On 2013/01/10 04:45:49, epenner wrote:
> > Could this just go straight into the resource provider?
> > 
> > In general I'm starting to think the upload query and flushes should come
out
> of
> > the resource provider so we could do things in batches rather than do
> everything
> > per-upload (query / flush / etc.)  But maybe the overhead is negligible? It
> > looks significant on traces, but maybe that's just tracing overhead?
> 
> I think the tile manager is the right place for this. Batches might be the way
> to go but I think that should be controlled by the tile manager rather than
> being hidden behind the resource provider interface.

Yeah sorry, this patch is good as is, just thinking out loud on the batching.

But could shallowFlush call not just go in beginSetPixels for now?  Seems like
we could benefit on any beginSetPixels call.

[epenner, 2013-01-10 17:45:42]

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

https://codereview.chromium.org/11830041/diff/1/cc/tile_manager.cc#newcode553
cc/tile_manager.cc:553:
resource_pool_->resource_provider()->shallowFlushIfSupported();
On 2013/01/10 17:34:42, epenner wrote:
> On 2013/01/10 14:46:58, David Reveman wrote:
> > On 2013/01/10 04:45:49, epenner wrote:
> > > Could this just go straight into the resource provider?
> > > 
> > > In general I'm starting to think the upload query and flushes should come
> out
> > of
> > > the resource provider so we could do things in batches rather than do
> > everything
> > > per-upload (query / flush / etc.)  But maybe the overhead is negligible?
It
> > > looks significant on traces, but maybe that's just tracing overhead?
> > 
> > I think the tile manager is the right place for this. Batches might be the
way
> > to go but I think that should be controlled by the tile manager rather than
> > being hidden behind the resource provider interface.
> 
> Yeah sorry, this patch is good as is, just thinking out loud on the batching.
> 
> But could shallowFlush call not just go in beginSetPixels for now?  Seems like
> we could benefit on any beginSetPixels call.

So actually I talked with Dave and this is good as is. The flush can later be
used to kick off batches of uploads, which means it should stay outside of
resource provider.

[nduca, 2013-01-10 20:03:48]

lgtm if it helps. if y'all are happy, I'm just a rubber stamp. :)

[commit-bot: I haz the power, 2013-01-10 20:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830041/1

[commit-bot: I haz the power, 2013-01-10 20:08:15]

Failed to apply patch for cc/tile_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/tile_manager.cc
  Hunk #1 FAILED at 549.
  1 out of 1 hunk FAILED -- saving rejects to file cc/tile_manager.cc.rej

Patch:       cc/tile_manager.cc
Index: cc/tile_manager.cc
diff --git a/cc/tile_manager.cc b/cc/tile_manager.cc
index
58e3b3df2f2f787f24b9b639a47bca1ab8032335..5ad985ec81efc0b8a222304b69d1408d22066bbc
100644
--- a/cc/tile_manager.cc
+++ b/cc/tile_manager.cc
@@ -549,6 +549,8 @@ void TileManager::OnRasterTaskCompleted(
     resource_pool_->resource_provider()->beginSetPixels(resource->id());
     managed_tile_state.resource = resource.Pass();
     tiles_with_pending_set_pixels_.push(tile);
+
+    resource_pool_->resource_provider()->shallowFlushIfSupported();
   } else {
     resource_pool_->resource_provider()->releasePixelBuffer(resource->id());
     resource_pool_->ReleaseResource(resource.Pass());

[commit-bot: I haz the power, 2013-01-11 03:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830041/8001

[commit-bot: I haz the power, 2013-01-11 13:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830041/8001

[commit-bot: I haz the power, 2013-01-11 13:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830041/8001

[commit-bot: I haz the power, 2013-01-11 13:57:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11830041/8001

[commit-bot: I haz the power, 2013-01-11 16:28:17]

Change committed as 176368