GNOME.org
 

Re: [PATCH] Reordering interface base initialization

On Wed, 2003-08-27 at 03:07, Tim Janik wrote:
> On Wed, 27 Aug 2003, Owen Taylor wrote:
> 
> > On Wed, 2003-08-27 at 00:39, Tim Janik wrote:
> > > On Wed, 27 Aug 2003, Owen Taylor wrote:
> 
> > > 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.)
> 
> if we really worry about size here, it might be better to go for
> the bit squeezing then, i.e.:
> 
> struct _IFaceEntry
> {
>   GType           iface_type;
>   GTypeInterface *vtable;
> };
> becomes
> struct _IFaceEntry
> {
>   GType   iface_type;
>   gulong  mfield;    /* field used to store a vtable pointer and two flags */
> };
> #define	IFACE_ENTRY_GET_BITS(ie)     (ie->mfield & 3)
> #define	IFACE_ENTRY_SET_BITS(ie,b)   (ie)->mfield = ((ie)->mfield & ~(gulong) 3) | (b)
> #define	IFACE_ENTRY_GET_VTABLE(ie)   ((GTypeInterface*) ((ie)->mfield & ~(gulong) 3))
> #define	IFACE_ENTRY_SET_VTABLE(ie,v) (ie)->mfield = IFACE_ENTRY_GET_BITS (ie) | (gulong) (v)
> 
> i'd say using 4 set/get macros involves less hackery
> than the lookaside aproach (and certainly less code).

I mentioned this approach in a earlier mail, but less hackery? Certainly
wouldn't agree there. I think the savings have to be pretty vast before
you want to store stuff in the low bits of a pointer.

Attached is a further evolution of my lookaside patch. It uses
GByteArray and some reuse/abuse of GSList functions to cut the amount
of code substantially - it's only 21 lines longer than version
of the same code that extends IFaceEntry.

Regards,
						Owen


Index: gtype.c
===================================================================
RCS file: /cvs/gnome/glib/gobject/gtype.c,v
retrieving revision 1.64
diff -u -p -r1.64 gtype.c
--- gtype.c	19 Aug 2003 03:25:46 -0000	1.64
+++ gtype.c	27 Aug 2003 13:58:21 -0000
@@ -155,6 +155,23 @@ static gboolean				type_node_is_a_L		(Ty
 									 TypeNode		*iface_node);
 
 
