Re: [PATCH v5 12/12] perf annotate: Use a hashmap to save type data

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Aug 20, 2025 at 06:37:18PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Aug 15, 2025 at 08:16:35PM -0700, Namhyung Kim wrote:
> > It can slowdown annotation browser if objdump is processing large DWARF
> > data.  Let's add a hashmap to save the data type info for each line.
> > 
> > Note that this is needed for TUI only because stdio only processes each
> > line once.  TUI will display the same line whenever it refreshes the
> > screen.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/browsers/annotate.c | 34 ++++++++++++++++++++++++++++++-
> >  tools/perf/util/annotate.c        | 27 ++++++++++++++++++++++--
> >  tools/perf/util/annotate.h        |  2 ++
> >  3 files changed, 60 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 9aa3c1ba22f52789..9677a3763a290a3d 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -6,6 +6,7 @@
> >  #include "../../util/debug.h"
> >  #include "../../util/debuginfo.h"
> >  #include "../../util/dso.h"
> > +#include "../../util/hashmap.h"
> >  #include "../../util/hist.h"
> >  #include "../../util/sort.h"
> >  #include "../../util/map.h"
> > @@ -15,6 +16,7 @@
> >  #include "../../util/evlist.h"
> >  #include "../../util/thread.h"
> >  #include <inttypes.h>
> > +#include <linux/err.h>
> >  #include <linux/kernel.h>
> >  #include <linux/string.h>
> >  #include <linux/zalloc.h>
> > @@ -36,6 +38,7 @@ struct annotate_browser {
> >  	struct hist_entry	   *he;
> >  	struct debuginfo	   *dbg;
> >  	struct evsel		   *evsel;
> > +	struct hashmap		   *type_hash;
> >  	bool			    searching_backwards;
> >  	char			    search_bf[128];
> >  };
> > @@ -43,6 +46,16 @@ struct annotate_browser {
> >  /* A copy of target hist_entry for perf top. */
> >  static struct hist_entry annotate_he;
> >  
> > +static size_t type_hash(long key, void *ctx __maybe_unused)
> > +{
> > +	return key;
> > +}
> > +
> > +static bool type_equal(long key1, long key2, void *ctx __maybe_unused)
> > +{
> > +	return key1 == key2;
> > +}
> > +
> >  static inline struct annotation *browser__annotation(struct ui_browser *browser)
> >  {
> >  	struct map_symbol *ms = browser->priv;
> > @@ -130,6 +143,9 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
> >  	if (!browser->navkeypressed)
> >  		ops.width += 1;
> >  
> > +	if (!IS_ERR_OR_NULL(ab->type_hash))
> > +		apd.type_hash = ab->type_hash;
> > +
> >  	annotation_line__write(al, notes, &ops, &apd);
> >  
> >  	if (ops.current_entry)
> > @@ -1051,6 +1067,10 @@ static int annotate_browser__run(struct annotate_browser *browser,
> >  			annotate_opts.code_with_type ^= 1;
> >  			if (browser->dbg == NULL)
> >  				browser->dbg = dso__debuginfo(map__dso(ms->map));
> > +			if (browser->type_hash == NULL) {
> > +				browser->type_hash = hashmap__new(type_hash, type_equal,
> > +								  /*ctx=*/NULL);
> > +			}
> >  			annotate_browser__show(&browser->b, title, help);
> >  			annotate_browser__debuginfo_warning(browser);
> >  			continue;
> > @@ -1145,8 +1165,10 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> >  
> >  	ui_helpline__push("Press ESC to exit");
> >  
> > -	if (annotate_opts.code_with_type)
> > +	if (annotate_opts.code_with_type) {
> >  		browser.dbg = dso__debuginfo(dso);
> > +		browser.type_hash = hashmap__new(type_hash, type_equal, /*ctx=*/NULL);
> > +	}
> >  
> >  	browser.b.width = notes->src->widths.max_line_len;
> >  	browser.b.nr_entries = notes->src->nr_entries;
> > @@ -1159,6 +1181,16 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> >  	ret = annotate_browser__run(&browser, evsel, hbt);
> >  
> >  	debuginfo__delete(browser.dbg);
> > +
> > +	if (!IS_ERR_OR_NULL(browser.type_hash)) {
> > +		struct hashmap_entry *cur;
> > +		size_t bkt;
> > +
> > +		hashmap__for_each_entry(browser.type_hash, cur, bkt)
> > +			free(cur->pvalue);
> 
> 			zfree(&cur->pvalue);

Yep. :)

> 
> > +		hashmap__free(browser.type_hash);
> > +	}
> > +
> >  	if (not_annotated && !notes->src->tried_source)
> >  		annotated_source__purge(notes->src);
> >  
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index bea3457a00632fd7..77414e04d99bb4f2 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1954,11 +1954,17 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
> >  	return -ENOMEM;
> >  }
> >  
> > +struct type_hash_entry {
> > +	struct annotated_data_type *type;
> > +	int offset;
> > +};
> > +
> >  static int disasm_line__snprint_type_info(struct disasm_line *dl,
> >  					  char *buf, int len,
> >  					  struct annotation_print_data *apd)
> >  {
> > -	struct annotated_data_type *data_type;
> > +	struct annotated_data_type *data_type = NULL;
> > +	struct type_hash_entry *entry = NULL;
> >  	char member[256];
> >  	int offset = 0;
> >  	int printed;
> > @@ -1968,7 +1974,24 @@ static int disasm_line__snprint_type_info(struct disasm_line *dl,
> >  	if (!annotate_opts.code_with_type || apd->dbg == NULL)
> >  		return 1;
> >  
> > -	data_type = __hist_entry__get_data_type(apd->he, apd->arch, apd->dbg, dl, &offset);
> > +	if (apd->type_hash) {
> > +		hashmap__find(apd->type_hash, dl->al.offset, &entry);
> > +		if (entry != NULL) {
> > +			data_type = entry->type;
> > +			offset = entry->offset;
> > +		}
> > +	}
> > +	if (data_type == NULL)
> > +		data_type = __hist_entry__get_data_type(apd->he, apd->arch, apd->dbg, dl, &offset);
> 
> 
> add space?

Sure.

> 
> > +	if (apd->type_hash && entry == NULL) {
> > +		entry = malloc(sizeof(*entry));
> 
> Is the 'entry' variable needed anywhere else? Not, so could be declared
> here to save a line at the start of the function. Or is it used in a
> later patch outside of this scope?

It was used in the earlier block to look up an existing entry.

Thanks,
Namhyung

> 
> > +		if (entry != NULL) {
> > +			entry->type = data_type;
> > +			entry->offset = offset;
> > +			hashmap__add(apd->type_hash, dl->al.offset, entry);
> > +		}
> > +	}
> > +
> >  	if (!needs_type_info(data_type))
> >  		return 1;
> >  
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 86e858f5bf173152..eaf6c8aa7f473959 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -204,6 +204,8 @@ struct annotation_print_data {
> >  	struct evsel *evsel;
> >  	struct arch *arch;
> >  	struct debuginfo *dbg;
> > +	/* save data type info keyed by al->offset */
> > +	struct hashmap *type_hash;
> >  	/* It'll be set in hist_entry__annotate_printf() */
> >  	int addr_fmt_width;
> >  };
> > -- 
> > 2.50.1
> >