Optimize GLES2DecoderImpl::ApplyDirtyState.

Optimize GLES2DecoderImpl::ApplyDirtyState.

ApplyDirtyState can on average 30% of the time of GLES2
draw calls.  It previously set Color, Depth, Stencil masks
and several enables without checking if they changed.

This change caches values used in ApplyDirtyState among
other places, and adds ContextState methods for setting
state with cache checks.

New cached state aware Set methods:
 * ContextState::SetDeviceCapabilityState
 * ContextState::SetDeviceColorMask
 * ContextState::SetDeviceDepthMask
 * ContextState::SetDeviceStencilMaskSeparate

Testing:
 * gpu_unit tests now run both with cache on and off (ignore_cached_state_for_test = true)
   to validate behavior independent of cache layering.
 * New tests specific for caching behavior:
 ** GLES2DecoderManualInitTest.ContextStateCapabilityCaching
 ** GLES2DecoderManualInitTest.CachedColorMask
 ** GLES2DecoderManualInitTest.CachedDepthMask
 ** GLES2DecoderManualInitTest.CachedStencilMask

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

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

[vmiura, 2014-04-29 18:19:57]

PTAL.

Originally I wanted to implement bit-fields to speed up
code for state change checking, but the meat of this change is adding state
caching, and unit test updates for dealing with cached state.

Changing some states to bit-fields could be an update.

NOTE: Formatting updates to generated files are landing
in https://codereview.chromium.org/247283005/, so will
rebase after that.

[piman, 2014-04-29 21:49:33]

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:207: bool ignore_cached_state;
It's rather unfortunate that we disable caching for tests. It feels like we're
not testing what we have in production, which means we'd miss regressions due to
the caching layer.

Do we have a way to modify the tests so that they exercise the caching layer?

[vmiura, 2014-04-29 23:15:20]

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:207: bool ignore_cached_state;
On 2014/04/29 21:49:33, piman wrote:
> It's rather unfortunate that we disable caching for tests. It feels like we're
> not testing what we have in production, which means we'd miss regressions due
to
> the caching layer.
> 
> Do we have a way to modify the tests so that they exercise the caching layer?

I'd like to get your opinion.

As I looked at our tests I got the opinion that if I turn on caching everywhere
we would be weakening our tests.  Caching can mask bugs in higher layers, so I
think we can get better coverage by testing some layers independently, and some
integrated.  That's why I added the flag, but it's not enabled for all tests.

E.g. test GLES2DecoderManualInitTest.DepthEnableWithoutRequestedDepth exist to
make sure that ApplyDirtyState disables GL_DEPTH_TEST when there's no bound
depth buffer.  We can disable the cache layer, and confirm that ApplyDirtyState
calls glDisable(GL_DEPTH_TEST) always.

I've set 'ignore_cached_state_for_test = true' for this class of test, and for
command decoder tests.  For example in glEnable test with invalid arguments, we
test that glEnable is not called.  Turning off caching confirms that.

Separately I added specific tests (CachedColorMask, CachedDepthMask,
CachedStencilMask) that test ApplyDirtyState with expected caching and
ContextStateCapabilityCaching for glEnable/glDisable cache tests.

[vmiura, 2014-04-29 23:39:48]

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/245923008/diff/190001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:207: bool ignore_cached_state;
On 2014/04/29 23:15:21, vmiura wrote:
> On 2014/04/29 21:49:33, piman wrote:
> > It's rather unfortunate that we disable caching for tests. It feels like
we're
> > not testing what we have in production, which means we'd miss regressions
due
> to
> > the caching layer.
> > 
> > Do we have a way to modify the tests so that they exercise the caching
layer?
> 
> I'd like to get your opinion.
> 
> As I looked at our tests I got the opinion that if I turn on caching
everywhere
> we would be weakening our tests.  Caching can mask bugs in higher layers, so I
> think we can get better coverage by testing some layers independently, and
some
> integrated.  That's why I added the flag, but it's not enabled for all tests.
> 
> E.g. test GLES2DecoderManualInitTest.DepthEnableWithoutRequestedDepth exist to
> make sure that ApplyDirtyState disables GL_DEPTH_TEST when there's no bound
> depth buffer.  We can disable the cache layer, and confirm that
ApplyDirtyState
> calls glDisable(GL_DEPTH_TEST) always.
> 
> I've set 'ignore_cached_state_for_test = true' for this class of test, and for
> command decoder tests.  For example in glEnable test with invalid arguments,
we
> test that glEnable is not called.  Turning off caching confirms that.
> 
> Separately I added specific tests (CachedColorMask, CachedDepthMask,
> CachedStencilMask) that test ApplyDirtyState with expected caching and
> ContextStateCapabilityCaching for glEnable/glDisable cache tests.

