Re: [PATCH] perf annotate: Cache debuginfo for data type profiling

[Namhyung Kim <namhyung@kernel.org>]

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
> >