Re: [Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization
On 29 January 2014 01:24, Tapani Pälli tapani.pa...@intel.com wrote:
Class will be used by the shader binary cache implementation.
Signed-off-by: Tapani Pälli tapani.pa...@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 000..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
Re: [Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization
Whoops, I discovered another issue:
On 29 January 2014 01:24, Tapani Pälli tapani.pa...@intel.com wrote:
Class will be used by the shader binary cache implementation.
Signed-off-by: Tapani Pälli tapani.pa...@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 000..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;
+
+ memcpy(memory + pos, data, size);
+
+ pos += size;
+ }
+
+ void overwrite(const void *data, int size, int offset)
+ {
+ if (offset 0 || offset + size pos)
+ return;
+ 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);
+
+ if (str)
+ write(str, len + 1);
+ }
+
+ unsigned position()
+ {
+ return pos;
+ }
+
+ /**
+* check if some data was written
+*/
+ bool data_was_written(void *data, uint32_t id)
+ {
+ hash_entry *entry =
+ _mesa_hash_table_search(data_hash, hash_value,
+(void*) (intptr_t) id);
This isn't an efficient way to use a hashtable. You're always passing the
same hash_value (the one computed in the constructor), which means that
every single hash entry will be stored in the same chain, and lookups will
be really slow.
Since you're only using this hashtable to detect duplicate glsl_types,
there's a much easier approach: take advantage of the fact that glsl_type
is a flyweight class (in other words, code elsewhere in Mesa ensures that
no duplicate glsl_type objects exist anywhere in memory, so any two
glsl_type pointers can be compared simply using pointer equality). Because
of that, you should be able to just use a pointer-based hashtable, like
ir_variable_refcount_visitor does. In short, that
Re: [Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization
On 4 February 2014 19:52, Paul Berry stereotype...@gmail.com wrote:
Whoops, I discovered another issue:
On 29 January 2014 01:24, Tapani Pälli tapani.pa...@intel.com wrote:
Class will be used by the shader binary cache implementation.
Signed-off-by: Tapani Pälli tapani.pa...@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 000..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;
+
+ memcpy(memory + pos, data, size);
+
+ pos += size;
+ }
+
+ void overwrite(const void *data, int size, int offset)
+ {
+ if (offset 0 || offset + size pos)
+ return;
+ 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);
+
+ if (str)
+ write(str, len + 1);
+ }
+
+ unsigned position()
+ {
+ return pos;
+ }
+
+ /**
+* check if some data was written
+*/
+ bool data_was_written(void *data, uint32_t id)
+ {
+ hash_entry *entry =
+ _mesa_hash_table_search(data_hash, hash_value,
+(void*) (intptr_t) id);
This isn't an efficient way to use a hashtable. You're always passing the
same hash_value (the one computed in the constructor), which means that
every single hash entry will be stored in the same chain, and lookups will
be really slow.
Since you're only using this hashtable to detect duplicate glsl_types,
there's a much easier approach: take advantage of the fact that glsl_type
is a flyweight class (in other words, code elsewhere in Mesa ensures that
no duplicate glsl_type objects exist anywhere in memory, so any two
glsl_type pointers can be compared simply using pointer equality). Because
of that, you should be able to just use
[Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization
Class will be used by the shader binary cache implementation.
Signed-off-by: Tapani Pälli tapani.pa...@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 000..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;
+
+ memcpy(memory + pos, data, size);
+
+ pos += size;
+ }
+
+ void overwrite(const void *data, int size, int offset)
+ {
+ if (offset 0 || offset + size pos)
+ return;
+ 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);
+
+ if (str)
+ write(str, len + 1);
+ }
+
+ unsigned position()
+ {
+ return pos;
+ }
+
+ /**
+* check if some data was written
+*/
+ bool data_was_written(void *data, uint32_t id)
+ {
+ hash_entry *entry =
+ _mesa_hash_table_search(data_hash, hash_value,
+(void*) (intptr_t) id);
+
+ if (entry entry-data == data)
+ return true;
+
+ return false;
+ }
+
+ /* mark that some data was written */
+ void mark_data_written(void *data, uint32_t id)
+ {
+ _mesa_hash_table_insert(data_hash, hash_value,
+ (void*) (intptr_t) id, data);
+ }
+
+private:
+
+ /* reallocate more memory */
+ bool grow(int size)
+ {
+ unsigned new_size = 2 * (curr_size + size);
+ char *more_mem = (char *) realloc(memory, new_size);
+ if (more_mem == NULL) {
+ free(memory);
+ memory = NULL;
+ return false;
+ } else {
+ memory = more_mem;
+ curr_size = new_size;
+ return true;
+ }
+ }
+
+ /* allocated memory */
+ char *memory;
+
+ /* current size of the whole allocation */
+ int curr_size;
+
+ /* write position / size of the data written */
+ int pos;
+
+ /* this hash can be used to refer to data already written
+* to skip sequential writes of the
