Updates

2007-11-16  Soren Sandmann <sandmann@daimi.au.dk>

	* TODO: Updates
	
	* process.c (process_locate_map): Move map to front

	* profile.c (profile_load): Ignore the toplevel field in the file
	since we can compute it ourselves.

	* stackstash.c (stack_stash_decorate): New function
	
	* stackstash.c (stack_stash_add_trace): Decorate the tree lazily
	instead of on each sample.



svn path=/trunk/; revision=387

ChangeLog

@@ -1,3 +1,17 @@
+2007-11-16  Soren Sandmann <sandmann@daimi.au.dk>
+
+	* TODO: Updates
+	
+	* process.c (process_locate_map): Move map to front
+
+	* profile.c (profile_load): Ignore the toplevel field in the file
+	since we can compute it ourselves.
+
+	* stackstash.c (stack_stash_decorate): New function
+	
+	* stackstash.c (stack_stash_add_trace): Decorate the tree lazily
+	instead of on each sample.
+
 2007-10-22  Soren Sandmann <sandmann@daimi.au.dk>
 
 	* process.c (look_for_vmlinux): Use an array instead of a

TODO

@@ -23,6 +23,11 @@ Before 1.0.4:
 
 Before 1.2:
 
+* If we profile something that is not very CPU bound, sysprof itself
+  seems to get a disproportionate amount of the samples. Should look into this.
+
+* Is the move-to-front in process_locate_map() really worth it?
+
 * Apparently, if you upgrade the kernel, then don't re-run configure, 
   the kernel Makefile will delete all of /lib/modules/<release>/kernel 
   if you run make install in the module directory. Need to find out what

process.c

@@ -127,13 +127,13 @@ read_maps (int pid, int *n_maps)
 	    g_array_append_val (result, map);
 	}
     }
-    
+
     g_free (name);
     fclose (in);
 
     if (n_maps)
 	*n_maps = result->len;
-    
+
     return (Map *)g_array_free (result, FALSE);
 }
 
@@ -241,6 +241,18 @@ process_locate_map (Process *process, gulong addr)
 	if ((addr >= map->start) &&
 	    (addr < map->end))
 	{
+	    if (i > 4)
+	    {
+		/* FIXME: Is this move-to-front really worth it? */
+		Map tmp = *map;
+
+		memmove (process->maps + 1, process->maps, i * sizeof (Map));
+
+		*(process->maps) = tmp;
+
+		map = process->maps;
+	    }
+	    
 	    return map;
 	}
     }

profile.c

@@ -191,7 +191,6 @@ profile_load (const char *filename, GError **err)
     for (i = 0; i < n; ++i)
     {
 	StackNode *node = stack_node_new (profile->stash);
-	gboolean toplevel;
 	gint32 size;
 	gint32 total;
 	
@@ -203,10 +202,9 @@ profile_load (const char *filename, GError **err)
 	sfile_get_pointer (input, "parent", (gpointer *)&node->parent);
 	sfile_get_integer (input, "total", &total);
 	sfile_get_integer (input, "self", (gint32 *)&size);
-	sfile_get_integer (input, "toplevel", &toplevel);
+	sfile_get_integer (input, "toplevel", NULL);
 
 	node->total = total;
-	node->toplevel = toplevel;
 	node->size = size;
 	
 	sfile_end_get (input, "node", node);

stackstash.c

@@ -30,6 +30,80 @@ struct StackStash
     GPtrArray *		blocks;
 };
 
+static void
+decorate_node (StackNode *node,
+	       StackStash *stash)
+{
+    StackNode *n;
+
+    if (!node)
+	return;
+
+    decorate_node (node->siblings, stash);
+    decorate_node (node->children, stash);
+
+    node->next = g_hash_table_lookup (stash->nodes_by_data, node->address);
+    g_hash_table_insert (stash->nodes_by_data, node->address, node);
+	
+    /* FIXME: This could be done more efficiently
+     * by keeping track of the ancestors we have seen.
+     */
+    node->toplevel = TRUE;
+    for (n = node->parent; n != NULL; n = n->parent)
+    {
+	if (n->address == node->address)
+	{
+	    node->toplevel = FALSE;
+	    break;
+	}
+    }
+}
+
+static void
+stack_stash_decorate (StackStash *stash)
+{
+    if (stash->nodes_by_data)
+	return;
+
+    stash->nodes_by_data = g_hash_table_new (g_direct_hash, g_direct_equal);
+
+    decorate_node (stash->root, stash);
+}
+
+static void
+free_key (gpointer key,
+	  gpointer value,
+	  gpointer data)
+{
+    GDestroyNotify destroy = data;
+
+    destroy (key);
+}
+
+static void
+stack_stash_undecorate (StackStash *stash)
+{
+    if (stash->nodes_by_data)
+    {
+	if (stash->destroy)
+	{
+	    g_hash_table_foreach (
+		stash->nodes_by_data, free_key, stash->destroy);
+	}
+	
+	g_hash_table_destroy (stash->nodes_by_data);
+    }
+}
+
+static GHashTable *
+get_nodes_by_data (StackStash *stash)
+{
+    if (!stash->nodes_by_data)
+	stack_stash_decorate (stash);
+
+    return stash->nodes_by_data;
+}
+
 StackNode *
 stack_node_new (StackStash *stash)
 {
@@ -73,7 +147,7 @@ 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->nodes_by_data = NULL;
     stash->ref_count = 1;
     stash->destroy = destroy;
     
@@ -91,28 +165,12 @@ stack_stash_new (GDestroyNotify destroy)
 }
 
 
