perf annotate: Make annotation_options global (v1)

PATCH Applied Namhyung Kim 3 participants 22 messages Nov 28, 2023 – Dec 7, 2023 perf events

lore

Share & tools

lore

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #0 lore raw

cover perf annotate: Make annotation_options global (v1)

Hello,

It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.

This code is available at 'perf/annotate-option-v1' branch in

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (8):
perf annotate: Introduce global annotation_options
perf report: Convert to the global annotation_options perf top: Convert to the global annotation_options
perf annotate: Use global annotation_options
perf ui/browser/annotate: Use global annotation_options perf annotate: Ensure init/exit for global options
perf annotate: Remove remaining usages of local annotation options perf annotate: Get rid of local annotation options

 tools/perf/builtin-annotate.c     |  43 +++++----
 tools/perf/builtin-report.c       |  37 ++++----
 tools/perf/builtin-top.c          |  45 +++++-----
 tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
 tools/perf/ui/browsers/hists.c    |  34 +++----
 tools/perf/ui/browsers/hists.h    |   2 -
 tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
 tools/perf/util/annotate.h        |  38 ++++----
 tools/perf/util/block-info.c      |   6 +-
 tools/perf/util/block-info.h      |   3 +-
 tools/perf/util/hist.h            |  25 ++----
 tools/perf/util/top.h             |   1 -
 12 files changed, 206 insertions(+), 255 deletions(-)

base-commit: 757489991f7c08603395b85037a981c31719c92c

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #1 lore raw

PATCH 1/8 perf annotate: Introduce global annotation_options

The annotation options are to control the behavior of objdump and the output. It's basically used by perf annotate but perf report and perf top can call it on TUI dynamically. But it doesn't need to have a copy of annotation options in many places. As most of the work is done in the util/annotate.c file, add a global variable and set/use it instead of having their own copies.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-annotate.c | 43 +++++++++++++++++------------------
 tools/perf/util/annotate.c    |  3 +++
 tools/perf/util/annotate.h    |  2 ++
 3 files changed, 26 insertions(+), 22 deletions(-)

tools/perf/builtin-annotate.c +21 ~~-22~~

