Reduce internal Flush() in GL resource glGen/Delete APIs.

Reduce internal Flush() in GL resource glGen/Delete APIs.

Previously command buffer Flush() was inserted in all
glGen/glDelete APIs to preserve glGen/glDelete/glBind
ordering semantics across shared contexts.

This change removes auto Flush() on glGen*, and requires
that users call glFlush or glFinish explicitly to
guarantee ordering semantics.

Further for share groups without bind_generates_resouce
semantic, this change replaces the auto Flush() on
glDelete* with lazy ID collection.

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

[vmiura, 2014-02-12 23:28:11]

PTAL.

[piman, 2014-02-12 23:52:32]

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation_unittest.cc:2529: }
It would be good to add tests for multiple contexts that share.

For example, I think something like:

gl1_->GenTextures(1, &id1);
gl1_->DeleteTextures(1, &id1);
gl2_->Flush();   // Flush gl2, not gl1
gl2_->GenTextures(1, &id2);  // gl1 wasn't flushed, we shouldn't reuse the same
id, whose Delete (or even Gen) hasn't made it to the service side.
EXPECT_NE(id1, id2);

would fail with the current code (see comments I left in share_group.cc), but
should succeed.


It'd be good to test a variety of such cases.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:41: }
We should restore the Flush() here that was removed from the command buffer
generator, for the Pepper case, otherwise it would violate the spec if one
context gens (without flushing) and another context binds the texture id (should
not cause GL_INVALID_OPERATION).

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:75: class StrictIdHandler : public
IdHandler {
nit: at this point, there's little advantage to inherit from IdHandler, rather
inherit from IdHandlerInterface?
That way, IdHandler fields can become private.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:120: DCHECK(id == 0 ||
id_allocator_.InUse(id));
Can we update the DCHECK to take into account the ids in pending_free_ids_?

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:129: if (pending_flush_generation_ !=
flush_generation) {
gl_impl->helper()->flush_generation() is per-command buffer (i.e. per context),
whereas |this|, hence |pending_flush_generation_| is per share group, so it
doesn't really make sense to compare those (it'll do the wrong thing).

I suppose you could keep a per-command buffer {pending_free_ids_,
pending_flush_generation_} struct...

There's another issue which is that you only collect on subsequent Gen/Delete,
which means that, for example, if a command buffer gets destroyed, we won't ever
reclaim the ids that were last deleted.

[vmiura, 2014-02-13 00:20:16]

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:41: }
On 2014/02/12 23:52:34, piman wrote:
> We should restore the Flush() here that was removed from the command buffer
> generator, for the Pepper case, otherwise it would violate the spec if one
> context gens (without flushing) and another context binds the texture id
(should
> not cause GL_INVALID_OPERATION).

OK, but not here since we have to flush after the Gen* command is written in the
command buffer.

I'll add flush back in the command buffer generator, conditional on
bind_generate_resource.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:129: if (pending_flush_generation_ !=
flush_generation) {
On 2014/02/12 23:52:34, piman wrote:
> gl_impl->helper()->flush_generation() is per-command buffer (i.e. per
context),
> whereas |this|, hence |pending_flush_generation_| is per share group, so it
> doesn't really make sense to compare those (it'll do the wrong thing).
> 
> I suppose you could keep a per-command buffer {pending_free_ids_,
> pending_flush_generation_} struct...
> 
> There's another issue which is that you only collect on subsequent Gen/Delete,
> which means that, for example, if a command buffer gets destroyed, we won't
ever
> reclaim the ids that were last deleted.

Great point.

I think this pending free handling can move up into GLES2Implementation.

[piman, 2014-02-13 00:48:52]

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:41: }
On 2014/02/13 00:20:17, vmiura wrote:
> On 2014/02/12 23:52:34, piman wrote:
> > We should restore the Flush() here that was removed from the command buffer
> > generator, for the Pepper case, otherwise it would violate the spec if one
> > context gens (without flushing) and another context binds the texture id
> (should
> > not cause GL_INVALID_OPERATION).
> 
> OK, but not here since we have to flush after the Gen* command is written in
the
> command buffer.
> 
> I'll add flush back in the command buffer generator, conditional on
> bind_generate_resource.

Oh, yeah.

