* [PATCH] perf annotate: Cache debuginfo for data type profiling
@ 2024-07-04 1:58 Namhyung Kim
2024-07-12 5:29 ` Ian Rogers
0 siblings, 1 reply; 3+ messages in thread
From: Namhyung Kim @ 2024-07-04 1:58 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Athira Rajeev
In find_data_type(), it creates and deletes a debug info whenver it
tries to find data type for a sample. This is inefficient and it most
likely accesses the same binary again and again.
Let's add a single entry cache the debug info structure for the last DSO.
Depending on sample data, it usually gives me 2~3x (and sometimes more)
speed ups.
Note that this will introduce a little difference in the output due to
the order of checking stack operations. It used to check the stack ops
before checking the availability of debug info but I moved it after the
symbol check. So it'll report stack operations in DSOs without debug
info as unknown. But I think it's ok and better to have the checking
near the caching logic.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/annotate-data.c | 15 ++-------------
tools/perf/util/annotate-data.h | 2 +-
tools/perf/util/annotate.c | 29 +++++++++++++++++++++++++++++
tools/perf/util/annotate.h | 2 ++
tools/perf/util/session.c | 2 ++
5 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 965da6c0b542..24c03475b3f2 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -1764,16 +1764,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
*/
struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
{
- struct annotated_data_type *result = NULL;
struct dso *dso = map__dso(dloc->ms->map);
Dwarf_Die type_die;
- dloc->di = debuginfo__new(dso__long_name(dso));
- if (dloc->di == NULL) {
- pr_debug_dtp("cannot get the debug info\n");
- return NULL;
- }
-
/*
* The type offset is the same as instruction offset by default.
* But when finding a global variable, the offset won't be valid.
@@ -1783,13 +1776,9 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
dloc->fbreg = -1;
if (find_data_type_die(dloc, &type_die) < 0)
- goto out;
-
- result = dso__findnew_data_type(dso, &type_die);
+ return NULL;
-out:
- debuginfo__delete(dloc->di);
- return result;
+ return dso__findnew_data_type(dso, &type_die);
}
static int alloc_data_type_histograms(struct annotated_data_type *adt, int nr_entries)
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 0a57d9f5ee78..40c9377fa1a5 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -100,9 +100,9 @@ struct data_loc_info {
u64 var_addr;
u8 cpumode;
struct annotated_op_loc *op;
+ struct debuginfo *di;
/* These are used internally */
- struct debuginfo *di;
int fbreg;
bool fb_cfa;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 1451caf25e77..821e4b2c3bcd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -25,6 +25,7 @@
#include "srcline.h"
#include "units.h"
#include "debug.h"
+#include "debuginfo.h"
#include "annotate.h"
#include "annotate-data.h"
#include "evsel.h"
@@ -2313,6 +2314,20 @@ u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
return map__rip_2objdump(ms->map, addr);
}
+static struct debuginfo_cache {
+ struct dso *dso;
+ struct debuginfo *dbg;
+} di_cache;
+
+void debuginfo_cache__delete(void)
+{
+ dso__put(di_cache.dso);
+ di_cache.dso = NULL;
+
+ debuginfo__delete(di_cache.dbg);
+ di_cache.dbg = NULL;
+}
+
/**
* hist_entry__get_data_type - find data type for given hist entry
* @he: hist entry
@@ -2347,6 +2362,19 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
return NULL;
}
+ if (map__dso(ms->map) != di_cache.dso) {
+ dso__put(di_cache.dso);
+ di_cache.dso = dso__get(map__dso(ms->map));
+
+ debuginfo__delete(di_cache.dbg);
+ di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
+ }
+
+ if (di_cache.dbg == NULL) {
+ ann_data_stat.no_dbginfo++;
+ return NULL;
+ }
+
/* Make sure it has the disasm of the function */
if (symbol__annotate(ms, evsel, &arch) < 0) {
ann_data_stat.no_insn++;
@@ -2391,6 +2419,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
.ip = ms->sym->start + dl->al.offset,
.cpumode = he->cpumode,
.op = op_loc,
+ .di = di_cache.dbg,
};
if (!op_loc->mem_ref && op_loc->segment == INSN_SEG_NONE)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index d5c821c22f79..0686af637ed9 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -540,4 +540,6 @@ struct annotated_basic_block {
int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
struct list_head *head);
+void debuginfo_cache__delete(void);
+
#endif /* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 5596bed1b8c8..f9072e003367 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -36,6 +36,7 @@
#include "util.h"
#include "arch/common.h"
#include "units.h"
+#include "annotate.h"
#include <internal/lib.h>
#ifdef HAVE_ZSTD_SUPPORT
@@ -304,6 +305,7 @@ void perf_session__delete(struct perf_session *session)
return;
auxtrace__free(session);
auxtrace_index__free(&session->auxtrace_index);
+ debuginfo_cache__delete();
perf_session__destroy_kernel_maps(session);
perf_decomp__release_events(session->decomp_data.decomp);
perf_env__exit(&session->header.env);
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf annotate: Cache debuginfo for data type profiling
2024-07-04 1:58 [PATCH] perf annotate: Cache debuginfo for data type profiling Namhyung Kim
@ 2024-07-12 5:29 ` Ian Rogers
2024-07-12 6:04 ` Namhyung Kim
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2024-07-12 5:29 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
Athira Rajeev
On Wed, Jul 3, 2024 at 6:58 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> In find_data_type(), it creates and deletes a debug info whenver it
> tries to find data type for a sample. This is inefficient and it most
> likely accesses the same binary again and again.
>
> Let's add a single entry cache the debug info structure for the last DSO.
> Depending on sample data, it usually gives me 2~3x (and sometimes more)
> speed ups.
>
> Note that this will introduce a little difference in the output due to
> the order of checking stack operations. It used to check the stack ops
> before checking the availability of debug info but I moved it after the
> symbol check. So it'll report stack operations in DSOs without debug
> info as unknown. But I think it's ok and better to have the checking
> near the caching logic.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/annotate-data.c | 15 ++-------------
> tools/perf/util/annotate-data.h | 2 +-
> tools/perf/util/annotate.c | 29 +++++++++++++++++++++++++++++
> tools/perf/util/annotate.h | 2 ++
> tools/perf/util/session.c | 2 ++
> 5 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 965da6c0b542..24c03475b3f2 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -1764,16 +1764,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
> */
> struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> {
> - struct annotated_data_type *result = NULL;
> struct dso *dso = map__dso(dloc->ms->map);
> Dwarf_Die type_die;
>
> - dloc->di = debuginfo__new(dso__long_name(dso));
> - if (dloc->di == NULL) {
> - pr_debug_dtp("cannot get the debug info\n");
> - return NULL;
> - }
> -
> /*
> * The type offset is the same as instruction offset by default.
> * But when finding a global variable, the offset won't be valid.
> @@ -1783,13 +1776,9 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> dloc->fbreg = -1;
>
> if (find_data_type_die(dloc, &type_die) < 0)
> - goto out;
> -
> - result = dso__findnew_data_type(dso, &type_die);
> + return NULL;
>
> -out:
> - debuginfo__delete(dloc->di);
> - return result;
> + return dso__findnew_data_type(dso, &type_die);
> }
>
> static int alloc_data_type_histograms(struct annotated_data_type *adt, int nr_entries)
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index 0a57d9f5ee78..40c9377fa1a5 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -100,9 +100,9 @@ struct data_loc_info {
> u64 var_addr;
> u8 cpumode;
> struct annotated_op_loc *op;
> + struct debuginfo *di;
>
> /* These are used internally */
> - struct debuginfo *di;
> int fbreg;
> bool fb_cfa;
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 1451caf25e77..821e4b2c3bcd 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -25,6 +25,7 @@
> #include "srcline.h"
> #include "units.h"
> #include "debug.h"
> +#include "debuginfo.h"
> #include "annotate.h"
> #include "annotate-data.h"
> #include "evsel.h"
> @@ -2313,6 +2314,20 @@ u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
> return map__rip_2objdump(ms->map, addr);
> }
>
> +static struct debuginfo_cache {
> + struct dso *dso;
> + struct debuginfo *dbg;
> +} di_cache;
> +
> +void debuginfo_cache__delete(void)
> +{
> + dso__put(di_cache.dso);
> + di_cache.dso = NULL;
> +
> + debuginfo__delete(di_cache.dbg);
> + di_cache.dbg = NULL;
> +}
> +
> /**
> * hist_entry__get_data_type - find data type for given hist entry
> * @he: hist entry
> @@ -2347,6 +2362,19 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> return NULL;
> }
>
> + if (map__dso(ms->map) != di_cache.dso) {
Does the di_cache need a mutex? Otherwise another thread may be in
this same code racing with it.
Thanks,
Ian
> + dso__put(di_cache.dso);
> + di_cache.dso = dso__get(map__dso(ms->map));
> +
> + debuginfo__delete(di_cache.dbg);
> + di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
> + }
> +
> + if (di_cache.dbg == NULL) {
> + ann_data_stat.no_dbginfo++;
> + return NULL;
> + }
> +
> /* Make sure it has the disasm of the function */
> if (symbol__annotate(ms, evsel, &arch) < 0) {
> ann_data_stat.no_insn++;
> @@ -2391,6 +2419,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> .ip = ms->sym->start + dl->al.offset,
> .cpumode = he->cpumode,
> .op = op_loc,
> + .di = di_cache.dbg,
> };
>
> if (!op_loc->mem_ref && op_loc->segment == INSN_SEG_NONE)
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index d5c821c22f79..0686af637ed9 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -540,4 +540,6 @@ struct annotated_basic_block {
> int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
> struct list_head *head);
>
> +void debuginfo_cache__delete(void);
> +
> #endif /* __PERF_ANNOTATE_H */
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 5596bed1b8c8..f9072e003367 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -36,6 +36,7 @@
> #include "util.h"
> #include "arch/common.h"
> #include "units.h"
> +#include "annotate.h"
> #include <internal/lib.h>
>
> #ifdef HAVE_ZSTD_SUPPORT
> @@ -304,6 +305,7 @@ void perf_session__delete(struct perf_session *session)
> return;
> auxtrace__free(session);
> auxtrace_index__free(&session->auxtrace_index);
> + debuginfo_cache__delete();
> perf_session__destroy_kernel_maps(session);
> perf_decomp__release_events(session->decomp_data.decomp);
> perf_env__exit(&session->header.env);
> --
> 2.45.2.803.g4e1b14247a-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf annotate: Cache debuginfo for data type profiling
2024-07-12 5:29 ` Ian Rogers
@ 2024-07-12 6:04 ` Namhyung Kim
0 siblings, 0 replies; 3+ messages in thread
From: Namhyung Kim @ 2024-07-12 6:04 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
Athira Rajeev
Hi Ian,
On Thu, Jul 11, 2024 at 10:29 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Jul 3, 2024 at 6:58 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > In find_data_type(), it creates and deletes a debug info whenver it
> > tries to find data type for a sample. This is inefficient and it most
> > likely accesses the same binary again and again.
> >
> > Let's add a single entry cache the debug info structure for the last DSO.
> > Depending on sample data, it usually gives me 2~3x (and sometimes more)
> > speed ups.
> >
> > Note that this will introduce a little difference in the output due to
> > the order of checking stack operations. It used to check the stack ops
> > before checking the availability of debug info but I moved it after the
> > symbol check. So it'll report stack operations in DSOs without debug
> > info as unknown. But I think it's ok and better to have the checking
> > near the caching logic.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/annotate-data.c | 15 ++-------------
> > tools/perf/util/annotate-data.h | 2 +-
> > tools/perf/util/annotate.c | 29 +++++++++++++++++++++++++++++
> > tools/perf/util/annotate.h | 2 ++
> > tools/perf/util/session.c | 2 ++
> > 5 files changed, 36 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index 965da6c0b542..24c03475b3f2 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> > @@ -1764,16 +1764,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
> > */
> > struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> > {
> > - struct annotated_data_type *result = NULL;
> > struct dso *dso = map__dso(dloc->ms->map);
> > Dwarf_Die type_die;
> >
> > - dloc->di = debuginfo__new(dso__long_name(dso));
> > - if (dloc->di == NULL) {
> > - pr_debug_dtp("cannot get the debug info\n");
> > - return NULL;
> > - }
> > -
> > /*
> > * The type offset is the same as instruction offset by default.
> > * But when finding a global variable, the offset won't be valid.
> > @@ -1783,13 +1776,9 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> > dloc->fbreg = -1;
> >
> > if (find_data_type_die(dloc, &type_die) < 0)
> > - goto out;
> > -
> > - result = dso__findnew_data_type(dso, &type_die);
> > + return NULL;
> >
> > -out:
> > - debuginfo__delete(dloc->di);
> > - return result;
> > + return dso__findnew_data_type(dso, &type_die);
> > }
> >
> > static int alloc_data_type_histograms(struct annotated_data_type *adt, int nr_entries)
> > diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> > index 0a57d9f5ee78..40c9377fa1a5 100644
> > --- a/tools/perf/util/annotate-data.h
> > +++ b/tools/perf/util/annotate-data.h
> > @@ -100,9 +100,9 @@ struct data_loc_info {
> > u64 var_addr;
> > u8 cpumode;
> > struct annotated_op_loc *op;
> > + struct debuginfo *di;
> >
> > /* These are used internally */
> > - struct debuginfo *di;
> > int fbreg;
> > bool fb_cfa;
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 1451caf25e77..821e4b2c3bcd 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -25,6 +25,7 @@
> > #include "srcline.h"
> > #include "units.h"
> > #include "debug.h"
> > +#include "debuginfo.h"
> > #include "annotate.h"
> > #include "annotate-data.h"
> > #include "evsel.h"
> > @@ -2313,6 +2314,20 @@ u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
> > return map__rip_2objdump(ms->map, addr);
> > }
> >
> > +static struct debuginfo_cache {
> > + struct dso *dso;
> > + struct debuginfo *dbg;
> > +} di_cache;
> > +
> > +void debuginfo_cache__delete(void)
> > +{
> > + dso__put(di_cache.dso);
> > + di_cache.dso = NULL;
> > +
> > + debuginfo__delete(di_cache.dbg);
> > + di_cache.dbg = NULL;
> > +}
> > +
> > /**
> > * hist_entry__get_data_type - find data type for given hist entry
> > * @he: hist entry
> > @@ -2347,6 +2362,19 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> > return NULL;
> > }
> >
> > + if (map__dso(ms->map) != di_cache.dso) {
>
> Does the di_cache need a mutex? Otherwise another thread may be in
> this same code racing with it.
The whole data type profiling code assumes single threaded
so I don't think we need a mutex just for this di_cache.
But I can add an assertion in the code to make it explicit.
Thanks for your review!
Namhyung
>
> > + dso__put(di_cache.dso);
> > + di_cache.dso = dso__get(map__dso(ms->map));
> > +
> > + debuginfo__delete(di_cache.dbg);
> > + di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
> > + }
> > +
> > + if (di_cache.dbg == NULL) {
> > + ann_data_stat.no_dbginfo++;
> > + return NULL;
> > + }
> > +
> > /* Make sure it has the disasm of the function */
> > if (symbol__annotate(ms, evsel, &arch) < 0) {
> > ann_data_stat.no_insn++;
> > @@ -2391,6 +2419,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> > .ip = ms->sym->start + dl->al.offset,
> > .cpumode = he->cpumode,
> > .op = op_loc,
> > + .di = di_cache.dbg,
> > };
> >
> > if (!op_loc->mem_ref && op_loc->segment == INSN_SEG_NONE)
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index d5c821c22f79..0686af637ed9 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -540,4 +540,6 @@ struct annotated_basic_block {
> > int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
> > struct list_head *head);
> >
> > +void debuginfo_cache__delete(void);
> > +
> > #endif /* __PERF_ANNOTATE_H */
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 5596bed1b8c8..f9072e003367 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -36,6 +36,7 @@
> > #include "util.h"
> > #include "arch/common.h"
> > #include "units.h"
> > +#include "annotate.h"
> > #include <internal/lib.h>
> >
> > #ifdef HAVE_ZSTD_SUPPORT
> > @@ -304,6 +305,7 @@ void perf_session__delete(struct perf_session *session)
> > return;
> > auxtrace__free(session);
> > auxtrace_index__free(&session->auxtrace_index);
> > + debuginfo_cache__delete();
> > perf_session__destroy_kernel_maps(session);
> > perf_decomp__release_events(session->decomp_data.decomp);
> > perf_env__exit(&session->header.env);
> > --
> > 2.45.2.803.g4e1b14247a-goog
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-07-12 6:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-04 1:58 [PATCH] perf annotate: Cache debuginfo for data type profiling Namhyung Kim
2024-07-12 5:29 ` Ian Rogers
2024-07-12 6:04 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).