+/* --- enumeration --- */
+
+/* The InitState enumeration is used to track the progress of initializing
+ * both classes and interface vtables. Keeping the state of initialization
+ * is necessary to handle new interfaces being added while we are initializing
+ * the class or other interfaces.
+ */
+typedef enum
+{
+  UNINITIALIZED,
+  BASE_CLASS_INIT,
+  BASE_IFACE_INIT,
+  CLASS_INIT,
+  IFACE_INIT,
+  INITIALIZED
+} InitState;
+
 /* --- structures --- */
 struct _TypeNode
 {
@@ -228,6 +245,7 @@ struct _ClassData
 {
   CommonData         common;
   guint16            class_size;
+  guint              init_state : 4;
   GBaseInitFunc      class_init_base;
   GBaseFinalizeFunc  class_finalize_base;
   GClassInitFunc     class_init;
@@ -239,6 +257,7 @@ struct _InstanceData
 {
   CommonData         common;
   guint16            class_size;
+  guint              init_state : 4;
   GBaseInitFunc      class_init_base;
   GBaseFinalizeFunc  class_finalize_base;
   GClassInitFunc     class_init;
@@ -926,6 +945,7 @@ type_data_make_W (TypeNode              
       data->instance.class_finalize = info->class_finalize;
       data->instance.class_data = info->class_data;
       data->instance.class = NULL;
+      data->instance.init_state = UNINITIALIZED;
       data->instance.instance_size = info->instance_size;
       /* We'll set the final value for data->instance.private size
        * after the parent class has been initialized
@@ -951,6 +971,7 @@ type_data_make_W (TypeNode              
       data->class.class_finalize = info->class_finalize;
       data->class.class_data = info->class_data;
       data->class.class = NULL;
+      data->class.init_state = UNINITIALIZED;
     }
   else if (NODE_IS_IFACE (node))
     {
@@ -1555,9 +1576,17 @@ g_type_free_instance (GTypeInstance *ins
   g_type_class_unref (class);
 }
 
+/* This is called to allocate and do the first part of initializing
+ * the interface vtable; type_iface_vtable_iface_init_Wm() does the remainder.
+ *
+ * A FALSE return indicates that we didn't find an init function for
+ * this type/iface pair, so the vtable from the parent type should
+ * be used. Note that the write lock is not modified upon a FALSE
+ * return.
+ */
 static gboolean
-type_iface_vtable_init_Wm (TypeNode *iface,
-			   TypeNode *node)
+type_iface_vtable_base_init_Wm (TypeNode *iface,
+				TypeNode *node)
 {
   IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
   IFaceHolder *iholder;
@@ -1585,16 +1614,43 @@ type_iface_vtable_init_Wm (TypeNode *ifa
   vtable->g_type = NODE_TYPE (iface);
   vtable->g_instance_type = NODE_TYPE (node);
   
-  if (iface->data->iface.vtable_init_base || iholder->info->interface_init)
+  if (iface->data->iface.vtable_init_base)
     {
       G_WRITE_UNLOCK (&type_rw_lock);
       if (iface->data->iface.vtable_init_base)
 	iface->data->iface.vtable_init_base (vtable);
+      G_WRITE_LOCK (&type_rw_lock);
+    }
+  return TRUE;	/* initialized the vtable */
+}
+
+/* Finishes what type_iface_vtable_base_init_Wm started by
+ * calling the interface init function.
+ */
+static void
+type_iface_vtable_iface_init_Wm (TypeNode *iface,
+				 TypeNode *node)
+{
+  IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
+  IFaceHolder *iholder;
+  GTypeInterface *vtable = NULL;
+  
+  iholder = type_iface_peek_holder_L (iface, NODE_TYPE (node));
+  if (!iholder)
+    return;
+
+  /* iholder->info should have been filled in by type_iface_vtable_base_init_Wm() */
+  g_assert (iface->data && entry && iholder->info);
+  
+  vtable = entry->vtable;
+
+  if (iholder->info->interface_init)
+    {
+      G_WRITE_UNLOCK (&type_rw_lock);
       if (iholder->info->interface_init)
 	iholder->info->interface_init (vtable, iholder->info->interface_data);
       G_WRITE_LOCK (&type_rw_lock);
     }
-  return TRUE;	/* write lock modified */
 }
 
 static gboolean
@@ -1631,6 +1687,17 @@ type_iface_vtable_finalize_Wm (TypeNode 
   return TRUE;	/* write lock modified */
 }
 
+typedef struct _ClassInitInfo ClassInitInfo;
+
+struct _ClassInitInfo
+{
+  TypeNode *init_node;		/* GSList.data */
+  ClassInitInfo *next;		/* GSList.next */
+  GByteArray *iface_states;
+};
+
+static ClassInitInfo *initializing_classes = NULL;
+
 static void
 type_class_init_Wm (TypeNode   *node,
 		    GTypeClass *pclass)
@@ -1639,14 +1706,25 @@ type_class_init_Wm (TypeNode   *node,
   GTypeClass *class;
   IFaceEntry *entry;
   TypeNode *bnode, *pnode;
+  ClassInitInfo init_info;
   guint i;
   
   g_assert (node->is_classed && node->data &&
 	    node->data->class.class_size &&
-	    !node->data->class.class);
+	    !node->data->class.class &&
+	    node->data->class.init_state == UNINITIALIZED);
 
+  init_info.init_node = node;
+  init_info.iface_states = g_byte_array_sized_new (CLASSED_NODE_N_IFACES (node));
+  memset (init_info.iface_states->data, 0, CLASSED_NODE_N_IFACES (node));
+  init_info.iface_states->len = CLASSED_NODE_N_IFACES (node);
+
+  init_info.next = initializing_classes;
+  initializing_classes = &init_info;
+  
   class = g_malloc0 (node->data->class.class_size);
   node->data->class.class = class;
+  node->data->class.init_state = BASE_CLASS_INIT;
   
   if (pclass)
     {
@@ -1681,26 +1759,37 @@ type_class_init_Wm (TypeNode   *node,
     }
   g_slist_free (init_slist);
   
-  if (node->data->class.class_init)
-    node->data->class.class_init (class, (gpointer) node->data->class.class_data);
-  
   G_WRITE_LOCK (&type_rw_lock);
+
+  node->data->class.init_state = BASE_IFACE_INIT;
   
-  /* ok, we got the class done, now initialize all interfaces, either
+  /* Before we initialize the class, base initialize all interfaces, either
    * from parent, or through our holder info
    */
   pnode = lookup_type_node_I (NODE_PARENT_TYPE (node));
-  entry = CLASSED_NODE_IFACES_ENTRIES (node) + 0;
-  while (entry)
+
+  i = 0;
+  while (i < CLASSED_NODE_N_IFACES (node))
     {
-      g_assert (entry->vtable == NULL);
+      entry = &CLASSED_NODE_IFACES_ENTRIES (node)[i];
+      while (i < CLASSED_NODE_N_IFACES (node) &&
+	     init_info.iface_states->data[i] == IFACE_INIT)
+	{
+	  entry++;
+	  i++;
+	}
+
+      if (i == CLASSED_NODE_N_IFACES (node))
+	break;
+
+      init_info.iface_states->data[i] = IFACE_INIT;
       
-      if (!type_iface_vtable_init_Wm (lookup_type_node_I (entry->iface_type), node))
+      if (!type_iface_vtable_base_init_Wm (lookup_type_node_I (entry->iface_type), node))
 	{
 	  guint j;
 	  
-	  /* type_iface_vtable_init_Wm() doesn't modify write lock upon FALSE,
-	   * need to get this interface from parent
+	  /* need to get this interface from parent, type_iface_vtable_base_init_Wm()
+	   * doesn't modify write lock upon FALSE, so entry is still valid; 
 	   */
 	  g_assert (pnode != NULL);
 	  
@@ -1711,16 +1800,104 @@ type_class_init_Wm (TypeNode   *node,
 	      if (pentry->iface_type == entry->iface_type)
 		{
 		  entry->vtable = pentry->vtable;
+		  init_info.iface_states->data[i] = INITIALIZED;
 		  break;
 		}
 	    }
 	  g_assert (entry->vtable != NULL);
 	}
+
+      /* If the write lock was released, additional interface entries might
+       * have been inserted into CLASSED_NODE_IFACES_ENTRIES (node); they'll
+       * be base-initialized when inserted, so we don't have to worry that
+       * we might miss them. Uninitialized entries can only be moved higher
+       * when new ones are inserted.
+       */
+      i++;
+    }
+  
+  node->data->class.init_state = CLASS_INIT;
+  
+  G_WRITE_UNLOCK (&type_rw_lock);
+
+  if (node->data->class.class_init)
+    node->data->class.class_init (class, (gpointer) node->data->class.class_data);
+  
+  G_WRITE_LOCK (&type_rw_lock);
+
+  node->data->class.init_state = IFACE_INIT;
+  
+  /* finish initialize the interfaces through our holder info
+   */
+  i = 0;
+  while (TRUE)
+    {
+      entry = &CLASSED_NODE_IFACES_ENTRIES (node)[i];
+      while (i < CLASSED_NODE_N_IFACES (node) &&
+	     init_info.iface_states->data[i] == INITIALIZED)
+	{
+	  entry++;
+	  i++;
+	}
+
+      if (i == CLASSED_NODE_N_IFACES (node))
+	break;
+
+      init_info.iface_states->data[i] = INITIALIZED;
+      
+      type_iface_vtable_iface_init_Wm (lookup_type_node_I (entry->iface_type), node);
       
-      /* refetch entry, IFACES_ENTRIES might be modified */
-      for (entry = NULL, i = 0; i < CLASSED_NODE_N_IFACES (node); i++)
-	if (!CLASSED_NODE_IFACES_ENTRIES (node)[i].vtable)
-	  entry = CLASSED_NODE_IFACES_ENTRIES (node) + i;
+      /* As in the loop above, additional initialized entries might be inserted
+       * if the write lock is released, but that's harmless because the entries
+       * we need to initialize only move higher in the list.
+       */
+      i++;
+    }
+  
+  node->data->class.init_state = INITIALIZED;
+
+  initializing_classes = (ClassInitInfo *)g_slist_remove_link ((GSList *)initializing_classes,
+							       (GSList *)&init_info);
+  g_byte_array_free (init_info.iface_states, TRUE);
+}
+
+/* This function is called when we add a new interface to an existing type;
+ * depending on whether the type already has a class or not, we call
+ * base-init and interface-init functions. If we are called *while* the
+ * class is being initialized, then we need to update an array saying which
+ * interfaces for the class have been initialized.
+ */
+static void
+type_iface_vtable_init_Wm (TypeNode *node,
+			   TypeNode *iface)
+{
+  InitState class_state =
+    node->data ? node->data->class.init_state : UNINITIALIZED;
+  guint8 new_state = UNINITIALIZED;
+  
+  if (class_state >= BASE_IFACE_INIT)
+    {
+      type_iface_vtable_base_init_Wm (iface, node);
+      new_state = IFACE_INIT;
+    }
+
+  if (class_state >= IFACE_INIT)
+    {
+      type_iface_vtable_iface_init_Wm (iface, node);
+      new_state = INITIALIZED;
+    }
+  
+  if (class_state != UNINITIALIZED && class_state != INITIALIZED)
+    {
+      /* The interface was added while we were initializing the class
+       */
+      ClassInitInfo *init_info = (ClassInitInfo *)g_slist_find ((GSList *)initializing_classes, node);
+      IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
+      gint i = entry - CLASSED_NODE_IFACES_ENTRIES (node);
+      
+      g_assert (init_info && entry);
+
+      g_array_insert_val ((GArray *)init_info->iface_states, i, new_state);
     }
 }
 
@@ -2049,11 +2226,10 @@ g_type_add_interface_static (GType      
       if (check_interface_info_I (iface, NODE_TYPE (node), info))
 	{
 	  type_add_interface_W (node, iface, info, NULL);
-	  /* if we have a class already, the interface vtable needs to
-	   * be initialized as well
+	  /* If we have a class already, we may need to base initalize
+	   * and/or initialize the new interface.
 	   */
-	  if (node->data && node->data->class.class)
-	    type_iface_vtable_init_Wm (iface, node);
+	  type_iface_vtable_init_Wm (node, iface);
 	}
     }
   G_WRITE_UNLOCK (&type_rw_lock);
@@ -2080,11 +2256,10 @@ g_type_add_interface_dynamic (GType     
       TypeNode *iface = lookup_type_node_I (interface_type);
       
       type_add_interface_W (node, iface, NULL, plugin);
-      /* if we have a class already, the interface vtable needs to
-       * be initialized as well
+      /* If we have a class already, we may need to base initalize
+       * and/or initialize the new interface.
        */
-      if (node->data && node->data->class.class)
-	type_iface_vtable_init_Wm (iface, node);
+      type_iface_vtable_init_Wm (node, iface);
     }
   G_WRITE_UNLOCK (&type_rw_lock);
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]