-static void
-free_key (gpointer key,
-	  gpointer value,
-	  gpointer data)
-{
-    GDestroyNotify destroy = data;
-
-    destroy (key);
-}
-
 static void
 stack_stash_free (StackStash *stash)
 {
     int i;
     
-    if (stash->destroy)
+    stack_stash_undecorate (stash);
-    {
-	g_hash_table_foreach (stash->nodes_by_data, free_key,
-			      stash->destroy);
-    }
-    
-    g_hash_table_destroy (stash->nodes_by_data);
 
     for (i = 0; i < stash->blocks->len; ++i)
 	g_free (stash->blocks->pdata[i]);
@@ -122,33 +180,6 @@ stack_stash_free (StackStash *stash)
     g_free (stash);
 }
 
-static void
-decorate_node (StackStash *stash,
-	       StackNode  *node)
-{
-    StackNode *n;
-    gboolean toplevel = TRUE;
-
-    /* FIXME: we will probably want to do this lazily,
-     * and more efficiently (only walk the tree once).
-     */
-    for (n = node->parent; n != NULL; n = n->parent)
-    {
-	if (n->address == node->address)
-	{
-	    toplevel = FALSE;
-	    break;
-	}
-    }
-
-    node->toplevel = toplevel;
-
-    node->next = g_hash_table_lookup (
-	stash->nodes_by_data, node->address);
-    g_hash_table_insert (
-	stash->nodes_by_data, node->address, node);
-}
-
 void
 stack_stash_add_trace (StackStash *stash,
 		       gulong     *addrs,
@@ -161,6 +192,9 @@ stack_stash_add_trace (StackStash *stash,
 
     if (!n_addrs)
 	return;
+
+    if (stash->nodes_by_data)
+	stack_stash_undecorate (stash);
     
     for (i = n_addrs - 1; i >= 0; --i)
     {
@@ -168,7 +202,7 @@ stack_stash_add_trace (StackStash *stash,
 	StackNode *prev;
 
 	prev = NULL;
-	for (match = *location; match != NULL; prev = match, match = match->siblings)
+	for (match = *location; match; prev = match, match = match->siblings)
 	{
 	    if (match->address == (gpointer)addrs[i])
 	    {
@@ -191,8 +225,6 @@ stack_stash_add_trace (StackStash *stash,
 	    match->siblings = *location;
 	    match->parent = parent;
 	    *location = match;
-
-	    decorate_node (stash, match);
 	}
 
 	match->total += size;
@@ -235,9 +267,9 @@ do_callback (StackNode *node,
 }
 
 void
-stack_stash_foreach   (StackStash      *stash,
+stack_stash_foreach (StackStash      *stash,
-		       StackFunction    stack_func,
+		     StackFunction    stack_func,
-		       gpointer         data)
+		     gpointer         data)
 {
     do_callback (stash->root, NULL, stack_func, data);
 }
@@ -280,7 +312,7 @@ stack_stash_find_node (StackStash      *stash,
 {
     g_return_val_if_fail (stash != NULL, NULL);
     
-    return g_hash_table_lookup (stash->nodes_by_data, data);
+    return g_hash_table_lookup (get_nodes_by_data (stash), data);
 }
 
 typedef struct
@@ -306,38 +338,20 @@ stack_stash_foreach_by_address (StackStash *stash,
     info.func = func;
     info.data = data;
 	
-    g_hash_table_foreach (stash->nodes_by_data, do_foreach, &info);
+    g_hash_table_foreach (get_nodes_by_data (stash), do_foreach, &info);
 }
 
 StackNode  *
-stack_stash_get_root   (StackStash *stash)
+stack_stash_get_root (StackStash *stash)
 {
     return stash->root;
 }
 
-static void
-build_hash_table (StackNode *node,
-		  StackStash *stash)
-{
-    if (!node)
-	return;
-
-    build_hash_table (node->siblings, stash);
-    build_hash_table (node->children, stash);
-
-    node->next = g_hash_table_lookup (
-	stash->nodes_by_data, node->address);
-    g_hash_table_insert (
-	stash->nodes_by_data, node->address, node);
-}
-
 void
 stack_stash_set_root (StackStash     *stash,
 		      StackNode      *root)
 {
     g_return_if_fail (stash->root == NULL);
-    g_return_if_fail (g_hash_table_size (stash->nodes_by_data) == 0);
     
     stash->root = root;
-    build_hash_table (stash->root, stash);
 }