index a9129b51d511..67b36a7a12e3 100644
@@ -45,7 +45,6 @@
 struct perf_annotate {
 	struct perf_tool tool;
 	struct perf_session *session;
-	struct annotation_options opts;
 #ifdef HAVE_SLANG_SUPPORT
 	bool	   use_tui;
 #endif

@@ -318,9 +317,9 @@ static int hist_entry__tty_annotate(struct hist_entry *he,
 				    struct perf_annotate *ann)
 {
 	if (!ann->use_stdio2)
-		return symbol__tty_annotate(&he->ms, evsel, &ann->opts);
+		return symbol__tty_annotate(&he->ms, evsel, &annotate_opts);
 
-	return symbol__tty_annotate2(&he->ms, evsel, &ann->opts);
+	return symbol__tty_annotate2(&he->ms, evsel, &annotate_opts);
 }
 
 static void hists__find_annotations(struct hists *hists,

@@ -376,14 +375,14 @@ static void hists__find_annotations(struct hists *hists,
 				return;
 			}
 
-			ret = annotate(he, evsel, &ann->opts, NULL);
+			ret = annotate(he, evsel, &annotate_opts, NULL);
 			if (!ret || !ann->skip_missing)
 				return;
 
 			/* skip missing symbols */
 			nd = rb_next(nd);
 		} else if (use_browser == 1) {
-			key = hist_entry__tui_annotate(he, evsel, NULL, &ann->opts);
+			key = hist_entry__tui_annotate(he, evsel, NULL, &annotate_opts);
 
 			switch (key) {
 			case -1:

@@ -425,9 +424,9 @@ static int __cmd_annotate(struct perf_annotate *ann)
 			goto out;
 	}
 
-	if (!ann->opts.objdump_path) {
+	if (!annotate_opts.objdump_path) {
 		ret = perf_env__lookup_objdump(&session->header.env,
-					       &ann->opts.objdump_path);
+					       &annotate_opts.objdump_path);
 		if (ret)
 			goto out;
 	}

@@ -561,9 +560,9 @@ int cmd_annotate(int argc, const char **argv)
 		   "file", "vmlinux pathname"),
 	OPT_BOOLEAN('m', "modules", &symbol_conf.use_modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
-	OPT_BOOLEAN('l', "print-line", &annotate.opts.print_lines,
+	OPT_BOOLEAN('l', "print-line", &annotate_opts.print_lines,
 		    "print matching source lines (may be slow)"),
-	OPT_BOOLEAN('P', "full-paths", &annotate.opts.full_path,
+	OPT_BOOLEAN('P', "full-paths", &annotate_opts.full_path,
 		    "Don't shorten the displayed pathnames"),
 	OPT_BOOLEAN(0, "skip-missing", &annotate.skip_missing,
 		    "Skip symbols that cannot be annotated"),

@@ -574,15 +573,15 @@ int cmd_annotate(int argc, const char **argv)
 	OPT_CALLBACK(0, "symfs", NULL, "directory",
 		     "Look for files with symbols relative to this directory",
 		     symbol__config_symfs),
-	OPT_BOOLEAN(0, "source", &annotate.opts.annotate_src,
+	OPT_BOOLEAN(0, "source", &annotate_opts.annotate_src,
 		    "Interleave source code with assembly code (default)"),
-	OPT_BOOLEAN(0, "asm-raw", &annotate.opts.show_asm_raw,
+	OPT_BOOLEAN(0, "asm-raw", &annotate_opts.show_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
-	OPT_STRING(0, "prefix", &annotate.opts.prefix, "prefix",
+	OPT_STRING(0, "prefix", &annotate_opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
-	OPT_STRING(0, "prefix-strip", &annotate.opts.prefix_strip, "N",
+	OPT_STRING(0, "prefix-strip", &annotate_opts.prefix_strip, "N",
 		    "Strip first N entries of source file path name in programs (with --prefix)"),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),

@@ -601,7 +600,7 @@ int cmd_annotate(int argc, const char **argv)
 	OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
 			     "'always' (default), 'never' or 'auto' only applicable to --stdio mode",
 			     stdio__config_color, "always"),
-	OPT_CALLBACK(0, "percent-type", &annotate.opts, "local-period",
+	OPT_CALLBACK(0, "percent-type", &annotate_opts, "local-period",
 		     "Set percent type local/global-period/hits",
 		     annotate_parse_percent_type),
 	OPT_CALLBACK(0, "percent-limit", &annotate, "percent",

@@ -617,13 +616,13 @@ int cmd_annotate(int argc, const char **argv)
 	set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
 	set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
 
-	annotation_options__init(&annotate.opts);
+	annotation_options__init(&annotate_opts);
 
 	ret = hists__init();
 	if (ret < 0)
 		return ret;
 
-	annotation_config__init(&annotate.opts);
+	annotation_config__init(&annotate_opts);
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
 	if (argc) {

@@ -638,13 +637,13 @@ int cmd_annotate(int argc, const char **argv)
 	}
 
 	if (disassembler_style) {
-		annotate.opts.disassembler_style = strdup(disassembler_style);
-		if (!annotate.opts.disassembler_style)
+		annotate_opts.disassembler_style = strdup(disassembler_style);
+		if (!annotate_opts.disassembler_style)
 			return -ENOMEM;
 	}
 	if (objdump_path) {
-		annotate.opts.objdump_path = strdup(objdump_path);
-		if (!annotate.opts.objdump_path)
+		annotate_opts.objdump_path = strdup(objdump_path);
+		if (!annotate_opts.objdump_path)
 			return -ENOMEM;
 	}
 	if (addr2line_path) {

@@ -653,7 +652,7 @@ int cmd_annotate(int argc, const char **argv)
 			return -ENOMEM;
 	}
 
-	if (annotate_check_args(&annotate.opts) < 0)
+	if (annotate_check_args(&annotate_opts) < 0)
 		return -EINVAL;
 
 #ifdef HAVE_GTK2_SUPPORT

@@ -734,7 +733,7 @@ int cmd_annotate(int argc, const char **argv)
 #ifndef NDEBUG
 	perf_session__delete(annotate.session);
 #endif
-	annotation_options__exit(&annotate.opts);
+	annotation_options__exit(&annotate_opts);
 
 	return ret;
 }

tools/perf/util/annotate.c +3 -0

index 9a828dc601c7..77b78001b94d 100644
@@ -57,6 +57,9 @@
 
 #include <linux/ctype.h>
 
+/* global annotation options */
+struct annotation_options annotate_opts;
+
 static regex_t	 file_lineno;
 
 static struct ins_ops *ins__find(struct arch *arch, const char *name);

tools/perf/util/annotate.h +2 -0

index b64a2be287b3..8c1a070725fa 100644
@@ -105,6 +105,8 @@ struct annotation_options {
 	unsigned int percent_type;
 };
 
+extern struct annotation_options annotate_opts;
+
 enum {
 	ANNOTATION__OFFSET_JUMP_TARGETS = 1,
 	ANNOTATION__OFFSET_CALL,

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #2 lore raw

PATCH 2/8 perf report: Convert to the global annotation_options

In reply to Namhyung Kim

Use the global option and drop the local copy.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-report.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

tools/perf/builtin-report.c +16 ~~-17~~

index 121a2781323c..90f98953587c 100644
@@ -98,7 +98,6 @@ struct report {
 	bool			skip_empty;
 	int			max_stack;
 	struct perf_read_values	show_threads_values;
-	struct annotation_options annotation_opts;
 	const char		*pretty_printing_style;
 	const char		*cpu_list;
 	const char		*symbol_filter_str;

@@ -542,7 +541,7 @@ static int evlist__tui_block_hists_browse(struct evlist *evlist, struct report *
 		ret = report__browse_block_hists(&rep->block_reports[i++].hist,
 						 rep->min_percent, pos,
 						 &rep->session->header.env,
-						 &rep->annotation_opts);
+						 &annotate_opts);
 		if (ret != 0)
 			return ret;
 	}

@@ -670,7 +669,7 @@ static int report__browse_hists(struct report *rep)
 		}
 
 		ret = evlist__tui_browse_hists(evlist, help, NULL, rep->min_percent,
-					       &session->header.env, true, &rep->annotation_opts);
+					       &session->header.env, true, &annotate_opts);
 		/*
 		 * Usually "ret" is the last pressed key, and we only
 		 * care if the key notifies us to switch data file.

@@ -745,7 +744,7 @@ static int hists__resort_cb(struct hist_entry *he, void *arg)
 	if (rep->symbol_ipc && sym && !sym->annotate2) {
 		struct evsel *evsel = hists_to_evsel(he->hists);
 
-		symbol__annotate2(&he->ms, evsel, &rep->annotation_opts, NULL);
+		symbol__annotate2(&he->ms, evsel, &annotate_opts, NULL);
 	}
 
 	return 0;

@@ -1341,15 +1340,15 @@ int cmd_report(int argc, const char **argv)
 		   "list of cpus to profile"),
 	OPT_BOOLEAN('I', "show-info", &report.show_full_info,
 		    "Display extended information about perf.data file"),
-	OPT_BOOLEAN(0, "source", &report.annotation_opts.annotate_src,
+	OPT_BOOLEAN(0, "source", &annotate_opts.annotate_src,
 		    "Interleave source code with assembly code (default)"),
-	OPT_BOOLEAN(0, "asm-raw", &report.annotation_opts.show_asm_raw,
+	OPT_BOOLEAN(0, "asm-raw", &annotate_opts.show_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
-	OPT_STRING(0, "prefix", &report.annotation_opts.prefix, "prefix",
+	OPT_STRING(0, "prefix", &annotate_opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
-	OPT_STRING(0, "prefix-strip", &report.annotation_opts.prefix_strip, "N",
+	OPT_STRING(0, "prefix-strip", &annotate_opts.prefix_strip, "N",
 		    "Strip first N entries of source file path name in programs (with --prefix)"),
 	OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
 		    "Show a column with the sum of periods"),

@@ -1401,7 +1400,7 @@ int cmd_report(int argc, const char **argv)
 		   "Time span of interest (start,stop)"),
 	OPT_BOOLEAN(0, "inline", &symbol_conf.inline_name,
 		    "Show inline function"),
-	OPT_CALLBACK(0, "percent-type", &report.annotation_opts, "local-period",
+	OPT_CALLBACK(0, "percent-type", &annotate_opts, "local-period",
 		     "Set percent type local/global-period/hits",
 		     annotate_parse_percent_type),
 	OPT_BOOLEAN(0, "ns", &symbol_conf.nanosecs, "Show times in nanosecs"),

@@ -1433,7 +1432,7 @@ int cmd_report(int argc, const char **argv)
 	 */
 	symbol_conf.keep_exited_threads = true;
 
-	annotation_options__init(&report.annotation_opts);
+	annotation_options__init(&annotate_opts);
 
 	ret = perf_config(report__config, &report);
 	if (ret)

@@ -1452,13 +1451,13 @@ int cmd_report(int argc, const char **argv)
 	}
 
 	if (disassembler_style) {
-		report.annotation_opts.disassembler_style = strdup(disassembler_style);
-		if (!report.annotation_opts.disassembler_style)
+		annotate_opts.disassembler_style = strdup(disassembler_style);
+		if (!annotate_opts.disassembler_style)
 			return -ENOMEM;
 	}
 	if (objdump_path) {
-		report.annotation_opts.objdump_path = strdup(objdump_path);
-		if (!report.annotation_opts.objdump_path)
+		annotate_opts.objdump_path = strdup(objdump_path);
+		if (!annotate_opts.objdump_path)
 			return -ENOMEM;
 	}
 	if (addr2line_path) {

@@ -1467,7 +1466,7 @@ int cmd_report(int argc, const char **argv)
 			return -ENOMEM;
 	}
 
-	if (annotate_check_args(&report.annotation_opts) < 0) {
+	if (annotate_check_args(&annotate_opts) < 0) {
 		ret = -EINVAL;
 		goto exit;
 	}

@@ -1699,7 +1698,7 @@ int cmd_report(int argc, const char **argv)
 			 */
 			symbol_conf.priv_size += sizeof(u32);
 		}
-		annotation_config__init(&report.annotation_opts);
+		annotation_config__init(&annotate_opts);
 	}
 
 	if (symbol__init(&session->header.env) < 0)

@@ -1753,7 +1752,7 @@ int cmd_report(int argc, const char **argv)
 	zstd_fini(&(session->zstd_data));
 	perf_session__delete(session);
 exit:
-	annotation_options__exit(&report.annotation_opts);
+	annotation_options__exit(&annotate_opts);
 	free(sort_order_help);
 	free(field_order_help);
 	return ret;

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #3 lore raw

PATCH 3/8 perf top: Convert to the global annotation_options

In reply to Namhyung Kim

Use the global option and drop the local copy.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-top.c | 44 ++++++++++++++++++++--------------------
 tools/perf/util/top.h    |  1 -
 2 files changed, 22 insertions(+), 23 deletions(-)

tools/perf/builtin-top.c +22 ~~-22~~

index ea8c7eca5eee..52930b71f660 100644
@@ -147,7 +147,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 		return err;
 	}
 
-	err = symbol__annotate(&he->ms, evsel, &top->annotation_opts, NULL);
+	err = symbol__annotate(&he->ms, evsel, &annotate_opts, NULL);
 	if (err == 0) {
 		top->sym_filter_entry = he;
 	} else {

@@ -261,9 +261,9 @@ static void perf_top__show_details(struct perf_top *top)
 		goto out_unlock;
 
 	printf("Showing %s for %s\n", evsel__name(top->sym_evsel), symbol->name);
-	printf("  Events  Pcnt (>=%d%%)\n", top->annotation_opts.min_pcnt);
+	printf("  Events  Pcnt (>=%d%%)\n", annotate_opts.min_pcnt);
 
-	more = symbol__annotate_printf(&he->ms, top->sym_evsel, &top->annotation_opts);
+	more = symbol__annotate_printf(&he->ms, top->sym_evsel, &annotate_opts);
 
 	if (top->evlist->enabled) {
 		if (top->zero)

@@ -450,7 +450,7 @@ static void perf_top__print_mapped_keys(struct perf_top *top)
 
 	fprintf(stdout, "\t[f]     profile display filter (count).    \t(%d)\n", top->count_filter);
 
-	fprintf(stdout, "\t[F]     annotate display filter (percent). \t(%d%%)\n", top->annotation_opts.min_pcnt);
+	fprintf(stdout, "\t[F]     annotate display filter (percent). \t(%d%%)\n", annotate_opts.min_pcnt);
 	fprintf(stdout, "\t[s]     annotate symbol.                   \t(%s)\n", name?: "NULL");
 	fprintf(stdout, "\t[S]     stop annotation.\n");

@@ -553,7 +553,7 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
 			prompt_integer(&top->count_filter, "Enter display event count filter");
 			break;
 		case 'F':
-			prompt_percent(&top->annotation_opts.min_pcnt,
+			prompt_percent(&annotate_opts.min_pcnt,
 				       "Enter details display event filter (percent)");
 			break;
 		case 'K':

@@ -647,7 +647,7 @@ static void *display_thread_tui(void *arg)
 
 	ret = evlist__tui_browse_hists(top->evlist, help, &hbt, top->min_percent,
 				       &top->session->header.env, !top->record_opts.overwrite,
-				       &top->annotation_opts);
+				       &annotate_opts);
 	if (ret == K_RELOAD) {
 		top->zero = true;
 		goto repeat;

@@ -1241,9 +1241,9 @@ static int __cmd_top(struct perf_top *top)
 	pthread_t thread, thread_process;
 	int ret;
 
-	if (!top->annotation_opts.objdump_path) {
+	if (!annotate_opts.objdump_path) {
 		ret = perf_env__lookup_objdump(&top->session->header.env,
-					       &top->annotation_opts.objdump_path);
+					       &annotate_opts.objdump_path);
 		if (ret)
 			return ret;
 	}

@@ -1536,9 +1536,9 @@ int cmd_top(int argc, const char **argv)
 		   "only consider symbols in these comms"),
 	OPT_STRING(0, "symbols", &symbol_conf.sym_list_str, "symbol[,symbol...]",
 		   "only consider these symbols"),
-	OPT_BOOLEAN(0, "source", &top.annotation_opts.annotate_src,
+	OPT_BOOLEAN(0, "source", &annotate_opts.annotate_src,
 		    "Interleave source code with assembly code (default)"),
-	OPT_BOOLEAN(0, "asm-raw", &top.annotation_opts.show_asm_raw,
+	OPT_BOOLEAN(0, "asm-raw", &annotate_opts.show_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
 	OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
 		    "Enable kernel symbol demangling"),

@@ -1549,9 +1549,9 @@ int cmd_top(int argc, const char **argv)
 		   "addr2line binary to use for line numbers"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
-	OPT_STRING(0, "prefix", &top.annotation_opts.prefix, "prefix",
+	OPT_STRING(0, "prefix", &annotate_opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
-	OPT_STRING(0, "prefix-strip", &top.annotation_opts.prefix_strip, "N",
+	OPT_STRING(0, "prefix-strip", &annotate_opts.prefix_strip, "N",
 		    "Strip first N entries of source file path name in programs (with --prefix)"),
 	OPT_STRING('u', "uid", &target->uid_str, "user", "user to profile"),
 	OPT_CALLBACK(0, "percent-limit", &top, "percent",

@@ -1609,10 +1609,10 @@ int cmd_top(int argc, const char **argv)
 	if (status < 0)
 		return status;
 
-	annotation_options__init(&top.annotation_opts);
+	annotation_options__init(&annotate_opts);
 
-	top.annotation_opts.min_pcnt = 5;
-	top.annotation_opts.context  = 4;
+	annotate_opts.min_pcnt = 5;
+	annotate_opts.context  = 4;
 
 	top.evlist = evlist__new();
 	if (top.evlist == NULL)

@@ -1642,13 +1642,13 @@ int cmd_top(int argc, const char **argv)
 		usage_with_options(top_usage, options);
 
 	if (disassembler_style) {
-		top.annotation_opts.disassembler_style = strdup(disassembler_style);
-		if (!top.annotation_opts.disassembler_style)
+		annotate_opts.disassembler_style = strdup(disassembler_style);
+		if (!annotate_opts.disassembler_style)
 			return -ENOMEM;
 	}
 	if (objdump_path) {
-		top.annotation_opts.objdump_path = strdup(objdump_path);
-		if (!top.annotation_opts.objdump_path)
+		annotate_opts.objdump_path = strdup(objdump_path);
+		if (!annotate_opts.objdump_path)
 			return -ENOMEM;
 	}
 	if (addr2line_path) {

@@ -1661,7 +1661,7 @@ int cmd_top(int argc, const char **argv)
 	if (status)
 		goto out_delete_evlist;
 
-	if (annotate_check_args(&top.annotation_opts) < 0)
+	if (annotate_check_args(&annotate_opts) < 0)
 		goto out_delete_evlist;
 
 	if (!top.evlist->core.nr_entries) {

@@ -1787,7 +1787,7 @@ int cmd_top(int argc, const char **argv)
 	if (status < 0)
 		goto out_delete_evlist;
 
-	annotation_config__init(&top.annotation_opts);
+	annotation_config__init(&annotate_opts);
 
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
 	status = symbol__init(NULL);

@@ -1840,7 +1840,7 @@ int cmd_top(int argc, const char **argv)
 out_delete_evlist:
 	evlist__delete(top.evlist);
 	perf_session__delete(top.session);
-	annotation_options__exit(&top.annotation_opts);
+	annotation_options__exit(&annotate_opts);
 
 	return status;
 }

tools/perf/util/top.h +0 -1

index a8b0d79bd96c..4c5588dbb131 100644
@@ -21,7 +21,6 @@ struct perf_top {
 	struct perf_tool   tool;
 	struct evlist *evlist, *sb_evlist;
 	struct record_opts record_opts;
-	struct annotation_options annotation_opts;
 	struct evswitch	   evswitch;
 	/*
 	 * Symbols will be added here in perf_event__process_sample and will

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #4 lore raw

PATCH 4/8 perf annotate: Use global annotation_options

In reply to Namhyung Kim

Now it can directly use the global options and no need to pass it as an argument.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-annotate.c     |   4 +-
 tools/perf/builtin-report.c       |   2 +-
 tools/perf/builtin-top.c          |   4 +-
 tools/perf/ui/browsers/annotate.c |   6 +-
 tools/perf/util/annotate.c        | 118 ++++++++++++++----------------
 tools/perf/util/annotate.h        |  15 ++--
 6 files changed, 68 insertions(+), 81 deletions(-)

tools/perf/builtin-annotate.c +2 -2

index 67b36a7a12e3..a53a4e711899 100644
@@ -317,9 +317,9 @@ static int hist_entry__tty_annotate(struct hist_entry *he,
 				    struct perf_annotate *ann)
 {
 	if (!ann->use_stdio2)
-		return symbol__tty_annotate(&he->ms, evsel, &annotate_opts);
+		return symbol__tty_annotate(&he->ms, evsel);
 
-	return symbol__tty_annotate2(&he->ms, evsel, &annotate_opts);
+	return symbol__tty_annotate2(&he->ms, evsel);
 }
 
 static void hists__find_annotations(struct hists *hists,

tools/perf/builtin-report.c +1 -1

index 90f98953587c..2b86651615cd 100644
@@ -744,7 +744,7 @@ static int hists__resort_cb(struct hist_entry *he, void *arg)
 	if (rep->symbol_ipc && sym && !sym->annotate2) {
 		struct evsel *evsel = hists_to_evsel(he->hists);
 
-		symbol__annotate2(&he->ms, evsel, &annotate_opts, NULL);
+		symbol__annotate2(&he->ms, evsel, NULL);
 	}
 
 	return 0;

tools/perf/builtin-top.c +2 -2

index 52930b71f660..80d126279208 100644
@@ -147,7 +147,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 		return err;
 	}
 
-	err = symbol__annotate(&he->ms, evsel, &annotate_opts, NULL);
+	err = symbol__annotate(&he->ms, evsel, NULL);
 	if (err == 0) {
 		top->sym_filter_entry = he;
 	} else {

@@ -263,7 +263,7 @@ static void perf_top__show_details(struct perf_top *top)
 	printf("Showing %s for %s\n", evsel__name(top->sym_evsel), symbol->name);
 	printf("  Events  Pcnt (>=%d%%)\n", annotate_opts.min_pcnt);
 
-	more = symbol__annotate_printf(&he->ms, top->sym_evsel, &annotate_opts);
+	more = symbol__annotate_printf(&he->ms, top->sym_evsel);
 
 	if (top->evlist->enabled) {
 		if (top->zero)

tools/perf/ui/browsers/annotate.c +3 -3

index 163f916fff68..ed0e692afdbe 100644
@@ -114,7 +114,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 	if (!browser->navkeypressed)
 		ops.width += 1;
 
-	annotation_line__write(al, notes, &ops, ab->opts);
+	annotation_line__write(al, notes, &ops);
 
 	if (ops.current_entry)
 		ab->selection = al;

@@ -884,7 +884,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
 			continue;
 		}
 		case 'P':
-			map_symbol__annotation_dump(ms, evsel, browser->opts);
+			map_symbol__annotation_dump(ms, evsel);
 			continue;
 		case 't':
 			if (symbol_conf.show_total_period) {

@@ -979,7 +979,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
 		return -1;
 
 	if (not_annotated) {
-		err = symbol__annotate2(ms, evsel, opts, &browser.arch);
+		err = symbol__annotate2(ms, evsel, &browser.arch);
 		if (err) {
 			char msg[BUFSIZ];
 			dso->annotate_warned = true;

tools/perf/util/annotate.c +55 ~~-63~~

370 lines not on this page · raw message

tools/perf/util/annotate.h +5 ~~-10~~

index 8c1a070725fa..7bf29baa43f5 100644
@@ -224,8 +224,7 @@ struct annotation_write_ops {
 };
 
 void annotation_line__write(struct annotation_line *al, struct annotation *notes,
-			    struct annotation_write_ops *ops,
-			    struct annotation_options *opts);
+			    struct annotation_write_ops *ops);
 
 int __annotation__scnprintf_samples_period(struct annotation *notes,
 					   char *bf, size_t size,

@@ -375,11 +374,9 @@ void symbol__annotate_zero_histograms(struct symbol *sym);
 
 int symbol__annotate(struct map_symbol *ms,
 		     struct evsel *evsel,
-		     struct annotation_options *options,
 		     struct arch **parch);
 int symbol__annotate2(struct map_symbol *ms,
 		      struct evsel *evsel,
-		      struct annotation_options *options,
 		      struct arch **parch);
 
 enum symbol_disassemble_errno {

@@ -406,20 +403,18 @@ enum symbol_disassemble_errno {
 
 int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, size_t buflen);
 
-int symbol__annotate_printf(struct map_symbol *ms, struct evsel *evsel,
-			    struct annotation_options *options);
+int symbol__annotate_printf(struct map_symbol *ms, struct evsel *evsel);
 void symbol__annotate_zero_histogram(struct symbol *sym, int evidx);
 void symbol__annotate_decay_histogram(struct symbol *sym, int evidx);
 void annotated_source__purge(struct annotated_source *as);
 
-int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
-				struct annotation_options *opts);
+int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel);
 
 bool ui__has_annotation(void);
 
-int symbol__tty_annotate(struct map_symbol *ms, struct evsel *evsel, struct annotation_options *opts);
+int symbol__tty_annotate(struct map_symbol *ms, struct evsel *evsel);
 
-int symbol__tty_annotate2(struct map_symbol *ms, struct evsel *evsel, struct annotation_options *opts);
+int symbol__tty_annotate2(struct map_symbol *ms, struct evsel *evsel);
 
 #ifdef HAVE_SLANG_SUPPORT
 int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Arnaldo Carvalho de Melo

maintainer

2023-12-07 20:17 UTC #5 lore raw

Re: [PATCH 4/8] perf annotate: Use global annotation_options

Em Tue, Nov 28, 2023 at 09:54:37AM -0800, Namhyung Kim escreveu:

Quoted 2 lines

Now it can directly use the global options and no need to pass it as an argument.

At this point the build breaks when using GTK2=1 on the make command line, as done in 'make -C tools/perf build-test', so I had to add the following patch on top of this 4/8 patch:

tools/perf/builtin-annotate.c +1 -2

index a53a4e711899f20d..9c1e2b2b5bc0b730 100644
@@ -365,7 +365,6 @@ static void hists__find_annotations(struct hists *hists,
 			int ret;
 			int (*annotate)(struct hist_entry *he,
 					struct evsel *evsel,
-					struct annotation_options *options,
 					struct hist_browser_timer *hbt);
 
 			annotate = dlsym(perf_gtk_handle,

@@ -375,7 +374,7 @@ static void hists__find_annotations(struct hists *hists,
 				return;
 			}
 
-			ret = annotate(he, evsel, &annotate_opts, NULL);
+			ret = annotate(he, evsel, NULL);
 			if (!ret || !ann->skip_missing)
 				return;

tools/perf/ui/gtk/annotate.c +2 -4

index 2effac77ca8c6742..394861245fd3e48f 100644
@@ -162,7 +162,6 @@ static int perf_gtk__annotate_symbol(GtkWidget *window, struct map_symbol *ms,
 }
 
 static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
-				struct annotation_options *options,
 				struct hist_browser_timer *hbt)
 {
 	struct dso *dso = map__dso(ms->map);

@@ -176,7 +175,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 	if (dso->annotate_warned)
 		return -1;
 
-	err = symbol__annotate(ms, evsel, options, NULL);
+	err = symbol__annotate(ms, evsel, NULL);
 	if (err) {
 		char msg[BUFSIZ];
 		dso->annotate_warned = true;

@@ -244,10 +243,9 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 
 int hist_entry__gtk_annotate(struct hist_entry *he,
 			     struct evsel *evsel,
-			     struct annotation_options *options,
 			     struct hist_browser_timer *hbt)
 {
-	return symbol__gtk_annotate(&he->ms, evsel, options, hbt);
+	return symbol__gtk_annotate(&he->ms, evsel, hbt);
 }
 
 void perf_gtk__show_annotations(void)

tools/perf/ui/gtk/gtk.h +0 -2

index 1e84dceb52671385..a2b497f03fd6e478 100644
@@ -56,13 +56,11 @@ struct evsel;
 struct evlist;
 struct hist_entry;
 struct hist_browser_timer;
-struct annotation_options;
 
 int evlist__gtk_browse_hists(struct evlist *evlist, const char *help,
 			     struct hist_browser_timer *hbt, float min_pcnt);
 int hist_entry__gtk_annotate(struct hist_entry *he,
 			     struct evsel *evsel,
-			     struct annotation_options *options,
 			     struct hist_browser_timer *hbt);
 void perf_gtk__show_annotations(void);

Namhyung Kim

author maintainer

2023-12-07 20:34 UTC #6 lore raw

Re: [PATCH 4/8] perf annotate: Use global annotation_options

Quoted 8 lines · Arnaldo Carvalho de Melo

Em Tue, Nov 28, 2023 at 09:54:37AM -0800, Namhyung Kim escreveu:
Quoted 2 lines
Now it can directly use the global options and no need to pass it as an argument.
At this point the build breaks when using GTK2=1 on the make command line, as done in 'make -C tools/perf build-test', so I had to add the following patch on top of this 4/8 patch:

Oops, I forgot to test with it. Thanks for fixing this!

Thanks,
Namhyung

Quoted 74 lines

tools/perf/builtin-annotate.c +1 -2

index a53a4e711899f20d..9c1e2b2b5bc0b730 100644
@@ -365,7 +365,6 @@ static void hists__find_annotations(struct hists *hists,
                        int ret;
                        int (*annotate)(struct hist_entry *he,
                                        struct evsel *evsel,
-                                       struct annotation_options *options,
                                        struct hist_browser_timer *hbt);
 
                        annotate = dlsym(perf_gtk_handle,

@@ -375,7 +374,7 @@ static void hists__find_annotations(struct hists *hists,
                                return;
                        }
 
-                       ret = annotate(he, evsel, &annotate_opts, NULL);
+                       ret = annotate(he, evsel, NULL);
                        if (!ret || !ann->skip_missing)
                                return;

tools/perf/ui/gtk/annotate.c +2 -4

index 2effac77ca8c6742..394861245fd3e48f 100644
@@ -162,7 +162,6 @@ static int perf_gtk__annotate_symbol(GtkWidget *window, struct map_symbol *ms,
 }
 
 static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
-                               struct annotation_options *options,
                                struct hist_browser_timer *hbt)
 {
        struct dso *dso = map__dso(ms->map);

@@ -176,7 +175,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
        if (dso->annotate_warned)
                return -1;
 
-       err = symbol__annotate(ms, evsel, options, NULL);
+       err = symbol__annotate(ms, evsel, NULL);
        if (err) {
                char msg[BUFSIZ];
                dso->annotate_warned = true;

@@ -244,10 +243,9 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 
 int hist_entry__gtk_annotate(struct hist_entry *he,
                             struct evsel *evsel,
-                            struct annotation_options *options,
                             struct hist_browser_timer *hbt)
 {
-       return symbol__gtk_annotate(&he->ms, evsel, options, hbt);
+       return symbol__gtk_annotate(&he->ms, evsel, hbt);
 }
 
 void perf_gtk__show_annotations(void)

tools/perf/ui/gtk/gtk.h +0 -2

index 1e84dceb52671385..a2b497f03fd6e478 100644
@@ -56,13 +56,11 @@ struct evsel;
 struct evlist;
 struct hist_entry;
 struct hist_browser_timer;
-struct annotation_options;
 
 int evlist__gtk_browse_hists(struct evlist *evlist, const char *help,
                             struct hist_browser_timer *hbt, float min_pcnt);
 int hist_entry__gtk_annotate(struct hist_entry *he,
                             struct evsel *evsel,
-                            struct annotation_options *options,
                             struct hist_browser_timer *hbt);
 void perf_gtk__show_annotations(void);

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #7 lore raw

PATCH 5/8 perf ui/browser/annotate: Use global annotation_options

In reply to Namhyung Kim

Now it can use the global options and no need save local browser options separately.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-annotate.c     |  2 +-
 tools/perf/builtin-report.c       |  8 ++--
 tools/perf/builtin-top.c          |  3 +-
 tools/perf/ui/browsers/annotate.c | 65 ++++++++++++++-----------------
 tools/perf/ui/browsers/hists.c    | 34 ++++++----------
 tools/perf/ui/browsers/hists.h    |  2 -
 tools/perf/util/annotate.h        |  6 +--
 tools/perf/util/block-info.c      |  6 +--
 tools/perf/util/block-info.h      |  3 +-
 tools/perf/util/hist.h            | 25 ++++--------
 10 files changed, 59 insertions(+), 95 deletions(-)

tools/perf/builtin-annotate.c +1 -1

index a53a4e711899..87af95634879 100644
@@ -382,7 +382,7 @@ static void hists__find_annotations(struct hists *hists,
 			/* skip missing symbols */
 			nd = rb_next(nd);
 		} else if (use_browser == 1) {
-			key = hist_entry__tui_annotate(he, evsel, NULL, &annotate_opts);
+			key = hist_entry__tui_annotate(he, evsel, NULL);
 
 			switch (key) {
 			case -1:

tools/perf/builtin-report.c +3 -5

index 2b86651615cd..bc0d986c1e0c 100644
@@ -540,8 +540,7 @@ static int evlist__tui_block_hists_browse(struct evlist *evlist, struct report *
 	evlist__for_each_entry(evlist, pos) {
 		ret = report__browse_block_hists(&rep->block_reports[i++].hist,
 						 rep->min_percent, pos,
-						 &rep->session->header.env,
-						 &annotate_opts);
+						 &rep->session->header.env);
 		if (ret != 0)
 			return ret;
 	}

@@ -573,8 +572,7 @@ static int evlist__tty_browse_hists(struct evlist *evlist, struct report *rep, c
 
 		if (rep->total_cycles_mode) {
 			report__browse_block_hists(&rep->block_reports[i++].hist,
-						   rep->min_percent, pos,
-						   NULL, NULL);
+						   rep->min_percent, pos, NULL);
 			continue;
 		}

@@ -669,7 +667,7 @@ static int report__browse_hists(struct report *rep)
 		}
 
 		ret = evlist__tui_browse_hists(evlist, help, NULL, rep->min_percent,
-					       &session->header.env, true, &annotate_opts);
+					       &session->header.env, true);
 		/*
 		 * Usually "ret" is the last pressed key, and we only
 		 * care if the key notifies us to switch data file.

tools/perf/builtin-top.c +1 -2

index 80d126279208..60399e4233ee 100644
@@ -646,8 +646,7 @@ static void *display_thread_tui(void *arg)
 	}
 
 	ret = evlist__tui_browse_hists(top->evlist, help, &hbt, top->min_percent,
-				       &top->session->header.env, !top->record_opts.overwrite,
-				       &annotate_opts);
+				       &top->session->header.env, !top->record_opts.overwrite);
 	if (ret == K_RELOAD) {
 		top->zero = true;
 		goto repeat;

tools/perf/ui/browsers/annotate.c +29 ~~-36~~

210 lines not on this page · raw message

tools/perf/ui/browsers/hists.c +12 ~~-22~~

index f4812b226818..3061dea29e6b 100644
@@ -2250,8 +2250,7 @@ struct hist_browser *hist_browser__new(struct hists *hists)
 static struct hist_browser *
 perf_evsel_browser__new(struct evsel *evsel,
 			struct hist_browser_timer *hbt,
-			struct perf_env *env,
-			struct annotation_options *annotation_opts)
+			struct perf_env *env)
 {
 	struct hist_browser *browser = hist_browser__new(evsel__hists(evsel));

@@ -2259,7 +2258,6 @@ perf_evsel_browser__new(struct evsel *evsel,
 		browser->hbt   = hbt;
 		browser->env   = env;
 		browser->title = hists_browser__scnprintf_title;
-		browser->annotation_opts = annotation_opts;
 	}
 	return browser;
 }

@@ -2432,8 +2430,8 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
 	struct hist_entry *he;
 	int err;
 
-	if (!browser->annotation_opts->objdump_path &&
-	    perf_env__lookup_objdump(browser->env, &browser->annotation_opts->objdump_path))
+	if (!annotate_opts.objdump_path &&
+	    perf_env__lookup_objdump(browser->env, &annotate_opts.objdump_path))
 		return 0;
 
 	notes = symbol__annotation(act->ms.sym);

@@ -2445,8 +2443,7 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
 	else
 		evsel = hists_to_evsel(browser->hists);
 
-	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt,
-				       browser->annotation_opts);
+	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
 	he = hist_browser__selected_entry(browser);
 	/*
 	 * offer option to annotate the other branch source or target

@@ -2943,11 +2940,10 @@ static void hist_browser__update_percent_limit(struct hist_browser *hb,
 
 static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *helpline,
 			       bool left_exits, struct hist_browser_timer *hbt, float min_pcnt,
-			       struct perf_env *env, bool warn_lost_event,
-			       struct annotation_options *annotation_opts)
+			       struct perf_env *env, bool warn_lost_event)
 {
 	struct hists *hists = evsel__hists(evsel);
-	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env, annotation_opts);
+	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
 	struct branch_info *bi = NULL;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];

@@ -3398,7 +3394,6 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 struct evsel_menu {
 	struct ui_browser b;
 	struct evsel *selection;
-	struct annotation_options *annotation_opts;
 	bool lost_events, lost_events_warned;
 	float min_pcnt;
 	struct perf_env *env;

@@ -3499,8 +3494,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
 				hbt->timer(hbt->arg);
 			key = evsel__hists_browse(pos, nr_events, help, true, hbt,
 						  menu->min_pcnt, menu->env,
-						  warn_lost_event,
-						  menu->annotation_opts);
+						  warn_lost_event);
 			ui_browser__show_title(&menu->b, title);
 			switch (key) {
 			case K_TAB:

@@ -3557,7 +3551,7 @@ static bool filter_group_entries(struct ui_browser *browser __maybe_unused,
 
 static int __evlist__tui_browse_hists(struct evlist *evlist, int nr_entries, const char *help,
 				      struct hist_browser_timer *hbt, float min_pcnt, struct perf_env *env,
-				      bool warn_lost_event, struct annotation_options *annotation_opts)
+				      bool warn_lost_event)
 {
 	struct evsel *pos;
 	struct evsel_menu menu = {

@@ -3572,7 +3566,6 @@ static int __evlist__tui_browse_hists(struct evlist *evlist, int nr_entries, con
 		},
 		.min_pcnt = min_pcnt,
 		.env = env,
-		.annotation_opts = annotation_opts,
 	};
 
 	ui_helpline__push("Press ESC to exit");

@@ -3607,8 +3600,7 @@ static bool evlist__single_entry(struct evlist *evlist)
 }
 
 int evlist__tui_browse_hists(struct evlist *evlist, const char *help, struct hist_browser_timer *hbt,
-			     float min_pcnt, struct perf_env *env, bool warn_lost_event,
-			     struct annotation_options *annotation_opts)
+			     float min_pcnt, struct perf_env *env, bool warn_lost_event)
 {
 	int nr_entries = evlist->core.nr_entries;

@@ -3617,7 +3609,7 @@ single_entry: {
 		struct evsel *first = evlist__first(evlist);
 
 		return evsel__hists_browse(first, nr_entries, help, false, hbt, min_pcnt,
-					   env, warn_lost_event, annotation_opts);
+					   env, warn_lost_event);
 	}
 	}

@@ -3635,7 +3627,7 @@ single_entry: {
 	}
 
 	return __evlist__tui_browse_hists(evlist, nr_entries, help, hbt, min_pcnt, env,
-					  warn_lost_event, annotation_opts);
+					  warn_lost_event);
 }
 
 static int block_hists_browser__title(struct hist_browser *browser, char *bf,

@@ -3654,8 +3646,7 @@ static int block_hists_browser__title(struct hist_browser *browser, char *bf,
 }
 
 int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
-			   float min_percent, struct perf_env *env,
-			   struct annotation_options *annotation_opts)
+			   float min_percent, struct perf_env *env)
 {
 	struct hists *hists = &bh->block_hists;
 	struct hist_browser *browser;

@@ -3672,7 +3663,6 @@ int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
 	browser->title = block_hists_browser__title;
 	browser->min_pcnt = min_percent;
 	browser->env = env;
-	browser->annotation_opts = annotation_opts;
 
 	/* reset abort key so that it can get Ctrl-C as a key */
 	SLang_reset_tty();

tools/perf/ui/browsers/hists.h +0 -2

index 1e938d9ffa5e..de46f6c56b0e 100644
@@ -4,7 +4,6 @@
 
 #include "ui/browser.h"
 
-struct annotation_options;
 struct evsel;
 
 struct hist_browser {

@@ -15,7 +14,6 @@ struct hist_browser {
 	struct hist_browser_timer *hbt;
 	struct pstack	    *pstack;
 	struct perf_env	    *env;
-	struct annotation_options *annotation_opts;
 	struct evsel	    *block_evsel;
 	int		     print_seq;
 	bool		     show_dso;

tools/perf/util/annotate.h +2 -4

index 7bf29baa43f5..857c5fa0e6b1 100644
@@ -418,13 +418,11 @@ int symbol__tty_annotate2(struct map_symbol *ms, struct evsel *evsel);
 
 #ifdef HAVE_SLANG_SUPPORT
 int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
-			 struct hist_browser_timer *hbt,
-			 struct annotation_options *opts);
+			 struct hist_browser_timer *hbt);
 #else
 static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
 				struct evsel *evsel  __maybe_unused,
-				struct hist_browser_timer *hbt __maybe_unused,
-				struct annotation_options *opts __maybe_unused)
+				struct hist_browser_timer *hbt __maybe_unused)
 {
 	return 0;
 }

tools/perf/util/block-info.c +2 -4

index 08f82c1f166c..dec910989701 100644
@@ -464,8 +464,7 @@ void block_info__free_report(struct block_report *reps, int nr_reps)
 }
 
 int report__browse_block_hists(struct block_hist *bh, float min_percent,
-			       struct evsel *evsel, struct perf_env *env,
-			       struct annotation_options *annotation_opts)
+			       struct evsel *evsel, struct perf_env *env)
 {
 	int ret;

@@ -477,8 +476,7 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
 		return 0;
 	case 1:
 		symbol_conf.report_individual_block = true;
-		ret = block_hists_tui_browse(bh, evsel, min_percent,
-					     env, annotation_opts);
+		ret = block_hists_tui_browse(bh, evsel, min_percent, env);
 		return ret;
 	default:
 		return -1;

tools/perf/util/block-info.h +1 -2

index 42e9dcc4cf0a..96f53e89795e 100644
@@ -78,8 +78,7 @@ struct block_report *block_info__create_report(struct evlist *evlist,
 void block_info__free_report(struct block_report *reps, int nr_reps);
 
 int report__browse_block_hists(struct block_hist *bh, float min_percent,
-			       struct evsel *evsel, struct perf_env *env,
-			       struct annotation_options *annotation_opts);
+			       struct evsel *evsel, struct perf_env *env);
 
 float block_info__total_cycles_percent(struct hist_entry *he);

tools/perf/util/hist.h +8 ~~-17~~

index afc9f1c7f4dc..5d0db96609df 100644
@@ -457,7 +457,6 @@ struct hist_browser_timer {
 	int refresh;
 };
 
-struct annotation_options;
 struct res_sample;
 
 enum rstype {

@@ -473,16 +472,13 @@ struct block_hist;
 void attr_to_script(char *buf, struct perf_event_attr *attr);
 
 int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
-			     struct hist_browser_timer *hbt,
-			     struct annotation_options *annotation_opts);
+			     struct hist_browser_timer *hbt);
 
 int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
-			     struct hist_browser_timer *hbt,
-			     struct annotation_options *annotation_opts);
+			     struct hist_browser_timer *hbt);
 
 int evlist__tui_browse_hists(struct evlist *evlist, const char *help, struct hist_browser_timer *hbt,
-			     float min_pcnt, struct perf_env *env, bool warn_lost_event,
-			     struct annotation_options *annotation_options);
+			     float min_pcnt, struct perf_env *env, bool warn_lost_event);
 
 int script_browse(const char *script_opt, struct evsel *evsel);

@@ -492,8 +488,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
 void res_sample_init(void);
 
 int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
-			   float min_percent, struct perf_env *env,
-			   struct annotation_options *annotation_opts);
+			   float min_percent, struct perf_env *env);
 #else
 static inline
 int evlist__tui_browse_hists(struct evlist *evlist __maybe_unused,

@@ -501,23 +496,20 @@ int evlist__tui_browse_hists(struct evlist *evlist __maybe_unused,
 			     struct hist_browser_timer *hbt __maybe_unused,
 			     float min_pcnt __maybe_unused,
 			     struct perf_env *env __maybe_unused,
-			     bool warn_lost_event __maybe_unused,
-			     struct annotation_options *annotation_options __maybe_unused)
+			     bool warn_lost_event __maybe_unused)
 {
 	return 0;
 }
 static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
 					   struct evsel *evsel __maybe_unused,
-					   struct hist_browser_timer *hbt __maybe_unused,
-					   struct annotation_options *annotation_options __maybe_unused)
+					   struct hist_browser_timer *hbt __maybe_unused)
 {
 	return 0;
 }
 
 static inline int hist_entry__tui_annotate(struct hist_entry *he __maybe_unused,
 					   struct evsel *evsel __maybe_unused,
-					   struct hist_browser_timer *hbt __maybe_unused,
-					   struct annotation_options *annotation_opts __maybe_unused)
+					   struct hist_browser_timer *hbt __maybe_unused)
 {
 	return 0;
 }

@@ -541,8 +533,7 @@ static inline void res_sample_init(void) {}
 static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
 					 struct evsel *evsel __maybe_unused,
 					 float min_percent __maybe_unused,
-					 struct perf_env *env __maybe_unused,
-					 struct annotation_options *annotation_opts __maybe_unused)
+					 struct perf_env *env __maybe_unused)
 {
 	return 0;
 }

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #8 lore raw