Or be more symmetrical with FreeIds and provide a functor. whichever is easy.

[vmiura, 2014-02-14 00:11:11]

PTAL.  This is getting more chunky, but tried to change as little as poss.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation_unittest.cc:2529: }
On 2014/02/12 23:52:34, piman wrote:
> It would be good to add tests for multiple contexts that share.
> 
> For example, I think something like:
> 
> gl1_->GenTextures(1, &id1);
> gl1_->DeleteTextures(1, &id1);
> gl2_->Flush();   // Flush gl2, not gl1
> gl2_->GenTextures(1, &id2);  // gl1 wasn't flushed, we shouldn't reuse the
same
> id, whose Delete (or even Gen) hasn't made it to the service side.
> EXPECT_NE(id1, id2);
> 
> would fail with the current code (see comments I left in share_group.cc), but
> should succeed.
> 
> 
> It'd be good to test a variety of such cases.

Added.  Please let me know if you think other cases would be useful.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:75: class StrictIdHandler : public
IdHandler {
On 2014/02/12 23:52:34, piman wrote:
> nit: at this point, there's little advantage to inherit from IdHandler, rather
> inherit from IdHandlerInterface?
> That way, IdHandler fields can become private.

Done.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:120: DCHECK(id == 0 ||
id_allocator_.InUse(id));
On 2014/02/12 23:52:34, piman wrote:
> Can we update the DCHECK to take into account the ids in pending_free_ids_?

Done.

https://codereview.chromium.org/162023002/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:129: if (pending_flush_generation_ !=
flush_generation) {
On 2014/02/13 00:20:17, vmiura wrote:
> On 2014/02/12 23:52:34, piman wrote:
> > gl_impl->helper()->flush_generation() is per-command buffer (i.e. per
> context),
> > whereas |this|, hence |pending_flush_generation_| is per share group, so it
> > doesn't really make sense to compare those (it'll do the wrong thing).
> > 
> > I suppose you could keep a per-command buffer {pending_free_ids_,
> > pending_flush_generation_} struct...
> > 
> > There's another issue which is that you only collect on subsequent
Gen/Delete,
> > which means that, for example, if a command buffer gets destroyed, we won't
> ever
> > reclaim the ids that were last deleted.
> 
> Great point.
> 
> I think this pending free handling can move up into GLES2Implementation.

Moved these into per-GLES2Implementation data, but same idea.

[piman, 2014-02-14 01:56:19]

LGTM.

https://codereview.chromium.org/162023002/diff/200001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/200001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:82: StrictIdHandler(int id_namespace) :
id_namespace_(id_namespace) {}
nit: explicit

[vmiura, 2014-02-14 02:18:12]

https://codereview.chromium.org/162023002/diff/200001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/200001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:82: StrictIdHandler(int id_namespace) :
id_namespace_(id_namespace) {}
On 2014/02/14 01:56:20, piman wrote:
> nit: explicit

Done.

[vmiura, 2014-02-14 21:36:21]

Hmm, one (and only one) trybot WebGl conformance test is failing.  Can't repro
this locally.

http://build.chromium.org/p/tryserver.chromium/builders/linux_gpu_triggered_t...

[6112:6112:0214/184348:ERROR:gpu_scheduler.cc(94)] [0x3c006bf8c900] GPU PARSE
ERROR: 4
[6112:6112:0214/184348:ERROR:gles2_cmd_decoder.cc(10064)]
[.Compositor-0x1c9b6b6a0b00]GL ERROR :GL_INVALID_OPERATION :
glConsumeTextureCHROMIUM: invalid mailbox name
[6112:6112:0214/184348:ERROR:gles2_cmd_decoder.cc(5994)]
[.Compositor-0x1c9b6b6a0b00]RENDER WARNING: texture bound to texture unit 0 is
not renderable. It maybe non-power-of-2 and have incompatible texture filtering
or is not 'texture complete'

I guess this is related to us not auto flushing at some point.

[piman, 2014-02-14 22:08:53]

