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

[Tapani Pälli]

On 11/06/2013 12:02 AM, Paul Berry wrote:
> On 1 November 2013 02:16, Tapani Pälli <tapani.palli at intel.com 
> <mailto: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.


> 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/a3e5f547/attachment.html>