Long name mapping needs to be consistent between vertex/fragment shaders.

Long name mapping needs to be consistent between vertex/fragment shaders.

For example: varying variables, uniforms.

This CL makes MapLongVariableNames a ref-counted singleton and therefore, the map is shared by all shaders.

Also, function/variable names changes from Varying to Global because uniforms also need to be consistent, not just varying variables.

ANGLEBUG=279
TEST=webgl conformance tests, especially invalid-passed-params.html and glsl-long-variable-names.html
Committed: http://code.google.com/p/angleproject/source/detail?r=942

[zmo, 2012-01-12 19:51:23]

Tested with WebGL conformance tests suite.  Please review.

[kbr1, 2012-01-12 21:28:52]

Rather than making the innards of the MapLongVariableNames class static, is it
possible to leave the map as an instance variable of the MapLongVariableNames
class, and put an instance of MapVariableLongNames in TCompiler? That would be
much cleaner.

[zmo, 2012-01-12 21:41:42]

On 2012/01/12 21:28:52, kbr1 wrote:
> Rather than making the innards of the MapLongVariableNames class static, is it
> possible to leave the map as an instance variable of the MapLongVariableNames
> class, and put an instance of MapVariableLongNames in TCompiler? That would be
> much cleaner.

If we want to manage this map, it needs to be higher than TCompiler, because the
map needs to be shared between vertex shader compiler and fragment shader
compiler.  So we need to expose this map to chromium command buffer, because
that's the place that manages the two compilers.

Comparing with that, I feel using static member is a better solution.

[zmo, 2012-01-12 21:53:31]

On 2012/01/12 21:41:42, zmo wrote:
> On 2012/01/12 21:28:52, kbr1 wrote:
> > Rather than making the innards of the MapLongVariableNames class static, is
it
> > possible to leave the map as an instance variable of the
MapLongVariableNames
> > class, and put an instance of MapVariableLongNames in TCompiler? That would
be
> > much cleaner.
> 
> If we want to manage this map, it needs to be higher than TCompiler, because
the
> map needs to be shared between vertex shader compiler and fragment shader
> compiler.  So we need to expose this map to chromium command buffer, because
> that's the place that manages the two compilers.
> 
> Comparing with that, I feel using static member is a better solution.

Another option would be: leave the map as a local variable in anonymous
namespace in Compiler.cpp.

[kbr1, 2012-01-12 21:58:03]

On 2012/01/12 21:41:42, zmo wrote:
> On 2012/01/12 21:28:52, kbr1 wrote:
> > Rather than making the innards of the MapLongVariableNames class static, is
it
> > possible to leave the map as an instance variable of the
MapLongVariableNames
> > class, and put an instance of MapVariableLongNames in TCompiler? That would
be
> > much cleaner.
> 
> If we want to manage this map, it needs to be higher than TCompiler, because
the
> map needs to be shared between vertex shader compiler and fragment shader
> compiler.  So we need to expose this map to chromium command buffer, because
> that's the place that manages the two compilers.
> 
> Comparing with that, I feel using static member is a better solution.

I strongly feel that we should avoid introducing more statics which the
application has no control over and can't clean up.

One solution avoiding public API changes would be to make MapLongVariableNames
reference counted, and add a static method which returns a MapLongVariableNames
instance. Internally, it would lazily allocate a global instance, increment its
reference count and return it. The TCompiler instance would contain a
MapLongVariableNames* and would release it in its destructor. Is this feasible?
If so, could you implement that instead?

[zmo, 2012-01-12 22:01:56]

On 2012/01/12 21:58:03, kbr1 wrote:
> On 2012/01/12 21:41:42, zmo wrote:
> > On 2012/01/12 21:28:52, kbr1 wrote:
> > > Rather than making the innards of the MapLongVariableNames class static,
is
> it
> > > possible to leave the map as an instance variable of the
> MapLongVariableNames
> > > class, and put an instance of MapVariableLongNames in TCompiler? That
would
> be
> > > much cleaner.
> > 
> > If we want to manage this map, it needs to be higher than TCompiler, because
> the
> > map needs to be shared between vertex shader compiler and fragment shader
> > compiler.  So we need to expose this map to chromium command buffer, because
> > that's the place that manages the two compilers.
> > 
> > Comparing with that, I feel using static member is a better solution.
> 
> I strongly feel that we should avoid introducing more statics which the
> application has no control over and can't clean up.
> 
> One solution avoiding public API changes would be to make MapLongVariableNames
> reference counted, and add a static method which returns a
MapLongVariableNames
> instance. Internally, it would lazily allocate a global instance, increment
its
> reference count and return it. The TCompiler instance would contain a
> MapLongVariableNames* and would release it in its destructor. Is this
feasible?
> If so, could you implement that instead?

This sounds good.  I'll do this then.

[apatrick1, 2012-01-12 23:07:38]

The map should probably be protected with some kind of lock. The shader compiler
gets called on multiple threads. As one example, in chrome single process mode,
it could be called on the renderer thread by WebKit and the GPU thread by ANGLE.

As an alternative to a global map, how about just appending a cryptographic hash
of the long name to the end of the shortened name? That would also avoid the
unbounded growth of the map.

[zmo, 2012-01-12 23:50:54]

On 2012/01/12 23:07:38, apatrick1 wrote:
> The map should probably be protected with some kind of lock. The shader
compiler
> gets called on multiple threads. As one example, in chrome single process
mode,
> it could be called on the renderer thread by WebKit and the GPU thread by
ANGLE.
> 
> As an alternative to a global map, how about just appending a cryptographic
hash
> of the long name to the end of the shortened name? That would also avoid the
> unbounded growth of the map.

Per offline discussion, we will go ahead with Ken's suggestion and not dealing
with thread safety for now.

[zmo, 2012-01-12 23:59:10]

Revised and tested with WebGL Conformance Tests.  Please have another look.

[kbr1, 2012-01-13 00:23:55]

Thanks. Big thumbs up. LGTM