[Mesa-dev] [PATCH 3/4] glsl: add ir_cache class and functions for shader serialization

[Paul Berry]

On 29 October 2013 00:53, Tapani <tapani.palli at intel.com> wrote:

> On 10/28/2013 10:09 PM, Paul Berry wrote:
>
> +static uint32_t _unique_id(ir_variable *var)
>> +{
>> +   char buffer[256];
>> +   _mesa_snprintf(buffer, 256, "%s_%p", var->name, var);
>> +   return _mesa_str_checksum(buffer);
>>
>
>  Two problems:
>
>  1. This is a lot of work to obtain a unique identifier.  The pointer is
> already unique; we should just use that.
>
>
> I was originally using a pointer but then there's the 32 vs 64bit address
> problem. I wanted to avoid usage of pointers.
>
>
>    2. There's no guarantee that the checksum of the string will be
> unique.  Because of the birthday paradox, checksum collisions are probably
> going to happen all the time.
>
>
> I think there is because the pointer address is unique and is used as part
> of the string.
>

Yes, but the checksumming process destroys uniqueness (it has to, because
the output of the checksum operation is a 32-bit int, and the input is a
string.  There are more than 2^32 possible strings, so by the pigeon hole
principle at least one checksum must correspond to more than one possible
input string).  In the case of _mesa_str_checksum() (which is not a
particularly good hash from a cryptographic perspective) the collisions are
pretty easy to come by.  For example, these were the first two collisions I
found by writing a quick test program:

   printf("checksum of \"%s\" is 0x%x\n", "foo_0000003c",
          _mesa_str_checksum("foo_0000003c"));
   printf("checksum of \"%s\" is 0x%x\n", "foo_000001a8",
          _mesa_str_checksum("foo_000001a8"));

checksum of "foo_0000003c" is 0x1360
checksum of "foo_000001a8" is 0x1360



>
>
>    Instead of going to a lot of work to generate a unique id for each
> ir_variable, I think we should just serialize the ir_variable pointer
> address.  If we are trying to use the same binary format for 32-bit and
> 64-bit builds (see my clarifying question at the top of this email), we'll
> need to extend the pointer to 64 bits before serializing it.
>
>  Alternatively, if you want to keep the unique ID's small, we'll have to
> use some hashtable mechanism to map each variable to its own ID.
>
>
> OK, I will verify if this is a problem. For me it does not seem like a
> problem as I'm using the address there.
>
>
>
>
>> +}
>> +
>> +
>> +/* data required to serialize ir_variable */
>> +struct ir_cache_variable_data
>>
>
>  As with glsl_type_data, I don't understand why we need to have this
> class rather than just adding a serialize() method to the ir_variable class.
>
>
>
> This could be a serialize function in ir_cache_serialize.cpp. I did not
> want to touch existing ir classes, I think it helps to have the cache code
> separated and in one place. This is of course debatable, every ir
> instruction could have serialize() and unserialize() to deal with this.
>

My chief worry with separating them is that people will forget to update
the serialization code when they change the ir class, because the thing
they need to update will be far away.  This is particularly a problem when
adding new members to the class (something we do quite frequently when
implementing new GLSL features).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131029/896c5382/attachment-0001.html>