I've pushed this patch. Thanks, Andreas
2016-01-21 1:35 GMT+01:00 Jeremy Huddleston Sequoia <jerem...@apple.com>: > Sorry, I thought I responded to this a while ago. > > Reviewed-by: Jeremy Huddleston Sequoia <jerem...@apple.com> > Tested-by: Jeremy Huddleston Sequoia <jerem...@apple.com> > > --Jeremy > >> On Jan 19, 2016, at 05:21, Andreas Boll <andreas.boll....@gmail.com> wrote: >> >> Jeremy, did you have a chance to test this patch? >> This patch would be still useful for OS X. For non-OS X this patch [1] >> reduces the size of libGL.so further more. >> >> Thanks, >> Andreas >> >> [1] https://patchwork.freedesktop.org/patch/70372/ >> >> 2015-09-28 19:46 GMT+02:00 Jeremy Huddleston Sequoia <jerem...@apple.com>: >>> I'll give it a go. >>> >>> It is still needed on OS X (and I think Windows). It's just not used by >>> the X server any more. >>> >>> --Jeremy >>> >>>> On Sep 28, 2015, at 10:26, Arlie Davis <arl...@google.com> wrote: >>>> >>>> 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. >>>> >>>> If there is no need for this on OS X any longer, then the best thing might >>>> be to remove it entirely. >>>> >>>> On Sat, Sep 26, 2015 at 5:56 PM, Jeremy Huddleston Sequoia >>>> <jerem...@apple.com> wrote: >>>> 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 <jerem...@apple.com> >>>> >>>> --- >>>> >>>> You're right that this used to be use in xserver as well, but that was >>>> removed in: >>>> >>>> commit e61e19959d9138d5b81b1f25b7aa3e257918170d >>>> Author: Adam Jackson <a...@redhat.com> >>>> 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...@redhat.com> >>>> Reviewed-by: Jeremy Huddleston Sequoia <jerem...@apple.com> >>>> >>>> >>>>> On Sep 22, 2015, at 15:55, Ian Romanick <i...@freedesktop.org> 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 <ian.d.roman...@intel.com> >>>>> >>>>> 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 <arl...@google.com> >>>>>> 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' : f.name } >>>>>> >>>>>> - 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, f.name)) >>>>>> + funcnames[f.offset] = f.name >>>>>> + >>>>>> + # 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 <arl...@google.com> 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 <arl...@google.com> >>>>>>>> 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' : f.name } >>>>>>>> >>>>>>>> - 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, f.name)) >>>>>>>> + funcnames[f.offset] = f.name >>>>>>>> + >>>>>>>> + # 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 >>>>>> mesa-dev@lists.freedesktop.org >>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>>>>> >>>>> >>>> >>>> >>> >>> >>> _______________________________________________ >>> mesa-dev mailing list >>> mesa-dev@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
