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
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 121a2781323c..90f98953587c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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;
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index a9129b51d511..67b36a7a12e3 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -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;
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 9a828dc601c7..77b78001b94d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -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);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index b64a2be287b3..8c1a070725fa 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -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,
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ea8c7eca5eee..52930b71f660 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -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;
}
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index a8b0d79bd96c..4c5588dbb131 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -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
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 09c399ab0384..c81fa0791918 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -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);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 6d5a6bb49a97..589f8aaf0236 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -294,7 +294,6 @@ struct annotated_branch {
struct LOCKABLE annotation {
u64 start;
- struct annotation_options *options;
int nr_events;
int max_jump_sources;
struct {
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index a53a4e711899..87af95634879 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -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:
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2b86651615cd..bc0d986c1e0c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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.
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 80d126279208..60399e4233ee 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -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;
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index ed0e692afdbe..fda17c1f2031 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -27,7 +27,6 @@ struct annotate_browser {
struct rb_node *curr_hot;
struct annotation_line *selection;
struct arch *arch;
- struct annotation_options *opts;
bool searching_backwards;
char search_bf[128];
};
@@ -97,7 +96,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
struct annotation_write_ops ops = {
.first_line = row == 0,
.current_entry = is_current_entry,
- .change_color = (!notes->options->hide_src_code &&
+ .change_color = (!annotate_opts.hide_src_code &&
(!is_current_entry ||
(browser->use_navkeypressed &&
!browser->navkeypressed))),
@@ -128,7 +127,7 @@ static int is_fused(struct annotate_browser *ab, struct disasm_line *cursor)
while (pos && pos->al.offset == -1) {
pos = list_prev_entry(pos, al.node);
- if (!ab->opts->hide_src_code)
+ if (!annotate_opts.hide_src_code)
diff++;
}
@@ -195,7 +194,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
return;
}
- if (notes->options->hide_src_code) {
+ if (annotate_opts.hide_src_code) {
from = cursor->al.idx_asm;
to = target->idx_asm;
} else {
@@ -224,7 +223,7 @@ static unsigned int annotate_browser__refresh(struct ui_browser *browser)
int ret = ui_browser__list_head_refresh(browser);
int pcnt_width = annotation__pcnt_width(notes);
- if (notes->options->jump_arrows)
+ if (annotate_opts.jump_arrows)
annotate_browser__draw_current_jump(browser);
ui_browser__set_color(browser, HE_COLORSET_NORMAL);
@@ -258,7 +257,7 @@ static void disasm_rb_tree__insert(struct annotate_browser *browser,
parent = *p;
l = rb_entry(parent, struct annotation_line, rb_node);
- if (disasm__cmp(al, l, browser->opts->percent_type) < 0)
+ if (disasm__cmp(al, l, annotate_opts.percent_type) < 0)
p = &(*p)->rb_left;
else
p = &(*p)->rb_right;
@@ -294,11 +293,10 @@ static void annotate_browser__set_top(struct annotate_browser *browser,
static void annotate_browser__set_rb_top(struct annotate_browser *browser,
struct rb_node *nd)
{
- struct annotation *notes = browser__annotation(&browser->b);
struct annotation_line * pos = rb_entry(nd, struct annotation_line, rb_node);
u32 idx = pos->idx;
- if (notes->options->hide_src_code)
+ if (annotate_opts.hide_src_code)
idx = pos->idx_asm;
annotate_browser__set_top(browser, pos, idx);
browser->curr_hot = nd;
@@ -331,7 +329,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
double percent;
percent = annotation_data__percent(&pos->al.data[i],
- browser->opts->percent_type);
+ annotate_opts.percent_type);
if (max_percent < percent)
max_percent = percent;
@@ -380,12 +378,12 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
browser->b.seek(&browser->b, offset, SEEK_CUR);
al = list_entry(browser->b.top, struct annotation_line, node);
- if (notes->options->hide_src_code) {
+ if (annotate_opts.hide_src_code) {
if (al->idx_asm < offset)
offset = al->idx;
browser->b.nr_entries = notes->src->nr_entries;
- notes->options->hide_src_code = false;
+ annotate_opts.hide_src_code = false;
browser->b.seek(&browser->b, -offset, SEEK_CUR);
browser->b.top_idx = al->idx - offset;
browser->b.index = al->idx;
@@ -403,7 +401,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
offset = al->idx_asm;
browser->b.nr_entries = notes->src->nr_asm_entries;
- notes->options->hide_src_code = true;
+ annotate_opts.hide_src_code = true;
browser->b.seek(&browser->b, -offset, SEEK_CUR);
browser->b.top_idx = al->idx_asm - offset;
browser->b.index = al->idx_asm;
@@ -483,8 +481,8 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
target_ms.map = ms->map;
target_ms.sym = dl->ops.target.sym;
annotation__unlock(notes);
- symbol__tui_annotate(&target_ms, evsel, hbt, browser->opts);
- sym_title(ms->sym, ms->map, title, sizeof(title), browser->opts->percent_type);
+ symbol__tui_annotate(&target_ms, evsel, hbt);
+ sym_title(ms->sym, ms->map, title, sizeof(title), annotate_opts.percent_type);
ui_browser__show_title(&browser->b, title);
return true;
}
@@ -659,7 +657,6 @@ bool annotate_browser__continue_search_reverse(struct annotate_browser *browser,
static int annotate_browser__show(struct ui_browser *browser, char *title, const char *help)
{
- struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
struct map_symbol *ms = browser->priv;
struct symbol *sym = ms->sym;
char symbol_dso[SYM_TITLE_MAX_SIZE];
@@ -667,7 +664,7 @@ static int annotate_browser__show(struct ui_browser *browser, char *title, const
if (ui_browser__show(browser, title, help) < 0)
return -1;
- sym_title(sym, ms->map, symbol_dso, sizeof(symbol_dso), ab->opts->percent_type);
+ sym_title(sym, ms->map, symbol_dso, sizeof(symbol_dso), annotate_opts.percent_type);
ui_browser__gotorc_title(browser, 0, 0);
ui_browser__set_color(browser, HE_COLORSET_ROOT);
@@ -809,7 +806,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
annotate_browser__show(&browser->b, title, help);
continue;
case 'k':
- notes->options->show_linenr = !notes->options->show_linenr;
+ annotate_opts.show_linenr = !annotate_opts.show_linenr;
continue;
case 'l':
annotate_browser__show_full_location (&browser->b);
@@ -822,18 +819,18 @@ static int annotate_browser__run(struct annotate_browser *browser,
ui_helpline__puts(help);
continue;
case 'o':
- notes->options->use_offset = !notes->options->use_offset;
+ annotate_opts.use_offset = !annotate_opts.use_offset;
annotation__update_column_widths(notes);
continue;
case 'O':
- if (++notes->options->offset_level > ANNOTATION__MAX_OFFSET_LEVEL)
- notes->options->offset_level = ANNOTATION__MIN_OFFSET_LEVEL;
+ if (++annotate_opts.offset_level > ANNOTATION__MAX_OFFSET_LEVEL)
+ annotate_opts.offset_level = ANNOTATION__MIN_OFFSET_LEVEL;
continue;
case 'j':
- notes->options->jump_arrows = !notes->options->jump_arrows;
+ annotate_opts.jump_arrows = !annotate_opts.jump_arrows;
continue;
case 'J':
- notes->options->show_nr_jumps = !notes->options->show_nr_jumps;
+ annotate_opts.show_nr_jumps = !annotate_opts.show_nr_jumps;
annotation__update_column_widths(notes);
continue;
case '/':
@@ -897,15 +894,15 @@ static int annotate_browser__run(struct annotate_browser *browser,
annotation__update_column_widths(notes);
continue;
case 'c':
- if (notes->options->show_minmax_cycle)
- notes->options->show_minmax_cycle = false;
+ if (annotate_opts.show_minmax_cycle)
+ annotate_opts.show_minmax_cycle = false;
else
- notes->options->show_minmax_cycle = true;
+ annotate_opts.show_minmax_cycle = true;
annotation__update_column_widths(notes);
continue;
case 'p':
case 'b':
- switch_percent_type(browser->opts, key == 'b');
+ switch_percent_type(&annotate_opts, key == 'b');
hists__scnprintf_title(hists, title, sizeof(title));
annotate_browser__show(&browser->b, title, help);
continue;
@@ -932,26 +929,23 @@ static int annotate_browser__run(struct annotate_browser *browser,
}
int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
- struct hist_browser_timer *hbt,
- struct annotation_options *opts)
+ struct hist_browser_timer *hbt)
{
- return symbol__tui_annotate(ms, evsel, hbt, opts);
+ return symbol__tui_annotate(ms, evsel, hbt);
}
int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
- struct hist_browser_timer *hbt,
- struct annotation_options *opts)
+ struct hist_browser_timer *hbt)
{
/* reset abort key so that it can get Ctrl-C as a key */
SLang_reset_tty();
SLang_init_tty(0, 0, 0);
- return map_symbol__tui_annotate(&he->ms, evsel, hbt, opts);
+ return map_symbol__tui_annotate(&he->ms, evsel, hbt);
}
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)
{
struct symbol *sym = ms->sym;
struct annotation *notes = symbol__annotation(sym);
@@ -965,7 +959,6 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
.priv = ms,
.use_navkeypressed = true,
},
- .opts = opts,
};
struct dso *dso;
int ret = -1, err;
@@ -996,7 +989,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
browser.b.entries = ¬es->src->source,
browser.b.width += 18; /* Percentage */
- if (notes->options->hide_src_code)
+ if (annotate_opts.hide_src_code)
ui_browser__init_asm_mode(&browser.b);
ret = annotate_browser__run(&browser, evsel, hbt);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f4812b226818..3061dea29e6b 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -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();
diff --git a/tools/perf/ui/browsers/hists.h b/tools/perf/ui/browsers/hists.h
index 1e938d9ffa5e..de46f6c56b0e 100644
--- a/tools/perf/ui/browsers/hists.h
+++ b/tools/perf/ui/browsers/hists.h
@@ -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;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 7bf29baa43f5..857c5fa0e6b1 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -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;
}
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 08f82c1f166c..dec910989701 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -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;
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index 42e9dcc4cf0a..96f53e89795e 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -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);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index afc9f1c7f4dc..5d0db96609df 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -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;
}
--
2.43.0.rc1.413.gea7ed67945-goog
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(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 87af95634879..9b3dd456a1ee 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -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;
}
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index bc0d986c1e0c..17fb171e898b 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 60399e4233ee..0de963ca3196 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -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;
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index daff9af552f4..626ff3baeb85 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -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;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 857c5fa0e6b1..4283eb4522b2 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -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
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(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index fda17c1f2031..cb2eb6dcb532 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -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, ¬es->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, ¬es->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, ¬es->src->source, node) {
- if (annotation_line__filter(al, notes))
+ if (annotation_line__filter(al))
continue;
--*idx;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 626ff3baeb85..09c399ab0384 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2727,7 +2727,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
struct annotation_line *al;
list_for_each_entry(al, ¬es->src->source, node) {
- if (annotation_line__filter(al, notes))
+ if (annotation_line__filter(al))
continue;
annotation_line__write(al, notes, &wops);
fputc('\n', fp);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 4283eb4522b2..6d5a6bb49a97 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -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);
--
2.43.0.rc1.413.gea7ed67945-goog
On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
>
> 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
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
> 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
>
On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
>
> 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
> 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(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 87af95634879..9b3dd456a1ee 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -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;
> }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index bc0d986c1e0c..17fb171e898b 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -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;
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 60399e4233ee..0de963ca3196 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -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;
> }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index daff9af552f4..626ff3baeb85 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -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;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 857c5fa0e6b1..4283eb4522b2 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -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
>
Hi Ian,
On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <[email protected]> wrote:
>
> On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
> >
> > 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
>
> 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.
> 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.
>
> 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.
>
> 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
>
>
> > 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
> >
On Tue, Nov 28, 2023 at 11:21 AM Ian Rogers <[email protected]> wrote:
>
> On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
> >
> > 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
On Wed, Nov 29, 2023 at 3:56 PM Namhyung Kim <[email protected]> wrote:
>
> Hi Ian,
>
> On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <[email protected]> wrote:
> >
> > On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
> > >
> > > 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
> >
> > 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.
>
>
> > 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.
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.
> >
> > 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.
>
> >
> > 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.
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
> 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
> > > --
> > > 2.43.0.rc1.413.gea7ed67945-goog
> > >
On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
>
> On Wed, Nov 29, 2023 at 3:56 PM Namhyung Kim <[email protected]> wrote:
> >
> > Hi Ian,
> >
> > On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <[email protected]> wrote:
> > >
> > > On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
> > > >
> > > > 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
> > >
> > > 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.
> >
> >
> > > 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.
>
> 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.
>
> > >
> > > 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.
> >
> > >
> > > 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.
>
> 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
On Mon, Dec 4, 2023 at 2:46 PM Namhyung Kim <[email protected]> wrote:
>
> On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
> >
> > On Wed, Nov 29, 2023 at 3:56 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <[email protected]> wrote:
> > > >
> > > > On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <[email protected]> wrote:
> > > > >
> > > > > 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
> > > >
> > > > 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.
> > >
> > >
> > > > 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.
> >
> > 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.
> >
> > > >
> > > > 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.
> > >
> > > >
> > > > 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.
> >
> > 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
> Namhyung
Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
> On Mon, Dec 4, 2023 at 2:46 PM Namhyung Kim <[email protected]> wrote:
> > On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
> > > 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
Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
> > On Mon, Dec 4, 2023 at 2:46 PM Namhyung Kim <[email protected]> wrote:
> > > On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
> > > > 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.
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
On Thu, Dec 7, 2023 at 11:52 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
> > > On Mon, Dec 4, 2023 at 2:46 PM Namhyung Kim <[email protected]> wrote:
> > > > On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
> > > > > 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.
>
>
> 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
Maybe a signal to remove the gtk support :-)
Ian
On Thu, Dec 7, 2023 at 12:14 PM Ian Rogers <[email protected]> wrote:
>
> On Thu, Dec 7, 2023 at 11:52 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Thu, Dec 07, 2023 at 04:50:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Dec 05, 2023 at 09:59:02AM -0800, Ian Rogers escreveu:
> > > > On Mon, Dec 4, 2023 at 2:46 PM Namhyung Kim <[email protected]> wrote:
> > > > > On Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <[email protected]> wrote:
> > > > > > 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.
> >
> >
> > 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
>
> Maybe a signal to remove the gtk support :-)
+1
Thanks,
Namhyung