Add a destroy notifier to StackStash

2006-07-31  Soren Sandmann <sandmann@redhat.com>

	* stackstash.[ch]: Add a destroy notifier to StackStash

	* collector.c (collector_create_profile): Pass g_free as destroy
	notifier.

	* collector.c (collector_reset): Pass NULL as destroy notifier

	* profile.c (profile_load): Pass g_free here.

	* profile.c (struct Profile): Remove unused "Node" typedef

	* collector.c (resolve_symbols): Free the array here.

	* TODO: various updates.

ChangeLog

@@ -1,3 +1,20 @@
+2006-07-31  Soren Sandmann <sandmann@redhat.com>
+
+	* stackstash.[ch]: Add a destroy notifier to StackStash
+
+	* collector.c (collector_create_profile): Pass g_free as destroy
+	notifier.
+
+	* collector.c (collector_reset): Pass NULL as destroy notifier
+
+	* profile.c (profile_load): Pass g_free here.
+
+	* profile.c (struct Profile): Remove unused "Node" typedef 
+
+	* collector.c (resolve_symbols): Free the array here.
+
+	* TODO: various updates.
+	
 2006-07-30  Soren Sandmann <sandmann@redhat.com>
 
 	* signal-handler.c: Simplify this file a bit, and make it not rely

TODO

@@ -96,6 +96,11 @@ Before 1.2:
 
 - plug all the leaks
 	- don't leak the presentation strings/objects
+		- maybe add stack_stash_set_free_func() or something
+		- perhaps think a little bit more about the presentation
+		  object idea. Having pango markup available might be nice
+		  as well ... Plus the unique_dup() scheme is somewhat
+		  confusing. 
 	- loading and saving probably leak right now
 
 - rethink loading and saving. Goals
@@ -146,7 +151,22 @@ Before 1.2:
 	  input and output. As a first cut, perhaps just split out the 
 	  Instruction code.
 
+		(done, somewhat).
+
 	- make the api saner; add format/content structs
+		Idea: all types have to be declared:
+
+		SFormat *format = sformat_new()
+		SType   *object_list = stype_new (format);
+		SType   *object = stype_new (format);
+		...
+
+		stype_define_list (object_list, "objects", object);
+		stype_define_record (object, "object",
+				     name, total, self, NULL);
+		stype_define_pointer (...);
+		
+
 	
 * See if the auto-expanding can be made more intelligent
 	- "Everything" should be expanded exactly one level

collector.c

@@ -246,7 +246,7 @@ collector_reset (Collector *collector)
 
     process_flush_caches();
     
-    collector->stash = stack_stash_new ();
+    collector->stash = stack_stash_new (NULL);
     collector->n_samples = 0;
     
     g_get_current_time (&collector->latest_reset);
@@ -315,6 +315,8 @@ resolve_symbols (GList *trace, gint size, gpointer data)
     stack_stash_add_trace (info->resolved_stash,
 			   (gulong *)resolved_trace->pdata,
 			   resolved_trace->len, size);
+
+    g_ptr_array_free (resolved_trace, TRUE);
 }
 
 Profile *
@@ -323,11 +325,13 @@ collector_create_profile (Collector *collector)
     ResolveInfo info;
     Profile *profile;
     
-    info.resolved_stash = stack_stash_new ();
+    info.resolved_stash = stack_stash_new ((GDestroyNotify)g_free);
     info.unique_symbols = g_hash_table_new (g_direct_hash, g_direct_equal);
     
     stack_stash_foreach (collector->stash, resolve_symbols, &info);
 
+    /* FIXME: we are leaking the value strings in info.unique_symbols */
+    
     g_hash_table_destroy (info.unique_symbols);
 
     profile = profile_new (info.resolved_stash);

profile.c

@@ -27,8 +27,6 @@
 #include "profile.h"
 #include "sfile.h"
 
-typedef struct Node Node;
-
 struct Profile
 {
     StackStash *stash;
@@ -227,7 +225,7 @@ profile_load (const char *filename, GError **err)
     sformat_free (format);
     sfile_input_free (input);
 
-    profile->stash = stack_stash_new_from_root (root);
+    profile->stash = stack_stash_new_from_root (root, (GDestroyNotify)g_free);
     
     return profile;
 }
@@ -518,10 +516,6 @@ profile_get_objects (Profile *profile)
     stack_stash_foreach_by_address (
 	profile->stash, build_object_list, &objects);
     
