cc: Reduce command buffer flushes related to creating texture ids.

cc: Reduce command buffer flushes related to creating texture ids.

This significantly reduces command buffer flushes by allocating
texture ids in chunks of 64.

BUG=309455
TEST=cc_unittests --gtest_filter=ResourceProviderTest.TextureAllocationChunk

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

[reveman, 2013-10-28 22:47:38]

This needs some more work for all unit tests to pass but I'd like some feedback
on the approach before I go ahead and fix that.

I like a simple approach like this patch as it should work well independent of
how we allocate and use textures. An alternative is to allocate whatever texture
ids that can be predicted based on our lazy texture create system but that
depends on a specific type of usage to work well.

[piman, 2013-10-28 23:08:58]

On 2013/10/28 22:47:38, David Reveman wrote:
> This needs some more work for all unit tests to pass but I'd like some
feedback
> on the approach before I go ahead and fix that.
> 
> I like a simple approach like this patch as it should work well independent of
> how we allocate and use textures. An alternative is to allocate whatever
texture
> ids that can be predicted based on our lazy texture create system but that
> depends on a specific type of usage to work well.

I've heard vague suggestions that on some android drivers creating texture ids
was "costly" (for some vague notion of costly), which is part of the reason we
hold on to our textures in the various managers. I haven't seen hard data on it,
but if that's true, we may want to be tighter with the number of allocations.
Otherwise, this approach seems ok.

Either way, it'd be nice to fix other uses of createTexture in ResourceProvider,
I'm thinking in particular ResourceProvider::ReceiveFromChild because we're
likely to allocate many textures in this case.

[reveman, 2013-10-28 23:23:32]

On 2013/10/28 23:08:58, piman wrote:
> On 2013/10/28 22:47:38, David Reveman wrote:
> > This needs some more work for all unit tests to pass but I'd like some
> feedback
> > on the approach before I go ahead and fix that.
> > 
> > I like a simple approach like this patch as it should work well independent
of
> > how we allocate and use textures. An alternative is to allocate whatever
> texture
> > ids that can be predicted based on our lazy texture create system but that
> > depends on a specific type of usage to work well.
> 
> I've heard vague suggestions that on some android drivers creating texture ids
> was "costly" (for some vague notion of costly), which is part of the reason we
> hold on to our textures in the various managers. I haven't seen hard data on
it,
> but if that's true, we may want to be tighter with the number of allocations.
> Otherwise, this approach seems ok.

Ok, that sounds like something we might want to address using some lazy texture
creation on the service side. For now, how about we expose this texture id
allocation chunk size using a setter on the RP and a layer tree setting? That
way we can also unit test this separately and avoid changes to existing unit
tests by using a chunk size of 1.

> 
> Either way, it'd be nice to fix other uses of createTexture in
ResourceProvider,
> I'm thinking in particular ResourceProvider::ReceiveFromChild because we're
> likely to allocate many textures in this case.

Oh, missed that. Will be fixed in next patch.

[piman, 2013-10-29 01:15:24]

On 2013/10/28 23:23:32, David Reveman wrote:
> On 2013/10/28 23:08:58, piman wrote:
> > On 2013/10/28 22:47:38, David Reveman wrote:
> > > This needs some more work for all unit tests to pass but I'd like some
> > feedback
> > > on the approach before I go ahead and fix that.
> > > 
> > > I like a simple approach like this patch as it should work well
independent
> of
> > > how we allocate and use textures. An alternative is to allocate whatever
> > texture
> > > ids that can be predicted based on our lazy texture create system but that
> > > depends on a specific type of usage to work well.
> > 
> > I've heard vague suggestions that on some android drivers creating texture
ids
> > was "costly" (for some vague notion of costly), which is part of the reason
we
> > hold on to our textures in the various managers. I haven't seen hard data on
> it,
> > but if that's true, we may want to be tighter with the number of
allocations.
> > Otherwise, this approach seems ok.
> 
> Ok, that sounds like something we might want to address using some lazy
texture
> creation on the service side. For now, how about we expose this texture id
> allocation chunk size using a setter on the RP and a layer tree setting? That
> way we can also unit test this separately and avoid changes to existing unit
> tests by using a chunk size of 1.