PATCH 6/8 perf annotate: Ensure init/exit for global options

In reply to Namhyung Kim

Now it only cares about the global options so it can just handle it without the argument.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-annotate.c |  8 ++++----
 tools/perf/builtin-report.c   |  8 ++++----
 tools/perf/builtin-top.c      |  8 ++++----
 tools/perf/util/annotate.c    | 19 +++++++++++--------
 tools/perf/util/annotate.h    |  8 ++++----
 5 files changed, 27 insertions(+), 24 deletions(-)

tools/perf/builtin-annotate.c +4 -4

index 87af95634879..9b3dd456a1ee 100644
@@ -616,13 +616,13 @@ int cmd_annotate(int argc, const char **argv)
 	set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
 	set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
 
-	annotation_options__init(&annotate_opts);
+	annotation_options__init();
 
 	ret = hists__init();
 	if (ret < 0)
 		return ret;
 
-	annotation_config__init(&annotate_opts);
+	annotation_config__init();
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
 	if (argc) {

@@ -652,7 +652,7 @@ int cmd_annotate(int argc, const char **argv)
 			return -ENOMEM;
 	}
 
-	if (annotate_check_args(&annotate_opts) < 0)
+	if (annotate_check_args() < 0)
 		return -EINVAL;
 
 #ifdef HAVE_GTK2_SUPPORT

