Interface design for user-defined name hashing.

Interface design for user-defined name hashing.

1) We use BuiltInResources to pass the hash function to 
   ANGLE, deciding whether we applies hash function or not.
2) We use 64 bits hashing function, because 64 bits is 16
   bytes using hex representation, plus the "webgl_" prefix,
   we can keep the names under 128 (WebGL allows 5 levels of
   nesting in structures).  If chooseing 128 bits, we will
   go beyond 128 characters, and some drivers can't handle
   that safely.

ANGLEBUG=315
Committed: https://code.google.com/p/angleproject/source/detail?r=1384

[Zhenyao Mo, 2012-11-02 19:38:02]

Ken, Bonoit: please have a look and let me know if this interface is reasonable.

[kbr1, 2012-11-02 23:25:11]

+alokp and gman for additional review of API
Correcting (I believe) Benoit's email address

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:295: // SH_HASHED_NAMES_COUNT: the number of hashed
names.
Perhaps a note that SH_HASHED_NAMES_COUNT will change over time as ShCompile is
called? See also question below.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:379: // index: Specifies the index of the name
hashing entry to be queried.
Some questions:

1) Is the calling code expected to just iterate from
1..ShGetInfo(SH_HASHED_NAMES_COUNT) and build up its own table? If so this seems
to defeat the purpose of ANGLE maintaining its own map internally.

2)  How or when ANGLE's table could ever be cleared out?

3) There seem to be two main use cases for this API:

3a) Mapping from a hashed uniform or attribute name to its original name.

3b) Processing shader compilation and program linking logs to map all
identifiers (which are hashed) back to their original names.

If the caller is expected to build up a map for each shader object they compile,
then perhaps ANGLE needs to clear out its map at the beginning of ShCompile, and
the description for SH_HASHED_NAMES_COUNT needs to indicate that this value is
reset every time ShCompile is called.

Otherwise, if ANGLE is maintaining a persistent table for each ShCompiler
instance, then maybe it should just provide APIs for looking up original uniform
and variable names, and post-processing a shader or program info log.

https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (right):

https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
src/compiler/ShaderLang.cpp:239: *params = 16 + sizeof(HashedNamePrefix);
Is this length computation missing the null terminator?

[Zhenyao Mo, 2012-11-02 23:42:51]

Thanks for reviewing this.  I'll wait until I gather all the comments to do the
next update.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:295: // SH_HASHED_NAMES_COUNT: the number of hashed
names.
On 2012/11/02 23:25:12, kbr1 wrote:
> Perhaps a note that SH_HASHED_NAMES_COUNT will change over time as ShCompile
is
> called? See also question below.

This value is only valid for the previous shader compilation.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:379: // index: Specifies the index of the name
hashing entry to be queried.
On 2012/11/02 23:25:12, kbr1 wrote:
> Some questions:
> 
> 1) Is the calling code expected to just iterate from
> 1..ShGetInfo(SH_HASHED_NAMES_COUNT) and build up its own table? If so this
seems
> to defeat the purpose of ANGLE maintaining its own map internally.

No ANGLE will not maintain the own map.

> 
> 2)  How or when ANGLE's table could ever be cleared out?

It will discard the previous map when compiling a new shader.

> 
> 3) There seem to be two main use cases for this API:
> 
> 3a) Mapping from a hashed uniform or attribute name to its original name.
> 
> 3b) Processing shader compilation and program linking logs to map all
> identifiers (which are hashed) back to their original names.
> 
> If the caller is expected to build up a map for each shader object they
compile,
> then perhaps ANGLE needs to clear out its map at the beginning of ShCompile,
and
> the description for SH_HASHED_NAMES_COUNT needs to indicate that this value is
> reset every time ShCompile is called.

OK, I'll update the description.

> 
> Otherwise, if ANGLE is maintaining a persistent table for each ShCompiler
> instance, then maybe it should just provide APIs for looking up original
uniform
> and variable names, and post-processing a shader or program info log.

[bjacob, 2012-11-03 00:13:10]

2012/11/2  <kbr@chromium.org>:
> +alokp and gman for additional review of API
> Correcting (I believe) Benoit's email address
>
>
>
> https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
> File include/GLSLANG/ShaderLang.h (right):
>
>
https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
> include/GLSLANG/ShaderLang.h:295: // SH_HASHED_NAMES_COUNT: the number
> of hashed names.
> Perhaps a note that SH_HASHED_NAMES_COUNT will change over time as
> ShCompile is called? See also question below.
>
>
https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
> include/GLSLANG/ShaderLang.h:379: // index: Specifies the index of the
> name hashing entry to be queried.
> Some questions:
>
> 1) Is the calling code expected to just iterate from
> 1..ShGetInfo(SH_HASHED_NAMES_COUNT) and build up its own table? If so
> this seems to defeat the purpose of ANGLE maintaining its own map
> internally.