SGTM.

> 
> > 
> > Either way, it'd be nice to fix other uses of createTexture in
> ResourceProvider,
> > I'm thinking in particular ResourceProvider::ReceiveFromChild because we're
> > likely to allocate many textures in this case.
> 
> Oh, missed that. Will be fixed in next patch.

[reveman, 2013-10-30 00:01:19]

This is ready for another review. I kept the default chunk size at 64, which
might be a bit aggressive but I'd like to give that a try. PTAL.

[piman, 2013-10-31 01:32:57]

LGTM overall but a couple of questions/nits.

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1757: unused_texture_ids_.pop_front();
nit: why a deque rather than a vector? You can take from the end and still not
incur reallocs/copies. deque is slightly bigger.

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
cc/resources/resource_provider_unittest.cc:2391: MOCK_METHOD0(NextTextureId,
WebGLId());
Why are we mocking the internals of the TestWebGraphicsContext3D rather than the
API we're calling from ResourceProvider? It seems more fragile.

[reveman, 2013-10-31 13:38:31]

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1757: unused_texture_ids_.pop_front();
On 2013/10/31 01:32:58, piman wrote:
> nit: why a deque rather than a vector? You can take from the end and still not
> incur reallocs/copies. deque is slightly bigger.

I think we should give out ids in the order they are generated as that might be
useful for debugging purposes and we might even have some unit test code that
rely on it. ie. if you create one resource it's given ids[0] not ids[63]. We can
of course accomplish that while using std::vector but we have to replace the use
of .assign(ids..) above with a loop. I prefer using a deque for that reason but
don't feel that strongly about it.

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_pro...
cc/resources/resource_provider_unittest.cc:2391: MOCK_METHOD0(NextTextureId,
WebGLId());
On 2013/10/31 01:32:58, piman wrote:
> Why are we mocking the internals of the TestWebGraphicsContext3D rather than
the
> API we're calling from ResourceProvider? It seems more fragile.

I could override genTextures/deleteTextures/deleteTexture instead but it seemed
a bit more complicated as I don't necessarily care about the order of ids or if
deleteTexture vs deleteTextures is used.

[piman, 2013-10-31 17:54:37]

Ok, LGTM as is if you prefer this way.


