Re: [PATCH 2/5] perf annotate: Split struct annotated_branch

From: Namhyung Kim
Date: Fri Nov 03 2023 - 14:50:18 EST

Next message: Alexandre Belloni: "Re: [PATCH v4 0/6] i3c: master: svc: collection of bugs fixes"
Previous message: Philipp Stanner: "[PATCH v2] fs/nilfs2: use standard array-copy-function"
In reply to: Ian Rogers: "Re: [PATCH 2/5] perf annotate: Split struct annotated_branch"
Next in thread: Namhyung Kim: "[PATCH 3/5] perf annotate: Move max_coverage to annotated_branch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Nov 2, 2023 at 3:58 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Thu, Nov 2, 2023 at 3:26 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > The cycles info is only meaninful when sample has branch stacks. To
>
> nit: meaningful

Oops, will fix.

>
> > save the memory for normal cases, move those fields to annotated_branch
> > and dynamically allocate it when needed. Also move cycles_hist from
> > annotated_source as it's related here.
> >
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> > tools/perf/util/annotate.c | 97 ++++++++++++++++++++----------------
> > tools/perf/util/annotate.h | 17 ++++---
> > tools/perf/util/block-info.c | 4 +-
> > tools/perf/util/sort.c | 14 +++---
> > 4 files changed, 72 insertions(+), 60 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 3e7f75827270..2fa1ce3a0858 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -810,7 +810,6 @@ static __maybe_unused void annotated_source__delete(struct annotated_source *src
> > if (src == NULL)
> > return;
> > zfree(&src->histograms);
> > - zfree(&src->cycles_hist);
> > free(src);
> > }
> >
> > @@ -845,18 +844,6 @@ static int annotated_source__alloc_histograms(struct annotated_source *src,
> > return src->histograms ? 0 : -1;
> > }
> >
> > -/* The cycles histogram is lazily allocated. */
> > -static int symbol__alloc_hist_cycles(struct symbol *sym)
> > -{
> > - struct annotation *notes = symbol__annotation(sym);
> > - const size_t size = symbol__size(sym);
> > -
> > - notes->src->cycles_hist = calloc(size, sizeof(struct cyc_hist));
> > - if (notes->src->cycles_hist == NULL)
> > - return -1;
> > - return 0;
> > -}
> > -
> > void symbol__annotate_zero_histograms(struct symbol *sym)
> > {
> > struct annotation *notes = symbol__annotation(sym);
> > @@ -865,9 +852,10 @@ void symbol__annotate_zero_histograms(struct symbol *sym)
> > if (notes->src != NULL) {
> > memset(notes->src->histograms, 0,
> > notes->src->nr_histograms * notes->src->sizeof_sym_hist);
> > - if (notes->src->cycles_hist)
> > - memset(notes->src->cycles_hist, 0,
> > - symbol__size(sym) * sizeof(struct cyc_hist));
> > + }
> > + if (notes->branch && notes->branch->cycles_hist) {
> > + memset(notes->branch->cycles_hist, 0,
> > + symbol__size(sym) * sizeof(struct cyc_hist));
> > }
> > annotation__unlock(notes);
> > }
> > @@ -958,23 +946,33 @@ static int __symbol__inc_addr_samples(struct map_symbol *ms,
> > return 0;
> > }
> >
> > +static struct annotated_branch *annotation__get_branch(struct annotation *notes)
> > +{
> > + if (notes == NULL)
> > + return NULL;
> > +
> > + if (notes->branch == NULL)
> > + notes->branch = zalloc(sizeof(*notes->branch));
> > +
> > + return notes->branch;
> > +}
> > +
> > static struct cyc_hist *symbol__cycles_hist(struct symbol *sym)
> > {
> > struct annotation *notes = symbol__annotation(sym);
> > + struct annotated_branch *branch;
> >
> > - if (notes->src == NULL) {
> > - notes->src = annotated_source__new();
> > - if (notes->src == NULL)
> > - return NULL;
> > - goto alloc_cycles_hist;
> > - }
> > + branch = annotation__get_branch(notes);
> > + if (branch == NULL)
> > + return NULL;
> > +
> > + if (branch->cycles_hist == NULL) {
> > + const size_t size = symbol__size(sym);
> >
> > - if (!notes->src->cycles_hist) {
> > -alloc_cycles_hist:
> > - symbol__alloc_hist_cycles(sym);
> > + branch->cycles_hist = calloc(size, sizeof(struct cyc_hist));
> > }
> >
> > - return notes->src->cycles_hist;
> > + return branch->cycles_hist;
> > }
> >
> > struct annotated_source *symbol__hists(struct symbol *sym, int nr_hists)
> > @@ -1083,6 +1081,14 @@ static unsigned annotation__count_insn(struct annotation *notes, u64 start, u64
> > return n_insn;
> > }
> >
> > +static void annotated_branch__delete(struct annotated_branch *branch)
> > +{
> > + if (branch) {
> > + free(branch->cycles_hist);
> > + free(branch);
> > + }
> > +}
> > +
> > static void annotation__count_and_fill(struct annotation *notes, u64 start, u64 end, struct cyc_hist *ch)
> > {
> > unsigned n_insn;
> > @@ -1091,6 +1097,7 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
> >
> > n_insn = annotation__count_insn(notes, start, end);
> > if (n_insn && ch->num && ch->cycles) {
> > + struct annotated_branch *branch;
> > float ipc = n_insn / ((double)ch->cycles / (double)ch->num);
> >
> > /* Hide data when there are too many overlaps. */
> > @@ -1106,10 +1113,11 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
> > }
> > }
> >
> > - if (cover_insn) {
> > - notes->hit_cycles += ch->cycles;
> > - notes->hit_insn += n_insn * ch->num;
> > - notes->cover_insn += cover_insn;
> > + branch = annotation__get_branch(notes);
> > + if (cover_insn && branch) {
> > + branch->hit_cycles += ch->cycles;
> > + branch->hit_insn += n_insn * ch->num;
> > + branch->cover_insn += cover_insn;
> > }
> > }
> > }
> > @@ -1118,19 +1126,19 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
> > {
> > s64 offset;
> >
> > - if (!notes->src || !notes->src->cycles_hist)
> > + if (!notes->branch || !notes->branch->cycles_hist)
> > return;
> >
> > - notes->total_insn = annotation__count_insn(notes, 0, size - 1);
> > - notes->hit_cycles = 0;
> > - notes->hit_insn = 0;
> > - notes->cover_insn = 0;
> > + notes->branch->total_insn = annotation__count_insn(notes, 0, size - 1);
> > + notes->branch->hit_cycles = 0;
> > + notes->branch->hit_insn = 0;
> > + notes->branch->cover_insn = 0;
> >
> > annotation__lock(notes);
> > for (offset = size - 1; offset >= 0; --offset) {
> > struct cyc_hist *ch;
> >
> > - ch = &notes->src->cycles_hist[offset];
> > + ch = &notes->branch->cycles_hist[offset];
> > if (ch && ch->cycles) {
> > struct annotation_line *al;
> >
> > @@ -1147,7 +1155,6 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
> > al->cycles->max = ch->cycles_max;
> > al->cycles->min = ch->cycles_min;
> > }
> > - notes->have_cycles = true;
> > }
> > }
> > annotation__unlock(notes);
> > @@ -1305,6 +1312,7 @@ int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool r
> > void annotation__exit(struct annotation *notes)
> > {
> > annotated_source__delete(notes->src);
> > + annotated_branch__delete(notes->branch);
> > }
> >
> > static struct sharded_mutex *sharded_mutex;
> > @@ -3058,13 +3066,14 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > static void ipc_coverage_string(char *bf, int size, struct annotation *notes)
> > {
> > double ipc = 0.0, coverage = 0.0;
> > + struct annotated_branch *branch = annotation__get_branch(notes);
> >
> > - if (notes->hit_cycles)
> > - ipc = notes->hit_insn / ((double)notes->hit_cycles);
> > + if (branch && branch->hit_cycles)
> > + ipc = branch->hit_insn / ((double)branch->hit_cycles);
> >
> > - if (notes->total_insn) {
> > - coverage = notes->cover_insn * 100.0 /
> > - ((double)notes->total_insn);
> > + if (branch && branch->total_insn) {
> > + coverage = branch->cover_insn * 100.0 /
> > + ((double)branch->total_insn);
> > }
> >
> > scnprintf(bf, size, "(Average IPC: %.2f, IPC Coverage: %.1f%%)",
> > @@ -3089,7 +3098,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > int printed;
> >
> > if (first_line && (al->offset == -1 || percent_max == 0.0)) {
> > - if (notes->have_cycles && al->cycles) {
> > + if (notes->branch && al->cycles) {
> > if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
> > show_title = true;
> > } else
> > @@ -3126,7 +3135,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > }
> > }
> >
> > - if (notes->have_cycles) {
> > + if (notes->branch) {
> > if (al->cycles && al->cycles->ipc)
> > obj__printf(obj, "%*.2f ", ANNOTATION__IPC_WIDTH - 1, al->cycles->ipc);
> > else if (!show_title)
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 16d27952fd5c..9c199629305d 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -271,17 +271,20 @@ struct annotated_source {
> > struct list_head source;
> > int nr_histograms;
> > size_t sizeof_sym_hist;
> > - struct cyc_hist *cycles_hist;
> > struct sym_hist *histograms;
> > };
> >
> > -struct LOCKABLE annotation {
> > - u64 max_coverage;
> > - u64 start;
> > +struct annotated_branch {
>
> nit: it'd be really cool to have more comments with these structs,
> explaining how they get used.

Yep, maybe a separate patch after the cleanup. :)

Thanks,
Namhyung

>
> > u64 hit_cycles;
> > u64 hit_insn;
> > unsigned int total_insn;
> > unsigned int cover_insn;
> > + struct cyc_hist *cycles_hist;
> > +};

Next message: Alexandre Belloni: "Re: [PATCH v4 0/6] i3c: master: svc: collection of bugs fixes"
Previous message: Philipp Stanner: "[PATCH v2] fs/nilfs2: use standard array-copy-function"
In reply to: Ian Rogers: "Re: [PATCH 2/5] perf annotate: Split struct annotated_branch"
Next in thread: Namhyung Kim: "[PATCH 3/5] perf annotate: Move max_coverage to annotated_branch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]