Re: [PATCH] Reordering interface base initialization

[Owen Taylor <otaylor redhat com>]

On Wed, 2003-08-27 at 00:39, Tim Janik wrote:
> On Wed, 27 Aug 2003, Owen Taylor wrote:
> 
> > On Tue, 2003-08-26 at 20:25, Owen Taylor wrote:
> >
> > > There were spare bits in ClassData where this information could
> > > be put but none in IFaceEntry. If you wanted to get funky, you
> > > could probably squeeze the the necessary information into the
> > > 2 spare alignment bits at the bottom of iface_entry->vtable, but
> > > that would be icky. A better possibility would be to keep a
> > > look-aside data structure holding information about what interfaces
> > > have been initialized for each class we are currently initializing.
> >
> > I went ahead and added the lookaside.
>
> what's the point in the lookaside structure?
> is this all about sparing the initialization state info from
> IFaceEntry? (the lookaside munching makes your patch buttugly
> if i may say that ;)

Yeah, it's about making the change not increase the memory consumption
of GObject. 4 bytes per interface/object pair is sort of marginal
in terms of "do we care?" It's not too hard for me to imagine an
app with 300 classes with 10 interfaces each (if you add some 
interfaces to base classes, the interface/object pairs add up quickly.)

> i don#t quite like type_iface_vtable_init_Wm() being split up
> into two functions btw, that duplicates a bunch of code and
> makes the logic inside the two functions only slightly simpler.

There's virtually no duplication of code, if you look at them
in detail. The structure is similar, but the details are different.

> > +struct _ClassInitInfo
> > +{
> > +};
> 
> i could be misstaken, but i think empty structures are a gcc
> extension. (yep, i noticed you don't use it so it's porlly
> a temporary left over)

Yeah, just a leftover.

> >    /* type_iface_retrieve_holder_info_Wm() doesn't modify write lock for returning NULL */
> >    iholder = type_iface_retrieve_holder_info_Wm (iface, NODE_TYPE (node), TRUE);
> >    if (!iholder)
> > -    return FALSE;      /* we don't modify write lock upon FALSE */
> > +    return FALSE;
> 
> i don't like that comment being removed. i know you added a blurb
> mentioning this before the function now, but i want to be reminded
> about the lock behaviour when comming across this return ;)

Sorry, not intentionally removed.

> > +  init_info.init_node = node;
> > +  if (CLASSED_NODE_N_IFACES (node) > MAX_STACK_IFACE_STATES)
> > +    init_info.iface_states = g_malloc (CLASSED_NODE_N_IFACES (node));
> > +  else
> > +    init_info.iface_states = stack_iface_states;
> 
> for what it's worth (since i don't like the lookaside stuff anyways),
> you could have made your code a good deal simpler just for forgoing
> the stack allocation here. class/iface initialization isn't as time
> critical or frequently called, that a single malloc would make a
> difference (and if it were, the g_slist_prepend() you use would
> be potentially way more offending).

A) g_slist_prepend() is pretty much free with the free lists.
B) The code doesn't get that much simpler without the stack allocation;
   most of the extra complexity is in code paths that never get hit
   as well. If you used GByteArray, then it gets easier, but two
   extra mallocs..  
C) I think saving a couple of hundred malloc/frees on app startup
   is worth thinking about. Marginally worth thinking about, but'
   worth thinking about.

> > Also wrote a fairly comprehensive
> > test case that adds classes at various different points in the
> > during class init.
> 
> great, been waiting for that.
> can you commit/intergrate the test case already (yes it
> obviously breaks for everyone, but that doesn't matter).

Committed.

In terms of what exact degree of optimization/simplicity tradeoff we
pick, I don't think it matters a whole lot. Can always fix up based on
profiling later.

Regards,
						Owen