<div dir="ltr">On 29 January 2014 01:24, Tapani Pälli <span dir="ltr"><<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Class will be used by the shader binary cache implementation.<br>
<br>
Signed-off-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>><br>
---<br>
src/glsl/memory_writer.h | 188 +++++++++++++++++++++++++++++++++++++++++++++++<br>
1 file changed, 188 insertions(+)<br>
create mode 100644 src/glsl/memory_writer.h<br>
<br>
diff --git a/src/glsl/memory_writer.h b/src/glsl/memory_writer.h<br>
new file mode 100644<br>
index 0000000..979169f<br>
--- /dev/null<br>
+++ b/src/glsl/memory_writer.h<br>
@@ -0,0 +1,188 @@<br>
+/* -*- c++ -*- */<br>
+/*<br>
+ * Copyright © 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER<br>
+ * DEALINGS IN THE SOFTWARE.<br>
+ */<br>
+<br>
+#pragma once<br>
+#ifndef MEMORY_WRITER_H<br>
+#define MEMORY_WRITER_H<br>
+<br>
+#include <stdlib.h><br>
+#include <unistd.h><br>
+#include <string.h><br>
+<br>
+#include "main/hash_table.h"<br>
+<br>
+#ifdef __cplusplus<br>
+/**<br>
+ * Helper class for writing data to memory<br>
+ *<br>
+ * This class maintains a dynamically-sized memory buffer and allows<br>
+ * for data to be efficiently appended to it with automatic resizing.<br>
+ */<br>
+class memory_writer<br>
+{<br>
+public:<br>
+ memory_writer() :<br>
+ memory(NULL),<br>
+ curr_size(0),<br>
+ pos(0)<br>
+ {<br>
+ data_hash = _mesa_hash_table_create(0, int_equal);<br>
+ hash_value = _mesa_hash_data(this, sizeof(memory_writer));<br>
+ }<br>
+<br>
+ ~memory_writer()<br>
+ {<br>
+ free(memory);<br>
+ _mesa_hash_table_destroy(data_hash, NULL);<br>
+ }<br>
+<br>
+ /* user wants to claim the memory */<br>
+ char *release_memory(size_t *size)<br>
+ {<br>
+ /* final realloc to free allocated but unused memory */<br>
+ char *result = (char *) realloc(memory, pos);<br>
+ *size = pos;<br>
+ memory = NULL;<br>
+ curr_size = 0;<br>
+ pos = 0;<br>
+ return result;<br>
+ }<br>
+<br>
+/**<br>
+ * write functions per type<br>
+ */<br>
+#define DECL_WRITER(type) void write_ ##type (const type data) {\<br>
+ write(&data, sizeof(type));\<br>
+}<br>
+<br>
+ DECL_WRITER(int32_t);<br>
+ DECL_WRITER(int64_t);<br>
+ DECL_WRITER(uint8_t);<br>
+ DECL_WRITER(uint32_t);<br>
+<br>
+ void write_bool(bool data)<br>
+ {<br>
+ uint8_t val = data;<br>
+ write_uint8_t(val);<br>
+ }<br>
+<br>
+ /* write function that reallocates more memory if required */<br>
+ void write(const void *data, int size)<br>
+ {<br>
+ if (!memory || pos > (curr_size - size))<br>
+ if (!grow(size))<br>
+ return;<br></blockquote><div><br></div><div>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:<br>
<br></div><div>if (!memory || pos > (curr_size - size)) {<br></div><div> if (!grow(size)) {<br></div><div> assert(!"Out of memory while serializing a shader");<br></div><div> return;<br> }<br>}<br>
<br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ memcpy(memory + pos, data, size);<br>
+<br>
+ pos += size;<br>
+ }<br>
+<br>
+ void overwrite(const void *data, int size, int offset)<br>
+ {<br>
+ if (offset < 0 || offset + size > pos)<br>
+ return;<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ memcpy(memory + offset, data, size);<br>
+ }<br>
+<br>
+ /* length is written to make reading safe */<br>
+ void write_string(const char *str)<br>
+ {<br>
+ uint32_t len = str ? strlen(str) : 0;<br>
+ write_uint32_t(len);<br></blockquote><div><br></div><div>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.<br>
<br>I'd suggest that instead, we store strlen + 1, so that the reader can tell the difference between NULL and "".<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ if (str)<br>
+ write(str, len + 1);<br></blockquote><div><br></div><div>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:<br>
<br></div><div>- First it reads the length<br></div><div>- Then it checks that a null terminator exists in the expected place<br></div><div>- Then it calls ralloc_strdup(), which walks the string to see how long it is (totally unnecessary, since the length is known)<br>
</div><div>- ralloc_strdup() calls ralloc_array() to allocate memory for the result<br></div><div>- ralloc_strdup() calls memcpy() to make a copy of the string<br></div><div>- ralloc_strdup() stores a null terminator at the end of the copied string.<br>
<br></div><div>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.<br>
<br></div><div>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. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+<br>
+ unsigned position()<br>
+ {<br>
+ return pos;<br>
+ }<br>
+<br>
+ /**<br>
+ * check if some data was written<br>
+ */<br>
+ bool data_was_written(void *data, uint32_t id)<br></blockquote><div><br></div><div>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().<br>
<br></div><div>But I'm nit picking again here. With the other issues addressed, this patch is:<br><br></div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
</div><br></div></div></div>