On 2014/02/14 21:36:21, vmiura wrote:
> Hmm, one (and only one) trybot WebGl conformance test is failing.  Can't repro
> this locally.
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_gpu_triggered_t...
> 
> [6112:6112:0214/184348:ERROR:gpu_scheduler.cc(94)] [0x3c006bf8c900] GPU PARSE
> ERROR: 4
> [6112:6112:0214/184348:ERROR:gles2_cmd_decoder.cc(10064)]
> [.Compositor-0x1c9b6b6a0b00]GL ERROR :GL_INVALID_OPERATION :
> glConsumeTextureCHROMIUM: invalid mailbox name
> [6112:6112:0214/184348:ERROR:gles2_cmd_decoder.cc(5994)]
> [.Compositor-0x1c9b6b6a0b00]RENDER WARNING: texture bound to texture unit 0 is
> not renderable. It maybe non-power-of-2 and have incompatible texture
filtering
> or is not 'texture complete'
> 
> I guess this is related to us not auto flushing at some point.

It bothers me that not flushing loses the context (kInvalidArguments). GL errors
are fine, but losing the context leads to havok.

Do you know what causes the kInvalidArguments?

[vmiura, 2014-02-15 01:18:57]

Could reproduce the error on the gpu-bot, however not when any logging is
enabled.

There was a lock missing in FreeContext() though.  Will retry with this fix.

[vmiura, 2014-02-16 17:41:31]

A few bugs to solve.

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:117: CollectPendingFreeIds(gl_impl);
Bug: CollectPendingFreeIds() is recording the current 'flush_generation_', 
however the glDelete*() command itself is written to the command buffer AFTER
this FreeIds() returns.

If an internal CommandBufferHelper::Flush() occurs in
CommandBufferHelper::GetSpace() then the 'flush_generation_' will change before
the glDelete* command is flushed.

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:125: 
Bug: Keeping freed_ids_ per context is not sufficient in this case:

ctxt1:
  glGenTextures() -> id1
  glFlush()
  glDeleteTextures(id1)  // adds id1 to |ctxt1| freed_ids_

ctxt2:
  glDeleteTextures(id1)  // adds id1 to |ctxt2| freed_ids_

-> Subsequently id1 may be freed and re-used twice.

A set of freed_ids_ will be needed both per-context and per-share_group to avoid
this.

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:151: base::AutoLock auto_lock(lock_);
Bug: This lock was missing.

[vmiura, 2014-02-16 20:28:00]

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:117: CollectPendingFreeIds(gl_impl);
On 2014/02/16 17:41:31, vmiura wrote:
> Bug: CollectPendingFreeIds() is recording the current 'flush_generation_', 
> however the glDelete*() command itself is written to the command buffer AFTER
> this FreeIds() returns.
> 
> If an internal CommandBufferHelper::Flush() occurs in
> CommandBufferHelper::GetSpace() then the 'flush_generation_' will change
before
> the glDelete* command is flushed.

Correction, the gl_impl->*delete_fn() call is causing the internal flush.

Need to move the gl_impl->*delete_fn() call before calling
CollectPendingFreeIds().

[vmiura, 2014-02-18 20:08:08]

https://chromiumcodereview.appspot.com/162023002/diff/760001/gpu/command_buff...
File gpu/command_buffer/client/share_group.cc (right):

https://chromiumcodereview.appspot.com/162023002/diff/760001/gpu/command_buff...
gpu/command_buffer/client/share_group.cc:125: 
On 2014/02/16 17:41:31, vmiura wrote:
> Bug: Keeping freed_ids_ per context is not sufficient in this case:
> 
> ctxt1:
>   glGenTextures() -> id1
>   glFlush()
>   glDeleteTextures(id1)  // adds id1 to |ctxt1| freed_ids_
> 
> ctxt2:
>   glDeleteTextures(id1)  // adds id1 to |ctxt2| freed_ids_
> 
> -> Subsequently id1 may be freed and re-used twice.
> 
> A set of freed_ids_ will be needed both per-context and per-share_group to
avoid
> this.

I see that deleting the same Ids twice is a GL error in this case, so instead of
handling it I will add a DCHECK here.

[vmiura, 2014-02-18 21:15:01]

Please take another look.