It would be really nice if user code could rely on ANGLE's hash tables
instead of having to maintain its own.

From an API perspective, this requires allowing to do backward
mappings (give mapped name, obtain original long name).

Am I missing something else?

>
> 2)  How or when ANGLE's table could ever be cleared out?

If the hash tables were properties of the program object (as opposed
to having a single per-compiler table), then they could be cleared
whenever the program is relinked or deleted.

>
> 3) There seem to be two main use cases for this API:
>
> 3a) Mapping from a hashed uniform or attribute name to its original
> name.

And conversely, of course, mapping am original long name to its hash form.

>
> 3b) Processing shader compilation and program linking logs to map all
> identifiers (which are hashed) back to their original names.
>
> If the caller is expected to build up a map for each shader object they
> compile, then perhaps ANGLE needs to clear out its map at the beginning
> of ShCompile, and the description for SH_HASHED_NAMES_COUNT needs to
> indicate that this value is reset every time ShCompile is called.
>
> Otherwise, if ANGLE is maintaining a persistent table for each
> ShCompiler instance, then maybe it should just provide APIs for looking
> up original uniform and variable names, and post-processing a shader or
> program info log.

Again, I believe that this would be well addressed by having ANGLE
maintain per-program tables and expose them to the user, so the user
would only have to query them instead of building his own.

Benoit

>
> https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
> File src/compiler/ShaderLang.cpp (right):
>
>
https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
> src/compiler/ShaderLang.cpp:239: *params = 16 +
> sizeof(HashedNamePrefix);
> Is this length computation missing the null terminator?
>
> https://codereview.appspot.com/6822077/

[kbr1, 2012-11-03 00:54:09]

On 2012/11/03 00:13:10, bjacob wrote:
> If the hash tables were properties of the program object (as opposed
> to having a single per-compiler table), then they could be cleared
> whenever the program is relinked or deleted.

ANGLE doesn't maintain shader or program objects on any platform where just the
shader translator/validator is used. Therefore the only two options for the
shader translator API are (a) have ANGLE maintain one table per compiler
instance which grows over time, or (b) clear out the table after each
compilation. After reading Mo's responses, (b) sounds better aligned with how
the other APIs in this header work.

[kbr1, 2012-11-03 00:54:44]

https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (right):

https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
src/compiler/ShaderLang.cpp:239: *params = 16 + sizeof(HashedNamePrefix);
On 2012/11/02 23:25:12, kbr1 wrote:
> Is this length computation missing the null terminator?

Mo, what about this question?

[dgkoch, 2012-11-03 17:30:26]

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:125: SH_HASHED_NAMES_COUNT          =  0x8B8E,
Don't leave a trailing comma. Some compilers don't like that.

[Alok Priyadarshi, 2012-11-06 17:24:32]

A few nits.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:26: #if !defined(uint64)
rename it to sh_uint64?

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:121: SH_ACTIVE_ATTRIBUTE_MAX_LENGTH =  0x8B8A,
SH_FOO has the same value as GL_FOO. If there is no GL equivalent, you should
use a value that does not conflict with that in GL.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:177: typedef uint64 (*ShHash64)(const char*,
size_t);
rename it to ShHashNameFunction64 to make it clearer?

https://codereview.appspot.com/6822077/diff/4009/src/build_angle.gypi
File src/build_angle.gypi (right):

https://codereview.appspot.com/6822077/diff/4009/src/build_angle.gypi#newcode83
src/build_angle.gypi:83: 'compiler/HashNames.h',
rename file to hash_names.h because it does not contain a class.

https://codereview.appspot.com/6822077/diff/4009/src/compiler/HashNames.h
File src/compiler/HashNames.h (right):

https://codereview.appspot.com/6822077/diff/4009/src/compiler/HashNames.h#new...
src/compiler/HashNames.h:17: typedef std::map<std::string, std::string> NameMap;
use TPersistString

[Zhenyao Mo, 2012-11-06 18:42:10]

