Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 3 Nov 2023 11:49:57 -0700 | Subject | Re: [PATCH 2/5] perf annotate: Split struct annotated_branch |
| |
On Thu, Nov 2, 2023 at 3:58 PM Ian Rogers <irogers@google.com> wrote: > > On Thu, Nov 2, 2023 at 3:26 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> > > --- > > 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 = ¬es->src->cycles_hist[offset]; > > + ch = ¬es->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; > > +};
| |