I think CachedProgramInfoManager::DeleteInfo(GLuint program) is missing a lock,
but it's an unrelated bug so I'll create a separate CL for that.

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:117: CollectPendingFreeIds(gl_impl);
On 2014/02/16 20:28:01, vmiura wrote:
> On 2014/02/16 17:41:31, vmiura wrote:
> > Bug: CollectPendingFreeIds() is recording the current 'flush_generation_', 
> > however the glDelete*() command itself is written to the command buffer
AFTER
> > this FreeIds() returns.
> > 
> > If an internal CommandBufferHelper::Flush() occurs in
> > CommandBufferHelper::GetSpace() then the 'flush_generation_' will change
> before
> > the glDelete* command is flushed.
> 
> Correction, the gl_impl->*delete_fn() call is causing the internal flush.
> 
> Need to move the gl_impl->*delete_fn() call before calling
> CollectPendingFreeIds().

Fixed by moving order of gl_impl->*delete_fn().

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:125: 
On 2014/02/18 20:08:08, vmiura wrote:
> On 2014/02/16 17:41:31, vmiura wrote:
> > Bug: Keeping freed_ids_ per context is not sufficient in this case:
> > 
> > ctxt1:
> >   glGenTextures() -> id1
> >   glFlush()
> >   glDeleteTextures(id1)  // adds id1 to |ctxt1| freed_ids_
> > 
> > ctxt2:
> >   glDeleteTextures(id1)  // adds id1 to |ctxt2| freed_ids_
> > 
> > -> Subsequently id1 may be freed and re-used twice.
> > 
> > A set of freed_ids_ will be needed both per-context and per-share_group to
> avoid
> > this.
> 
> I see that deleting the same Ids twice is a GL error in this case, so instead
of
> handling it I will add a DCHECK here.

Fixed by adding DCHECKs.

https://codereview.chromium.org/162023002/diff/760001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:151: base::AutoLock auto_lock(lock_);
On 2014/02/16 17:41:31, vmiura wrote:
> Bug: This lock was missing.

Done.

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation_unittest.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation_unittest.cc:653: // the Delete().
 Tests an edge case regression.
This unit test catches the edge case bug caused by internal flush happening
during glDelete*.

[piman, 2014-02-18 22:16:47]

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
Isn't there a race where another thread might Gen/Bind those IDs, and Flush
before the below Flush happens?

I think we want to keep the lock up to the Flush.

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:193: std::set<GLuint> group_freed_ids_;
Adding one more set seems overkill, it sounds like you'd want the entries in
id_allocator_.used_ids_ or free_ids_ to have a flag "deleted_but_dont_reuse".

Looking at it closer, for the StrictIdHandler, we could simplify dramatically
because we don't need to handle the Bind-creates-id case:
- replace the sets (used_ids_, free_ids_) by a single vector (indexed by id) of
a tri-state for the id (IN_USE vs FREE vs DELETED_WAITING_FOR_FLUSH).
- keep a free list for allocating faster, but it can be something very efficient
like a queue or a stack.

Allocations and frees would become O(1). I think it would make the code clearer
too.

[vmiura, 2014-02-18 23:04:20]

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
On 2014/02/18 22:16:48, piman wrote:
> Isn't there a race where another thread might Gen/Bind those IDs, and Flush
> before the below Flush happens?
> 
> I think we want to keep the lock up to the Flush.

I don't think another thread can be using the same gl_impl* at the same time. 
It's not thread safe.  Am I wrong?

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:193: std::set<GLuint> group_freed_ids_;
On 2014/02/18 22:16:48, piman wrote:
> Adding one more set seems overkill, it sounds like you'd want the entries in
> id_allocator_.used_ids_ or free_ids_ to have a flag "deleted_but_dont_reuse".
> 
> Looking at it closer, for the StrictIdHandler, we could simplify dramatically
> because we don't need to handle the Bind-creates-id case:
> - replace the sets (used_ids_, free_ids_) by a single vector (indexed by id)
of
> a tri-state for the id (IN_USE vs FREE vs DELETED_WAITING_FOR_FLUSH).
> - keep a free list for allocating faster, but it can be something very
efficient
> like a queue or a stack.
> 
> Allocations and frees would become O(1). I think it would make the code
clearer
> too.

Will do.

[vmiura, 2014-02-18 23:28:01]

