<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi;<br>
<br>
Thanks for the review, some replies below ..<br>
<br>
On 10/28/2013 10:09 PM, Paul Berry wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">
<div>Before we start, I have a clarifying question: Is the
binary format intended to be the same for 32- and 64-bit
builds, or different? At least one of my comments below will
depend on the answer to this question.<br>
</div>
<div>
<div><br>
</div>
</div>
</div>
</blockquote>
<br>
It's intended to be the same, I've tried to avoid of using long or
pointer addresses.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>On 24 October 2013 01:28, Tapani Pälli <span dir="ltr"><<a
moz-do-not-send="true"
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">+/* cache specific
debug output */<br>
+#ifdef SHADER_CACHE_DEBUG<br>
+#define CACHE_DEBUG(fmt, args...) printf(fmt, ##
args)<br>
+#else<br>
+#define CACHE_DEBUG(fmt, args...) do {} while (0)<br>
+#endif<br>
</blockquote>
<div><br>
</div>
<div>This is the gcc-specific variadic argument syntax.
To be compatible with MSVC, we have to use the C99
syntax, which is:<br>
<br>
#ifdef SHADER_CACHE_DEBUG<br>
#define CACHE_DEBUG(fmt, ...) printf(fmt, ##
__VA_ARGS__)<br>
#else<br>
#define CACHE_DEBUG(fmt, ...) do {} while (0)<br>
#endif<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok, will fix<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+/* C API for the cache */<br>
+#ifdef __cplusplus<br>
+#define _EXTC extern "C"<br>
+#else<br>
+#define _EXTC<br>
+#endif<br>
+<br>
+_EXTC char *_mesa_shader_serialize(struct gl_shader
*shader,<br>
+ struct _mesa_glsl_parse_state *state,<br>
+ const char *mesa_sha, size_t *size);<br>
+<br>
+_EXTC struct gl_shader *_mesa_shader_unserialize(void
*mem_ctx,<br>
+ void *blob, const char *mesa_sha, size_t size);<br>
+<br>
+_EXTC char *_mesa_program_serialize(struct
gl_shader_program *prog,<br>
+ size_t *size, const char *mesa_sha);<br>
+<br>
+_EXTC int _mesa_program_unserialize(struct
gl_shader_program *prog,<br>
+ const GLvoid *blob, size_t size, const char
*mesa_sha);<br>
</blockquote>
<div><br>
</div>
<div>This _EXTC stuff is nonstandard. What we typically
do is to surround the block with<br>
<br>
#ifdef __cplusplus<br>
extern "C" {<br>
#endif<br>
<br>
</div>
<div>and<br>
<br>
#ifdef __cplusplus<br>
</div>
<div>} /* extern "C" */<br>
</div>
<div>#endif<br>
<br>
</div>
<div>See, for example, src/glsl/program.h.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is what I originally did but with many functions it looks quite
ugly, this is why I wanted to try to make it a bit more compact. But
I can switch it back.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+#ifdef __cplusplus<br>
+<br>
+<br>
+/* helper class for writing data to memory */<br>
</blockquote>
<div><br>
</div>
<div>Please use doxygen-formatted comments for
documenting structs, classes, functions, and
struct/class members, i.e.:<br>
<br>
/**<br>
* blah blah blah<br>
*/<br>
<br>
</div>
<div>Also, comments of the form "helper class for XXX"
are generally hard to follow. We really need a
comment explaining what the class does. For example,
in this case, maybe it should be something like:<br>
<br>
/**<br>
* This class maintains a dynamically-sized memory
buffer and allows for data<br>
* to be efficiently appended to it with automatic
resizing.<br>
*/<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thanks, will do. I agree that current comments don't tell enough.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+struct memory_writer<br>
</blockquote>
<div><br>
</div>
<div>Let's make this a class rather than a struct to
clarify that it's C++ only.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+public:<br>
+ memory_writer() :<br>
+ memory(NULL),<br>
+ memory_p(NULL),<br>
+ curr_size(0),<br>
+ pos(0)<br>
+ { }<br>
+<br>
+ /* NOTE - there is no dtor to free memory, user is
responsible */<br>
+ void free_memory()<br>
+ {<br>
+ if (memory)<br>
+ free(memory);<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>I'm assuming the reason you did this rather than
write a destructor was that you wanted the option of
having the user of the class claim ownership of the
memory by calling mem()?<br>
<br>
The safer way to do this is to go ahead and write the
destructor, and change mem() to something like this:<br>
<br>
</div>
<div>inline char *release_memory(size_t *size)<br>
{<br>
</div>
<div> char *result = memory;<br>
</div>
<div> *size = pos;<br>
</div>
<div> memory = NULL;<br>
</div>
<div> memory_p = NULL;<br>
</div>
<div> curr_size = 0;<br>
</div>
<div> pos = 0;<br>
</div>
<div> return result;<br>
}<br>
<br>
</div>
<div>That way there is no danger of someone forgetting
to call free_memory() (or calling it when they
shouldn't).<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
good point, I will change this.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ /* realloc more memory */<br>
+ int grow(int32_t size)<br>
</blockquote>
<div><br>
</div>
<div>This function should be private.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
agreed, it was originally public just for debug purposes and
mistakenly left public.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ char *more_mem = (char *) realloc(memory,
curr_size + size);<br>
</blockquote>
<div><br>
</div>
<div>Growing the allocation to just curr_size + size
will give this class O(n^2) performance (since in
general, every call to write() will require a call to
grow(), which may need to memcpy all of the
previously-written data). What you need to do is
something like this (ignoring error handling):<br>
<br>
</div>
<div>size_t new_size = 2 * (curr_size + size);<br>
</div>
<div>char *more_mem = (char *) realloc(memory,
new_size);<br>
</div>
<div>memory = more_mem;<br>
</div>
<div>memory_p = memory + pos;<br>
</div>
<div>curr_size = new_size;<br>
<br>
</div>
<div>By doubling the size on each call to grow() we
guarantee that we won't have to call realloc() more
than log(n) times, so the total runtime of all the
realloc's will be bounded by O(n).<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is taken care by the caller which has some 'extra' on top to
help this but 2 * sounds fine for me. I thought this is more of a
concern of the caller and not this class but it is true that having
it here makes caller's life more simpler.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>Note: if we do this, then sometimes when we're done
serializing we'll have allocated more memory than we
need. To avoid wasting memory over the long term we
should probably do a final realloc() down to the exact
size needed. Assuming you take my advice about
release_memory() above, an easy way to do that would
be to do a final realloc() in release_memory(). Note
that this final realloc() will be efficient because
it's guaranteed not to need to memcpy the data.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thanks, makes sense. Currently the extra memory is there but only
for quite a short time.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ if (more_mem == NULL) {<br>
+ free(memory);<br>
+ memory = NULL;<br>
+ return -1;<br>
+ } else {<br>
+ memory = more_mem;<br>
+ memory_p = memory + pos;<br>
+ curr_size += size;<br>
+ return 0;<br>
+ }<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>Generally in Mesa we don't worry too much about
memory allocation failures, since in an out of memory
condition it's not really feasible to recover. I'd
recommend eliminating the out-of-memory-handling code
and changing the return type of grow() (and the
write() functions) to void.<br>
<br>
If we do decide to keep the out-of-memory handling,
then:<br>
- return type of grow() and write() should be bool,
and they should return true on success.<br>
</div>
<div>- we need to change all the code that calls write()
to make sure that it properly checks the return value.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, I'm fine with removing out-of-memory handling.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ /* write functions for different types */<br>
+#define _WRITE_TYPE(type) inline int write(type *val)
{\<br>
+ return write(val, sizeof(*val), 1, -1);\<br>
+}<br>
+<br>
+ _WRITE_TYPE(uint8_t)<br>
+ _WRITE_TYPE(int32_t)<br>
+ _WRITE_TYPE(uint32_t)<br>
+ _WRITE_TYPE(bool)<br>
+ _WRITE_TYPE(gl_texture_index)<br>
+ _WRITE_TYPE(ir_node_type)<br>
+ _WRITE_TYPE(ir_loop_jump::jump_mode)<br>
+ _WRITE_TYPE(ir_expression_operation)<br>
+ _WRITE_TYPE(GLbitfield64)<br>
</blockquote>
<div><br>
</div>
<div>Because of implicit type conversions, these
definitions essentially allow any integer type to be
passed to write(), and that increases the risk of
mistakes. For example, I believe this would be
allowed, in both 32 and 64 bit builds:<br>
<br>
</div>
<div>size_t s;<br>
</div>
<div>write(s);<br>
<br>
</div>
<div>But in 32 bit builds it'll resolve to
_WRITE_TYPE(uint32_t) and in 64 bit builds it'll
resolve to _WRITE_TYPE(GLbitfield64).<br>
<br>
I'd prefer if we just made the type explicit in each
write function, i.e. write_uint8(), write_int32(),
write_bool(), and so on. It's a little more typing at
the call site, but it carries the advantage that we
always know exactly what we're writing.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I'll open them up, Eric mentioned of this as well. I was hoping to
get around it (either with templates or macros) because it will make
code harder to maintain as these types and structures might change
any day.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ int write(const void *data, int32_t size, int32_t
nmemb,<br>
+ int32_t offset = -1)<br>
</blockquote>
<div><br>
</div>
<div>It looks like nearly all callers set either size or
nmemb to 1. I think the code would be easier to
follow if we just had a single size argument, and in
the rare cases where we are serializing multiple
objects whose size != 1, just do the multiply in the
caller.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Fair enough, this can be simplified. I did this to stay closer to
fwrite api which this code was originally using.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ int32_t amount = size * nmemb;<br>
+ int32_t extra = 8192;<br>
+<br>
+ /* reallocate more if does not fix to current
memory */<br>
+ if (!memory || pos > (int32_t)(curr_size -
amount))<br>
+ if (grow(amount + extra))<br>
+ return -1;<br>
</blockquote>
<div><br>
</div>
<div>For the case where offset != -1, this will do the
wrong thing (it will try to reserve memory when it's
not necessary to do so). It looks like in all cases
where offset != -1, the necessary memory is guaranteed
to be already reserved (because we are overwriting a
value we wrote previously) so I'd recommend that in
this case we simply do:<br>
<br>
assert(amount + extra <= pos);<br>
<br>
</div>
<div>Also, given how differently this function needs to
behave when offset != -1, and given how infrequently
it is called with an offset other than -1, I'd
recommend breaking the offset != -1 case out to its
own function (maybe call it overwrite()).<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
True, I did not think this case through. I will write it as separate
function.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ * by default data is written to pos memory_p,
however<br>
+ * if an offset value >= 0 is passed then
that value is<br>
+ * used as offset from start of the memory blob<br>
+ */<br>
+ char *dst = memory_p;<br>
+<br>
+ if (offset > -1)<br>
+ dst = memory + offset;<br>
+<br>
+ memcpy(dst, data, amount);<br>
+<br>
+ /* if no offset given, forward the pointer */<br>
+ if (offset == -1) {<br>
+ memory_p += amount;<br>
+ pos += amount;<br>
+ }<br>
+ return 0;<br>
+ }<br>
+<br>
+ int write_string(const char *str)<br>
+ {<br>
+ if (!str)<br>
+ return -1;<br>
+ uint32_t len = strlen(str);<br>
+ write(&len);<br>
+ write(str, 1, len);<br>
+ return 0;<br>
+ }<br>
+<br>
+ inline int32_t position() { return pos; }<br>
+ inline char *mem() { return memory; }<br>
+<br>
+private:<br>
+ char *memory;<br>
+ char *memory_p;<br>
+ int32_t curr_size;<br>
+ int32_t pos;<br>
</blockquote>
<div><br>
</div>
<div>Please add a short comment above each of these
private data members explaining what it stores.<br>
<br>
</div>
<div>I'm not comfortable with storing both memory_p and
pos in this class--they are redundant, so it means we
have to always double check that we keep the two of
them in sync. I'd suggest getting rid of memory_p,
since that looks like it would require the smallest
number of changes to the rest of the class.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, will fix.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+<br>
+/* helper for string serialization */<br>
</blockquote>
<div><br>
</div>
<div>Maybe this comment could be something like:<br>
<br>
/**<br>
* Wrapper for a C string that manages allocation and
deallocation of the<br>
* underlying memory.<br>
*/<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thanks, I'll go through the code and try to improve documentation in
general.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+struct string_data<br>
</blockquote>
<div><br>
</div>
<div>This should also be a class.<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>
+public:<br>
+ string_data() : len(0), data(NULL) {}<br>
+ string_data(const char *cstr) :<br>
+ data(NULL)<br>
+ {<br>
+ if (cstr) {<br>
</blockquote>
<div><br>
</div>
<div>It looks like we almost never expect to pass NULL
to the string_data constructor (other than by
mistake). I'd prefer to change things so that we
never pass NULL to the string_data constructor, and
get rid of this test, so that if we ever do pass NULL
to string_data by mistake, we'll notice the bug as
quickly as possible.<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Fine, will change.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+ len = strlen(cstr);<br>
+ data = _mesa_strdup(cstr);<br>
+ }<br>
+ }<br>
+<br>
+ ~string_data() {<br>
+ if (data) {<br>
+ free(data);<br>
+ data = NULL;<br>
</blockquote>
<div><br>
</div>
<div>It's not necessary to set data to NULL here, since
after the destructor finishes the class won't be
accessible anymore.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+<br>
+ int serialize(memory_writer &blob)<br>
</blockquote>
<div><br>
</div>
<div>As with the grow() and write() functions above, I
think this should return void. However, if we do
decide to give it a return value, it should be a bool,
with true indicating success.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I was hoping to use the 'int' everywhere as then I can specify not
only failure but also error code if wanted.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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 (!data)<br>
+ return -1;<br>
</blockquote>
<div><br>
</div>
<div>We never expect to call serialize() on string_data
whose data is NULL, right? If so, this should be an
assertion:<br>
<br>
assert(data != NULL);<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ blob.write(&len);<br>
+ blob.write(data, 1, len);<br>
+ return 0;<br>
+ }<br>
+<br>
+ void set(const char *cstr)<br>
+ {<br>
+ if (data)<br>
+ free(data);<br>
</blockquote>
<div><br>
</div>
<div>With one exception (which I have a suggestion about
changing, see ir_cache_variable_data below), it looks
like we only ever call set() in circumstances where
data is NULL. I'd suggest changing this to:<br>
<br>
</div>
<div>assert(data == NULL);<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok, can be changed.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ data = _mesa_strdup(cstr);<br>
+ len = strlen(cstr);<br>
+ }<br>
+<br>
+ uint32_t len;<br>
+ char *data;<br>
</blockquote>
<div><br>
</div>
<div>These two data members should be private.
Currently their only users outside of the class are
glsl_type_data::serialize() and the
ir_cache_variable_data constructor, both of which can
be easily changed not to access them. <br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I thought this more as a little helper / container and not a *real*
class but I can change this.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+<br>
+/* data required to serialize glsl_type */<br>
+struct glsl_type_data<br>
</blockquote>
<div><br>
</div>
<div>Can you help me understand why it's necessary to
have a separate class for this, rather than simply
give glsl_type a serialize() method? At the moment
this seems like needless copying.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I wanted to try to serialize everything as a struct to make the
serialize and unserialize code 'more compact'. It is true that this
could be just a function like serialization of ir instructions are.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+public:<br>
+ glsl_type_data() :<br>
+ name(NULL),<br>
+ element_type(NULL),<br>
+ field_names(NULL),<br>
+ field_types(NULL),<br>
+ field_major(NULL) {}<br>
+<br>
+ glsl_type_data(const glsl_type *t) :<br>
+ base_type(t->base_type),<br>
+ length(t->length),<br>
+ vector_elms(t->vector_elements),<br>
+ matrix_cols(t->matrix_columns),<br>
+
sampler_dimensionality(t->sampler_dimensionality),<br>
+ sampler_shadow(t->sampler_shadow),<br>
+ sampler_array(t->sampler_array),<br>
+ sampler_type(t->sampler_type),<br>
+ interface_packing(t->interface_packing),<br>
+ element_type(NULL),<br>
+ field_names(NULL),<br>
+ field_types(NULL),<br>
+ field_major(NULL)<br>
</blockquote>
<div><br>
</div>
<div>These inline functions are starting to get large
for my taste. Starting with this class, let's move
them into the .cpp file. If we later find that it's
necessary to inline some of them for performance
reasons, we can always change them back to inline.
But I doubt this will ever be a hot enough code path
for that to be necessary.<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Agreed. Originally I thought this to be much smaller class.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+ {<br>
+ name = new string_data(t->name);<br>
+<br>
+ /* for array, save element type information */<br>
+ if (t->base_type == GLSL_TYPE_ARRAY)<br>
+ element_type =<br>
+ new glsl_type_data(t->element_type());<br>
+<br>
+ /* with structs, copy each struct field name +
type */<br>
+ else if (t->base_type == GLSL_TYPE_STRUCT) {<br>
</blockquote>
<div><br>
</div>
<div>Don't we need to handle interface block types as
well? They have additional information in
glsl_struct_field that's important (location,
interpolation, and centroid).<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, this is actually missing implementation. I think I'm just
missing a test case to implement this, current ones never hit
interface path.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ field_names = new string_data[t->length];<br>
+ field_types = new
glsl_type_data*[t->length];<br>
+ field_major = new uint32_t[t->length];<br>
+<br>
+ glsl_struct_field *field =
t->fields.structure;<br>
+ glsl_type_data **field_t = field_types;<br>
+ for (unsigned k = 0; k < t->length;
k++, field++, field_t++) {<br>
+ field_names[k].set(field->name);<br>
+ *field_t = new
glsl_type_data(field->type);<br>
+ field_major[k] = field->row_major;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ ~glsl_type_data() {<br>
+ delete name;<br>
+ delete element_type;<br>
+ delete [] field_names;<br>
+ delete [] field_major;<br>
+ if (field_types) {<br>
+ struct glsl_type_data **data = field_types;<br>
+ for (int k = 0; k < length; k++, data++)<br>
+ delete *data;<br>
+ delete [] field_types;<br>
+ }<br>
+ }<br>
+<br>
+ int serialize(memory_writer &blob)<br>
+ {<br>
+ uint32_t ir_len = 666;<br>
+ blob.write(&name->len);<br>
+ blob.write(name->data, 1, name->len);<br>
</blockquote>
<div><br>
</div>
<div>These two lines can be replaced with:<br>
<br>
name->serialize(blob);<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
yep, will change<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ int32_t start_pos = blob.position();<br>
+ blob.write(&ir_len);<br>
</blockquote>
<div><br>
</div>
<div>It seeems like this pattern crops up a lot: write
out a bogus length value, keep track of its position,
and then later overwrite that with the true length
once the rest of the data has been written. How about
if we make a class that does all this work? I'm
thinking something like:<br>
<br>
</div>
<div>struct memory_writer<br>
{<br>
...<br>
class scoped_length<br>
{<br>
public:<br>
explicit scoped_length(memory_writer *writer)<br>
: writer(writer)<br>
{<br>
uint32_t dummy;<br>
writer->write(&dummy);<br>
start_pos = writer->position();<br>
}<br>
<br>
~scoped_length()<br>
{<br>
uint32_t len = writer->position() -
start_pos;<br>
writer->write(&len, sizeof(len), 1,
start_pos);<br>
}<br>
<br>
private:<br>
memory_writer *writer;<br>
uint32_t start_pos;<br>
};<br>
...<br>
};<br>
<br>
</div>
<div>And then any time you need to serialize the length
of some data that follows you can just do:<br>
<br>
{<br>
</div>
<div> memory_writer::scoped_length len(&blob); /*
reserves space for the length */<br>
</div>
<div> ... /* write out variable length data */<br>
</div>
<div>} /* when len goes out of scope, the proper length
is automatically written */<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I think this happens only in 3 places but I will try this, it looks
cleaner.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ blob.write(this, sizeof(*this), 1);<br>
+<br>
+ if (base_type == GLSL_TYPE_ARRAY)<br>
+ element_type->serialize(blob);<br>
+ else if (base_type == GLSL_TYPE_STRUCT) {<br>
</blockquote>
<div><br>
</div>
<div>As above, I'm worried that we may have to handle
interface block types as well.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yep, missing implementation.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ struct string_data *data = field_names;<br>
+ glsl_type_data **field_t = field_types;<br>
+ for (int k = 0; k < length; k++, data++,
field_t++) {<br>
+ data->serialize(blob);<br>
+ (*field_t)->serialize(blob);<br>
+ blob.write(&field_major[k]);<br>
+ }<br>
+ }<br>
+<br>
+ ir_len = blob.position() - start_pos -
sizeof(ir_len);<br>
+ blob.write(&ir_len, sizeof(ir_len), 1,
start_pos);<br>
+ return 0;<br>
+ }<br>
+<br>
+ int32_t base_type;<br>
+ int32_t length;<br>
+ int32_t vector_elms;<br>
+ int32_t matrix_cols;<br>
+<br>
+ uint32_t sampler_dimensionality;<br>
+ uint32_t sampler_shadow;<br>
+ uint32_t sampler_array;<br>
+ uint32_t sampler_type;<br>
+<br>
+ uint32_t interface_packing;<br>
+<br>
+ struct string_data *name;<br>
+<br>
+ /* array element type */<br>
+ struct glsl_type_data *element_type;<br>
+<br>
+ /* structure fields */<br>
+ struct string_data *field_names;<br>
+ struct glsl_type_data **field_types;<br>
+ uint32_t *field_major;<br>
+};<br>
+<br>
+<br>
+/* helper to create a unique id from a ir_variable
address */<br>
+static uint32_t _unique_id(ir_variable *var)<br>
+{<br>
+ char buffer[256];<br>
+ _mesa_snprintf(buffer, 256, "%s_%p", var->name,
var);<br>
+ return _mesa_str_checksum(buffer);<br>
</blockquote>
<div><br>
</div>
<div>Two problems:<br>
<br>
</div>
<div>1. This is a lot of work to obtain a unique
identifier. The pointer is already unique; we should
just use that.<br>
</div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I was originally using a pointer but then there's the 32 vs 64bit
address problem. I wanted to avoid usage of pointers.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>2. There's no guarantee that the checksum of the
string will be unique. Because of the birthday
paradox, checksum collisions are probably going to
happen all the time.<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I think there is because the pointer address is unique and is used
as part of the string.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>Instead of going to a lot of work to generate a
unique id for each ir_variable, I think we should just
serialize the ir_variable pointer address. If we are
trying to use the same binary format for 32-bit and
64-bit builds (see my clarifying question at the top
of this email), we'll need to extend the pointer to 64
bits before serializing it.<br>
<br>
</div>
<div>Alternatively, if you want to keep the unique ID's
small, we'll have to use some hashtable mechanism to
map each variable to its own ID.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, I will verify if this is a problem. For me it does not seem like
a problem as I'm using the address there.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+<br>
+/* data required to serialize ir_variable */<br>
+struct ir_cache_variable_data<br>
</blockquote>
<div><br>
</div>
<div>As with glsl_type_data, I don't understand why we
need to have this class rather than just adding a
serialize() method to the ir_variable class.<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This could be a serialize function in ir_cache_serialize.cpp. I did
not want to touch existing ir classes, I think it helps to have the
cache code separated and in one place. This is of course debatable,
every ir instruction could have serialize() and unserialize() to
deal with this.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+{<br>
+public:<br>
+ ir_cache_variable_data() :<br>
+ type(NULL),<br>
+ name(NULL),<br>
+ unique_name(NULL),<br>
+ state_slots(NULL) {}<br>
+<br>
+ ir_cache_variable_data(ir_variable *ir) :<br>
+ unique_id(_unique_id(ir)),<br>
+ max_array_access(ir->max_array_access),<br>
+ ir_type(ir->ir_type),<br>
+ mode(ir->mode),<br>
+ location(ir->location),<br>
+ read_only(ir->read_only),<br>
+ centroid(ir->centroid),<br>
+ invariant(ir->invariant),<br>
+ interpolation(ir->interpolation),<br>
+ origin_upper_left(ir->origin_upper_left),<br>
+
pixel_center_integer(ir->pixel_center_integer),<br>
+ explicit_location(ir->explicit_location),<br>
+ explicit_index(ir->explicit_index),<br>
+ explicit_binding(ir->explicit_binding),<br>
+ has_initializer(ir->has_initializer),<br>
+ depth_layout(ir->depth_layout),<br>
+ location_frac(ir->location_frac),<br>
+ num_state_slots(ir->num_state_slots),<br>
+ has_constant_value(ir->constant_value ? 1 :
0),<br>
+
has_constant_initializer(ir->constant_initializer
? 1 : 0)<br>
+ {<br>
+ name = new string_data(ir->name);<br>
+<br>
+ /* name can be NULL, see ir_print_visitor for
explanation */<br>
+ if (!ir->name)<br>
+ name->set("parameter");<br>
</blockquote>
<div><br>
</div>
<div>To avoid having to call set() on a non-NULL
string_data, I think we should change this to:<br>
<br>
</div>
<div>const char *non_null_name = ir->name ?
ir->name : "parameter";<br>
</div>
<div>name = new string_data(non_null_name);<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ char uniq[256];<br>
+ _mesa_snprintf(uniq, 256, "%s_%d",
name->data, unique_id);<br>
</blockquote>
<div><br>
</div>
To avoid having to access an element of a string_data
that ought to be private, I think we should change this
to:<br>
<div><br>
</div>
<div>_mesa_snprintf(uniq, 256, "%s_%d", non_null_name,
unique_id);<br>
<br>
</div>
<div>(Note, however, that if we decide to use a pointer
as the unique ID, this will have to change to ensure
that on 64-bit builds, the entire pointer is included
in the printf)<br>
<br>
</div>
<div>Two additional problems:<br>
</div>
<div><br>
1. If the name is too long, uniq won't get null
terminated, and the call to string_data(uniq) below
will go outside its bounds. Since the name comes from
the client-supplied shader, this could conceivably
lead to a security exploit.<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yep, I've been thinking about this. There's many places in the code
using hardcoded length. I did not check what is the max length, if
any, specified in GLSL but there might be one.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>2. If the name is too long, unique_name won't be
guaranteed to be unique.<br>
<br>
</div>
<div>I'd recommend just using ralloc_asprintf(), which
allocates a string of the appropriate length.<br>
</div>
<div>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, I'll try using ralloc_asprintf().<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+ unique_name = new string_data(uniq);<br>
+ type = new glsl_type_data(ir->type);<br>
+<br>
+ state_slots = (ir_state_slot *) malloc<br>
+ (ir->num_state_slots *
sizeof(ir->state_slots[0]));<br>
+ memcpy(state_slots, ir->state_slots,<br>
+ ir->num_state_slots *
sizeof(ir->state_slots[0]));<br>
+ }<br>
+<br>
+ ~ir_cache_variable_data()<br>
+ {<br>
+ delete name;<br>
+ delete unique_name;<br>
+ delete type;<br>
+<br>
+ if (state_slots)<br>
+ free(state_slots);<br>
+ }<br>
+<br>
+ int serialize(memory_writer &blob)<br>
+ {<br>
+ type->serialize(blob);<br>
+ name->serialize(blob);<br>
+ unique_name->serialize(blob);<br>
+ blob.write(this, sizeof(*this), 1);<br>
+<br>
+ for (unsigned i = 0; i < num_state_slots;
i++) {<br>
+ blob.write(&state_slots[i].swizzle);<br>
+ for (unsigned j = 0; j < 5; j++) {<br>
+
blob.write(&state_slots[i].tokens[j]);<br>
+ }<br>
+ }<br>
+ return 0;<br>
+ }<br>
+<br>
+ struct glsl_type_data *type;<br>
+ struct string_data *name;<br>
+ struct string_data *unique_name;<br>
+<br>
+ uint32_t unique_id;<br>
+<br>
+ uint32_t max_array_access;<br>
+ int32_t ir_type;<br>
+ int32_t mode;<br>
+ int32_t location;<br>
+ uint32_t read_only;<br>
+ uint32_t centroid;<br>
+ uint32_t invariant;<br>
+ uint32_t interpolation;<br>
+ uint32_t origin_upper_left;<br>
+ uint32_t pixel_center_integer;<br>
+ uint32_t explicit_location;<br>
+ uint32_t explicit_index;<br>
+ uint32_t explicit_binding;<br>
+ uint8_t has_initializer;<br>
+ int32_t depth_layout;<br>
+ uint32_t location_frac;<br>
+<br>
+ uint32_t num_state_slots;<br>
+ struct ir_state_slot *state_slots;<br>
+<br>
+ uint8_t has_constant_value;<br>
+ uint8_t has_constant_initializer;<br>
+};<br>
+<br>
+<br>
+/* helper class to read instructions */<br>
</blockquote>
<div><br>
</div>
<div>This comment should be expanded to explain that
this class wraps around the posix functions for
mapping a file into the process's address space.<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, agreed.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+struct memory_map<br>
+{<br>
+public:<br>
+ memory_map() :<br>
+ fd(0),<br>
+ cache_size(0),<br>
+ cache_mmap(NULL),<br>
+ cache_mmap_p(NULL) { }<br>
+<br>
+ /* read from disk */<br>
+ int map(const char *path)<br>
</blockquote>
<div><br>
</div>
<div>Return value should be a boolean, with true
indicating success.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I would prefer int because everywhere else in the code I'm using
int.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ struct stat stat_info;<br>
+ if (stat(path, &stat_info) != 0)<br>
+ return -1;<br>
+<br>
+ cache_size = stat_info.st_size;<br>
+<br>
+ fd = open(path, O_RDONLY);<br>
+ if (fd) {<br>
+ cache_mmap_p = cache_mmap = (char *)<br>
+ mmap(NULL, cache_size, PROT_READ,
MAP_PRIVATE, fd, 0);<br>
</blockquote>
<div><br>
</div>
<div>Will this work on Windows?<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
It won't, this is something I have left open for now. For Windows I
could write alternative that simply reads the whole file to a buffer
which would work on any system or then try to use a mmap equivalent
('File mapping object'), don't have much knowledge on win API
though.<br>
<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ return (cache_mmap == MAP_FAILED) ? -1 : 0;<br>
+ }<br>
+ return -1;<br>
+ }<br>
+<br>
+ /* read from memory */<br>
+ int map(const void *memory, size_t size)<br>
+ {<br>
+ cache_mmap_p = cache_mmap = (char *) memory;<br>
+ cache_size = size;<br>
+ return 0;<br>
+ }<br>
+<br>
+ ~memory_map() {<br>
+ if (cache_mmap) {<br>
+ munmap(cache_mmap, cache_size);<br>
+ close(fd);<br>
+ }<br>
+ }<br>
+<br>
+ /* move read pointer forward */<br>
+ inline void ffwd(int len)<br>
+ {<br>
+ cache_mmap_p += len;<br>
+ }<br>
+<br>
+ /* position of read pointer */<br>
+ inline uint32_t position()<br>
+ {<br>
+ return cache_mmap_p - cache_mmap;<br>
+ }<br>
+<br>
+ inline void read_string(char *str)<br>
+ {<br>
+ uint32_t len;<br>
+ read(&len);<br>
+ memcpy(str, cache_mmap_p, len);<br>
+ str[len] = '\0';<br>
+ ffwd(len);<br>
</blockquote>
<div><br>
</div>
<div>If the string is larger than the caller expects (or
the contents of the shader binary are corrupted), this
function may overwrite arbitrarily large areas of
memory. Since the shader binary may be supplied by
client code (in the case of OES_get_program_binary),
this is a security risk.<br>
<br>
</div>
<div>I can think of two possible ways of fixing this:<br>
<br>
</div>
<div>1. Change this function so that it copies the
string to a newly allocated chunk of memory.<br>
<br>
</div>
<div>2. Change the format of the shader binary so that
strings are stored as null-terminated (rather than
length followed by string data); that way this
function can simply return cache_mmap_p and fast
forward to the next byte after the end of the string.<br>
</div>
<div><br>
I'd lean in favor of #2. <br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Thanks, #2 sounds good to me.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+ }<br>
+<br>
+ /* read functions for different types */<br>
+#define _READ_TYPE(type) inline void read(type *val)
{\<br>
+ *val = *(type *) cache_mmap_p;\<br>
+ ffwd(sizeof(type));\<br>
+}<br>
+ _READ_TYPE(int32_t)<br>
+ _READ_TYPE(uint32_t)<br>
+ _READ_TYPE(bool)<br>
+ _READ_TYPE(GLboolean)<br>
+ _READ_TYPE(gl_texture_index)<br>
+ _READ_TYPE(ir_expression_operation)<br>
+<br>
+ inline void read(void *dst, size_t size)<br>
+ {<br>
+ memcpy(dst, cache_mmap_p, size);<br>
+ ffwd(size);<br>
+ }<br>
+<br>
+ /* read and serialize a gl_shader */<br>
+ inline struct gl_shader *read_shader(void
*mem_ctx,<br>
+ const char *mesa_sha, size_t size)<br>
+ {<br>
+ struct gl_shader *sha =
_mesa_shader_unserialize(mem_ctx,<br>
+ cache_mmap_p, mesa_sha, size);<br>
+ ffwd(size);<br>
+ return sha;<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>It surprises me to see us fast-forward by a
caller-supplied size here. Shouldn't we encode the
size in the first 4 bytes of the shader binary?<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is sort of how it works now, the size is just read on the
caller side as I've left it open for caller to define the format of
the file. But it is true that size is found now before the shader so
should be able to move reading of it here.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+<br>
+private:<br>
+<br>
+ int32_t fd;<br>
+ int32_t cache_size;<br>
+ char *cache_mmap;<br>
+ char *cache_mmap_p;<br>
+};<br>
+<br>
+<br>
+/* class to read and write gl_shader */<br>
</blockquote>
<div><br>
</div>
<div>This is one of the cases where I need more
information in the comment above the class
definition. Is this a class that should be created
once per context, and represents a proxy to an on-disk
cache of shaders? Or is it a class that should be
created once per load/save of a single shader, to aid
in saving/loading the shader? I'm guessing it's the
latter, because prototypes_only seems like the sort of
thing that will change from one
serialization/deserialization to the next.<br>
<br>
Perhaps a clearer name for this class would be
something like "ir_serializer"?<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, it is of the latter form. I will add more documentation and
rename the class.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>Also, it looks like there's very little crossover
between the code needed to serialize a shader and the
code needed to deserialize a shader. Perhaps this
should be split into two classes, ir_serializer and
ir_deserializer.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is fine for me. I wanted to keep read and write methods 'close'
to each other but I agree separation will make it more readable.<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+struct ir_cache<br>
+{<br>
+public:<br>
+ ir_cache(bool prototypes = false) :<br>
+ prototypes_only(prototypes)<br>
+ {<br>
+ var_ht = hash_table_ctor(0,
hash_table_string_hash,<br>
+ hash_table_string_compare);<br>
+ }<br>
+<br>
+ ~ir_cache()<br>
+ {<br>
+ hash_table_call_foreach(this->var_ht,
delete_key, NULL);<br>
+ hash_table_dtor(this->var_ht);<br>
+ }<br>
+<br>
+ /* serialize gl_shader to memory */<br>
+ char *serialize(struct gl_shader *shader,<br>
+ struct _mesa_glsl_parse_state *state,<br>
+ const char *mesa_sha, size_t *size);<br>
</blockquote>
<div><br>
</div>
<div>Style nit-pick: the arguments should be lined up in
the column after the opening paren, like this:<br>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok, will fix<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> char *serialize(struct gl_shader *shader,<br>
struct _mesa_glsl_parse_state
*state,<br>
const char *mesa_sha, size_t
*size);<br>
<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>
+ /* unserialize gl_shader from mapped memory */<br>
+ struct gl_shader *unserialize(void *mem_ctx,
memory_map &map,<br>
+ uint32_t shader_size,<br>
+ struct _mesa_glsl_parse_state *state,<br>
+ const char *mesa_sha,<br>
+ int *error_code);<br>
+<br>
+ enum cache_error {<br>
+ GENERAL_READ_ERROR = -1,<br>
+ DIFFERENT_MESA_SHA = -2,<br>
+ DIFFERENT_LANG_VER = -3,<br>
+ };<br>
+<br>
+ /**<br>
+ * this method is public so that gl_shader_program<br>
+ * unserialization can use it when reading the<br>
+ * uniform storage<br>
+ */<br>
+ const glsl_type *read_glsl_type(memory_map
&map,<br>
+ struct _mesa_glsl_parse_state *state);<br>
+<br>
+private:<br>
+<br>
+ /* variables and methods required for
serialization */<br>
+<br>
+ memory_writer blob;<br>
+<br>
+ bool prototypes_only;<br>
+<br>
+ /**<br>
+ * writes ir_type and instruction dump size as a
'header'<br>
+ * for each instruction before calling save_ir<br>
+ */<br>
+ int save(ir_instruction *ir);<br>
+<br>
+ int save_ir(ir_variable *ir);<br>
+ int save_ir(ir_assignment *ir);<br>
+ int save_ir(ir_call *ir);<br>
+ int save_ir(ir_constant *ir);<br>
+ int save_ir(ir_dereference_array *ir);<br>
+ int save_ir(ir_dereference_record *ir);<br>
+ int save_ir(ir_dereference_variable *ir);<br>
+ int save_ir(ir_discard *ir);<br>
+ int save_ir(ir_expression *ir);<br>
+ int save_ir(ir_function *ir);<br>
+ int save_ir(ir_function_signature *ir);<br>
+ int save_ir(ir_if *ir);<br>
+ int save_ir(ir_loop *ir);<br>
+ int save_ir(ir_loop_jump *ir);<br>
+ int save_ir(ir_return *ir);<br>
+ int save_ir(ir_swizzle *ir);<br>
+ int save_ir(ir_texture *ir);<br>
+ int save_ir(ir_emit_vertex *ir);<br>
+ int save_ir(ir_end_primitive *ir);<br>
+<br>
+<br>
+ /* variables and methods required for
unserialization */<br>
+<br>
+ struct _mesa_glsl_parse_state *state;<br>
+ void *mem_ctx;<br>
+<br>
+ struct exec_list *top_level;<br>
+ struct exec_list *prototypes;<br>
+ struct exec_list *current_function;<br>
</blockquote>
<div><br>
</div>
<div>These private data members need to have
documentation comments. In particular, since an
exec_list can in principle store anything that derives
from exec_node, the comments should explain what kind
of data is stored in each list.<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok, more documentation will be added<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+<br>
+ int read_header(struct gl_shader *shader,
memory_map &map,<br>
+ const char *mesa_sha);<br>
+ int read_prototypes(memory_map &map);<br>
+<br>
+ int read_instruction(struct exec_list *list,
memory_map &map,<br>
+ bool ignore = false);<br>
+<br>
+ int read_ir_variable(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_assignment(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_function(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_if(struct exec_list *list, memory_map
&map);<br>
+ int read_ir_return(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_call(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_discard(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_loop(struct exec_list *list,
memory_map &map);<br>
+ int read_ir_loop_jump(struct exec_list *list,
memory_map &map);<br>
+ int read_emit_vertex(struct exec_list *list,
memory_map &map);<br>
+ int read_end_primitive(struct exec_list *list,
memory_map &map);<br>
+<br>
+ /* rvalue readers */<br>
+ ir_rvalue *read_ir_rvalue(memory_map &map);<br>
+ ir_constant *read_ir_constant(memory_map &map,<br>
+ struct exec_list *list = NULL);<br>
+ ir_swizzle *read_ir_swizzle(memory_map &map);<br>
+ ir_texture *read_ir_texture(memory_map &map);<br>
+ ir_expression *read_ir_expression(memory_map
&map);<br>
+ ir_dereference_array
*read_ir_dereference_array(memory_map &map);<br>
+ ir_dereference_record
*read_ir_dereference_record(memory_map &map);<br>
+ ir_dereference_variable
*read_ir_dereference_variable(memory_map &map);<br>
+<br>
+ /**<br>
+ * var_ht is used to store created ir_variables
with a unique_key for<br>
+ * each so that ir_dereference_variable creation
can find the variable<br>
+ */<br>
+ struct hash_table *var_ht;<br>
+<br>
+ /**<br>
+ * these 2 functions are ~copy-pasta from
string_to_uint_map,<br>
</blockquote>
<div><br>
</div>
<div>I think you mean "copy-pasted" :)<br>
</div>
<div> </div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
yes!<br>
<br>
<blockquote
cite="mid:CA+yLL64EVuNJh3mvi56dsFo2HeFce4ygJE90f-VaoeYGumBQag@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<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">
+ * we need a hash here with a string key and
pointer value<br>
+ */<br>
+ void hash_store(void * value, const char *key)<br>
+ {<br>
+ char *dup_key = _mesa_strdup(key);<br>
+ bool result =
hash_table_replace(this->var_ht, value, dup_key);<br>
+ if (result)<br>
+ free(dup_key);<br>
+ }<br>
+<br>
+ static void delete_key(const void *key, void
*data, void *closure)<br>
+ {<br>
+ (void) data;<br>
+ (void) closure;<br>
+ free((char *)key);<br>
+ }<br>
+<br>
+};<br>
+#endif /* ifdef __cplusplus */<br>
+<br>
+#endif /* IR_CACHE_H */<br>
</blockquote>
<div><br>
</div>
<div>Unfortunately, I'm out of time to review this patch
series today. I'm not sure when I'll be able to get
back to it.<br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>
Thanks for taking the time Paul! I will work the changes you
proposed.<br>
<br>
// Tapani<br>
<br>
</body>
</html>