The Shader class now support deferred shader compiling.

The Shader class now support deferred shader compiling.

In preperation for deferring the shader compiles within the gles2
command decoder, I have added additional shader states which contains
all the information we need to support deferred shader compiling.

Once RequestCompile() is called, when DoCompile() is called it will
compile the last shader set when RequestCompile() was called.
RequestCompile() must be called before DoCompile(), or it is assumed
the user did not call glCompileShader and the shader will continually
be invalid.

Additionally, the |signature_source_| has been renamed to a more
descriptive |last_compiled_source_|. The |last_compiled_source_|
corresponds to the last shader set during either RequestCompile().
This string should be used as the key for any shader caches before
linking.

R=kbr@chromium.org, vmiura@chromium.org
BUG=450690
TEST=gpu_unittests && trybots

Committed: https://crrev.com/ef47e29f950b03afa9a3ec81282480476138a20f
Cr-Commit-Position: refs/heads/master@{#314817}

[vmiura, 2015-02-04 21:18:56]

lgtm

https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:52: // the time of the compile
request is the proper source.
nit: DCHECK(shader_state_ != kShaderStateCompiled)

[David Yen, 2015-02-04 22:02:14]

New patchsets have been uploaded after l-g-t-m from vmiura@chromium.org

[David Yen, 2015-02-04 22:05:40]

On 2015/02/04 21:18:56, vmiura wrote:
> lgtm
> 
>
https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
> File gpu/command_buffer/service/shader_manager.cc (right):
> 
>
https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
> gpu/command_buffer/service/shader_manager.cc:52: // the time of the compile
> request is the proper source.
> nit: DCHECK(shader_state_ != kShaderStateCompiled)

As discussed, I made RequestCompile() mandatory before calling DoCompile().
DoCompile() will simply return if RequestCompile() was not called. This would
only happen if user never called glCompileShader, or user called glLinkProgram()
or glGetShaderiv() multiple times (in which case we wouldn't compile it multiple
times).

PTAL, will wait for another lgtm.

[David Yen, 2015-02-04 22:05:49]

https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/894093004/diff/1/gpu/command_buffer/service/s...
gpu/command_buffer/service/shader_manager.cc:52: // the time of the compile
request is the proper source.
On 2015/02/04 21:18:56, vmiura wrote:
> nit: DCHECK(shader_state_ != kShaderStateCompiled)

After discussing in person, we decided to simplify the API to only support
Compile after RequestCompile.

[Ken Russell (switch to Gerrit), 2015-02-04 22:59:59]

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

https://codereview.chromium.org/894093004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7092: // requested. Eventually
this should be deffered to the linker stage.
typo: deferred

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

https://codereview.chromium.org/894093004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_manager.cc:48: return;
The lack of error checking in this new code path seems problematic. It seems
shaders can transition from the "compiled" state back to the "compile requested"
state and back? Are there any assertions that can be made about the shader's
state? What happens if a new compilation request comes in while one is underway?
Etc.

Have you thought about how guarantees like those in section 2.10.3 "Program
Objects" of the OpenGL ES 3.0.3 spec are going to be maintained with this coming
asynchronous compilation and linking? For example, if a program's in use (via
UseProgram), and it's recompiled/relinked and the relink fails, then the
currently active program does not change. I'm not sure how the command buffer
handles this today.

[David Yen, 2015-02-05 00:09:02]

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

https://codereview.chromium.org/894093004/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_manager.cc:48: return;
On 2015/02/04 22:59:59, Ken Russell wrote:
> The lack of error checking in this new code path seems problematic. It seems
> shaders can transition from the "compiled" state back to the "compile
requested"
> state and back? Are there any assertions that can be made about the shader's
> state? What happens if a new compilation request comes in while one is
underway?
> Etc.
> 
> Have you thought about how guarantees like those in section 2.10.3 "Program
> Objects" of the OpenGL ES 3.0.3 spec are going to be maintained with this
coming
> asynchronous compilation and linking? For example, if a program's in use (via
> UseProgram), and it's recompiled/relinked and the relink fails, then the
> currently active program does not change. I'm not sure how the command buffer
> handles this today.

