<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">I tried building Mesa on OS X, but I'm not nearly as familiar with development on OS X, so I wasn't able to get it to build. If someone could build / test that on OS X, it would certainly give more confidence in its correctness. I *think* the generated code is correct, but you know how that is.</div><div class="gmail_default" style="font-family:monospace,monospace"> </div><div class="gmail_default" style="font-family:monospace,monospace">If there is no need for this on OS X any longer, then the best thing might be to remove it entirely.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Sat, Sep 26, 2015 at 5:56 PM, Jeremy Huddleston Sequoia <<a href="mailto:jeremyhu@apple.com" target="_blank">jeremyhu@apple.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Reviewing diffs of code that generates code is always ick. =( This *looks* right to me, but has it been given a beating for correctness? If not, let me know, and I'll give it a whirl when I have some cycles. Reviewed-by: Jeremy Huddleston Sequoia <<a href="mailto:jeremyhu@apple.com">jeremyhu@apple.com</a>> --- You're right that this used to be use in xserver as well, but that was removed in: commit e61e19959d9138d5b81b1f25b7aa3e257918170d Author: Adam Jackson <<a href="mailto:ajax@redhat.com">ajax@redhat.com</a>> Date: Tue Dec 3 13:45:43 2013 -0500 xquartz/glx: Convert to non-glapi dispatch CGL doesn't have anything like glXGetProcAddress, and the old code just called down to dlsym in any case. It's a little mind-warping since dlopening a framework actually loads multiple dylibs, but that's just how OSX rolls. Signed-off-by: Adam Jackson <<a href="mailto:ajax@redhat.com">ajax@redhat.com</a>> Reviewed-by: Jeremy Huddleston Sequoia <<a href="mailto:jeremyhu@apple.com">jeremyhu@apple.com</a>> <div class="HOEnZb"><div class="h5"> > On Sep 22, 2015, at 15:55, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: > > On 09/17/2015 03:19 PM, Arlie Davis wrote: >> Ok, here's v2 of the change, with the suggested edits. > > So... I think this code is fine, and I admire the effort. I have a > couple concerns. > > 1. We have no way to test this, so it's quite possible something was broken. > > 2. This function is only used in the OSX builds. Jeremy is the > maintainer for those builds, so I've added him to the CC list. > > For every non-OSX build, we should just stop linking > src/mapi/glapi/glapi_gentable.c. I thought maybe the X sever used it, > but I couldn't find any evidence of that. > > If this is still a viable route, I have a few suggestions of follow-on > patches... > > I guess this patch is > > Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> > > but I really think we need to get Jeremy's approval before pushing it. > >> From 5f393faa058f453408dfc640eecae3fe6335dfed Mon Sep 17 00:00:00 2001 >> From: Arlie Davis <<a href="mailto:arlied@google.com">arlied@google.com</a>> >> Date: Tue, 15 Sep 2015 09:58:34 -0700 >> Subject: [PATCH] This patch significantly reduces the size of the libGL.so >> binary. It does not change the (externally visible) behavior of libGL.so at >> all. >> >> gl_gentable.py generates a function, _glapi_create_table_from_handle. >> This function allocates a large dispatch table, consisting of 1300 or so >> function pointers, and fills this dispatch table by doing symbol lookups >> on a given shared library. Previously, gl_gentable.py would generate a >> single, very large _glapi_create_table_from_handle function, with a short >> cluster of lines for each entry point (function). The idiom it generates >> was a NULL check, a call to snprintf, a call to dlsym / GetProcAddress, >> and then a store into the dispatch table. Since this function processes >> a large number of entry points, this code is duplicated many times over. >> >> We can encode the same information much more compactly, by using a lookup >> table. The previous total size of _glapi_create_table_from_handle on x64 >> was 125848 bytes. By using a lookup table, the size of >> _glapi_create_table_from_handle (and the related lookup tables) is reduced >> to 10840 bytes. In other words, this enormous function is reduced by 91%. >> The size of the entire libGL.so binary (measured when stripped) itself drops >> by 15%. >> >> So the purpose of this change is to reduce the binary size, which frees up >> disk space, memory, etc. >> --- >> src/mapi/glapi/gen/gl_gentable.py | 57 ++++++++++++++++++++++++++++----------- >> 1 file changed, 41 insertions(+), 16 deletions(-) >> >> diff --git a/src/mapi/glapi/gen/gl_gentable.py b/src/mapi/glapi/gen/gl_gentable.py >> index 1b3eb72..7cd475a 100644 >> --- a/src/mapi/glapi/gen/gl_gentable.py >> +++ b/src/mapi/glapi/gen/gl_gentable.py >> @@ -113,6 +113,9 @@ __glapi_gentable_set_remaining_noop(struct _glapi_table *disp) { >> dispatch[i] = p.v; >> } >> >> +""" >> + >> +footer = """ >> struct _glapi_table * >> _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) { >> struct _glapi_table *disp = calloc(_glapi_get_dispatch_table_size(), sizeof(_glapi_proc)); >> @@ -123,27 +126,28 @@ _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) { >> >> if(symbol_prefix == NULL) >> symbol_prefix = ""; >> -""" >> >> -footer = """ >> - __glapi_gentable_set_remaining_noop(disp); >> - >> - return disp; >> -} >> -""" >> + /* Note: This code relies on _glapi_table_func_names being sorted by the >> + * entry point index of each function. >> + */ >> + for (int func_index = 0; func_index < GLAPI_TABLE_COUNT; ++func_index) { >> + const char *name = _glapi_table_func_names[func_index]; >> + void ** procp = &((void **)disp)[func_index]; >> >> -body_template = """ >> - if(!disp->%(name)s) { >> - void ** procp = (void **) &disp->%(name)s; >> - snprintf(symboln, sizeof(symboln), "%%s%(entry_point)s", symbol_prefix); >> + snprintf(symboln, sizeof(symboln), \"%s%s\", symbol_prefix, name); >> #ifdef _WIN32 >> *procp = GetProcAddress(handle, symboln); >> #else >> *procp = dlsym(handle, symboln); >> #endif >> } >> + __glapi_gentable_set_remaining_noop(disp); >> + >> + return disp; >> +} >> """ >> >> + >> class PrintCode(gl_XML.gl_print_base): >> >> def __init__(self): >> @@ -180,12 +184,33 @@ class PrintCode(gl_XML.gl_print_base): >> >> >> def printBody(self, api): >> - for f in api.functionIterateByOffset(): >> - for entry_point in f.entry_points: >> - vars = { 'entry_point' : entry_point, >> - 'name' : <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a> } >> >> - print body_template % vars >> + # Determine how many functions have a defined offset. >> + func_count = 0 >> + for f in api.functions_by_name.itervalues(): >> + if f.offset != -1: >> + func_count += 1 >> + >> + # Build the mapping from offset to function name. >> + funcnames = [None] * func_count >> + for f in api.functions_by_name.itervalues(): >> + if f.offset != -1: >> + if not (funcnames[f.offset] is None): >> + 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>)) >> + funcnames[f.offset] = <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a> >> + >> + # Check that the table has no gaps. We expect a function at every offset, >> + # and the code which generates the table relies on this. >> + for i in xrange(0, func_count): >> + if funcnames[i] is None: >> + raise Exception("Function table has no function at offset %d" % (i)) >> + >> + print "#define GLAPI_TABLE_COUNT %d" % func_count >> + print "static const char * const _glapi_table_func_names[GLAPI_TABLE_COUNT] = {" >> + for i in xrange(0, func_count): >> + print " /* %5d */ \"%s\"," % (i, funcnames[i]) >> + print "};" >> + >> return >> >> >> >> >> >> >> On 09/16/2015 07:07 AM, Matt Turner wrote: >>> On Tue, Sep 15, 2015 at 10:38 AM, Arlie Davis <<a href="mailto:arlied@google.com">arlied@google.com</a>> wrote: >>>> >>>> Hello! I noticed an inefficiency in libGL.so, so I thought I'd take a >>>> stab at fixing it. This is my first patch submitted to mesa-dev, so >>>> if I'm doing anything dumb, let me know. I can't use git send-email, >>>> but I've formatted the patch using git format-patch, which should >>>> hopefully produce similar output. The patch text (below) describes >>>> the inefficiency and the improvement. >>>> >>>> >>>> >>>> From 0abde226eed1b9f6052193f36f6cdc060698f621 Mon Sep 17 00:00:00 2001 >>>> From: Arlie Davis <<a href="mailto:arlied@google.com">arlied@google.com</a>> >>>> Date: Tue, 15 Sep 2015 09:58:34 -0700 >>>> Subject: [PATCH] This patch significantly reduces the size of the libGL.so >>>> binary. It does not change the (externally visible) behavior of libGL.so at >>>> all. >>>> >>>> gl_gentable.py generates a function, _glapi_create_table_from_handle. >>>> This function allocates a large dispatch table, consisting of 1300 or so >>>> function pointers, and fills this dispatch table by doing symbol lookups >>>> on a given shared library. Previously, gl_gentable.py would generate a >>>> single, very large _glapi_create_table_from_handle function, with a short >>>> cluster of lines for each entry point (function). The idiom it generates >>>> was a NULL check, a call to snprintf, a call to dlsym / GetProcAddress, >>>> and then a store into the dispatch table. Since this function processes >>>> a large number of entry points, this code is duplicated many times over. >>>> >>>> We can encode the same information much more compactly, by using a lookup >>>> table. The previous total size of _glapi_create_table_from_handle on x64 >>>> was 125848 bytes. By using a lookup table, the size of >>>> _glapi_create_table_from_handle (and the related lookup tables) is reduced >>>> to 10840 bytes. In other words, this enormous function is reduced by 91%. >>>> The size of the entire libGL.so binary (measured when stripped) itself drops >>>> by 15%. >>>> >>>> So the purpose of this change is to reduce the binary size, which frees up >>>> disk space, memory, etc. >>>> --- >>> >>> Seems like a nice change. size lib/libGL.so.1.2.0 on my system shows >>> >>> text data bss dec hex filename >>> 604031 11360 2792 618183 96ec7 lib/libGL.so.1.2.0 before >>> 490751 21920 2792 515463 7dd87 lib/libGL.so.1.2.0 after >>> >>> Feel free to include that in the commit message. >>> >>>> src/mapi/glapi/gen/gl_gentable.py | 56 ++++++++++++++++++++++++++++----------- >>>> 1 file changed, 40 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/src/mapi/glapi/gen/gl_gentable.py b/src/mapi/glapi/gen/gl_gentable.py >>>> index 1b3eb72..2563b6b 100644 >>>> --- a/src/mapi/glapi/gen/gl_gentable.py >>>> +++ b/src/mapi/glapi/gen/gl_gentable.py >>>> @@ -113,6 +113,9 @@ __glapi_gentable_set_remaining_noop(struct _glapi_table *disp) { >>>> dispatch[i] = p.v; >>>> } >>>> >>>> +""" >>>> + >>>> +footer = """ >>>> struct _glapi_table * >>>> _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) { >>>> struct _glapi_table *disp = calloc(_glapi_get_dispatch_table_size(), sizeof(_glapi_proc)); >>>> @@ -123,27 +126,27 @@ _glapi_create_table_from_handle(void *handle, const char *symbol_prefix) { >>>> >>>> if(symbol_prefix == NULL) >>>> symbol_prefix = ""; >>>> -""" >>>> >>>> -footer = """ >>>> - __glapi_gentable_set_remaining_noop(disp); >>>> - >>>> - return disp; >>>> -} >>>> -""" >>>> + /* Note: This code relies on _glapi_table_func_names being sorted by the >>>> + entry point index of each function. */ >>> >>> Mesa style puts the */ on its own line for multiline comments. >>> >>>> + for (int func_index = 0; func_index < GLAPI_TABLE_COUNT; ++func_index) { >>>> + const char* name = _glapi_table_func_names[func_index]; >>> >>> * goes with the var name, not the type. That is, "char* " should be "char *" >>> >>>> + void ** procp = &((void **)disp)[func_index]; >>>> >>>> -body_template = """ >>>> - if(!disp->%(name)s) { >>> >>> We're removing the null check. Is that okay to do? >>> >>>> - void ** procp = (void **) &disp->%(name)s; >>>> - snprintf(symboln, sizeof(symboln), "%%s%(entry_point)s", symbol_prefix); >>>> + snprintf(symboln, sizeof(symboln), \"%s%s\", symbol_prefix, name); >>>> #ifdef _WIN32 >>>> *procp = GetProcAddress(handle, symboln); >>>> #else >>>> *procp = dlsym(handle, symboln); >>>> #endif >>>> } >>>> + __glapi_gentable_set_remaining_noop(disp); >>>> + >>>> + return disp; >>>> +} >>>> """ >>>> >>>> + >>>> class PrintCode(gl_XML.gl_print_base): >>>> >>>> def __init__(self): >>>> @@ -180,12 +183,33 @@ class PrintCode(gl_XML.gl_print_base): >>>> >>>> >>>> def printBody(self, api): >>>> - for f in api.functionIterateByOffset(): >>>> - for entry_point in f.entry_points: >>>> - vars = { 'entry_point' : entry_point, >>>> - 'name' : <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a> } >>>> >>>> - print body_template % vars >>>> + # Determine how many functions have a defined offset. >>>> + func_count = 0 >>>> + for f in api.functions_by_name.itervalues(): >>>> + if f.offset != -1: >>>> + func_count += 1 >>>> + >>>> + # Build the mapping from offset to function name. >>>> + funcnames = [None] * func_count >>>> + for f in api.functions_by_name.itervalues(): >>>> + if f.offset != -1: >>>> + if not (funcnames[f.offset] is None): >>>> + 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>)) >>>> + funcnames[f.offset] = <a href="http://f.name" rel="noreferrer" target="_blank">f.name</a> >>>> + >>>> + # Check that the table has no gaps. We expect a function at every offset, >>>> + # and the code which generates the table relies on this. >>>> + for i in range(0, func_count): >>> >>> I don't know much python, but I think you want to use xrange instead of range. >>> >>>> + if funcnames[i] is None: >>>> + raise Exception("Function table has no function at offset %d" % (i)) >>>> + >>>> + print "#define GLAPI_TABLE_COUNT %d" % func_count >>>> + print "static const char* const _glapi_table_func_names[GLAPI_TABLE_COUNT] = {" >>> >>> "char*" -> "char *" >>> >>>> + for i in range(0, func_count): >>> >>> xrange instead of range, I think. >>> >>>> + print " /* %5d */ \"%s\"," % (i, funcnames[i]) >>>> + print "};" >>>> + >>>> return >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> > </div></div></blockquote></div> </div>