On 2012/11/03 00:54:44, kbr1 wrote:
> https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
> File src/compiler/ShaderLang.cpp (right):
> 
>
https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
> src/compiler/ShaderLang.cpp:239: *params = 16 + sizeof(HashedNamePrefix);
> On 2012/11/02 23:25:12, kbr1 wrote:
> > Is this length computation missing the null terminator?
> 
> Mo, what about this question?

It checked on Windows, seems like sizeof(char*) already counts the null in.

[bjacob, 2012-11-06 19:12:30]

2012/11/6  <zmo@chromium.org>:
> On 2012/11/03 00:54:44, kbr1 wrote:
>
> https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
>>
>> File src/compiler/ShaderLang.cpp (right):
>
>
>
>
https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
>>
>> src/compiler/ShaderLang.cpp:239: *params = 16 +
>
> sizeof(HashedNamePrefix);
>>
>> On 2012/11/02 23:25:12, kbr1 wrote:
>> > Is this length computation missing the null terminator?
>
>
>> Mo, what about this question?
>
>
> It checked on Windows, seems like sizeof(char*) already counts the null
> in.

Guess you meant sizeof(char[])  ;-)

Benoit

>
> https://codereview.appspot.com/6822077/

[Zhenyao Mo, 2012-11-06 19:18:56]

On 2012/11/06 19:12:30, bjacob wrote:
> 2012/11/6  <zmo@chromium.org>:
> > On 2012/11/03 00:54:44, kbr1 wrote:
> >
> > https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp
> >>
> >> File src/compiler/ShaderLang.cpp (right):
> >
> >
> >
> >
>
https://codereview.appspot.com/6822077/diff/4009/src/compiler/ShaderLang.cpp#...
> >>
> >> src/compiler/ShaderLang.cpp:239: *params = 16 +
> >
> > sizeof(HashedNamePrefix);
> >>
> >> On 2012/11/02 23:25:12, kbr1 wrote:
> >> > Is this length computation missing the null terminator?
> >
> >
> >> Mo, what about this question?
> >
> >
> > It checked on Windows, seems like sizeof(char*) already counts the null
> > in.
> 
> Guess you meant sizeof(char[])  ;-)
> 
> Benoit

right.  thanks :)

By the way, do you still have questions after kbr's reply?  about ANGLE not
maintaining the map?

> 
> >
> > https://codereview.appspot.com/6822077/

[dgkoch, 2012-11-06 21:09:20]

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:26: #if !defined(uint64)
On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> rename it to sh_uint64?

You should probably just use the khronos_uint64_t types from KHR/khrplatform.h

[Zhenyao Mo, 2012-11-06 21:27:53]

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:26: #if !defined(uint64)
On 2012/11/06 21:09:20, dgkoch wrote:
> On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> > rename it to sh_uint64?
> 
> You should probably just use the khronos_uint64_t types from KHR/khrplatform.h

Done.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:121: SH_ACTIVE_ATTRIBUTE_MAX_LENGTH =  0x8B8A,
On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> SH_FOO has the same value as GL_FOO. If there is no GL equivalent, you should
> use a value that does not conflict with that in GL.

Will do before landing.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:125: SH_HASHED_NAMES_COUNT          =  0x8B8E,
On 2012/11/03 17:30:27, dgkoch wrote:
> Don't leave a trailing comma. Some compilers don't like that.

Done.

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:177: typedef uint64 (*ShHash64)(const char*,
size_t);
On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> rename it to ShHashNameFunction64 to make it clearer?

Done.

https://codereview.appspot.com/6822077/diff/4009/src/build_angle.gypi
File src/build_angle.gypi (right):

https://codereview.appspot.com/6822077/diff/4009/src/build_angle.gypi#newcode83
src/build_angle.gypi:83: 'compiler/HashNames.h',
On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> rename file to hash_names.h because it does not contain a class.

This is only for the interface.  I will add a class and a .cpp file later in
implementation CL.

https://codereview.appspot.com/6822077/diff/4009/src/compiler/HashNames.h
File src/compiler/HashNames.h (right):

https://codereview.appspot.com/6822077/diff/4009/src/compiler/HashNames.h#new...
src/compiler/HashNames.h:17: typedef std::map<std::string, std::string> NameMap;
On 2012/11/06 17:24:32, Alok Priyadarshi wrote:
> use TPersistString

Done.

[Zhenyao Mo, 2012-11-06 21:38:11]

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/4009/include/GLSLANG/ShaderLang.h...
include/GLSLANG/ShaderLang.h:121: SH_ACTIVE_ATTRIBUTE_MAX_LENGTH =  0x8B8A,
Per offline discussion with Daniel, we will use the reserved temp range GL
values (0x6000 - 0x7fff).

