GNOME.org
 

[PATCH] Reordering interface base initialization

Hi Tim -

Here's the first part of what we discussed for redoing the
interface property changes. What this patch does is split
out allocation and base initialization of interface vtables
from calling the interface init function, and order initialization
as:

 Call base_init functions for class
 Call base_init functions for interfaces
 Call class_init function
 Call interface_init functions

The point being, as you recall, we need to register the 
interface properties we are going to override before we call
the class_init function, so that they can be found by
g_object_class_override_property().


The implementation here is relatively straightforward. To handle
reentrancy, I found it necessary to add an "initialization state"
enumeration field to the ClassData and IFaceEntry structures.

The cost of this is 

 4 bytes * sum_{classes} (class->n_ifaces)

So, on the order of maybe 500-600 bytes for a GTK+ program currently.

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.

The way I have it here is fairly clean and simple, which may
be more important than a small amount of memory saving.


The initialization state field for ClassData may also be useful
for fixing the thread-safety-for-class-initialization problem:

 http://bugzilla.gnome.org/show_bug.cgi?id=64764 

Though I haven't really thought through the consequences.


Please give the patch a read-through; it would be nice to
get this out of the way before tacking class_init for interfaces,
which is going to overlap rather heavily.

Thanks,
						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 00:19:23 -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
 {
@@ -211,6 +228,7 @@ struct _IFaceEntry
 {
   GType           iface_type;
   GTypeInterface *vtable;
+  InitState       init_state;
 };
 struct _CommonData
 {
@@ -228,6 +246,7 @@ struct _ClassData
 {
   CommonData         common;
   guint16            class_size;
+  guint              init_state : 4;
   GBaseInitFunc      class_init_base;
   GBaseFinalizeFunc  class_finalize_base;
   GClassInitFunc     class_init;
@@ -245,6 +264,7 @@ struct _InstanceData
   GClassFinalizeFunc class_finalize;
   gconstpointer      class_data;
   gpointer           class;
+  InitState          init_state;
   guint16            instance_size;
   guint16            private_size;
   guint16            n_preallocs;
@@ -359,7 +379,10 @@ type_node_any_new_W (TypeNode           
 							 sizeof (CLASSED_NODE_IFACES_ENTRIES (pnode)[0]) *
 							 CLASSED_NODE_N_IFACES (node));
 	  for (j = 0; j < CLASSED_NODE_N_IFACES (node); j++)
-	    CLASSED_NODE_IFACES_ENTRIES (node)[j].vtable = NULL;
+	    {
+	      CLASSED_NODE_IFACES_ENTRIES (node)[j].vtable = NULL;
+	      CLASSED_NODE_IFACES_ENTRIES (node)[j].init_state = UNINITIALIZED;
+	    }
 	}
       
       i = pnode->n_children++;
@@ -926,6 +949,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 +975,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))
     {
@@ -1095,6 +1120,7 @@ type_node_add_iface_entry_W (TypeNode *n
   g_memmove (entries + i + 1, entries + i, sizeof (entries[0]) * (CLASSED_NODE_N_IFACES (node) - i - 1));
   entries[i].iface_type = iface_type;
   entries[i].vtable = NULL;
+  entries[i].init_state = UNINITIALIZED;
   
   for (i = 0; i < node->n_children; i++)
     type_node_add_iface_entry_W (lookup_type_node_I (node->children[i]), iface_type);
@@ -1555,19 +1581,28 @@ 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_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.
+ */
 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;
   GTypeInterface *vtable = NULL;
   TypeNode *pnode;
+
+  entry->init_state = IFACE_INIT;
   
   /* 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;
   
   g_assert (iface->data && entry && entry->vtable == NULL && iholder && iholder->info);
   
@@ -1585,16 +1620,45 @@ 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_init_Wm (TypeNode *iface,
+			   TypeNode *node)
+{
+  IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
+  IFaceHolder *iholder;
+  GTypeInterface *vtable = NULL;
+  
+  entry->init_state = INITIALIZED;
+  
+  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
@@ -1613,6 +1677,7 @@ type_iface_vtable_finalize_Wm (TypeNode 
   g_assert (entry && entry->vtable == vtable && iholder->info);
   
   entry->vtable = NULL;
+  entry->init_state = UNINITIALIZED;
   if (iholder->info->interface_finalize || iface->data->iface.vtable_finalize_base)
     {
       G_WRITE_UNLOCK (&type_rw_lock);
@@ -1643,10 +1708,12 @@ type_class_init_Wm (TypeNode   *node,
   
   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);
 
   class = g_malloc0 (node->data->class.class_size);
   node->data->class.class = class;
+  node->data->class.init_state = BASE_CLASS_INIT;
   
   if (pclass)
     {
@@ -1681,21 +1748,30 @@ 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) &&
+	     entry->init_state != UNINITIALIZED)
+	{
+	  entry++;
+	  i++;
+	}
+
+      if (i == CLASSED_NODE_N_IFACES (node))
+	break;
       
-      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;
 	  
@@ -1711,17 +1787,61 @@ type_class_init_Wm (TypeNode   *node,
 	      if (pentry->iface_type == entry->iface_type)
 		{
 		  entry->vtable = pentry->vtable;
+		  entry->init_state = 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) &&
+	     entry->init_state == INITIALIZED)
+	{
+	  entry++;
+	  i++;
+	}
+
+      if (i == CLASSED_NODE_N_IFACES (node))
+	break;
+
+      g_assert (entry->init_state == IFACE_INIT);
+      
+      type_iface_vtable_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;
 }
 
 static void
@@ -2049,10 +2169,13 @@ 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)
+	  if (node->data && node->data->class.init_state >= BASE_IFACE_INIT)
+	    type_iface_vtable_base_init_Wm (iface, node);
+	  
+	  if (node->data && node->data->class.init_state >= IFACE_INIT)
 	    type_iface_vtable_init_Wm (iface, node);
 	}
     }
@@ -2080,10 +2203,13 @@ 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)
+      if (node->data && node->data->class.init_state >= BASE_IFACE_INIT)
+	type_iface_vtable_base_init_Wm (iface, node);
+      
+      if (node->data && node->data->class.init_state >= IFACE_INIT)
 	type_iface_vtable_init_Wm (iface, node);
     }
   G_WRITE_UNLOCK (&type_rw_lock);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]