Reduce cost of glBind* on contexts without bind_generates_resource.

Reduce cost of glBind* on contexts without bind_generates_resource.

Contexts without bind_generates_resource utilize StrictIdHandler.
Binding ungenerated Ids is an error on StrictIdHandler, but there
were still calls made to IdAllocator::MarkAsUsed, which has the
overhead of taking a lock and doing a std::map lookup.

- Changed to generate GL error on service side instead of DCHECK
  on client side.
- StrictIdHandler::MarkAsUsedForBind becomes no-op.
- Merged lock functionality of ThreadSafeIdHandlerWrapper up into
  the base handlers to reduce indirection and use finer grained
  locking.
- Removed unused SharedIdHandler class.

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

[vmiura, 2014-02-12 00:41:10]

PTAL.

[piman, 2014-02-12 00:52:53]

LGTM+couple of nits.

In the future, it's conceivable that we could have contexts that don't share
across threads (or at all), alleviating the need for the lock (WebGL is one
example), but we'll see when we get there.

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

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:67: base::Lock lock_;
nit: a common pattern is to put the lock before the variables it protects (looks
symmetrical with the AutoLock at the call sites).

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:77: virtual bool
MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
nit: it'd be nice to keep the DCHECK. We can do:
#ifndef NDEBUG
  {
    base::AutoLock auto_lock(lock_);
    DCHECK(id == 0 || id_allocator_.InUse(id));
  }
#endif

[no sievers, 2014-02-12 01:09:04]

lgtm

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

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:77: virtual bool
MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
On 2014/02/12 00:52:53, piman wrote:
> nit: it'd be nice to keep the DCHECK. We can do:
> #ifndef NDEBUG
>   {
>     base::AutoLock auto_lock(lock_);
>     DCHECK(id == 0 || id_allocator_.InUse(id));
>   }
> #endif

Although at other times we argue that you shouldn't hit any DCHECKs() when you
lose a context (for example for testing) :)

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:92: ids[ii] = ++last_id_ + id_offset;
I guess we could make that use an atomic as a follow-up.

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3819: return;
nit: indent

[piman, 2014-02-12 01:11:19]

On Tue, Feb 11, 2014 at 5:09 PM, <sievers@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/154263011/diff/1/gpu/
> command_buffer/client/share_group.cc
> File gpu/command_buffer/client/share_group.cc (right):
>
> https://codereview.chromium.org/154263011/diff/1/gpu/
> command_buffer/client/share_group.cc#newcode77
> gpu/command_buffer/client/share_group.cc:77: virtual bool
> MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
> On 2014/02/12 00:52:53, piman wrote:
>
>> nit: it'd be nice to keep the DCHECK. We can do:
>> #ifndef NDEBUG
>>    {
>>      base::AutoLock auto_lock(lock_);
>>      DCHECK(id == 0 || id_allocator_.InUse(id));
>>    }
>> #endif
>>
>
> Although at other times we argue that you shouldn't hit any DCHECKs()
> when you lose a context (for example for testing) :)
>

If we hit a DCHECK, it's a critical bug and we need to fix it.


>
> https://codereview.chromium.org/154263011/diff/1/gpu/
> command_buffer/client/share_group.cc#newcode92
> gpu/command_buffer/client/share_group.cc:92: ids[ii] = ++last_id_ +
> id_offset;
> I guess we could make that use an atomic as a follow-up.
>
> https://codereview.chromium.org/154263011/diff/1/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/154263011/diff/1/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode3819
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3819: return;
> nit: indent
>
> https://codereview.chromium.org/154263011/
>

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

[no sievers, 2014-02-12 01:13:21]

On 2014/02/12 01:11:19, piman wrote:
> On Tue, Feb 11, 2014 at 5:09 PM, <mailto:sievers@chromium.org> wrote:
> 
> > lgtm
> >
> >
> >
> > https://codereview.chromium.org/154263011/diff/1/gpu/
> > command_buffer/client/share_group.cc
> > File gpu/command_buffer/client/share_group.cc (right):
> >
> > https://codereview.chromium.org/154263011/diff/1/gpu/
> > command_buffer/client/share_group.cc#newcode77
> > gpu/command_buffer/client/share_group.cc:77: virtual bool
> > MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
> > On 2014/02/12 00:52:53, piman wrote:
> >
> >> nit: it'd be nice to keep the DCHECK. We can do:
> >> #ifndef NDEBUG
> >>    {
> >>      base::AutoLock auto_lock(lock_);
> >>      DCHECK(id == 0 || id_allocator_.InUse(id));
> >>    }
> >> #endif
> >>
> >
> > Although at other times we argue that you shouldn't hit any DCHECKs()
> > when you lose a context (for example for testing) :)
> >
> 
> If we hit a DCHECK, it's a critical bug and we need to fix it.
> 

