<div dir="ltr"><div>The null check is safe to remove, for two reasons. First, we're allocating with calloc, so we know for sure that the entire structure is zero-filled. Second, we're assigning every byte of the table, so we don't even need to rely on zero-filling it. (If this were a function that was frequently called, I'd change it to use malloc instead of calloc -- but it isn't.)<br></div><div><br></div><div>Will move the * and make the other */-related style changes.</div><div><br></div><div>Hmm, wasn't aware of xrange(). Will change to use that.</div><div><br></div><div>After making those edits, what's the next step to push / commit this?</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 16, 2015 at 7:07 AM, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Sep 15, 2015 at 10:38 AM, Arlie Davis <<a href="mailto:arlied@google.com">arlied@google.com</a>> wrote:<br>
><br>
> Hello! I noticed an inefficiency in libGL.so, so I thought I'd take a<br>
> stab at fixing it. This is my first patch submitted to mesa-dev, so<br>
> if I'm doing anything dumb, let me know. I can't use git send-email,<br>
> but I've formatted the patch using git format-patch, which should<br>
> hopefully produce similar output. The patch text (below) describes<br>
> the inefficiency and the improvement.<br>
><br>
><br>
><br>
> From 0abde226eed1b9f6052193f36f6cdc060698f621 Mon Sep 17 00:00:00 2001<br>
> From: Arlie Davis <<a href="mailto:arlied@google.com">arlied@google.com</a>><br>
> Date: Tue, 15 Sep 2015 09:58:34 -0700<br>
> Subject: [PATCH] This patch significantly reduces the size of the libGL.so<br>
> binary. It does not change the (externally visible) behavior of libGL.so at<br>
> all.<br>
><br>
> gl_gentable.py generates a function, _glapi_create_table_from_handle.<br>
> This function allocates a large dispatch table, consisting of 1300 or so<br>
> function pointers, and fills this dispatch table by doing symbol lookups<br>
> on a given shared library. Previously, gl_gentable.py would generate a<br>
> single, very large _glapi_create_table_from_handle function, with a short<br>
> cluster of lines for each entry point (function). The idiom it generates<br>
> was a NULL check, a call to snprintf, a call to dlsym / GetProcAddress,<br>
> and then a store into the dispatch table. Since this function processes<br>
> a large number of entry points, this code is duplicated many times over.<br>
><br>
> We can encode the same information much more compactly, by using a lookup<br>
> table. The previous total size of _glapi_create_table_from_handle on x64<br>
> was 125848 bytes. By using a lookup table, the size of<br>
> _glapi_create_table_from_handle (and the related lookup tables) is reduced<br>
> to 10840 bytes. In other words, this enormous function is reduced by 91%.<br>
> The size of the entire libGL.so binary (measured when stripped) itself drops<br>
> by 15%.<br>
><br>
> So the purpose of this change is to reduce the binary size, which frees up<br>
> disk space, memory, etc.<br>
> ---<br>
<br>
</div></div>Seems like a nice change. size lib/libGL.so.1.2.0 on my system shows<br>
<br>
text data bss dec hex filename<br>
604031 11360 2792 618183 96ec7 lib/libGL.so.1.2.0 before<br>
490751 21920 2792 515463 7dd87 lib/libGL.so.1.2.0 after<br>
<br>
Feel free to include that in the commit message.<br>
<div><div class="h5"><br>
> src/mapi/glapi/gen/gl_gentable.py | 56 ++++++++++++++++++++++++++++-----------<br>
> 1 file changed, 40 insertions(+), 16 deletions(-)<br>
><br>
> diff --git a/src/mapi/glapi/gen/gl_gentable.py b/src/mapi/glapi/gen/gl_gentable.py<br>
> index 1b3eb72..2563b6b 100644<br>
> --- a/src/mapi/glapi/gen/gl_gentable.py<br>
> +++ b/src/mapi/glapi/gen/gl_gentable.py<br>
> @@ -113,6 +113,9 @@ __glapi_gentable_set_remaining_noop(struct _glapi_table *disp) {<br>
> dispatch[i] = p.v;<br>
> }<br>
><br>
> +"""<br>
> +<br>
> +footer = """<br>
> struct _glapi_table *<br>
> _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) {<br>
> struct _glapi_table *disp = calloc(_glapi_get_dispatch_table_size(), sizeof(_glapi_proc));<br>
> @@ -123,27 +126,27 @@ _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) {<br>
><br>
> if(symbol_prefix == NULL)<br>
> symbol_prefix = "";<br>
> -"""<br>
><br>
> -footer = """<br>
> - __glapi_gentable_set_remaining_noop(disp);<br>
> -<br>
> - return disp;<br>
> -}<br>
> -"""<br>
> + /* Note: This code relies on _glapi_table_func_names being sorted by the<br>
> + entry point index of each function. */<br>
<br>
</div></div>Mesa style puts the */ on its own line for multiline comments.<br>
<span class=""><br>
> + for (int func_index = 0; func_index < GLAPI_TABLE_COUNT; ++func_index) {<br>
> + const char* name = _glapi_table_func_names[func_index];<br>
<br>
</span>* goes with the var name, not the type. That is, "char* " should be "char *"<br>
<span class=""><br>
> + void ** procp = &((void **)disp)[func_index];<br>
><br>
> -body_template = """<br>
> - if(!disp->%(name)s) {<br>
<br>
</span>We're removing the null check. Is that okay to do?<br>
<div><div class="h5"><br>
> - void ** procp = (void **) &disp->%(name)s;<br>
> - snprintf(symboln, sizeof(symboln), "%%s%(entry_point)s", symbol_prefix);<br>
> + snprintf(symboln, sizeof(symboln), \"%s%s\", symbol_prefix, name);<br>
> #ifdef _WIN32<br>
> *procp = GetProcAddress(handle, symboln);<br>
> #else<br>
> *procp = dlsym(handle, symboln);<br>
> #endif<br>
> }<br>
> + __glapi_gentable_set_remaining_noop(disp);<br>
> +<br>
> + return disp;<br>
> +}<br>
> """<br>
><br>
> +<br>
> class PrintCode(gl_XML.gl_print_base):<br>
><br>
> def __init__(self):<br>
> @@ -180,12 +183,33 @@ class PrintCode(gl_XML.gl_print_base):<br>
><br>
><br>
> def printBody(self, api):<br>
> - for f in api.functionIterateByOffset():<br>
> - for entry_point in f.entry_points:<br>
> - vars = { 'entry_point' : entry_point,<br>
> - 'name' : <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a> }<br>
><br>
> - print body_template % vars<br>
> + # Determine how many functions have a defined offset.<br>
> + func_count = 0<br>
> + for f in api.functions_by_name.itervalues():<br>
> + if f.offset != -1:<br>
> + func_count += 1<br>
> +<br>
> + # Build the mapping from offset to function name.<br>
> + funcnames = [None] * func_count<br>
> + for f in api.functions_by_name.itervalues():<br>
> + if f.offset != -1:<br>
> + if not (funcnames[f.offset] is None):<br>
> + raise Exception("Function table has more than one function with same offset (offset %d, func %s)" % (f.offset, <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a>))<br>
> + funcnames[f.offset] = <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a><br>
> +<br>
> + # Check that the table has no gaps. We expect a function at every offset,<br>
> + # and the code which generates the table relies on this.<br>
> + for i in range(0, func_count):<br>
<br>
</div></div>I don't know much python, but I think you want to use xrange instead of range.<br>
<span class=""><br>
> + if funcnames[i] is None:<br>
> + raise Exception("Function table has no function at offset %d" % (i))<br>
> +<br>
> + print "#define GLAPI_TABLE_COUNT %d" % func_count<br>
> + print "static const char* const _glapi_table_func_names[GLAPI_TABLE_COUNT] = {"<br>
<br>
</span>"char*" -> "char *"<br>
<span class=""><br>
> + for i in range(0, func_count):<br>
<br>
</span>xrange instead of range, I think.<br>
<div class="HOEnZb"><div class="h5"><br>
> + print " /* %5d */ \"%s\"," % (i, funcnames[i])<br>
> + print "};"<br>
> +<br>
> return<br>
</div></div></blockquote></div><br></div>