diff mbox series

[13/52] perf annotate-data: Add dso->data_types tree

Message ID 20231110000012.3538610-14-namhyung@kernel.org (mailing list archive)
State Superseded
Headers show
Series perf tools: Introduce data type profiling (v2) | expand

Commit Message

Namhyung Kim Nov. 9, 2023, 11:59 p.m. UTC
To aggregate accesses to the same data type, add 'data_types' tree in
DSO to maintain data types and find it by name and size.  It might have
different data types that happen to have the same name.  So it also
compares the size of the type.  Even if it doesn't 100% guarantee, it'd
reduce the possiblility of mis-handling of such conflicts.  And I don't
think it's common to have different types with the same name.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 95 +++++++++++++++++++++++++++++----
 tools/perf/util/annotate-data.h |  9 ++++
 tools/perf/util/dso.c           |  4 ++
 tools/perf/util/dso.h           |  2 +
 4 files changed, 100 insertions(+), 10 deletions(-)

Comments

Arnaldo Carvalho de Melo Dec. 21, 2023, 8:10 p.m. UTC | #1
Em Thu, Nov 09, 2023 at 03:59:32PM -0800, Namhyung Kim escreveu:
> +++ b/tools/perf/util/dso.h
> @@ -154,6 +154,8 @@ struct dso {
>  	size_t		 symbol_names_len;
>  	struct rb_root_cached inlined_nodes;
>  	struct rb_root_cached srclines;
> +	struct rb_root	data_types;
> +
>  	struct {
>  		u64		addr;
>  		struct symbol	*symbol;

At some point we need to make these feature specific members to be
associated on demand, maybe thru some hash table, etc.

I.e. the most basic workflow, what everybody needs should be in 'struct
dso', something one _may_ want, like data profiling, should be in
associated with that DSO thru some other way.

I'm applying this now as this is a super cool feature, but think about
it.

- Arnaldo
Arnaldo Carvalho de Melo Dec. 21, 2023, 8:13 p.m. UTC | #2
Em Thu, Dec 21, 2023 at 05:10:53PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Nov 09, 2023 at 03:59:32PM -0800, Namhyung Kim escreveu:
> > +++ b/tools/perf/util/dso.h
> > @@ -154,6 +154,8 @@ struct dso {
> >  	size_t		 symbol_names_len;
> >  	struct rb_root_cached inlined_nodes;
> >  	struct rb_root_cached srclines;
> > +	struct rb_root	data_types;
> > +
> >  	struct {
> >  		u64		addr;
> >  		struct symbol	*symbol;
> 
> At some point we need to make these feature specific members to be
> associated on demand, maybe thru some hash table, etc.
> 
> I.e. the most basic workflow, what everybody needs should be in 'struct
> dso', something one _may_ want, like data profiling, should be in
> associated with that DSO thru some other way.
> 
> I'm applying this now as this is a super cool feature, but think about
> it.

I think I have this series applied up to this patch, the next one is not
applying cleanly, so I'll do the usual set of build tests so that I can
push this for linux-next consumption.

This should be on tmp.perf-tools-next in a few jiffies, in
perf-tools-next a bit later.

- Arnaldo
Arnaldo Carvalho de Melo Dec. 21, 2023, 8:32 p.m. UTC | #3
Em Thu, Dec 21, 2023 at 05:13:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> > At some point we need to make these feature specific members to be
> > associated on demand, maybe thru some hash table, etc.

> > I.e. the most basic workflow, what everybody needs should be in 'struct
> > dso', something one _may_ want, like data profiling, should be in
> > associated with that DSO thru some other way.

> > I'm applying this now as this is a super cool feature, but think about
> > it.

> I think I have this series applied up to this patch, the next one is not
> applying cleanly, so I'll do the usual set of build tests so that I can
> push this for linux-next consumption.

> This should be on tmp.perf-tools-next in a few jiffies, in
> perf-tools-next a bit later.

Ok, as discussed on google chat, I got v3, all is in my local tree,
build testing while I review.

- Arnaldo
diff mbox series

Patch

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 98c42dff2645..475cc30b33e1 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -17,6 +17,76 @@ 
 #include "strbuf.h"
 #include "symbol.h"
 
+/*
+ * Compare type name and size to maintain them in a tree.
+ * I'm not sure if DWARF would have information of a single type in many
+ * different places (compilation units).  If not, it could compare the
+ * offset of the type entry in the .debug_info section.
+ */
+static int data_type_cmp(const void *_key, const struct rb_node *node)
+{
+	const struct annotated_data_type *key = _key;
+	struct annotated_data_type *type;
+
+	type = rb_entry(node, struct annotated_data_type, node);
+
+	if (key->type_size != type->type_size)
+		return key->type_size - type->type_size;
+	return strcmp(key->type_name, type->type_name);
+}
+
+static bool data_type_less(struct rb_node *node_a, const struct rb_node *node_b)
+{
+	struct annotated_data_type *a, *b;
+
+	a = rb_entry(node_a, struct annotated_data_type, node);
+	b = rb_entry(node_b, struct annotated_data_type, node);
+
+	if (a->type_size != b->type_size)
+		return a->type_size < b->type_size;
+	return strcmp(a->type_name, b->type_name) < 0;
+}
+
+static struct annotated_data_type *dso__findnew_data_type(struct dso *dso,
+							  Dwarf_Die *type_die)
+{
+	struct annotated_data_type *result = NULL;
+	struct annotated_data_type key;
+	struct rb_node *node;
+	struct strbuf sb;
+	char *type_name;
+	Dwarf_Word size;
+
+	strbuf_init(&sb, 32);
+	if (die_get_typename_from_type(type_die, &sb) < 0)
+		strbuf_add(&sb, "(unknown type)", 14);
+	type_name = strbuf_detach(&sb, NULL);
+	dwarf_aggregate_size(type_die, &size);
+
+	/* Check existing nodes in dso->data_types tree */
+	key.type_name = type_name;
+	key.type_size = size;
+	node = rb_find(&key, &dso->data_types, data_type_cmp);
+	if (node) {
+		result = rb_entry(node, struct annotated_data_type, node);
+		free(type_name);
+		return result;
+	}
+
+	/* If not, add a new one */
+	result = zalloc(sizeof(*result));
+	if (result == NULL) {
+		free(type_name);
+		return NULL;
+	}
+
+	result->type_name = type_name;
+	result->type_size = size;
+
+	rb_add(&result->node, &dso->data_types, data_type_less);
+	return result;
+}
+
 static bool find_cu_die(struct debuginfo *di, u64 pc, Dwarf_Die *cu_die)
 {
 	Dwarf_Off off, next_off;
@@ -129,7 +199,6 @@  struct annotated_data_type *find_data_type(struct map_symbol *ms, u64 ip,
 	struct dso *dso = ms->map->dso;
 	struct debuginfo *di;
 	Dwarf_Die type_die;
-	struct strbuf sb;
 	u64 pc;
 
 	di = debuginfo__new(dso->long_name);
@@ -147,17 +216,23 @@  struct annotated_data_type *find_data_type(struct map_symbol *ms, u64 ip,
 	if (find_data_type_die(di, pc, reg, offset, &type_die) < 0)
 		goto out;
 
-	result = zalloc(sizeof(*result));
-	if (result == NULL)
-		goto out;
-
-	strbuf_init(&sb, 32);
-	if (die_get_typename_from_type(&type_die, &sb) < 0)
-		strbuf_add(&sb, "(unknown type)", 14);
-
-	result->type_name = strbuf_detach(&sb, NULL);
+	result = dso__findnew_data_type(dso, &type_die);
 
 out:
 	debuginfo__delete(di);
 	return result;
 }
+
+void annotated_data_type__tree_delete(struct rb_root *root)
+{
+	struct annotated_data_type *pos;
+
+	while (!RB_EMPTY_ROOT(root)) {
+		struct rb_node *node = rb_first(root);
+
+		rb_erase(node, root);
+		pos = rb_entry(node, struct annotated_data_type, node);
+		free(pos->type_name);
+		free(pos);
+	}
+}
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 633147f78ca5..ab9f187bd7f1 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -4,6 +4,7 @@ 
 
 #include <errno.h>
 #include <linux/compiler.h>
+#include <linux/rbtree.h>
 #include <linux/types.h>
 
 struct map_symbol;
@@ -16,6 +17,7 @@  struct map_symbol;
  * This represents a data type accessed by samples in the profile data.
  */
 struct annotated_data_type {
+	struct rb_node node;
 	char *type_name;
 	int type_size;
 };
@@ -26,6 +28,9 @@  struct annotated_data_type {
 struct annotated_data_type *find_data_type(struct map_symbol *ms, u64 ip,
 					   int reg, int offset);
 
+/* Release all data type information in the tree */
+void annotated_data_type__tree_delete(struct rb_root *root);
+
 #else /* HAVE_DWARF_SUPPORT */
 
 static inline struct annotated_data_type *
@@ -35,6 +40,10 @@  find_data_type(struct map_symbol *ms __maybe_unused, u64 ip __maybe_unused,
 	return NULL;
 }
 
+static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe_unused)
+{
+}
+
 #endif /* HAVE_DWARF_SUPPORT */
 
 #endif /* _PERF_ANNOTATE_DATA_H */
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 1f629b6fb7cf..22fd5fa806ed 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -31,6 +31,7 @@ 
 #include "debug.h"
 #include "string2.h"
 #include "vdso.h"
+#include "annotate-data.h"
 
 static const char * const debuglink_paths[] = {
 	"%.0s%s",
@@ -1327,6 +1328,7 @@  struct dso *dso__new_id(const char *name, struct dso_id *id)
 		dso->data.cache = RB_ROOT;
 		dso->inlined_nodes = RB_ROOT_CACHED;
 		dso->srclines = RB_ROOT_CACHED;
+		dso->data_types = RB_ROOT;
 		dso->data.fd = -1;
 		dso->data.status = DSO_DATA_STATUS_UNKNOWN;
 		dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
@@ -1370,6 +1372,8 @@  void dso__delete(struct dso *dso)
 	symbols__delete(&dso->symbols);
 	dso->symbol_names_len = 0;
 	zfree(&dso->symbol_names);
+	annotated_data_type__tree_delete(&dso->data_types);
+
 	if (dso->short_name_allocated) {
 		zfree((char **)&dso->short_name);
 		dso->short_name_allocated = false;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 3759de8c2267..ce9f3849a773 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -154,6 +154,8 @@  struct dso {
 	size_t		 symbol_names_len;
 	struct rb_root_cached inlined_nodes;
 	struct rb_root_cached srclines;
+	struct rb_root	data_types;
+
 	struct {
 		u64		addr;
 		struct symbol	*symbol;