command_buffer: Batch command processing to reduce handler overheads.

command_buffer: Batch command processing to reduce handler overheads.

Old processing order for each command:
  scheduler.PutChanged() -> parser::ParseCommand() -> decoder::DoCommand()

This patch runs commands in batches (default of 20):
  scheduler.PutChanged() -> CmdParser::ParseCommands(20) -> dec::DoCommands(..)

Performance improvement is an average 0.2ms/frame on N5 on GPU rasterized pages.

BUG=394570
Committed: https://crrev.com/8266ca7b30020175e2564b9760597672f7f3ad57
Cr-Commit-Position: refs/heads/master@{#294017}

[vmiura, 2014-09-09 00:00:02]

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

[vmiura, 2014-09-09 00:00:02]

ptal

[vmiura, 2014-09-09 00:23:37]

Discussing with John I think it makes sense to remove CommandParser entirely as
it's just duplicating state anyway.

Removing as a follow on patch makes sense.

[piman, 2014-09-09 00:59:01]

LGTM, couple of nits / suggestions.

Some thoughts for further improvement (maybe a later CL):
I wonder if it's worth having a specialized GLES2DecoderImpl::DoCommands for the
common case where logging/tracing/debug/etc. are all disabled, for a tighter
inner loop and possibly better I-cache?

We should also think about whether CommandHeader.size should be a size (as is
today) or an (unsigned) arg count, which would save a test in the inner loop.

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3792: if (header.size == 0) {
nit: It was already before, but it's easier to see now. We're using header.size
many times in this function. It's a bitfield, so each use might involve a mask
and/or shift, unless the compiler's aliasing analysis lets it figure out that it
can't change. It might be worth extracting the size once and for all into a
local variable.

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3809: {
nit: do you need this scope? I don't think you're using any Scoped thing in
there... It would help readability a bit if it wasn't there.

[piman, 2014-09-09 01:06:57]

On Mon, Sep 8, 2014 at 5:23 PM, <vmiura@chromium.org> wrote:

> Discussing with John I think it makes sense to remove CommandParser
> entirely as
> it's just duplicating state anyway.
>
> Removing as a follow on patch makes sense.
>

That's fair.
We may want to reuse some of the logic one day if we have different types
of command buffers, but we'll see then (the cut may not be in the same
place).


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

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

[vmiura, 2014-09-09 01:16:11]

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/common_decoder_unittest.cc:190: // TODO(vmiura): fix
this
I need to delete this comment.

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_mock.cc:11: using
::testing::Invoke;
I need to undo this redundant change.

[vmiura, 2014-09-09 01:38:56]

Fixed issues.  Also refactored the Error cases slightly in
GLES2DecoderImpl::DoCommands().

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/common_decoder_unittest.cc:190: // TODO(vmiura): fix
this
On 2014/09/09 01:16:10, vmiura wrote:
> I need to delete this comment.

Done.

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3792: if (header.size == 0) {
On 2014/09/09 00:59:01, piman (Very slow to review) wrote:
> nit: It was already before, but it's easier to see now. We're using
header.size
> many times in this function. It's a bitfield, so each use might involve a mask
> and/or shift, unless the compiler's aliasing analysis lets it figure out that
it
> can't change. It might be worth extracting the size once and for all into a
> local variable.

Done.

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3809: {
On 2014/09/09 00:59:01, piman (Very slow to review) wrote:
> nit: do you need this scope? I don't think you're using any Scoped thing in
> there... It would help readability a bit if it wasn't there.

Done.

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

https://codereview.chromium.org/558513003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_mock.cc:11: using
::testing::Invoke;
On 2014/09/09 01:16:10, vmiura wrote:
> I need to undo this redundant change.

Done.

[piman, 2014-09-09 01:46:41]

lgtm

[vmiura, 2014-09-09 20:00:08]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-09-09 20:14:31]

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

[commit-bot: I haz the power, 2014-09-09 21:37:44]

Committed patchset #5 (id:80001) as 16ac6ea5fb859186aae33d764bc8dfb10c01c50b

[commit-bot: I haz the power, 2014-09-10 03:55:55]

Patchset 5 (id:??) landed as
https://crrev.com/8266ca7b30020175e2564b9760597672f7f3ad57
Cr-Commit-Position: refs/heads/master@{#294017}