[Mesa-dev] [PATCH 08/37] glsl: add initial implementation of shader cache

[Timothy Arceri]

On Tue, 2017-01-24 at 07:54 +0200, Tapani Pälli wrote:
> Hi Timothy;
> 
> On 01/24/2017 01:12 AM, Timothy Arceri wrote:
> > From: Timothy Arceri <timothy.arceri at collabora.com>
> > 
> 
> 8<
> 
> > +
> > +static void
> > +write_uniforms(struct blob *metadata, struct gl_shader_program
> > *prog)
> > +{
> > +   blob_write_uint32(metadata, prog->SamplersValidated);
> > +   blob_write_uint32(metadata, prog->data->NumUniformStorage);
> > +   blob_write_uint32(metadata, prog->data->NumUniformDataSlots);
> > +
> > +   for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
> > +      encode_type_to_blob(metadata, prog->data-
> > >UniformStorage[i].type);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].array_elements);
> > +      blob_write_string(metadata, prog->data-
> > >UniformStorage[i].name);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].storage -
> > +                                  prog->data->UniformDataSlots);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].remap_location);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].block_index);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].atomic_buffer_index);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].offset);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].array_stride);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].matrix_stride);
> > +      blob_write_uint32(metadata, prog->data-
> > >UniformStorage[i].row_major);
> > +      blob_write_uint32(metadata,
> > +                        prog->data-
> > >UniformStorage[i].num_compatible_subroutines);
> > +      blob_write_uint32(metadata,
> > +                        prog->data-
> > >UniformStorage[i].top_level_array_size);
> > +      blob_write_uint32(metadata,
> > +                        prog->data-
> > >UniformStorage[i].top_level_array_stride);
> > +   }
> > +}
> 
> I realize I'm repeating myself here but please do consider read/write
> a 
> complete structure instead of individual fields whenever possible 

Yes. Since that last time you mentioned this I have since replaced any
read/write where the structure contains no pointers with whole
structure read/writes.

> (this 
> can be applied to many structures in this patch set). The reason is
> not 
> just to optimize, it is to make this code more maintainable and
> robust 
> so that it won't break when these structures change.

It will still break if a pointer is introduced. My thinking was its
more clear to just read/write all fields when ever we end up messing
about with pointers, other may disagree. I'm aware of the potential for
things to break but at this stage in OpenGL's life cycle the likelihood
is minimal and any breakage should definitely get picked up by the test
suites.

This struct is a bit of an exception where there are a large amount of
integers and just two pointers but I think you will find all other
structs not read/written as a whole in this file have a high percentage
of pointers as fields.

If more people agree with your point of view I'll change it :)


>  Ideally cache code 
> does not have to be touched when that happens. Cache can contain
> sizes 
> of structures so that we can just simply remove entries that were
> saved 
> with old ones to avoid issues.

We just keep track of Mesa versions to make that unnecessary :)

> 
> > +
> > +static void
> > +read_uniforms(struct blob_reader *metadata, struct
> > gl_shader_program *prog)
> > +{
> > +   struct gl_uniform_storage *uniforms;
> > +   union gl_constant_value *data;
> > +
> > +   prog->SamplersValidated = blob_read_uint32(metadata);
> > +   prog->data->NumUniformStorage = blob_read_uint32(metadata);
> > +   prog->data->NumUniformDataSlots = blob_read_uint32(metadata);
> > +
> > +   uniforms = rzalloc_array(prog, struct gl_uniform_storage,
> > +                            prog->data->NumUniformStorage);
> > +   prog->data->UniformStorage = uniforms;
> > +
> > +   data = rzalloc_array(uniforms, union gl_constant_value,
> > +                        prog->data->NumUniformDataSlots);
> > +   prog->data->UniformDataSlots = data;
> > +
> > +   prog->UniformHash = new string_to_uint_map;
> > +
> > +   for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
> > +      uniforms[i].type = decode_type_from_blob(metadata);
> > +      uniforms[i].array_elements = blob_read_uint32(metadata);
> > +      uniforms[i].name = ralloc_strdup(prog, blob_read_string
> > (metadata));
> > +      uniforms[i].storage = data + blob_read_uint32(metadata);
> > +      uniforms[i].remap_location = blob_read_uint32(metadata);
> > +      uniforms[i].block_index = blob_read_uint32(metadata);
> > +      uniforms[i].atomic_buffer_index =
> > blob_read_uint32(metadata);
> > +      uniforms[i].offset = blob_read_uint32(metadata);
> > +      uniforms[i].array_stride = blob_read_uint32(metadata);
> > +      uniforms[i].matrix_stride = blob_read_uint32(metadata);
> > +      uniforms[i].row_major = blob_read_uint32(metadata);
> > +      uniforms[i].num_compatible_subroutines =
> > blob_read_uint32(metadata);
> > +      uniforms[i].top_level_array_size =
> > blob_read_uint32(metadata);
> > +      uniforms[i].top_level_array_stride =
> > blob_read_uint32(metadata);
> > +      prog->UniformHash->put(i, uniforms[i].name);
> > +   }
> > +}
> > +
> 
> // Tapani