@@ -733,7 +733,7 @@ int cmd_annotate(int argc, const char **argv)
 #ifndef NDEBUG
 	perf_session__delete(annotate.session);
 #endif
-	annotation_options__exit(&annotate_opts);
+	annotation_options__exit();
 
 	return ret;
 }

tools/perf/builtin-report.c +4 -4

index bc0d986c1e0c..17fb171e898b 100644
@@ -1430,7 +1430,7 @@ int cmd_report(int argc, const char **argv)
 	 */
 	symbol_conf.keep_exited_threads = true;
 
-	annotation_options__init(&annotate_opts);
+	annotation_options__init();
 
 	ret = perf_config(report__config, &report);
 	if (ret)

@@ -1464,7 +1464,7 @@ int cmd_report(int argc, const char **argv)
 			return -ENOMEM;
 	}
 
-	if (annotate_check_args(&annotate_opts) < 0) {
+	if (annotate_check_args() < 0) {
 		ret = -EINVAL;
 		goto exit;
 	}

@@ -1696,7 +1696,7 @@ int cmd_report(int argc, const char **argv)
 			 */
 			symbol_conf.priv_size += sizeof(u32);
 		}
-		annotation_config__init(&annotate_opts);
+		annotation_config__init();
 	}
 
 	if (symbol__init(&session->header.env) < 0)

