Sorry, I thought I responded to this a while ago. Reviewed-by: Jeremy Huddleston Sequoia <[email protected]> Tested-by: Jeremy Huddleston Sequoia <[email protected]>
--Jeremy > On Jan 19, 2016, at 05:21, Andreas Boll <[email protected]> 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 <[email protected]>: >> 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 <[email protected]> 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 >>> <[email protected]> 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 <[email protected]> >>> >>> --- >>> >>> You're right that this used to be use in xserver as well, but that was >>> removed in: >>> >>> commit e61e19959d9138d5b81b1f25b7aa3e257918170d >>> Author: Adam Jackson <[email protected]> >>> 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 <[email protected]> >>> Reviewed-by: Jeremy Huddleston Sequoia <[email protected]> >>> >>> >>>> On Sep 22, 2015, at 15:55, Ian Romanick <[email protected]> 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 <[email protected]> >>>> >>>> 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 <[email protected]> >>>>> 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 <[email protected]> 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 <[email protected]> >>>>>>> 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 >>>>> [email protected] >>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>>>> >>>> >>> >>> >> >> >> _______________________________________________ >> mesa-dev mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >>
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