On Thu, Oct 31, 2013 at 6:38 AM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/49163004/diff/70001/cc/**
>
resources/resource_provider.cc<https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_provider.cc>
> File cc/resources/resource_**provider.cc (right):
>
> https://codereview.chromium.**org/49163004/diff/70001/cc/**
>
resources/resource_provider.**cc#newcode1757<https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_provider.cc#newcode1757>
> cc/resources/resource_**provider.cc:1757: unused_texture_ids_.pop_front(**
> );
> On 2013/10/31 01:32:58, piman wrote:
>
>> nit: why a deque rather than a vector? You can take from the end and
>>
> still not
>
>> incur reallocs/copies. deque is slightly bigger.
>>
>
> I think we should give out ids in the order they are generated as that
> might be useful for debugging purposes and we might even have some unit
> test code that rely on it. ie. if you create one resource it's given
> ids[0] not ids[63]. We can of course accomplish that while using
> std::vector but we have to replace the use of .assign(ids..) above with
> a loop. I prefer using a deque for that reason but don't feel that
> strongly about it.
>
>
> https://codereview.chromium.**org/49163004/diff/70001/cc/**
>
resources/resource_provider_**unittest.cc<https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_provider_unittest.cc>
> File cc/resources/resource_**provider_unittest.cc (right):
>
> https://codereview.chromium.**org/49163004/diff/70001/cc/**
>
resources/resource_provider_**unittest.cc#newcode2391<https://codereview.chromium.org/49163004/diff/70001/cc/resources/resource_provider_unittest.cc#newcode2391>
> cc/resources/resource_**provider_unittest.cc:2391:
> MOCK_METHOD0(NextTextureId, WebGLId());
> On 2013/10/31 01:32:58, piman wrote:
>
>> Why are we mocking the internals of the TestWebGraphicsContext3D
>>
> rather than the
>
>> API we're calling from ResourceProvider? It seems more fragile.
>>
>
> I could override genTextures/deleteTextures/**deleteTexture instead but it
> seemed a bit more complicated as I don't necessarily care about the
> order of ids or if deleteTexture vs deleteTextures is used.
>
>
https://codereview.chromium.**org/49163004/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-31 18:24:37]

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

[commit-bot: I haz the power, 2013-10-31 18:24:45]

Failed to apply patch for cc/debug/test_web_graphics_context_3d.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: cc/debug/test_web_graphics_context_3d.cc
  |diff --git a/cc/debug/test_web_graphics_context_3d.cc
b/cc/debug/test_web_graphics_context_3d.cc
  |index
74a6abc06e6ea661104cb36b94e7ab95931eea2f..67561a0d23e46fe403239353257e2cf65bb0ef17
100644
  |--- a/cc/debug/test_web_graphics_context_3d.cc
  |+++ b/cc/debug/test_web_graphics_context_3d.cc
  --------------------------
  No file to patch.  Skipping patch.
  5 out of 5 hunks ignored

Patch:       cc/debug/test_web_graphics_context_3d.cc
Index: cc/debug/test_web_graphics_context_3d.cc
diff --git a/cc/debug/test_web_graphics_context_3d.cc
b/cc/debug/test_web_graphics_context_3d.cc
index
74a6abc06e6ea661104cb36b94e7ab95931eea2f..67561a0d23e46fe403239353257e2cf65bb0ef17
100644
--- a/cc/debug/test_web_graphics_context_3d.cc
+++ b/cc/debug/test_web_graphics_context_3d.cc
@@ -216,13 +216,8 @@ void TestWebGraphicsContext3D::genTextures(WGC3Dsizei
count, WebGLId* ids) {
 }
 
 void TestWebGraphicsContext3D::deleteBuffers(WGC3Dsizei count, WebGLId* ids) {
-  base::AutoLock lock(namespace_->lock);
-  for (int i = 0; i < count; ++i) {
-    unsigned context_id = ids[i] >> 17;
-    unsigned buffer_id = ids[i] & 0x1ffff;
-    DCHECK(buffer_id && buffer_id < namespace_->next_buffer_id);
-    DCHECK_EQ(context_id, context_id_);
-  }
+  for (int i = 0; i < count; ++i)
+    RetireBufferId(ids[i]);
 }
 
 void TestWebGraphicsContext3D::deleteFramebuffers(
@@ -238,6 +233,8 @@ void TestWebGraphicsContext3D::deleteRenderbuffers(
 }
 
 void TestWebGraphicsContext3D::deleteTextures(WGC3Dsizei count, WebGLId* ids) {
+  for (int i = 0; i < count; ++i)
+    RetireTextureId(ids[i]);
   base::AutoLock lock(namespace_->lock);
   for (int i = 0; i < count; ++i) {
     namespace_->textures.Remove(ids[i]);
@@ -471,10 +468,11 @@ void
TestWebGraphicsContext3D::bindBuffer(WebKit::WGC3Denum target,
   bound_buffer_ = buffer;
   if (!bound_buffer_)
     return;
-  unsigned context_id = buffer >> 17;
-  unsigned buffer_id = buffer & 0x1ffff;
+  unsigned context_id = buffer >> 16;
+  unsigned buffer_id = buffer & 0xffff;
   base::AutoLock lock(namespace_->lock);
-  DCHECK(buffer_id && buffer_id < namespace_->next_buffer_id);
+  DCHECK(buffer_id);
+  DCHECK_LT(buffer_id, namespace_->next_buffer_id);
   DCHECK_EQ(context_id, context_id_);
 
   base::ScopedPtrHashMap<unsigned, Buffer>& buffers = namespace_->buffers;
@@ -543,11 +541,7 @@ WebKit::WGC3Duint
TestWebGraphicsContext3D::createImageCHROMIUM(
 
 void TestWebGraphicsContext3D::destroyImageCHROMIUM(
     WebKit::WGC3Duint id) {
-  base::AutoLock lock(namespace_->lock);
-  unsigned context_id = id >> 17;
-  unsigned image_id = id & 0x1ffff;
-  DCHECK(image_id && image_id < namespace_->next_image_id);
-  DCHECK_EQ(context_id, context_id_);
+  RetireImageId(id);
 }
 
 void TestWebGraphicsContext3D::getImageParameterivCHROMIUM(
@@ -598,22 +592,49 @@ WebGLId TestWebGraphicsContext3D::NextTextureId() {
   return texture_id;
 }
 
+void TestWebGraphicsContext3D::RetireTextureId(WebGLId id) {
+  base::AutoLock lock(namespace_->lock);
+  unsigned context_id = id >> 16;
+  unsigned texture_id = id & 0xffff;
+  DCHECK(texture_id);
+  DCHECK_LT(texture_id, namespace_->next_texture_id);
+  DCHECK_EQ(context_id, context_id_);
+}
+
 WebGLId TestWebGraphicsContext3D::NextBufferId() {
   base::AutoLock lock(namespace_->lock);
   WebGLId buffer_id = namespace_->next_buffer_id++;
-  DCHECK(buffer_id < (1 << 17));
-  buffer_id |= context_id_ << 17;
+  DCHECK(buffer_id < (1 << 16));
+  buffer_id |= context_id_ << 16;
   return buffer_id;
 }
 
+void TestWebGraphicsContext3D::RetireBufferId(WebGLId id) {
+  base::AutoLock lock(namespace_->lock);
+  unsigned context_id = id >> 16;
+  unsigned buffer_id = id & 0xffff;
+  DCHECK(buffer_id);
+  DCHECK_LT(buffer_id, namespace_->next_buffer_id);
+  DCHECK_EQ(context_id, context_id_);
+}
+
 WebKit::WGC3Duint TestWebGraphicsContext3D::NextImageId() {
   base::AutoLock lock(namespace_->lock);
   WGC3Duint image_id = namespace_->next_image_id++;
-  DCHECK(image_id < (1 << 17));
-  image_id |= context_id_ << 17;
+  DCHECK(image_id < (1 << 16));
+  image_id |= context_id_ << 16;
   return image_id;
 }
 
+void TestWebGraphicsContext3D::RetireImageId(WebGLId id) {
+  base::AutoLock lock(namespace_->lock);
+  unsigned context_id = id >> 16;
+  unsigned image_id = id & 0xffff;
+  DCHECK(image_id);
+  DCHECK_LT(image_id, namespace_->next_image_id);
+  DCHECK_EQ(context_id, context_id_);
+}
+
 size_t TestWebGraphicsContext3D::GetTransferBufferMemoryUsedBytes() const {
   size_t total_bytes = 0;
   base::ScopedPtrHashMap<unsigned, Buffer>& buffers = namespace_->buffers;

[jamesr, 2013-10-31 18:26:58]

On 2013/10/31 18:24:45, I haz the power (commit-bot) wrote:
> Failed to apply patch for cc/debug/test_web_graphics_context_3d.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   can't find file to patch at input line 6
>   Perhaps you used the wrong -p or --strip option?
>   The text leading up to this was:
>   --------------------------
>   |Index: cc/debug/test_web_graphics_context_3d.cc
>   |diff --git a/cc/debug/test_web_graphics_context_3d.cc
> b/cc/debug/test_web_graphics_context_3d.cc
>   |index
>
74a6abc06e6ea661104cb36b94e7ab95931eea2f..67561a0d23e46fe403239353257e2cf65bb0ef17
> 100644
>   |--- a/cc/debug/test_web_graphics_context_3d.cc
>   |+++ b/cc/debug/test_web_graphics_context_3d.cc
>   --------------------------

This file now lives in cc/test/test_web_graphics_context3d.cc, so you'll have to
rebase.

[commit-bot: I haz the power, 2013-10-31 19:52:31]

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

[commit-bot: I haz the power, 2013-10-31 23:31:37]

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

[commit-bot: I haz the power, 2013-11-01 11:52:36]

Change committed as 232332