[Zhenyao Mo, 2012-11-06 21:40:18]

OK, thank you all for the comments.  I think I addressed all the questions and
suggestions.  Let me know if it's good to go.

[Alok Priyadarshi, 2012-11-06 22:07:02]

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:26: #include "KHR/khrplatform.h"
We have so far tried to keep it independent of GL headers. So including KHR
headers is unfortunate. I think I prefer the earlier patch with sh_ prefix for
typename.

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:115: SH_MAPPED_NAME_MAX_LENGTH      =  0x8B8B,
could you also change SH_MAPPED_NAME_MAX_LENGTH to use reserved values.

[kbr1, 2012-11-06 22:33:58]

LGTM. A few minor comments and questions.

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:26: #include "KHR/khrplatform.h"
On 2012/11/06 22:07:02, Alok Priyadarshi wrote:
> We have so far tried to keep it independent of GL headers. So including KHR
> headers is unfortunate. I think I prefer the earlier patch with sh_ prefix for
> typename.

ANGLE and its users already have some dependencies on the GL and KHR headers
and, having looked at how complex the #ifdefs are around the khronos_uint64_t
type, I agree with Daniel to just reuse that typedef rather than doing a one-off
one here.

https://codereview.appspot.com/6822077/diff/12001/src/compiler/HashNames.h
File src/compiler/HashNames.h (right):

https://codereview.appspot.com/6822077/diff/12001/src/compiler/HashNames.h#ne...
src/compiler/HashNames.h:15: const char HashedNamePrefix[] = "webgl_";
Is it wise and/or necessary to put the definition of this constant in this
header file? Or should it go in some .cpp file?

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (right):

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp...
src/compiler/ShaderLang.cpp:332: strncpy(name, it->first.c_str(), len);
Would it be wise to write a '\0' at the end of "name" just to prevent against
accidental future bugs where the source happens to be longer than expected?

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp...
src/compiler/ShaderLang.cpp:341: strncpy(hashedName, it->second.c_str(), len);
Same question here about hashedName.

[Zhenyao Mo, 2012-11-06 23:20:51]

Revised, please have one more look.

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang.h
File include/GLSLANG/ShaderLang.h (right):

https://codereview.appspot.com/6822077/diff/12001/include/GLSLANG/ShaderLang....
include/GLSLANG/ShaderLang.h:115: SH_MAPPED_NAME_MAX_LENGTH      =  0x8B8B,
On 2012/11/06 22:07:02, Alok Priyadarshi wrote:
> could you also change SH_MAPPED_NAME_MAX_LENGTH to use reserved values.

Done.

https://codereview.appspot.com/6822077/diff/12001/src/compiler/HashNames.h
File src/compiler/HashNames.h (right):

https://codereview.appspot.com/6822077/diff/12001/src/compiler/HashNames.h#ne...
src/compiler/HashNames.h:15: const char HashedNamePrefix[] = "webgl_";
On 2012/11/06 22:33:59, kbr1 wrote:
> Is it wise and/or necessary to put the definition of this constant in this
> header file? Or should it go in some .cpp file?

sizeof(char[]) only works when it's defined.  So I changed the code a little bit
to achieve that.

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp
File src/compiler/ShaderLang.cpp (right):

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp...
src/compiler/ShaderLang.cpp:332: strncpy(name, it->first.c_str(), len);
On 2012/11/06 22:33:59, kbr1 wrote:
> Would it be wise to write a '\0' at the end of "name" just to prevent against
> accidental future bugs where the source happens to be longer than expected?

Done.

https://codereview.appspot.com/6822077/diff/12001/src/compiler/ShaderLang.cpp...
src/compiler/ShaderLang.cpp:341: strncpy(hashedName, it->second.c_str(), len);
On 2012/11/06 22:33:59, kbr1 wrote:
> Same question here about hashedName.

Done.

[kbr1, 2012-11-06 23:22:57]

LGTM again.

[Zhenyao Mo, 2012-11-07 00:18:59]

Interface design for user-defined name hashing.

1) We use BuiltInResources to pass the hash function to 
   ANGLE, deciding whether we applies hash function or not.
2) We use 64 bits hashing function, because 64 bits is 16
   bytes using hex representation, plus the "webgl_" prefix,
   we can keep the names under 128 (WebGL allows 5 levels of
   nesting in structures).  If chooseing 128 bits, we will
   go beyond 128 characters, and some drivers can't handle
   that safely.

ANGLEBUG=315