PTAL.

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:193: std::set<GLuint> group_freed_ids_;
On 2014/02/18 23:04:20, vmiura wrote:
> On 2014/02/18 22:16:48, piman wrote:
> > Adding one more set seems overkill, it sounds like you'd want the entries in
> > id_allocator_.used_ids_ or free_ids_ to have a flag
"deleted_but_dont_reuse".
> > 
> > Looking at it closer, for the StrictIdHandler, we could simplify
dramatically
> > because we don't need to handle the Bind-creates-id case:
> > - replace the sets (used_ids_, free_ids_) by a single vector (indexed by id)
> of
> > a tri-state for the id (IN_USE vs FREE vs DELETED_WAITING_FOR_FLUSH).
> > - keep a free list for allocating faster, but it can be something very
> efficient
> > like a queue or a stack.
> > 
> > Allocations and frees would become O(1). I think it would make the code
> clearer
> > too.
> 
> Will do.

Done.

[piman, 2014-02-18 23:28:26]

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
On 2014/02/18 23:04:20, vmiura wrote:
> On 2014/02/18 22:16:48, piman wrote:
> > Isn't there a race where another thread might Gen/Bind those IDs, and Flush
> > before the below Flush happens?
> > 
> > I think we want to keep the lock up to the Flush.
> 
> I don't think another thread can be using the same gl_impl* at the same time. 
> It's not thread safe.  Am I wrong?

Not the same gl_impl, but the same share group.

Say gl1 and gl2 share the group.

thread1:
1. gl1.GenTexture(1, &id);
2. gl1.DeleteTexture(1, &id);
  a- grabs lock
  b- frees id
  c- releases lock
  d- add delete command
  e- flush

thread2:
3. gl2.GenTexture(1, &id);
  a- grabs lock
  b- finds free id and mark it as used
  c- releases lock
  d- add gen command
  e- flush


3.b could happen as soon as 2.c is done, and reuse the same id, before 2.e. A
possible ordering pattern is:

1
2.a
2.b
2.c
3.a
3.b
3.c
3.d
3.e
2.d
2.e

That could mean the GPU process sees
Gen(1, id1)
Gen(1, id1)
Delete(1, id1)

And that is wrong.

[vmiura, 2014-02-18 23:36:48]

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
On 2014/02/18 23:28:26, piman wrote:
> On 2014/02/18 23:04:20, vmiura wrote:
> > On 2014/02/18 22:16:48, piman wrote:
> > > Isn't there a race where another thread might Gen/Bind those IDs, and
Flush
> > > before the below Flush happens?
> > > 
> > > I think we want to keep the lock up to the Flush.
> > 
> > I don't think another thread can be using the same gl_impl* at the same
time. 
> > It's not thread safe.  Am I wrong?
> 
> Not the same gl_impl, but the same share group.
> 
> Say gl1 and gl2 share the group.
> 
> thread1:
> 1. gl1.GenTexture(1, &id);
> 2. gl1.DeleteTexture(1, &id);
>   a- grabs lock
>   b- frees id
>   c- releases lock
>   d- add delete command
>   e- flush
> 
> thread2:
> 3. gl2.GenTexture(1, &id);
>   a- grabs lock
>   b- finds free id and mark it as used
>   c- releases lock
>   d- add gen command
>   e- flush
> 
> 
> 3.b could happen as soon as 2.c is done, and reuse the same id, before 2.e. A
> possible ordering pattern is:
> 
> 1
> 2.a
> 2.b
> 2.c
> 3.a
> 3.b
> 3.c
> 3.d
> 3.e
> 2.d
> 2.e
> 
> That could mean the GPU process sees
> Gen(1, id1)
> Gen(1, id1)
> Delete(1, id1)
> 
> And that is wrong.

2.b saves the id for later collection.  At this point it can't be reused by any
context.
3.b can not reuse the id until after 2.e.