-    /* FIXME: everybody still assumes that they don't have to free the
-     * objects in the list, but these days they do, and so we are leaking.
-     */
-    
     return objects;
 }
 

signal-handler.c

@@ -85,6 +85,12 @@ signal_handler (int        signo,
 {
     /* FIXME: I suppose we should handle short
      * and non-successful writes ...
+     *
+     * And also, there is a deadlock if so many signals arrive that
+     * write() blocks. Then we will be stuck right here, and the
+     * main loop will never run. Kinda hard to fix without dropping
+     * signals ...
+     *
      */
     write (write_end, &signo, sizeof (int));
 }

stackstash.c

@@ -21,9 +21,10 @@
 
 struct StackStash
 {
-    StackNode  *root;
+    int			ref_count;
-    GHashTable *nodes_by_data;
+    StackNode *		root;
-    int		ref_count;
+    GHashTable *	nodes_by_data;
+    GDestroyNotify	destroy;
 };
 
 static StackNode *
@@ -40,23 +41,25 @@ stack_node_new (void)
     return node;
 }
 
+/* "destroy", if non-NULL, is called once on every address */
 static StackStash *
-create_stack_stash (void)
+create_stack_stash (GDestroyNotify destroy)
 {
     StackStash *stash = g_new (StackStash, 1);
 
     stash->root = NULL;
     stash->nodes_by_data = g_hash_table_new (g_direct_hash, g_direct_equal);
     stash->ref_count = 1;
+    stash->destroy = destroy;
 
     return stash;
 }
 
 /* Stach */
 StackStash *
-stack_stash_new       (void)
+stack_stash_new (GDestroyNotify destroy)
 {
-    return create_stack_stash();
+    return create_stack_stash (destroy);
 }
 
 void
@@ -195,10 +198,27 @@ stack_node_free (StackNode *node)
     g_free (node);
 }
 
+static void
+free_key (gpointer key,
+	  gpointer value,
+	  gpointer data)
+{
+    GDestroyNotify destroy = data;
+
+    destroy (key);
+}
+
 static void
 stack_stash_free (StackStash *stash)
 {
     stack_node_free (stash->root);
+
+    if (stash->destroy)
+    {
+	g_hash_table_foreach (stash->nodes_by_data, free_key,
+			      stash->destroy);
+    }
+    
     g_hash_table_destroy (stash->nodes_by_data);
     
     g_free (stash);
@@ -277,9 +297,10 @@ build_hash_table (StackNode *node,
 }
 
 StackStash *
-stack_stash_new_from_root (StackNode *root)
+stack_stash_new_from_root (StackNode      *root,
+			   GDestroyNotify  destroy)
 {
-    StackStash *stash = create_stack_stash();
+    StackStash *stash = create_stack_stash (destroy);
 
     stash->root = root;
     

stackstash.h

@@ -48,7 +48,9 @@ typedef void (* StackNodeFunc) (StackNode *node,
 				gpointer data);
 
 /* Stach */
-StackStash *stack_stash_new       (void);
+StackStash *stack_stash_new       (GDestroyNotify  destroy);
+StackStash *stack_stash_new_from_root (StackNode *root,
+				       GDestroyNotify destroy);
 void        stack_stash_add_trace (StackStash      *stash,
 				   gulong          *addrs,
 				   gint	            n_addrs,
@@ -65,7 +67,6 @@ void	    stack_stash_foreach_by_address (StackStash *stash,
 					    StackNodeFunc func,
 					    gpointer	  data);
 StackNode  *stack_stash_get_root   (StackStash *stash);
-StackStash *stack_stash_new_from_root (StackNode *root);
 StackStash *stack_stash_ref       (StackStash      *stash);
 void	    stack_stash_unref	  (StackStash	   *stash);
 

sysprof.c

@@ -1558,6 +1558,14 @@ main (int    argc,
     
     gtk_init (&argc, &argv);
     
+#if 0
+    /* FIXME: enable this when compiled against the relevant glib
+     * version. (The reason we want to enable it is that gslice
+     * caches too much memory and also confuses valgrind).
+     */
+    g_slice_set_config (G_SLICE_CONFIG_ALWAYS_MALLOC, TRUE);
+#endif
+    
     app = application_new ();
     
     if (!build_gui (app))