@@ -1750,7 +1750,7 @@ int cmd_report(int argc, const char **argv)
 	zstd_fini(&(session->zstd_data));
 	perf_session__delete(session);
 exit:
-	annotation_options__exit(&annotate_opts);
+	annotation_options__exit();
 	free(sort_order_help);
 	free(field_order_help);
 	return ret;

tools/perf/builtin-top.c +4 -4

index 60399e4233ee..0de963ca3196 100644
@@ -1608,7 +1608,7 @@ int cmd_top(int argc, const char **argv)
 	if (status < 0)
 		return status;
 
-	annotation_options__init(&annotate_opts);
+	annotation_options__init();
 
 	annotate_opts.min_pcnt = 5;
 	annotate_opts.context  = 4;

@@ -1660,7 +1660,7 @@ int cmd_top(int argc, const char **argv)
 	if (status)
 		goto out_delete_evlist;
 
-	if (annotate_check_args(&annotate_opts) < 0)
+	if (annotate_check_args() < 0)
 		goto out_delete_evlist;
 
 	if (!top.evlist->core.nr_entries) {

@@ -1786,7 +1786,7 @@ int cmd_top(int argc, const char **argv)
 	if (status < 0)
 		goto out_delete_evlist;
 
-	annotation_config__init(&annotate_opts);
+	annotation_config__init();
 
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
 	status = symbol__init(NULL);

@@ -1839,7 +1839,7 @@ int cmd_top(int argc, const char **argv)
 out_delete_evlist:
 	evlist__delete(top.evlist);
 	perf_session__delete(top.session);
-	annotation_options__exit(&annotate_opts);
+	annotation_options__exit();
 
 	return status;
 }

tools/perf/util/annotate.c +11 -8

index daff9af552f4..626ff3baeb85 100644
@@ -3416,8 +3416,10 @@ static int annotation__config(const char *var, const char *value, void *data)
 	return 0;
 }
 
-void annotation_options__init(struct annotation_options *opt)
+void annotation_options__init(void)
 {
+	struct annotation_options *opt = &annotate_opts;
+
 	memset(opt, 0, sizeof(*opt));
 
 	/* Default values. */

@@ -3428,16 +3430,15 @@ void annotation_options__init(struct annotation_options *opt)
 	opt->percent_type = PERCENT_PERIOD_LOCAL;
 }
 
-
-void annotation_options__exit(struct annotation_options *opt)
+void annotation_options__exit(void)
 {
-	zfree(&opt->disassembler_style);
-	zfree(&opt->objdump_path);
+	zfree(&annotate_opts.disassembler_style);
+	zfree(&annotate_opts.objdump_path);
 }
 