[vmiura, 2014-02-18 23:42:25]

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
On 2014/02/18 23:36:48, vmiura wrote:
> On 2014/02/18 23:28:26, piman wrote:
> > On 2014/02/18 23:04:20, vmiura wrote:
> > > On 2014/02/18 22:16:48, piman wrote:
> > > > Isn't there a race where another thread might Gen/Bind those IDs, and
> Flush
> > > > before the below Flush happens?
> > > > 
> > > > I think we want to keep the lock up to the Flush.
> > > 
> > > I don't think another thread can be using the same gl_impl* at the same
> time. 
> > > It's not thread safe.  Am I wrong?
> > 
> > Not the same gl_impl, but the same share group.
> > 
> > Say gl1 and gl2 share the group.
> > 
> > thread1:
> > 1. gl1.GenTexture(1, &id);
> > 2. gl1.DeleteTexture(1, &id);
> >   a- grabs lock
> >   b- frees id
> >   c- releases lock
> >   d- add delete command
> >   e- flush
> > 
> > thread2:
> > 3. gl2.GenTexture(1, &id);
> >   a- grabs lock
> >   b- finds free id and mark it as used
> >   c- releases lock
> >   d- add gen command
> >   e- flush
> > 
> > 
> > 3.b could happen as soon as 2.c is done, and reuse the same id, before 2.e.
A
> > possible ordering pattern is:
> > 
> > 1
> > 2.a
> > 2.b
> > 2.c
> > 3.a
> > 3.b
> > 3.c
> > 3.d
> > 3.e
> > 2.d
> > 2.e
> > 
> > That could mean the GPU process sees
> > Gen(1, id1)
> > Gen(1, id1)
> > Delete(1, id1)
> > 
> > And that is wrong.
> 
> 2.b saves the id for later collection.  At this point it can't be reused by
any
> context.
> 3.b can not reuse the id until after 2.e.
> 

I'm sorry I am thinking of StrictIdHandler.
You're right, this is a bug for IdHandler.

[vmiura, 2014-02-19 19:39:22]

PTAL.

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/162023002/diff/1030002/gpu/command_buffer/cli...
gpu/command_buffer/client/share_group.cc:60: (gl_impl->*delete_fn)(n, ids);
On 2014/02/18 23:42:25, vmiura wrote:
> On 2014/02/18 23:36:48, vmiura wrote:
> > On 2014/02/18 23:28:26, piman wrote:
> > > On 2014/02/18 23:04:20, vmiura wrote:
> > > > On 2014/02/18 22:16:48, piman wrote:
> > > > > Isn't there a race where another thread might Gen/Bind those IDs, and
> > Flush
> > > > > before the below Flush happens?
> > > > > 
> > > > > I think we want to keep the lock up to the Flush.
> > > > 
> > > > I don't think another thread can be using the same gl_impl* at the same
> > time. 
> > > > It's not thread safe.  Am I wrong?
> > > 
> > > Not the same gl_impl, but the same share group.
> > > 
> > > Say gl1 and gl2 share the group.
> > > 
> > > thread1:
> > > 1. gl1.GenTexture(1, &id);
> > > 2. gl1.DeleteTexture(1, &id);
> > >   a- grabs lock
> > >   b- frees id
> > >   c- releases lock
> > >   d- add delete command
> > >   e- flush
> > > 
> > > thread2:
> > > 3. gl2.GenTexture(1, &id);
> > >   a- grabs lock
> > >   b- finds free id and mark it as used
> > >   c- releases lock
> > >   d- add gen command
> > >   e- flush
> > > 
> > > 
> > > 3.b could happen as soon as 2.c is done, and reuse the same id, before
2.e.
> A
> > > possible ordering pattern is:
> > > 
> > > 1
> > > 2.a
> > > 2.b
> > > 2.c
> > > 3.a
> > > 3.b
> > > 3.c
> > > 3.d
> > > 3.e
> > > 2.d
> > > 2.e
> > > 
> > > That could mean the GPU process sees
> > > Gen(1, id1)
> > > Gen(1, id1)
> > > Delete(1, id1)
> > > 
> > > And that is wrong.
> > 
> > 2.b saves the id for later collection.  At this point it can't be reused by
> any
> > context.
> > 3.b can not reuse the id until after 2.e.
> > 
> 
> I'm sorry I am thinking of StrictIdHandler.
> You're right, this is a bug for IdHandler.

Done. Set the lock to include the delete + flush.

[vmiura, 2014-02-21 18:35:06]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmiura@chromium.org/162023002/1240002

[commit-bot: I haz the power, 2014-02-21 19:13:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmiura@chromium.org/162023002/1240002

[commit-bot: I haz the power, 2014-02-21 20:42:53]

Change committed as 252635