Re: [PATCH 1/5] perf annotate: Split struct cycles_info
From: Arnaldo Carvalho de Melo
Date: Mon Nov 06 2023 - 14:35:00 EST
Em Fri, Nov 03, 2023 at 12:19:03PM -0700, Namhyung Kim escreveu:
> The cycles info is used only when branch stack is provided. Split them
> into a separate struct and lazy allocate them to save some memory.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> tools/perf/ui/browsers/annotate.c | 2 +-
> tools/perf/util/annotate.c | 34 ++++++++++++++++++-------------
> tools/perf/util/annotate.h | 14 ++++++++-----
> 3 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index ccdb2cd11fbf..d2470f87344d 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -337,7 +337,7 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
> max_percent = percent;
> }
>
> - if (max_percent < 0.01 && pos->al.ipc == 0) {
> + if (max_percent < 0.01 && (!pos->al.cycles || pos->al.cycles->ipc == 0)) {
> RB_CLEAR_NODE(&pos->al.rb_node);
> continue;
> }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 82956adf9963..3e7f75827270 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1100,8 +1100,8 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
> for (offset = start; offset <= end; offset++) {
> struct annotation_line *al = notes->offsets[offset];
>
> - if (al && al->ipc == 0.0) {
> - al->ipc = ipc;
> + if (al && al->cycles && al->cycles->ipc == 0.0) {
> + al->cycles->ipc = ipc;
> cover_insn++;
> }
> }
> @@ -1134,13 +1134,18 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
> if (ch && ch->cycles) {
> struct annotation_line *al;
>
> + al = notes->offsets[offset];
> + if (al && al->cycles == NULL) {
> + al->cycles = zalloc(sizeof(*al->cycles));
> + if (al->cycles == NULL)
Shouldn't we stop here and tell the user that his system is really tight
on memory instead of just ignoring it?
if (al->cycles == NULL) {
ui__error("Not enough memory for allocating branch stack cycles info!\n");
return ... its a void function :-\
}
Since its a void function, can't we detect that we need al->cycles
allocated at the tool start and allocate it there, then propagate back
the error?
Its per line, so doing it lazily is indeed easier, so make that function
return an error and bail out when not being able to calculate the ipc
for the remaining lines?
- Arnaldo
> + continue;
> + }
> if (ch->have_start)
> annotation__count_and_fill(notes, ch->start, offset, ch);
> - al = notes->offsets[offset];
> if (al && ch->num_aggr) {
> - al->cycles = ch->cycles_aggr / ch->num_aggr;
> - al->cycles_max = ch->cycles_max;
> - al->cycles_min = ch->cycles_min;
> + al->cycles->avg = ch->cycles_aggr / ch->num_aggr;
> + al->cycles->max = ch->cycles_max;
> + al->cycles->min = ch->cycles_min;
> }
> notes->have_cycles = true;
> }
> @@ -1225,6 +1230,7 @@ static void annotation_line__exit(struct annotation_line *al)
> {
> zfree_srcline(&al->path);
> zfree(&al->line);
> + zfree(&al->cycles);
> }
>
> static size_t disasm_line_size(int nr)
> @@ -3083,8 +3089,8 @@ 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) {
> - if (al->ipc == 0.0 && al->cycles == 0)
> + if (notes->have_cycles && al->cycles) {
> + if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
> show_title = true;
> } else
> show_title = true;
> @@ -3121,17 +3127,17 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> }
>
> if (notes->have_cycles) {
> - if (al->ipc)
> - obj__printf(obj, "%*.2f ", ANNOTATION__IPC_WIDTH - 1, al->ipc);
> + if (al->cycles && al->cycles->ipc)
> + obj__printf(obj, "%*.2f ", ANNOTATION__IPC_WIDTH - 1, al->cycles->ipc);
> else if (!show_title)
> obj__printf(obj, "%*s", ANNOTATION__IPC_WIDTH, " ");
> else
> obj__printf(obj, "%*s ", ANNOTATION__IPC_WIDTH - 1, "IPC");
>
> if (!notes->options->show_minmax_cycle) {
> - if (al->cycles)
> + if (al->cycles && al->cycles->avg)
> obj__printf(obj, "%*" PRIu64 " ",
> - ANNOTATION__CYCLES_WIDTH - 1, al->cycles);
> + ANNOTATION__CYCLES_WIDTH - 1, al->cycles->avg);
> else if (!show_title)
> obj__printf(obj, "%*s",
> ANNOTATION__CYCLES_WIDTH, " ");
> @@ -3145,8 +3151,8 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
>
> scnprintf(str, sizeof(str),
> "%" PRIu64 "(%" PRIu64 "/%" PRIu64 ")",
> - al->cycles, al->cycles_min,
> - al->cycles_max);
> + al->cycles->avg, al->cycles->min,
> + al->cycles->max);
>
> obj__printf(obj, "%*s ",
> ANNOTATION__MINMAX_CYCLES_WIDTH - 1,
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 962780559176..16d27952fd5c 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -130,6 +130,13 @@ struct annotation_data {
> struct sym_hist_entry he;
> };
>
> +struct cycles_info {
> + float ipc;
> + u64 avg;
> + u64 max;
> + u64 min;
> +};
> +
> struct annotation_line {
> struct list_head node;
> struct rb_node rb_node;
> @@ -137,12 +144,9 @@ struct annotation_line {
> char *line;
> int line_nr;
> char *fileloc;
> - int jump_sources;
> - float ipc;
> - u64 cycles;
> - u64 cycles_max;
> - u64 cycles_min;
> char *path;
> + struct cycles_info *cycles;
> + int jump_sources;
> u32 idx;
> int idx_asm;
> int data_nr;
> --
> 2.42.0.869.gea05f2083d-goog
>
--
- Arnaldo