-void annotation_config__init(struct annotation_options *opt)
+void annotation_config__init(void)
 {
-	perf_config(annotation__config, opt);
+	perf_config(annotation__config, &annotate_opts);
 }
 
 static unsigned int parse_percent_type(char *str1, char *str2)

@@ -3491,8 +3492,10 @@ int annotate_parse_percent_type(const struct option *opt __maybe_unused, const c
 	return err;
 }
 
-int annotate_check_args(struct annotation_options *args)
+int annotate_check_args(void)
 {
+	struct annotation_options *args = &annotate_opts;
+
 	if (args->prefix_strip && !args->prefix) {
 		pr_err("--prefix-strip requires --prefix\n");
 		return -1;

tools/perf/util/annotate.h +4 -4

index 857c5fa0e6b1..4283eb4522b2 100644
@@ -428,14 +428,14 @@ static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
 }
 #endif
 
-void annotation_options__init(struct annotation_options *opt);
-void annotation_options__exit(struct annotation_options *opt);
+void annotation_options__init(void);
+void annotation_options__exit(void);
 
-void annotation_config__init(struct annotation_options *opt);
+void annotation_config__init(void);
 
 int annotate_parse_percent_type(const struct option *opt, const char *_str,
 				int unset);
 
-int annotate_check_args(struct annotation_options *args);
+int annotate_check_args(void);
 
 #endif	/* __PERF_ANNOTATE_H */

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Ian Rogers

reviewer

2023-11-28 19:21 UTC #9 lore raw

Re: [PATCH 6/8] perf annotate: Ensure init/exit for global options

Quoted 3 lines · Namhyung Kim

Now it only cares about the global options so it can just handle it without the argument.

If annotate_opts were accessed by a function then you could pthread_once the initialization on the first call to get annotate_opts. Removing annotation_options__init/exit would remove some potential for error.

Thanks,
Ian

Quoted 203 lines

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-annotate.c |  8 ++++----
 tools/perf/builtin-report.c   |  8 ++++----
 tools/perf/builtin-top.c      |  8 ++++----
 tools/perf/util/annotate.c    | 19 +++++++++++--------
 tools/perf/util/annotate.h    |  8 ++++----
 5 files changed, 27 insertions(+), 24 deletions(-)

tools/perf/builtin-annotate.c +4 -4

index 87af95634879..9b3dd456a1ee 100644
@@ -616,13 +616,13 @@ int cmd_annotate(int argc, const char **argv)
        set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
        set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
 
-       annotation_options__init(&annotate_opts);
+       annotation_options__init();
 
        ret = hists__init();
        if (ret < 0)
                return ret;
 
-       annotation_config__init(&annotate_opts);
+       annotation_config__init();
 
        argc = parse_options(argc, argv, options, annotate_usage, 0);
        if (argc) {

@@ -652,7 +652,7 @@ int cmd_annotate(int argc, const char **argv)
                        return -ENOMEM;
        }
 
-       if (annotate_check_args(&annotate_opts) < 0)
+       if (annotate_check_args() < 0)
                return -EINVAL;
 
 #ifdef HAVE_GTK2_SUPPORT

@@ -733,7 +733,7 @@ int cmd_annotate(int argc, const char **argv)
 #ifndef NDEBUG
        perf_session__delete(annotate.session);
 #endif
-       annotation_options__exit(&annotate_opts);
+       annotation_options__exit();
 
        return ret;
 }

tools/perf/builtin-report.c +4 -4

index bc0d986c1e0c..17fb171e898b 100644
@@ -1430,7 +1430,7 @@ int cmd_report(int argc, const char **argv)
         */
        symbol_conf.keep_exited_threads = true;
 
-       annotation_options__init(&annotate_opts);
+       annotation_options__init();
 
        ret = perf_config(report__config, &report);
        if (ret)

@@ -1464,7 +1464,7 @@ int cmd_report(int argc, const char **argv)
                        return -ENOMEM;
        }
 
-       if (annotate_check_args(&annotate_opts) < 0) {
+       if (annotate_check_args() < 0) {
                ret = -EINVAL;
                goto exit;
        }

@@ -1696,7 +1696,7 @@ int cmd_report(int argc, const char **argv)
                         */
                        symbol_conf.priv_size += sizeof(u32);
                }
-               annotation_config__init(&annotate_opts);
+               annotation_config__init();
        }
 
        if (symbol__init(&session->header.env) < 0)

@@ -1750,7 +1750,7 @@ int cmd_report(int argc, const char **argv)
        zstd_fini(&(session->zstd_data));
        perf_session__delete(session);
 exit:
-       annotation_options__exit(&annotate_opts);
+       annotation_options__exit();
        free(sort_order_help);
        free(field_order_help);
        return ret;

tools/perf/builtin-top.c +4 -4

index 60399e4233ee..0de963ca3196 100644
@@ -1608,7 +1608,7 @@ int cmd_top(int argc, const char **argv)
        if (status < 0)
                return status;
 
-       annotation_options__init(&annotate_opts);
+       annotation_options__init();
 
        annotate_opts.min_pcnt = 5;
        annotate_opts.context  = 4;

@@ -1660,7 +1660,7 @@ int cmd_top(int argc, const char **argv)
        if (status)
                goto out_delete_evlist;
 
-       if (annotate_check_args(&annotate_opts) < 0)
+       if (annotate_check_args() < 0)
                goto out_delete_evlist;
 
        if (!top.evlist->core.nr_entries) {

@@ -1786,7 +1786,7 @@ int cmd_top(int argc, const char **argv)
        if (status < 0)
                goto out_delete_evlist;
 
-       annotation_config__init(&annotate_opts);
+       annotation_config__init();
 
        symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
        status = symbol__init(NULL);

@@ -1839,7 +1839,7 @@ int cmd_top(int argc, const char **argv)
 out_delete_evlist:
        evlist__delete(top.evlist);
        perf_session__delete(top.session);
-       annotation_options__exit(&annotate_opts);
+       annotation_options__exit();
 
        return status;
 }

tools/perf/util/annotate.c +11 -8

index daff9af552f4..626ff3baeb85 100644
@@ -3416,8 +3416,10 @@ static int annotation__config(const char *var, const char *value, void *data)
        return 0;
 }
 
-void annotation_options__init(struct annotation_options *opt)
+void annotation_options__init(void)
 {
+       struct annotation_options *opt = &annotate_opts;
+
        memset(opt, 0, sizeof(*opt));
 
        /* Default values. */

@@ -3428,16 +3430,15 @@ void annotation_options__init(struct annotation_options *opt)
        opt->percent_type = PERCENT_PERIOD_LOCAL;
 }
 
-
-void annotation_options__exit(struct annotation_options *opt)
+void annotation_options__exit(void)
 {
-       zfree(&opt->disassembler_style);
-       zfree(&opt->objdump_path);
+       zfree(&annotate_opts.disassembler_style);
+       zfree(&annotate_opts.objdump_path);
 }
 
-void annotation_config__init(struct annotation_options *opt)
+void annotation_config__init(void)
 {
-       perf_config(annotation__config, opt);
+       perf_config(annotation__config, &annotate_opts);
 }
 
 static unsigned int parse_percent_type(char *str1, char *str2)

@@ -3491,8 +3492,10 @@ int annotate_parse_percent_type(const struct option *opt __maybe_unused, const c
        return err;
 }
 
-int annotate_check_args(struct annotation_options *args)
+int annotate_check_args(void)
 {
+       struct annotation_options *args = &annotate_opts;
+
        if (args->prefix_strip && !args->prefix) {
                pr_err("--prefix-strip requires --prefix\n");
                return -1;

tools/perf/util/annotate.h +4 -5

index 857c5fa0e6b1..4283eb4522b2 100644
@@ -428,14 +428,14 @@ static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
 }
 #endif
 
-void annotation_options__init(struct annotation_options *opt);
-void annotation_options__exit(struct annotation_options *opt);
+void annotation_options__init(void);
+void annotation_options__exit(void);
 
-void annotation_config__init(struct annotation_options *opt);
+void annotation_config__init(void);
 
 int annotate_parse_percent_type(const struct option *opt, const char *_str,
                                int unset);
 
-int annotate_check_args(struct annotation_options *args);
+int annotate_check_args(void);
 
 #endif /* __PERF_ANNOTATE_H */
--

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-30 00:01 UTC #10 lore raw

Re: [PATCH 6/8] perf annotate: Ensure init/exit for global options

Quoted 10 lines · Ian Rogers

Quoted 3 lines · Namhyung Kim
Now it only cares about the global options so it can just handle it without the argument.
If annotate_opts were accessed by a function then you could pthread_once the initialization on the first call to get annotate_opts. Removing annotation_options__init/exit would remove some potential for error.

Currently all call sites (perf annotate, report and top) initialize the options and check if it has conflicting options before running the commands. So I'm not sure if it needs pthread_once() for that.

Thanks,
Namhyung

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #11 lore raw

PATCH 7/8 perf annotate: Remove remaining usages of local annotation options

In reply to Namhyung Kim

So that it can get rid of the unused data.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/ui/browsers/annotate.c | 14 ++++++--------
 tools/perf/util/annotate.c        |  2 +-
 tools/perf/util/annotate.h        |  6 +++---
 3 files changed, 10 insertions(+), 12 deletions(-)

tools/perf/ui/browsers/annotate.c +6 -8

index fda17c1f2031..cb2eb6dcb532 100644
@@ -37,11 +37,10 @@ static inline struct annotation *browser__annotation(struct ui_browser *browser)
 	return symbol__annotation(ms->sym);
 }
 
-static bool disasm_line__filter(struct ui_browser *browser, void *entry)
+static bool disasm_line__filter(struct ui_browser *browser __maybe_unused, void *entry)
 {
-	struct annotation *notes = browser__annotation(browser);
 	struct annotation_line *al = list_entry(entry, struct annotation_line, node);
-	return annotation_line__filter(al, notes);
+	return annotation_line__filter(al);
 }
 
 static int ui_browser__jumps_percent_color(struct ui_browser *browser, int nr, bool current)

