Improve BindBufferBase/BindBufferRange in GPU command buffer.

Improve BindBufferBase/BindBufferRange in GPU command buffer.

The first step to emulate ES3 BindBufferRange semantics on
Desktop GL 4.1 or lower.

BUG=604436
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/706399be0de1905a9aa03b47852c0dc093b7232b
Cr-Commit-Position: refs/heads/master@{#388607}

[Zhenyao Mo, 2016-04-20 21:02:17]

Description was changed from

==========
Improve BindBufferBase/BindBufferRange in GPU command buffer.

The first step to emulate ES3 BindBufferRange semantics on
Desktop GL 4.1 or lower.

BUG=604436
TEST=gpu_unittests
R=piman@chromium.org
==========

to

==========
Improve BindBufferBase/BindBufferRange in GPU command buffer.

The first step to emulate ES3 BindBufferRange semantics on
Desktop GL 4.1 or lower.

BUG=604436
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2016-04-20 21:04:57]

piman: PTAL
others: FYI

[Zhenyao Mo, 2016-04-20 21:05:04]

The CQ bit was checked by zmo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 21:05:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905743002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905743002/1

[piman, 2016-04-20 21:14:36]

LGTM+nits

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
"ContextGroup::Initialize failed because maximum "
nit: DLOG

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group.cc:198: LOG(ERROR) <<
"ContextGroup::Initialize failed because maximum "
nit: DLOG

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

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4753: // On Desktop GL 4.1 or
lower, clamp the offset/size not to exceed the
nit: is that a TODO? Mark it so (this reads as if we *are* doing it, but we
aren't).

[commit-bot: I haz the power, 2016-04-20 21:46:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 21:46:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhenyao Mo, 2016-04-20 22:08:50]

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
"ContextGroup::Initialize failed because maximum "
On 2016/04/20 21:14:35, piman wrote:
> nit: DLOG

This is consistent with the rest of the code.

We will want to know why we fail to create a context since these messages will
be displayed in about:gpu page.  If we use dlog, we lose them in official
release.

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group.cc:198: LOG(ERROR) <<
"ContextGroup::Initialize failed because maximum "
On 2016/04/20 21:14:35, piman wrote:
> nit: DLOG

Same here.

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

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4753: // On Desktop GL 4.1 or
lower, clamp the offset/size not to exceed the
On 2016/04/20 21:14:35, piman wrote:
> nit: is that a TODO? Mark it so (this reads as if we *are* doing it, but we
> aren't).

Done.

[Zhenyao Mo, 2016-04-20 22:08:55]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2016-04-20 22:08:56]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1905743002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-04-20 22:09:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905743002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905743002/20001

[piman, 2016-04-20 22:14:52]

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/context_group.cc (right):

https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
"ContextGroup::Initialize failed because maximum "
On 2016/04/20 22:08:50, Zhenyao Mo wrote:
> On 2016/04/20 21:14:35, piman wrote:
> > nit: DLOG
> 
> This is consistent with the rest of the code.
> 
> We will want to know why we fail to create a context since these messages will
> be displayed in about:gpu page.  If we use dlog, we lose them in official
> release.

We should stop doing that. Strings and logging code take a lot of space.

[Zhenyao Mo, 2016-04-20 22:20:54]

On 2016/04/20 22:14:52, piman wrote:
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/context_group.cc (right):
> 
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
> "ContextGroup::Initialize failed because maximum "
> On 2016/04/20 22:08:50, Zhenyao Mo wrote:
> > On 2016/04/20 21:14:35, piman wrote:
> > > nit: DLOG
> > 
> > This is consistent with the rest of the code.
> > 
> > We will want to know why we fail to create a context since these messages
will
> > be displayed in about:gpu page.  If we use dlog, we lose them in official
> > release.
> 
> We should stop doing that. Strings and logging code take a lot of space.

But DLOG won't give us what we need, say, a user filed a bug about not being
able to get a WebGL 2 context in a specific hardware, and we will want to look
at these log info at about:gpu.  Is there a way to achieve other than LOG?

[piman, 2016-04-20 22:22:26]

On Wed, Apr 20, 2016 at 3:20 PM, <zmo@chromium.org> wrote:

> On 2016/04/20 22:14:52, piman wrote:
> >
>
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/context_group.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
> > "ContextGroup::Initialize failed because maximum "
> > On 2016/04/20 22:08:50, Zhenyao Mo wrote:
> > > On 2016/04/20 21:14:35, piman wrote:
> > > > nit: DLOG
> > >
> > > This is consistent with the rest of the code.
> > >
> > > We will want to know why we fail to create a context since these
> messages
> will
> > > be displayed in about:gpu page. If we use dlog, we lose them in
> official
> > > release.
> >
> > We should stop doing that. Strings and logging code take a lot of space.
>
> But DLOG won't give us what we need, say, a user filed a bug about not
> being
> able to get a WebGL 2 context in a specific hardware, and we will want to
> look
> at these log info at about:gpu. Is there a way to achieve other than LOG?
>

Is that worth several hundred k on Android?


>
> https://codereview.chromium.org/1905743002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2016-04-20 22:31:21]

On 2016/04/20 22:22:26, piman wrote:
> On Wed, Apr 20, 2016 at 3:20 PM, <mailto:zmo@chromium.org> wrote:
> 
> > On 2016/04/20 22:14:52, piman wrote:
> > >
> >
> >
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > > File gpu/command_buffer/service/context_group.cc (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > > gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
> > > "ContextGroup::Initialize failed because maximum "
> > > On 2016/04/20 22:08:50, Zhenyao Mo wrote:
> > > > On 2016/04/20 21:14:35, piman wrote:
> > > > > nit: DLOG
> > > >
> > > > This is consistent with the rest of the code.
> > > >
> > > > We will want to know why we fail to create a context since these
> > messages
> > will
> > > > be displayed in about:gpu page. If we use dlog, we lose them in
> > official
> > > > release.
> > >
> > > We should stop doing that. Strings and logging code take a lot of space.
> >
> > But DLOG won't give us what we need, say, a user filed a bug about not
> > being
> > able to get a WebGL 2 context in a specific hardware, and we will want to
> > look
> > at these log info at about:gpu. Is there a way to achieve other than LOG?
> >
> 
> Is that worth several hundred k on Android?

That big? how and why?

If that's the case, I can display the cap numbers directly in about:gpu page and
remove the LOG here.

> 
> 
> >
> > https://codereview.chromium.org/1905743002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-04-20 23:08:59]

On Wed, Apr 20, 2016 at 3:31 PM, <zmo@chromium.org> wrote:

> On 2016/04/20 22:22:26, piman wrote:
> > On Wed, Apr 20, 2016 at 3:20 PM, <mailto:zmo@chromium.org> wrote:
> >
> > > On 2016/04/20 22:14:52, piman wrote:
> > > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > > > File gpu/command_buffer/service/context_group.cc (right):
> > > >
> > > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/1905743002/diff/1/gpu/command_buffer/service/...
> > > > gpu/command_buffer/service/context_group.cc:187: LOG(ERROR) <<
> > > > "ContextGroup::Initialize failed because maximum "
> > > > On 2016/04/20 22:08:50, Zhenyao Mo wrote:
> > > > > On 2016/04/20 21:14:35, piman wrote:
> > > > > > nit: DLOG
> > > > >
> > > > > This is consistent with the rest of the code.
> > > > >
> > > > > We will want to know why we fail to create a context since these
> > > messages
> > > will
> > > > > be displayed in about:gpu page. If we use dlog, we lose them in
> > > official
> > > > > release.
> > > >
> > > > We should stop doing that. Strings and logging code take a lot of
> space.
> > >
> > > But DLOG won't give us what we need, say, a user filed a bug about not
> > > being
> > > able to get a WebGL 2 context in a specific hardware, and we will want
> to
> > > look
> > > at these log info at about:gpu. Is there a way to achieve other than
> LOG?
> > >
> >
> > Is that worth several hundred k on Android?
>
> That big? how and why?
>

We have ~350 commands. Each of them has several error paths (autogened and
not), 5 is typical. Each of them has __FILE__, __LINE__, function name and
error message. On top of that the code to generate the LOG is very
non-trivial (look at disassembly), about 50-75 bytes each depending on
platform and complexity of the message. It adds up quickly.


> If that's the case, I can display the cap numbers directly in about:gpu
> page and
> remove the LOG here.
>
> >
> >
> > >
> > > https://codereview.chromium.org/1905743002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > 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/1905743002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
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, 2016-04-21 00:01:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-22 19:27:51]

Description was changed from

==========
Improve BindBufferBase/BindBufferRange in GPU command buffer.

The first step to emulate ES3 BindBufferRange semantics on
Desktop GL 4.1 or lower.

BUG=604436
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Improve BindBufferBase/BindBufferRange in GPU command buffer.

The first step to emulate ES3 BindBufferRange semantics on
Desktop GL 4.1 or lower.

BUG=604436
TEST=gpu_unittests
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/706399be0de1905a9aa03b47852c0dc093b7232b
Cr-Commit-Position: refs/heads/master@{#388607}
==========

[commit-bot: I haz the power, 2016-04-22 19:27:52]

Patchset 2 (id:??) landed as
https://crrev.com/706399be0de1905a9aa03b47852c0dc093b7232b
Cr-Commit-Position: refs/heads/master@{#388607}