Btw, I think to have the best of both worlds it would be beneficial to have
gpu_unittests run twice, once with a global cache disable, once with it enabled.
 I've spoken with kbr@ and it doesn't seem like a problem.

I'll make a start on adding a command line arg, and making the unit tests work
in both modes.

[piman, 2014-04-29 23:49:52]

On Tue, Apr 29, 2014 at 4:15 PM, <vmiura@chromium.org> wrote:

>
> https://codereview.chromium.org/245923008/diff/190001/gpu/
> command_buffer/service/context_state.h
> File gpu/command_buffer/service/context_state.h (right):
>
> https://codereview.chromium.org/245923008/diff/190001/gpu/
> command_buffer/service/context_state.h#newcode207
> gpu/command_buffer/service/context_state.h:207: bool
> ignore_cached_state;
> On 2014/04/29 21:49:33, piman wrote:
>
>> It's rather unfortunate that we disable caching for tests. It feels
>>
> like we're
>
>> not testing what we have in production, which means we'd miss
>>
> regressions due to
>
>> the caching layer.
>>
>
>  Do we have a way to modify the tests so that they exercise the caching
>>
> layer?
>
> I'd like to get your opinion.
>
> As I looked at our tests I got the opinion that if I turn on caching
> everywhere we would be weakening our tests.  Caching can mask bugs in
> higher layers, so I think we can get better coverage by testing some
> layers independently, and some integrated.  That's why I added the flag,
> but it's not enabled for all tests.
>
> E.g. test GLES2DecoderManualInitTest.DepthEnableWithoutRequestedDepth
> exist to make sure that ApplyDirtyState disables GL_DEPTH_TEST when
> there's no bound depth buffer.  We can disable the cache layer, and
> confirm that ApplyDirtyState calls glDisable(GL_DEPTH_TEST) always.
>
> I've set 'ignore_cached_state_for_test = true' for this class of test,
> and for command decoder tests.  For example in glEnable test with
> invalid arguments, we test that glEnable is not called.  Turning off
> caching confirms that.
>

I guess we'd like to have both:
1- make sure the logical state in the decoder is consistent with the
client's command stream
2- make sure the GL state is consistent with the client's command stream at
the point it matters (draws)

Disabling the cache sort of gives you (1), but it prevents testing (2) in a
production environment.
Maybe one way would be to run with both cache on and off?


> Separately I added specific tests (CachedColorMask, CachedDepthMask,
> CachedStencilMask) that test ApplyDirtyState with expected caching and
> ContextStateCapabilityCaching for glEnable/glDisable cache tests.
>
> https://codereview.chromium.org/245923008/
>

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

[piman, 2014-04-30 00:22:10]

On Tue, Apr 29, 2014 at 4:39 PM, <vmiura@chromium.org> wrote:

>
> https://codereview.chromium.org/245923008/diff/190001/gpu/
> command_buffer/service/context_state.h
> File gpu/command_buffer/service/context_state.h (right):
>
> https://codereview.chromium.org/245923008/diff/190001/gpu/
> command_buffer/service/context_state.h#newcode207
> gpu/command_buffer/service/context_state.h:207: bool
> ignore_cached_state;
> On 2014/04/29 23:15:21, vmiura wrote:
>
>> On 2014/04/29 21:49:33, piman wrote:
>> > It's rather unfortunate that we disable caching for tests. It feels
>>
> like we're
>
>> > not testing what we have in production, which means we'd miss
>>
> regressions due
>
>> to
>> > the caching layer.
>> >
>> > Do we have a way to modify the tests so that they exercise the
>>
> caching layer?
>
>  I'd like to get your opinion.
>>
>
>  As I looked at our tests I got the opinion that if I turn on caching
>>
> everywhere
>
>> we would be weakening our tests.  Caching can mask bugs in higher
>>
> layers, so I
>
>> think we can get better coverage by testing some layers independently,
>>
> and some
>
>> integrated.  That's why I added the flag, but it's not enabled for all
>>
> tests.
>
>  E.g. test GLES2DecoderManualInitTest.DepthEnableWithoutRequestedDepth
>>
> exist to
>
>> make sure that ApplyDirtyState disables GL_DEPTH_TEST when there's no
>>
> bound
>
>> depth buffer.  We can disable the cache layer, and confirm that
>>
> ApplyDirtyState
>
>> calls glDisable(GL_DEPTH_TEST) always.
>>
>
>  I've set 'ignore_cached_state_for_test = true' for this class of test,
>>
> and for
>
>> command decoder tests.  For example in glEnable test with invalid
>>
> arguments, we
>
>> test that glEnable is not called.  Turning off caching confirms that.
>>
>
>  Separately I added specific tests (CachedColorMask, CachedDepthMask,
>> CachedStencilMask) that test ApplyDirtyState with expected caching and
>> ContextStateCapabilityCaching for glEnable/glDisable cache tests.
>>
>
> Btw, I think to have the best of both worlds it would be beneficial to
> have gpu_unittests run twice, once with a global cache disable, once
> with it enabled.  I've spoken with kbr@ and it doesn't seem like a
> problem.
>
> I'll make a start on adding a command line arg, and making the unit
> tests work in both modes.
>

FYI there's a facility in gtest to "parametrize" a test, using
::testing::TestWithParam. You can use that to run the same test
with/without caching without having to meddle with command lines.


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

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

[vmiura, 2014-04-30 18:59:59]

Thanks for the tip!  I've parameterized the tests. PTAL

[piman, 2014-04-30 22:15:16]

lgtm

[vmiura, 2014-04-30 22:23:12]

The CQ bit was checked by vmiura@chromium.org

[vmiura, 2014-04-30 22:23:28]

The CQ bit was unchecked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:23:47]

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

[vmiura, 2014-04-30 22:24:23]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:27:01]

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

[vmiura, 2014-04-30 22:28:43]

The CQ bit was unchecked by vmiura@chromium.org

[vmiura, 2014-04-30 22:32:21]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:33:44]

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

[commit-bot: I haz the power, 2014-05-01 05:56:10]

Change committed as 267450

[tapted, 2014-05-01 07:40:24]

A revert of this CL has been created in
https://codereview.chromium.org/262793002/ by tapted@chromium.org.

The reason for reverting is: Failing compile on win64 debug waterfall bot

gpu\command_buffer\service\gles2_cmd_decoder_unittest.cc : fatalerror C1128:
number of sections exceeded object file format limit : compile with /bigobj

link:
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder%20%28db....

[vmiura, 2014-05-01 22:09:44]

On 2014/05/01 07:40:24, tapted wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/262793002/ by mailto:tapted@chromium.org.
> 
> The reason for reverting is: Failing compile on win64 debug waterfall bot
> 
> gpu\command_buffer\service\gles2_cmd_decoder_unittest.cc : fatalerror C1128:
> number of sections exceeded object file format limit : compile with /bigobj
> 
> link:
>
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder%20%28db....

@piman, would you split gles2_cmd_decoder_unittest.cc, or go for "/bigobj"?

[piman, 2014-05-01 22:32:38]

On 2014/05/01 22:09:44, vmiura wrote:
> On 2014/05/01 07:40:24, tapted wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/262793002/ by mailto:tapted@chromium.org.
> > 
> > The reason for reverting is: Failing compile on win64 debug waterfall bot
> > 
> > gpu\command_buffer\service\gles2_cmd_decoder_unittest.cc : fatalerror C1128:
> > number of sections exceeded object file format limit : compile with /bigobj
> > 
> > link:
> >
>
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder%20%28db....
> 
> @piman, would you split gles2_cmd_decoder_unittest.cc, or go for "/bigobj"?

It's already split, so splitting some more is reasonable. Those files are huge.

[vmiura, 2014-05-03 00:27:28]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2014-05-03 00:27:51]

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

[commit-bot: I haz the power, 2014-05-03 02:49:47]

Change committed as 268017