So if a context is lost, it's not expected that a client might temporarily use
an old texture id?


> 
> >
> > https://codereview.chromium.org/154263011/diff/1/gpu/
> > command_buffer/client/share_group.cc#newcode92
> > gpu/command_buffer/client/share_group.cc:92: ids[ii] = ++last_id_ +
> > id_offset;
> > I guess we could make that use an atomic as a follow-up.
> >
> > https://codereview.chromium.org/154263011/diff/1/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >
> > https://codereview.chromium.org/154263011/diff/1/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc#newcode3819
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:3819: return;
> > nit: indent
> >
> > https://codereview.chromium.org/154263011/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-02-12 01:35:52]

On Tue, Feb 11, 2014 at 5:13 PM, <sievers@chromium.org> wrote:

> On 2014/02/12 01:11:19, piman wrote:
>
>> On Tue, Feb 11, 2014 at 5:09 PM, <mailto:sievers@chromium.org> wrote:
>>
>
>  > lgtm
>> >
>> >
>> >
>> > https://codereview.chromium.org/154263011/diff/1/gpu/
>> > command_buffer/client/share_group.cc
>> > File gpu/command_buffer/client/share_group.cc (right):
>> >
>> > https://codereview.chromium.org/154263011/diff/1/gpu/
>>
>> > command_buffer/client/share_group.cc#newcode77
>> > gpu/command_buffer/client/share_group.cc:77: virtual bool
>> > MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
>> > On 2014/02/12 00:52:53, piman wrote:
>> >
>> >> nit: it'd be nice to keep the DCHECK. We can do:
>> >> #ifndef NDEBUG
>> >>    {
>> >>      base::AutoLock auto_lock(lock_);
>> >>      DCHECK(id == 0 || id_allocator_.InUse(id));
>> >>    }
>> >> #endif
>> >>
>> >
>> > Although at other times we argue that you shouldn't hit any DCHECKs()
>> > when you lose a context (for example for testing) :)
>> >
>>
>
>  If we hit a DCHECK, it's a critical bug and we need to fix it.
>>
>
>
> So if a context is lost, it's not expected that a client might temporarily
> use
> an old texture id?
>

No... if it's the case, it's a bug and we should fix it.


>
>
>
>  >
>> > https://codereview.chromium.org/154263011/diff/1/gpu/
>>
>> > command_buffer/client/share_group.cc#newcode92
>> > gpu/command_buffer/client/share_group.cc:92: ids[ii] = ++last_id_ +
>> > id_offset;
>> > I guess we could make that use an atomic as a follow-up.
>> >
>> > https://codereview.chromium.org/154263011/diff/1/gpu/
>> > command_buffer/service/gles2_cmd_decoder.cc
>> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>> >
>> > https://codereview.chromium.org/154263011/diff/1/gpu/
>> > command_buffer/service/gles2_cmd_decoder.cc#newcode3819
>> > gpu/command_buffer/service/gles2_cmd_decoder.cc:3819: return;
>> > nit: indent
>> >
>> > https://codereview.chromium.org/154263011/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/154263011/
>

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

[vmiura, 2014-02-12 02:03:52]

Fixed nits and restored DCHECK.

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

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:67: base::Lock lock_;
On 2014/02/12 00:52:53, piman wrote:
> nit: a common pattern is to put the lock before the variables it protects
(looks
> symmetrical with the AutoLock at the call sites).

Done.

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/client/sh...
gpu/command_buffer/client/share_group.cc:77: virtual bool
MarkAsUsedForBind(GLuint id) OVERRIDE { return true; }
On 2014/02/12 00:52:53, piman wrote:
> nit: it'd be nice to keep the DCHECK. We can do:
> #ifndef NDEBUG
>   {
>     base::AutoLock auto_lock(lock_);
>     DCHECK(id == 0 || id_allocator_.InUse(id));
>   }
> #endif

Done.

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/154263011/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3819: return;
On 2014/02/12 01:09:04, sievers wrote:
> nit: indent

Done.

[vmiura, 2014-02-12 02:27:27]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-02-12 02:27:45]

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

[commit-bot: I haz the power, 2014-02-12 11:15:25]

Change committed as 250681