[Mesa-dev] [v2 5/6] glsl: functions to serialize gl_shader and gl_shader_program

[Paul Berry]

On 6 November 2013 00:59, Tapani Pälli <tapani.palli at intel.com> wrote:

>  On 11/06/2013 12:02 AM, Paul Berry wrote:
>
> On 1 November 2013 02:16, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>> +static void
>> +calc_item(const void *key, void *data, void *closure)
>>
>
>  How about "increment_count" for the name of this function?
>
>
> ok
>
>
>
>
>> +{
>> +   unsigned *sz = (unsigned *) closure;
>> +   *sz = *sz + 1;
>> +}
>> +
>> +
>> +static unsigned
>> +_hash_table_size(struct string_to_uint_map *map)
>>
>
>  This function is global, so its name can't start with an
> underscore--such names are reserved for library functions.  Same goes for
> many of the other functions in this file.
>
>
> will change
>
>
>
>
>> +{
>> +   unsigned size = 0;
>> +   map->iterate(calc_item, &size);
>> +   return size;
>> +}
>> +
>> +
>> +static void
>> +serialize_item(const void *key, void *data, void *closure)
>> +{
>> +   memory_writer *blob = (memory_writer *) closure;
>> +   uint32_t value = ((intptr_t)data);
>> +
>> +   blob->write_string((char *)key);
>> +   blob->write_uint32(&value);
>> +}
>> +
>> +
>> +static void
>> +_serialize_hash_table(struct string_to_uint_map *map, memory_writer
>> *blob)
>> +{
>> +   uint32_t size = _hash_table_size(map);
>> +   blob->write_uint32(&size);
>> +   map->iterate(serialize_item, blob);
>>
>
>  Rather than take two passes over the hash table (one to compute its size
> and one to output its contents), why not do the trick that we do with
> overwrite() in ir_cache_serializer.cpp?  (In other words, reserve space for
> the size of the hashtable in bytes, then serialize the hashtable, then
> overwrite the reserved space with the actual size).
>
>
>
> yes this is faster, will change
>
>
>    +   /* Shaders IR, to be decided if we want these to be available */
>>
>
>  Has there been previous discussion on the mesa-dev list about whether or
> not we want these to be available?  If so, please include a link to
> previous discussion and a brief summary.  if not, what does the decision
> hinge on?  At first blush it seems to me that there's no reason to
> serialize the IR of the individual shaders, since ARB_get_program_binary
> operates on full programs rather than individual shaders.
>
>
> Nope there has been no previous discussion about this. Automatic cache
> will need these to be able to quick exit from glCompileShader (if wanted),
> it's easier for the implementation to have them as separate blobs than part
> of whole program blob though. With the extension I'm not sure if unlinked
> IR is required for possible run-time recompilations? I'd like someone to
> confirm this.
>

Yeah, you're probably right that we need to store the individual shaders'
IR for automatic caching.

For OES_get_program_binary, based on my reading of the spec, we shouldn't
store the individual shaders' IR.  ProgramBinaryOES only loads the linked
shader; it leaves the compiled shader objects (if any) alone.


>
>
>
>   Also, typically we prefer to disable code using "if (false)" rather
> than ifdefs, because that ensures that the disabled code still compiles.
> (Otherwise, it's much more likely to bit rot as the rest of the code base
> evolves).
>
>
> ok, will use if (false), I also changethe unlinked shader define to use
> your STORE_UNLINKED_SHADERS proposal
>
>
>
>   Are we going to attempt to store the back-end compiled shaders?  It
> seems like we'd have to do that in order to get a lot of the benefit of
> ARB_get_program_binary.
>
>
> Yes it is definitely required, there is big amount of time spent in
> Driver->LinkShader(). However, this was left out for now. It might be that
> several back-end blobs are needed and I'm not sure how well it works and
> what should happen when recompilation happens. This needs a bit more
> thought. At least new API is needed with driver to simply retrieve and put
> binary blobs, I would certainly like some advice how to approach the
> problem.
>
> // Tapani
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131106/2d4388ce/attachment.html>