Re: module cleanup [2/n]

[Gary V. Vaughan]

Hi Eric,

On Sep 13, 2007, at 1:40 PM, Eric Blake wrote:
> According to Gary V. Vaughan on 9/11/2007 4:06 PM:
> > >     Wrap lt_dlhandle in struct m4_module.
> >
> > Wrapping like this introduces an additional dereference every time  
> > a module
> > is accessed.  For this, and aesthetic reasons, I'd rather see  
> > m4_module as
> > a simple typedef alias.
>
> libltdl can only attach a single pointer to the lt_dlhandle.  So it  
> has to
> be a pointer to a struct for us to store more information than  
> that.  I'm
> still not convinced whether handing m4 clients the raw lt_dlhandle  
> (under
> a type-safe opaque alias) is any more efficient than handing them the
> pointer to the struct associated with the handle, and having that  
> struct
> point back to the handle (which is what I implemented).

It saves a pointer dereference at every call across the libltdl  
interface
boundary.  That isn't a big deal if it only happens during module  
load/unload,
though we need to be careful not to slow down any operations involved in
using the builtins from the loaded modules...

> One benefit of my
> approach is that we can cache anything in the struct, reducing the  
> need to
> continually rely on libltdl accessors to relearn something that hasn't
> changed since the last time we checked (for example, the  
> implementation of
> m4_get_module_name could be sped up by caching lt_dlgetinfo(handle)- 
> >name
> in the m4_module, at which point a fast accessor macro is easier to  
> write).

Although it adds a stack frame, lt_dlgetinfo merely returns the  
address of
its own cached values, although it is certainly worth measuring whether
duplicating that cache outside the ltdl layer speeds things up enough to
compensate for the slowdown accrued from the additional pointer  
dereferences
inherent in using a struct rather than an alias.

As you say, that might all be moot if it turns out we need to store  
more than
one datum address per module (and consequently have to use the  
struct)...

> > >     (m4_module): New declaration.  Simple for now, but intended for
> > >     growth.
> >
> > I would say that you have found a deficiency in the libltdl apis  
> > if you
> > need
> > to duplicate information that is already tracked by libltdl  
> > itself, or that
> > you are unable to store with lt_dlcaller_set_data...
>
> I found that storing the m4_module* using lt_dlcaller_set_data  
> worked all
> right, once I fixed the lt_dlcaller_get_data bug.

The benefit of the old approach is that if the only address that  
needs storing
is the address of the lt_dlhandle itself, we not only save the pointer
dereferences but also the stack frame for lt_dlcaller_get_data.

> > >     (m4_module_makeresident, m4_module_refcount): New functions.
> >
> > These should be macros (at least with NDEBUG) that defer to the
> > identical ltdl
> > calls, to avoid the extra stack frame.
>
> I'll keep that on my list of future cleanups.

Cool!

> > >     (module_close): Delete, and inline into module_remove.
> >
> > I rather liked the symmetry before this change :-(
>
> This whole area is still a bit awkward - particularly since both  
> functions
> were having to strdup the module name in case of error.
>
> You're right that there are still some symmetry issues to work out, as
> well as correctly dividing the work between the three layers:
>
> m4_module_import/m4_module_? - opens a module in order to import an  
> entry
> point from it (for example, libgnu's esyscmd needs to affect libm4's
> sysval builtin) - this currently affects the m4 symbol table, but  
> I'm not
> sure that it should, and I don't know if a symmetric partner is needed

Nice analysis :-)

In an ideal world, once we have an import that potentially lt_dlopen's a
module without sucking the builtins into the m4 symbol table, we should
also have an m4_module_unimport to decrement the refcount and possibly
lt_dlclose the module.

> m4_module_load/m4_module_unload - opens modules and affects m4 symbol
> table, designed for use by libload and other m4 modules
>
> m4__module_open/m4__module_? - opens a module, but leaves m4 symbol  
> table
> alone, designed for use by internals, such as src/freeze.c

This is where I'm missing m4__module_close.

> static module_?/static module_remove - handles the libltdl handle  
> open and
> close, designed for use only by module.c

static module_add?

> I'll think about it some more, and try to clean it up better.

Cool!

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_      Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912

PGP.sig
Description: This is a digitally signed message part