That functionality will still work, it is handled inside of
GLES2DecoderImpl::DoLinkProgram, it sets the current program after a successful
link if it is the currently used program.

Regarding the functionality of the compiler I believe returning here does the
right thing. Here are the relevant points from the spec:

  - If CompileShader failed, any information about a previous compile is lost.
Thus a failed compile does not restore the old state of shader.
  - Changing the source code of a shader object with ShaderSource does not
change its compile status or the compiled shader code

The way the current code is written, a valid shader must have both the state be
"compiled" and have the "valid" variable to be true. There are a couple of
situations that arise here:

1. User does not set the source and calls glGetShaderiv() or glLinkProgram(),
this will in turn calls DoCompile() which will immediately return since the
state is "waiting". The shader will be invalid which is correct.

2. User sets the source, calls glCompileShader(), then glGetShaderiv() or
glLinkProgram(). glCompileShader() will set the state to "CompileRequested" and
DoCompile() will set the state to "Compiled" while valid will be set to whether
or not the compile succeeded. This is the normal case and works as intended.

3. User finishes compiling something (so the state is "compiled"), then sets the
source using glSetSource, but then calls glGetShaderiv() or glLinkProgram()
without calling glCompileShader(). In that case the compiled shader should be
the originally compiled state and DoCompile() should not do anything (until
glCompileShader() is called and thus changes the state back to
"RequestCompile"). This also exhibits the correct behavior if you read the 2nd
bullet point I included from the spec.

4. User finished compiling something, then continually calls either
glGetShaderiv() or glLinkProgram(). This will not recompile the shader and just
continually return and not change the compile state or the valid boolean. This
is also the correct behavior.

I believe these 4 cases cover all the different cases that can happen. Let me
know if you think I have missed anything.

Perhaps I should document them in the comments?

[Ken Russell (switch to Gerrit), 2015-02-05 02:11:46]

On 2015/02/05 00:09:02, David Yen wrote:
>
https://codereview.chromium.org/894093004/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/shader_manager.cc (right):
> 
>
https://codereview.chromium.org/894093004/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/shader_manager.cc:48: return;
> On 2015/02/04 22:59:59, Ken Russell wrote:
> > The lack of error checking in this new code path seems problematic. It seems
> > shaders can transition from the "compiled" state back to the "compile
> requested"
> > state and back? Are there any assertions that can be made about the shader's
> > state? What happens if a new compilation request comes in while one is
> underway?
> > Etc.
> > 
> > Have you thought about how guarantees like those in section 2.10.3 "Program
> > Objects" of the OpenGL ES 3.0.3 spec are going to be maintained with this
> coming
> > asynchronous compilation and linking? For example, if a program's in use
(via
> > UseProgram), and it's recompiled/relinked and the relink fails, then the
> > currently active program does not change. I'm not sure how the command
buffer
> > handles this today.
> 
> That functionality will still work, it is handled inside of
> GLES2DecoderImpl::DoLinkProgram, it sets the current program after a
successful
> link if it is the currently used program.
> 
> Regarding the functionality of the compiler I believe returning here does the
> right thing. Here are the relevant points from the spec:
> 
>   - If CompileShader failed, any information about a previous compile is lost.
> Thus a failed compile does not restore the old state of shader.
>   - Changing the source code of a shader object with ShaderSource does not
> change its compile status or the compiled shader code
> 
> The way the current code is written, a valid shader must have both the state
be
> "compiled" and have the "valid" variable to be true. There are a couple of
> situations that arise here:
> 
> 1. User does not set the source and calls glGetShaderiv() or glLinkProgram(),
> this will in turn calls DoCompile() which will immediately return since the
> state is "waiting". The shader will be invalid which is correct.
> 
> 2. User sets the source, calls glCompileShader(), then glGetShaderiv() or
> glLinkProgram(). glCompileShader() will set the state to "CompileRequested"
and
> DoCompile() will set the state to "Compiled" while valid will be set to
whether
> or not the compile succeeded. This is the normal case and works as intended.
> 
> 3. User finishes compiling something (so the state is "compiled"), then sets
the
> source using glSetSource, but then calls glGetShaderiv() or glLinkProgram()
> without calling glCompileShader(). In that case the compiled shader should be
> the originally compiled state and DoCompile() should not do anything (until
> glCompileShader() is called and thus changes the state back to
> "RequestCompile"). This also exhibits the correct behavior if you read the 2nd
> bullet point I included from the spec.
> 
> 4. User finished compiling something, then continually calls either
> glGetShaderiv() or glLinkProgram(). This will not recompile the shader and
just
> continually return and not change the compile state or the valid boolean. This
> is also the correct behavior.
> 
> I believe these 4 cases cover all the different cases that can happen. Let me
> know if you think I have missed anything.
> 
> Perhaps I should document them in the comments?

