command_buffer: Optimize non-debug path of GLES2DecoderImpl::DoCommands().

command_buffer: Optimize non-debug path of GLES2DecoderImpl::DoCommands().

Moves debug logging and tracing logic off the command processing path
when they are disabled.

BUG=394570
Committed: https://crrev.com/1c2b1de60f2f3c4da41b29d5c6a886fec0409f7a
Cr-Commit-Position: refs/heads/master@{#295755}

[vmiura, 2014-09-10 01:39:26]

vmiura@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[vmiura, 2014-09-10 01:39:26]

ptal

[piman, 2014-09-10 04:05:17]

LGTM, nice.

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

https://codereview.chromium.org/553293002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3868: if (gpu_debug_commands_)
nit: style requires {}

[vmiura, 2014-09-10 17:09:32]

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

https://codereview.chromium.org/553293002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3868: if (gpu_debug_commands_)
On 2014/09/10 04:05:17, piman (Very slow to review) wrote:
> nit: style requires {}

Done.

https://codereview.chromium.org/553293002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/553293002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3795: GetCommandName(command));
Scoped TRACE_EVENT was wrong here, so changed to BEGIN/END form.

[piman, 2014-09-10 17:25:07]

Ah, yeah. LGTM.

[vmiura, 2014-09-10 18:19:42]

vmiura@chromium.org changed reviewers:
+ kbr@chromium.org

[vmiura, 2014-09-10 18:19:43]

kbr@: please check the change in gl_helper_unittest.cc

Thanks

[Ken Russell (switch to Gerrit), 2014-09-10 22:00:45]

content/common/gpu test change LGTM.

[vmiura, 2014-09-12 01:14:45]

This patch is causing failures on some WebGL tests to fail on Windows
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered....

The tests fail reliably, and I can repro on my Windows machine also.

Changing: if (DebugImpl && debug())
to: if (/*DebugImpl &&*/ debug())
or just adding a LOG to DoCommandsImpl makes the tests pass.

I don't see a problem with the patch so suspecting either a compiler bug, or
that we're exposing some existent bug in the command_buffer or in these tests.
If it was a compiler bug in DoCommandsImpl I would not expect so many other
tests to pass.

[vmiura, 2014-09-12 01:15:10]

cc: zmo

[Ken Russell (switch to Gerrit), 2014-09-12 01:39:05]

CC'ing a couple of members from the ANGLE team.

https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3826: if (DebugImpl && debug())
{
What does debug() ordinarily return? If unconditionally executing this loop
makes the tests pass, then I suspect that some OpenGL error is ordinarily being
consumed by this code.

[vmiura, 2014-09-12 01:57:36]

On 2014/09/12 01:39:05, Ken Russell wrote:
> CC'ing a couple of members from the ANGLE team.
> 
>
https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3826: if (DebugImpl &&
debug())
> {
> What does debug() ordinarily return? If unconditionally executing this loop
> makes the tests pass, then I suspect that some OpenGL error is ordinarily
being
> consumed by this code.

Ordinarily debug() == false unless '--enable-gpu-debugging' is used.

Using --enable-gpu-debugging does make the issue go away, but under normal
circumstances that should not be used I think.

On the other hand using --enable-gpu-service-logging sets log_commands() == true
and DebugImpl == true, but the bug still occurs.

[Ken Russell (switch to Gerrit), 2014-09-12 02:26:28]

On 2014/09/12 01:57:36, vmiura wrote:
> On 2014/09/12 01:39:05, Ken Russell wrote:
> > CC'ing a couple of members from the ANGLE team.
> > 
> >
>
https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/553293002/diff/60001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:3826: if (DebugImpl &&
> debug())
> > {
> > What does debug() ordinarily return? If unconditionally executing this loop
> > makes the tests pass, then I suspect that some OpenGL error is ordinarily
> being
> > consumed by this code.
> 
> Ordinarily debug() == false unless '--enable-gpu-debugging' is used.
> 
> Using --enable-gpu-debugging does make the issue go away, but under normal
> circumstances that should not be used I think.
> 
> On the other hand using --enable-gpu-service-logging sets log_commands() ==
true
> and DebugImpl == true, but the bug still occurs.

That is odd since the bots don't set either of those flags. It does sound like a
potential compiler bug. Can you try using #pragma optimize to disable
optimizations in that region and see if that works around it? I do realize that
would defeat the purpose of this change, but let's try to get to the bottom of
it.

scottmg@, can you offer any advice about how to deal with potential Visual
Studio compiler bugs?

[vmiura, 2014-09-15 21:52:35]

Tracked down the memory bug in ANGLE: crbug.com/414450

[vmiura, 2014-09-19 18:31:26]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:32:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/553293002/60001

[commit-bot: I haz the power, 2014-09-19 19:03:34]

Committed patchset #4 (id:60001) as 1cccfb94ab4ba4ee6365fed1c991da2722aacccd

[commit-bot: I haz the power, 2014-09-19 19:04:07]

Patchset 4 (id:??) landed as
https://crrev.com/1c2b1de60f2f3c4da41b29d5c6a886fec0409f7a
Cr-Commit-Position: refs/heads/master@{#295755}