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.)
Will move the * and make the other */-related style changes.
Hmm, wasn't aware of xrange(). Will change to use that.
After making those edits, what's the next step to push / commit this?
On Wed, Sep 16, 2015 at 7:07 AM, Matt Turner <matts...@gmail.com> 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