This analysis sounds good overall.

I'm not sure how you're going to manage the asynchronous compilations. Assuming
there's a worker thread, then for situation (3) there will need to be
synchronization between the offline compilation, and the attempt to set new
source on the shader object. Either you'll need to queue up the work for a
particular shader, or wait for the asynchronous compilation to finish before the
new SetSource command can run. We'll have to wait and see what the code looks
like.

One more point is that it's legal for the GLSL driver to defer compilation until
link time. Mobile drivers frequently do this. Chrome's
"compile-shader-always-succeeds" command line option mimics this behavior on
desktop drivers. We need to make sure that the "valid" state is still correctly
computed even if compilation and linking are done asynchronously. It may well be
the case that your current plan will handle this correctly.

LGTM too, though I'm not an owner.

[vmiura, 2015-02-05 03:17:04]

> I'm not sure how you're going to manage the asynchronous compilations.
Assuming
> there's a worker thread, then for situation (3) there will need to be
> synchronization between the offline compilation, and the attempt to set new
> source on the shader object.

There is no plan for asynchronous compilation I think, just deferring
glCompileShader() until it's results are needed either in a glGetShaderInfoLog()
or a glLinkProgram() that doesn't hit the in-memory cache.  Within that, the
tracking David is going looks good.

As far as asynchronously compiling shaders, that is something we may want to
look into at some point.  I suspect it would be a mixed bag in terms of:
 - Drivers not compiling on separate threads in a non-blocking manner.
 - Drivers deferring some of the compilation until a program is used in Draws.

[vmiura, 2015-02-05 03:19:52]

lgtm

[David Yen, 2015-02-05 15:48:16]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-02-05 15:48:34]

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

[commit-bot: I haz the power, 2015-02-05 15:52:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-05 15:53:21]

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

[Ken Russell (switch to Gerrit), 2015-02-06 02:04:04]

On 2015/02/05 03:17:04, vmiura wrote:
> > I'm not sure how you're going to manage the asynchronous compilations.
> Assuming
> > there's a worker thread, then for situation (3) there will need to be
> > synchronization between the offline compilation, and the attempt to set new
> > source on the shader object.
> 
> There is no plan for asynchronous compilation I think, just deferring
> glCompileShader() until it's results are needed either in a
glGetShaderInfoLog()
> or a glLinkProgram() that doesn't hit the in-memory cache.  Within that, the
> tracking David is going looks good.

OK, I misunderstood. I thought the command buffer was already deferring shader
compiles until they were needed.


> As far as asynchronously compiling shaders, that is something we may want to
> look into at some point.  I suspect it would be a mixed bag in terms of:
>  - Drivers not compiling on separate threads in a non-blocking manner.
>  - Drivers deferring some of the compilation until a program is used in Draws.

Yes, I agree it's problematic for those reasons.