[Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization

[Paul Berry]

On 29 January 2014 01:24, Tapani Pälli <tapani.palli at intel.com> wrote:

> Class will be used by the shader binary cache implementation.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/glsl/memory_writer.h | 188
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 188 insertions(+)
>  create mode 100644 src/glsl/memory_writer.h
>
> diff --git a/src/glsl/memory_writer.h b/src/glsl/memory_writer.h
> new file mode 100644
> index 0000000..979169f
> --- /dev/null
> +++ b/src/glsl/memory_writer.h
> @@ -0,0 +1,188 @@
> +/* -*- c++ -*- */
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#pragma once
> +#ifndef MEMORY_WRITER_H
> +#define MEMORY_WRITER_H
> +
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +
> +#include "main/hash_table.h"
> +
> +#ifdef __cplusplus
> +/**
> + * Helper class for writing data to memory
> + *
> + * This class maintains a dynamically-sized memory buffer and allows
> + * for data to be efficiently appended to it with automatic resizing.
> + */
> +class memory_writer
> +{
> +public:
> +   memory_writer() :
> +      memory(NULL),
> +      curr_size(0),
> +      pos(0)
> +   {
> +      data_hash = _mesa_hash_table_create(0, int_equal);
> +      hash_value = _mesa_hash_data(this, sizeof(memory_writer));
> +   }
> +
> +   ~memory_writer()
> +   {
> +      free(memory);
> +      _mesa_hash_table_destroy(data_hash, NULL);
> +   }
> +
> +   /* user wants to claim the memory */
> +   char *release_memory(size_t *size)
> +   {
> +      /* final realloc to free allocated but unused memory */
> +      char *result = (char *) realloc(memory, pos);
> +      *size = pos;
> +      memory = NULL;
> +      curr_size = 0;
> +      pos = 0;
> +      return result;
> +   }
> +
> +/**
> + * write functions per type
> + */
> +#define DECL_WRITER(type) void write_ ##type (const type data) {\
> +   write(&data, sizeof(type));\
> +}
> +
> +   DECL_WRITER(int32_t);
> +   DECL_WRITER(int64_t);
> +   DECL_WRITER(uint8_t);
> +   DECL_WRITER(uint32_t);
> +
> +   void write_bool(bool data)
> +   {
> +      uint8_t val = data;
> +      write_uint8_t(val);
> +   }
> +
> +   /* write function that reallocates more memory if required */
> +   void write(const void *data, int size)
> +   {
> +      if (!memory || pos > (curr_size - size))
> +         if (!grow(size))
> +            return;
>

Thanks for making the changes I asked for about error handling.  I think
this is much better than what we had before.  One minor comment: it is
helpful in debugging if we can make sure that the program stops executing
the moment an error condition is detected (because this gives us the best
chance of being able to break in with the debugger and be close to the
cause of the error).  Therefore, I'd recommend doing something like this:

if (!memory || pos > (curr_size - size)) {
   if (!grow(size)) {
      assert(!"Out of memory while serializing a shader");
      return;
   }
}

In release builds this is equivalent to what you wrote, but in debug builds
it ensures that the program will halt if it runs out of memory.  That way
if we ever happen to introduce a bug that makes the serializer go into an
infinite loop writing data, we'll hit the assertion rather than just loop
forever.


> +
> +      memcpy(memory + pos, data, size);
> +
> +      pos += size;
> +   }
> +
> +   void overwrite(const void *data, int size, int offset)
> +   {
> +      if (offset < 0 || offset + size > pos)
> +         return;
>

Similarly, there should be an assertion in this return path, since this
should never happen except in the case of a bug elsewhere in Mesa.


> +      memcpy(memory + offset, data, size);
> +   }
> +
> +   /* length is written to make reading safe */
> +   void write_string(const char *str)
> +   {
> +      uint32_t len = str ? strlen(str) : 0;
> +      write_uint32_t(len);
>

There's a problem here: write_string("") and write_string(NULL) have
indistinguishable effects on the output file.  That *might* be benign for
now (I haven't looked through all the callers to write_string to see
whether the distinction between "" and NULL matters) but even so, this is
the sort of thing that can lead to subtle bugs in the future.

I'd suggest that instead, we store strlen + 1, so that the reader can tell
the difference between NULL and "".


> +
> +      if (str)
> +         write(str, len + 1);
>

It's surprising to see you writing len + 1 bytes (so surprising that at
first I thought it must be a mistake).  Reading ahead to patch 3/10, it
looks like you did this so that you could call ralloc_strdup() in
read_string().  But that seems a little wasteful to me, not because we're
wasting a byte but because it means that read_string() traverses the string
twice:

- First it reads the length
- Then it checks that a null terminator exists in the expected place
- Then it calls ralloc_strdup(), which walks the string to see how long it
is (totally unnecessary, since the length is known)
- ralloc_strdup() calls ralloc_array() to allocate memory for the result
- ralloc_strdup() calls memcpy() to make a copy of the string
- ralloc_strdup() stores a null terminator at the end of the copied string.

Personally, I would just call ralloc_array() directly from read_string(),
then memcpy() the contents of the string and store the null terminator.  In
which case there is no need to store the null terminator when writing the
string.

But I'm nit picking at this point, so I'm not going to be a stickler about
it.  If you would prefer to just add a comment explaining why you're
serializing the null terminator, I would be ok with that too.


> +   }
> +
> +   unsigned position()
> +   {
> +      return pos;
> +   }
> +
> +   /**
> +    * check if some data was written
> +    */
> +   bool data_was_written(void *data, uint32_t id)
>

For functions that ask questions, I recommend using a word order that's
unambiguously a question (e.g. "was_data_written").  "data_was_written"
sounds more like a statement, so someone might misinterpret this function
as a synonym for mark_data_written().

But I'm nit picking again here.  With the other issues addressed, this
patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140204/577df499/attachment.html>