@@ -269,7 +268,6 @@ static void disasm_rb_tree__insert(struct annotate_browser *browser,
 static void annotate_browser__set_top(struct annotate_browser *browser,
 				      struct annotation_line *pos, u32 idx)
 {
-	struct annotation *notes = browser__annotation(&browser->b);
 	unsigned back;
 
 	ui_browser__refresh_dimensions(&browser->b);

@@ -279,7 +277,7 @@ static void annotate_browser__set_top(struct annotate_browser *browser,
 	while (browser->b.top_idx != 0 && back != 0) {
 		pos = list_entry(pos->node.prev, struct annotation_line, node);
 
-		if (annotation_line__filter(pos, notes))
+		if (annotation_line__filter(pos))
 			continue;
 
 		--browser->b.top_idx;

@@ -498,7 +496,7 @@ struct disasm_line *annotate_browser__find_offset(struct annotate_browser *brows
 	list_for_each_entry(pos, &notes->src->source, al.node) {
 		if (pos->al.offset == offset)
 			return pos;
-		if (!annotation_line__filter(&pos->al, notes))
+		if (!annotation_line__filter(&pos->al))
 			++*idx;
 	}

@@ -542,7 +540,7 @@ struct annotation_line *annotate_browser__find_string(struct annotate_browser *b
 
 	*idx = browser->b.index;
 	list_for_each_entry_continue(al, &notes->src->source, node) {
-		if (annotation_line__filter(al, notes))
+		if (annotation_line__filter(al))
 			continue;
 
 		++*idx;

@@ -579,7 +577,7 @@ struct annotation_line *annotate_browser__find_string_reverse(struct annotate_br
 
 	*idx = browser->b.index;
 	list_for_each_entry_continue_reverse(al, &notes->src->source, node) {
-		if (annotation_line__filter(al, notes))
+		if (annotation_line__filter(al))
 			continue;
 
 		--*idx;

tools/perf/util/annotate.c +1 -1

index 626ff3baeb85..09c399ab0384 100644
@@ -2727,7 +2727,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
 	struct annotation_line *al;
 
 	list_for_each_entry(al, &notes->src->source, node) {
-		if (annotation_line__filter(al, notes))
+		if (annotation_line__filter(al))
 			continue;
 		annotation_line__write(al, notes, &wops);
 		fputc('\n', fp);

tools/perf/util/annotate.h +3 -3

index 4283eb4522b2..6d5a6bb49a97 100644
@@ -320,7 +320,7 @@ bool annotation__trylock(struct annotation *notes) EXCLUSIVE_TRYLOCK_FUNCTION(tr
 
 static inline int annotation__cycles_width(struct annotation *notes)
 {
-	if (notes->branch && notes->options->show_minmax_cycle)
+	if (notes->branch && annotate_opts.show_minmax_cycle)
 		return ANNOTATION__IPC_WIDTH + ANNOTATION__MINMAX_CYCLES_WIDTH;
 
 	return notes->branch ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;

@@ -331,9 +331,9 @@ static inline int annotation__pcnt_width(struct annotation *notes)
 	return (symbol_conf.show_total_period ? 12 : 7) * notes->nr_events;
 }
 
-static inline bool annotation_line__filter(struct annotation_line *al, struct annotation *notes)
+static inline bool annotation_line__filter(struct annotation_line *al)
 {
-	return notes->options->hide_src_code && al->offset == -1;
+	return annotate_opts.hide_src_code && al->offset == -1;
 }
 
 void annotation__set_offsets(struct annotation *notes, s64 size);

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-28 17:54 UTC #12 lore raw

PATCH 8/8 perf annotate: Get rid of local annotation options

In reply to Namhyung Kim

It doesn't need the option in the struct annotation which is allocated for each symbol. It can directly use the global options and save 8 bytes per symbol.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/annotate.c | 2 --
 tools/perf/util/annotate.h | 1 -
 2 files changed, 3 deletions(-)

tools/perf/util/annotate.c +0 -2

index 09c399ab0384..c81fa0791918 100644
@@ -3333,8 +3333,6 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
 	if (err)
 		goto out_free_offsets;
 
-	notes->options = &annotate_opts;
-
 	symbol__calc_percent(sym, evsel);
 
 	annotation__set_offsets(notes, size);

tools/perf/util/annotate.h +0 -1

index 6d5a6bb49a97..589f8aaf0236 100644
@@ -294,7 +294,6 @@ struct annotated_branch {
 
 struct LOCKABLE annotation {
 	u64			start;
-	struct annotation_options *options;
 	int			nr_events;
 	int			max_jump_sources;
 	struct {

Signature · 1 line

2.43.0.rc1.413.gea7ed67945-goog

Ian Rogers

reviewer

2023-11-28 19:14 UTC #13 lore raw

In reply to Namhyung Kim

Quoted 8 lines · Namhyung Kim

Hello,
It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.

Quoted 6 lines

This code is available at 'perf/annotate-option-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Thanks,
Namhyung

Thanks for doing this and I think it is progress. I think there is a common problem with having things be an option rather than say part of session. Having a global variable seems unfortunate but I'm not sure if in all locations we have easy access to the session. The rough structure with annotations as I understand it is:

session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation

Annotation is something of unfortunate abstraction as it covers things like an IPC per symbol (why hard code to just IPC?) and things like source files and line numbers.

A recent success story where we got rid of a configuration variable was by switching to lazy allocation with sorting by name for symbols within a dso. If we could have a lazy allocation model with annotations then maybe we can do away with large hammers like global options.

Thanks,
Ian

Quoted 29 lines

Namhyung Kim (8):
perf annotate: Introduce global annotation_options
perf report: Convert to the global annotation_options perf top: Convert to the global annotation_options
perf annotate: Use global annotation_options
perf ui/browser/annotate: Use global annotation_options perf annotate: Ensure init/exit for global options
perf annotate: Remove remaining usages of local annotation options perf annotate: Get rid of local annotation options
 tools/perf/builtin-annotate.c     |  43 +++++----
 tools/perf/builtin-report.c       |  37 ++++----
 tools/perf/builtin-top.c          |  45 +++++-----
 tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
 tools/perf/ui/browsers/hists.c    |  34 +++----
 tools/perf/ui/browsers/hists.h    |   2 -
 tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
 tools/perf/util/annotate.h        |  38 ++++----
 tools/perf/util/block-info.c      |   6 +-
 tools/perf/util/block-info.h      |   3 +-
 tools/perf/util/hist.h            |  25 ++----
 tools/perf/util/top.h             |   1 -
 12 files changed, 206 insertions(+), 255 deletions(-)
base-commit: 757489991f7c08603395b85037a981c31719c92c --
2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-11-29 23:56 UTC #14 lore raw

Hi Ian,

Quoted 14 lines · Ian Rogers

Quoted 12 lines · Namhyung Kim
Hello,
It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.
This code is available at 'perf/annotate-option-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Quoted 7 lines

Quoted 2 lines
Thanks,
Namhyung
Thanks for doing this and I think it is progress. I think there is a common problem with having things be an option rather than say part of session. Having a global variable seems unfortunate but I'm not sure if in all locations we have easy access to the session.

That's not the case when you deal with hist entry or TUI browser. I think that's the reason we have the option in the annotation.

Quoted 7 lines

The rough
structure with annotations as I understand it is:
session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation

That's true. But the annotation struct is used only if symbol__annotation_init() is called.

Quoted 4 lines

Annotation is something of unfortunate abstraction as it covers things like an IPC per symbol (why hard code to just IPC?) and things like source files and line numbers.

Right, that's why I splitted the struct annotated_branch.

Quoted 6 lines

A recent success story where we got rid of a configuration variable was by switching to lazy allocation with sorting by name for symbols within a dso. If we could have a lazy allocation model with annotations then maybe we can do away with large hammers like global options.

Maybe I can move the pointer to option into the annotated_source which is allocated lazily. But I don't think it needs to keep the option for each symbol or annotation. It's usually to control some display behaviors in the disasm output globally. So I think it's better to have a global variable.

Thanks,
Namhyung

Quoted 31 lines

Quoted 31 lines
Namhyung Kim (8):
perf annotate: Introduce global annotation_options
perf report: Convert to the global annotation_options perf top: Convert to the global annotation_options
perf annotate: Use global annotation_options
perf ui/browser/annotate: Use global annotation_options perf annotate: Ensure init/exit for global options
perf annotate: Remove remaining usages of local annotation options perf annotate: Get rid of local annotation options
 tools/perf/builtin-annotate.c     |  43 +++++----
 tools/perf/builtin-report.c       |  37 ++++----
 tools/perf/builtin-top.c          |  45 +++++-----
 tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
 tools/perf/ui/browsers/hists.c    |  34 +++----
 tools/perf/ui/browsers/hists.h    |   2 -
 tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
 tools/perf/util/annotate.h        |  38 ++++----
 tools/perf/util/block-info.c      |   6 +-
 tools/perf/util/block-info.h      |   3 +-
 tools/perf/util/hist.h            |  25 ++----
 tools/perf/util/top.h             |   1 -
 12 files changed, 206 insertions(+), 255 deletions(-)
base-commit: 757489991f7c08603395b85037a981c31719c92c --
2.43.0.rc1.413.gea7ed67945-goog

Ian Rogers

reviewer

2023-11-30 18:37 UTC #15 lore raw

Quoted 33 lines · Namhyung Kim

Hi Ian,
Quoted 14 lines · Ian Rogers
Quoted 12 lines · Namhyung Kim
Hello,
It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.
This code is available at 'perf/annotate-option-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Quoted 7 lines
Quoted 2 lines
Thanks,
Namhyung
Thanks for doing this and I think it is progress. I think there is a common problem with having things be an option rather than say part of session. Having a global variable seems unfortunate but I'm not sure if in all locations we have easy access to the session.
That's not the case when you deal with hist entry or TUI browser. I think that's the reason we have the option in the annotation.
Quoted 3 lines
The rough
structure with annotations as I understand it is:

Quoted 7 lines

Quoted 4 lines
session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation
That's true. But the annotation struct is used only if symbol__annotation_init() is called.

Right. I find this approach likely to lead to errors, such as a symbol created globally before symbol__annotation_init() was called breaking the container_of assumptions.

Quoted 14 lines

Quoted 4 lines
Annotation is something of unfortunate abstraction as it covers things like an IPC per symbol (why hard code to just IPC?) and things like source files and line numbers.
Right, that's why I splitted the struct annotated_branch.
Quoted 6 lines
A recent success story where we got rid of a configuration variable was by switching to lazy allocation with sorting by name for symbols within a dso. If we could have a lazy allocation model with annotations then maybe we can do away with large hammers like global options.

Quoted 5 lines

Maybe I can move the pointer to option into the annotated_source which is allocated lazily. But I don't think it needs to keep the option for each symbol or annotation. It's usually to control some display behaviors in the disasm output globally. So I think it's better to have a global variable.

Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity

Thanks,
Ian

Quoted 34 lines

Thanks,
Namhyung
Quoted 31 lines
Quoted 31 lines
Namhyung Kim (8):
perf annotate: Introduce global annotation_options
perf report: Convert to the global annotation_options perf top: Convert to the global annotation_options
perf annotate: Use global annotation_options
perf ui/browser/annotate: Use global annotation_options perf annotate: Ensure init/exit for global options
perf annotate: Remove remaining usages of local annotation options perf annotate: Get rid of local annotation options
 tools/perf/builtin-annotate.c     |  43 +++++----
 tools/perf/builtin-report.c       |  37 ++++----
 tools/perf/builtin-top.c          |  45 +++++-----
 tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
 tools/perf/ui/browsers/hists.c    |  34 +++----
 tools/perf/ui/browsers/hists.h    |   2 -
 tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
 tools/perf/util/annotate.h        |  38 ++++----
 tools/perf/util/block-info.c      |   6 +-
 tools/perf/util/block-info.h      |   3 +-
 tools/perf/util/hist.h            |  25 ++----
 tools/perf/util/top.h             |   1 -
 12 files changed, 206 insertions(+), 255 deletions(-)
base-commit: 757489991f7c08603395b85037a981c31719c92c --
2.43.0.rc1.413.gea7ed67945-goog

Namhyung Kim

author maintainer

2023-12-04 22:46 UTC #16 lore raw

Quoted 40 lines · Ian Rogers

Quoted 38 lines · Namhyung Kim
Hi Ian,
Quoted 14 lines · Ian Rogers
Quoted 12 lines · Namhyung Kim
Hello,
It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.
This code is available at 'perf/annotate-option-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Quoted 7 lines
Quoted 2 lines
Thanks,
Namhyung
Thanks for doing this and I think it is progress. I think there is a common problem with having things be an option rather than say part of session. Having a global variable seems unfortunate but I'm not sure if in all locations we have easy access to the session.
That's not the case when you deal with hist entry or TUI browser. I think that's the reason we have the option in the annotation.
Quoted 7 lines
The rough
structure with annotations as I understand it is:
session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation

Quoted 6 lines

Quoted 2 lines
That's true. But the annotation struct is used only if symbol__annotation_init() is called.
Right. I find this approach likely to lead to errors, such as a symbol created globally before symbol__annotation_init() was called breaking the container_of assumptions.

Sure, but that's a different issue. Maybe we can add a hash table to map a symbol to annotation or annotated_source directly. But I don't think we need annotation_option for each symbol/annotation.

Quoted 21 lines

Quoted 15 lines
Quoted 5 lines
Annotation is something of unfortunate abstraction as it covers things like an IPC per symbol (why hard code to just IPC?) and things like source files and line numbers.
Right, that's why I splitted the struct annotated_branch.
Quoted 6 lines
A recent success story where we got rid of a configuration variable was by switching to lazy allocation with sorting by name for symbols within a dso. If we could have a lazy allocation model with annotations then maybe we can do away with large hammers like global options.
Quoted 5 lines
Maybe I can move the pointer to option into the annotated_source which is allocated lazily. But I don't think it needs to keep the option for each symbol or annotation. It's usually to control some display behaviors in the disasm output globally. So I think it's better to have a global variable.

Quoted 3 lines

Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity

Yep, thanks for your review. Can I get your ACKs? :) Namhyung

Ian Rogers

reviewer Reviewed-by

2023-12-05 17:59 UTC #17 lore raw

Quoted 45 lines · Namhyung Kim

Quoted 43 lines · Ian Rogers
Quoted 40 lines · Namhyung Kim
Hi Ian,
Quoted 14 lines · Ian Rogers
Quoted 12 lines · Namhyung Kim
Hello,
It used to have annotation_options for each command separately (for example, perf report, annotate, and top), but we can make it global as they never used together (with different settings). This would save some memory for each symbol when annotation is enabled.
This code is available at 'perf/annotate-option-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Quoted 7 lines
Quoted 2 lines
Thanks,
Namhyung
Thanks for doing this and I think it is progress. I think there is a common problem with having things be an option rather than say part of session. Having a global variable seems unfortunate but I'm not sure if in all locations we have easy access to the session.
That's not the case when you deal with hist entry or TUI browser. I think that's the reason we have the option in the annotation.
Quoted 7 lines
The rough
structure with annotations as I understand it is:
session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation
That's true. But the annotation struct is used only if symbol__annotation_init() is called.

Quoted 7 lines

Quoted 3 lines
Right. I find this approach likely to lead to errors, such as a symbol created globally before symbol__annotation_init() was called breaking the container_of assumptions.
Sure, but that's a different issue. Maybe we can add a hash table to map a symbol to annotation or annotated_source directly. But I don't think we need annotation_option for each symbol/annotation.

Agreed.

Quoted 21 lines

Quoted 21 lines
Quoted 20 lines
Quoted 5 lines
Annotation is something of unfortunate abstraction as it covers things like an IPC per symbol (why hard code to just IPC?) and things like source files and line numbers.
Right, that's why I splitted the struct annotated_branch.
Quoted 6 lines
A recent success story where we got rid of a configuration variable was by switching to lazy allocation with sorting by name for symbols within a dso. If we could have a lazy allocation model with annotations then maybe we can do away with large hammers like global options.
Maybe I can move the pointer to option into the annotated_source which is allocated lazily. But I don't think it needs to keep the option for each symbol or annotation. It's usually to control some display behaviors in the disasm output globally. So I think it's better to have a global variable.

Quoted 5 lines

Quoted 3 lines
Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity
Yep, thanks for your review. Can I get your ACKs? :)

For the series:

Reviewed-by Ian Rogers <[email protected]>

Thanks,
Ian

Quoted 1 line

Namhyung

Arnaldo Carvalho de Melo

maintainer Applied

2023-12-07 19:50 UTC #18 lore raw

Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:

Quoted 10 lines · Namhyung Kim

Quoted 6 lines · Ian Rogers
Quoted 3 lines
Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity
Yep, thanks for your review. Can I get your ACKs? :)
For the series:
Reviewed-by Ian Rogers <[email protected]>

Thanks, applied to perf-tools-next.

- Arnaldo

Arnaldo Carvalho de Melo

maintainer

2023-12-07 19:52 UTC #19 lore raw

Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 7 lines

Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
Quoted 5 lines · Namhyung Kim
Quoted 4 lines · Ian Rogers
Quoted 3 lines
Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Yep, thanks for your review. Can I get your ACKs? :)
For the series:
Reviewed-by Ian Rogers <[email protected]>
Thanks, applied to perf-tools-next.

Now trying to fix this:

CC bench/numa.o
CC tests/hists_cumulate.o
ui/gtk/annotate.c: In function ‘symbol__gtk_annotate’:
ui/gtk/annotate.c:179:43: error: passing argument 3 of ‘symbol__annotate’ from incompatible pointer type [-Werror=incompatible-pointer-types]

  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |                                           ^~~~~~~
      |                                           |
      |                                           struct annotation_options *
In file included from ui/gtk/annotate.c:5:
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:376:36: note: expected ‘struct arch **’ but argument is of type ‘struct annotation_options *’
  376 |                      struct arch **parch);
      |                      ~~~~~~~~~~~~~~^~~~~
ui/gtk/annotate.c:179:15: error: too many arguments to function ‘symbol__annotate’
  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |               ^~~~~~~~~~~~~~~~
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:374:5: note: declared here
  374 | int symbol__annotate(struct map_symbol *ms,
      |     ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
  CC      tests/python-use.o
  CC      trace/beauty/sockaddr.o
  CC      arch/x86/util/topdown.o

make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: ui/gtk/annotate.o] Error 1 make[6]: *** Waiting for unfinished jobs....
CC arch/x86/util/machine.o

Ian Rogers

reviewer

2023-12-07 20:14 UTC #20 lore raw

Quoted 41 lines · Arnaldo Carvalho de Melo

Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 7 lines
Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
Quoted 5 lines · Namhyung Kim
Quoted 4 lines · Ian Rogers
Quoted 3 lines
Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Yep, thanks for your review. Can I get your ACKs? :)
For the series:
Reviewed-by Ian Rogers <[email protected]>
Thanks, applied to perf-tools-next.
Now trying to fix this:
CC bench/numa.o
CC tests/hists_cumulate.o
ui/gtk/annotate.c: In function ‘symbol__gtk_annotate’:
ui/gtk/annotate.c:179:43: error: passing argument 3 of ‘symbol__annotate’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |                                           ^~~~~~~
      |                                           |
      |                                           struct annotation_options *
In file included from ui/gtk/annotate.c:5:
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:376:36: note: expected ‘struct arch **’ but argument is of type ‘struct annotation_options *’
  376 |                      struct arch **parch);
      |                      ~~~~~~~~~~~~~~^~~~~
ui/gtk/annotate.c:179:15: error: too many arguments to function ‘symbol__annotate’
  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |               ^~~~~~~~~~~~~~~~
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:374:5: note: declared here
  374 | int symbol__annotate(struct map_symbol *ms,
      |     ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
  CC      tests/python-use.o
  CC      trace/beauty/sockaddr.o
  CC      arch/x86/util/topdown.o

Quoted 3 lines

make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: ui/gtk/annotate.o] Error 1 make[6]: *** Waiting for unfinished jobs....
CC arch/x86/util/machine.o

Maybe a signal to remove the gtk support :-)

Ian

Namhyung Kim

author maintainer

2023-12-07 20:41 UTC #21 lore raw

Quoted 48 lines · Ian Rogers

Quoted 41 lines · Arnaldo Carvalho de Melo
Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 7 lines
Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
Quoted 5 lines · Namhyung Kim
Quoted 4 lines · Ian Rogers
Quoted 3 lines
Sgtm. My point wasn't to criticize, I think this is a good change, I was just trying to imagine doing things in a way that could overall reduce complexity
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Yep, thanks for your review. Can I get your ACKs? :)
For the series:
Reviewed-by Ian Rogers <[email protected]>
Thanks, applied to perf-tools-next.
Now trying to fix this:
CC bench/numa.o
CC tests/hists_cumulate.o
ui/gtk/annotate.c: In function ‘symbol__gtk_annotate’:
ui/gtk/annotate.c:179:43: error: passing argument 3 of ‘symbol__annotate’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |                                           ^~~~~~~
      |                                           |
      |                                           struct annotation_options *
In file included from ui/gtk/annotate.c:5:
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:376:36: note: expected ‘struct arch **’ but argument is of type ‘struct annotation_options *’
  376 |                      struct arch **parch);
      |                      ~~~~~~~~~~~~~~^~~~~
ui/gtk/annotate.c:179:15: error: too many arguments to function ‘symbol__annotate’
  179 |         err = symbol__annotate(ms, evsel, options, NULL);
      |               ^~~~~~~~~~~~~~~~
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:374:5: note: declared here
  374 | int symbol__annotate(struct map_symbol *ms,
      |     ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
  CC      tests/python-use.o
  CC      trace/beauty/sockaddr.o
  CC      arch/x86/util/topdown.o
Quoted 3 lines
make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: ui/gtk/annotate.o] Error 1 make[6]: *** Waiting for unfinished jobs....
CC arch/x86/util/machine.o

Quoted 1 line

Maybe a signal to remove the gtk support :-)

Thanks,
Namhyung