* [PATCH 1/5] perf annotate: Split struct cycles_info
2023-11-02 22:26 [PATCHSET 0/5] perf annotate: Reduce memory footprint (v1) Namhyung Kim
@ 2023-11-02 22:26 ` Namhyung Kim
2023-11-02 22:52 ` Ian Rogers
2023-11-02 22:26 ` [PATCH 2/5] perf annotate: Split struct annotated_branch Namhyung Kim
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Namhyung Kim @ 2023-11-02 22:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
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@kernel.org>
---
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)
+ 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] perf annotate: Split struct cycles_info
2023-11-02 22:26 ` [PATCH 1/5] perf annotate: Split struct cycles_info Namhyung Kim
@ 2023-11-02 22:52 ` Ian Rogers
2023-11-03 18:48 ` Namhyung Kim
0 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2023-11-02 22:52 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Thu, Nov 2, 2023 at 3:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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@kernel.org>
> ---
> 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)
> + 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;
Thanks for doing this! Would it make sense to do the zalloc here to be
lazier about allocation?
Ian
> + 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] perf annotate: Split struct cycles_info
2023-11-02 22:52 ` Ian Rogers
@ 2023-11-03 18:48 ` Namhyung Kim
0 siblings, 0 replies; 16+ messages in thread
From: Namhyung Kim @ 2023-11-03 18:48 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
Hi Ian,
On Thu, Nov 2, 2023 at 3:53 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 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@kernel.org>
> > ---
> > 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)
> > + 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;
>
> Thanks for doing this! Would it make sense to do the zalloc here to be
> lazier about allocation?
annotation__count_and_fill() also needs it.
Thanks,
Namhyung
>
> Ian
>
> > + 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;
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] perf annotate: Split struct annotated_branch
2023-11-02 22:26 [PATCHSET 0/5] perf annotate: Reduce memory footprint (v1) Namhyung Kim
2023-11-02 22:26 ` [PATCH 1/5] perf annotate: Split struct cycles_info Namhyung Kim
@ 2023-11-02 22:26 ` Namhyung Kim
2023-11-02 22:58 ` Ian Rogers
2023-11-02 22:26 ` [PATCH 3/5] perf annotate: Move max_coverage to annotated_branch Namhyung Kim
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Namhyung Kim @ 2023-11-02 22:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
The cycles info is only meaninful when sample has branch stacks. To
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 {
u64 hit_cycles;
u64 hit_insn;
unsigned int total_insn;
unsigned int cover_insn;
+ struct cyc_hist *cycles_hist;
+};
+
+struct LOCKABLE annotation {
+ u64 max_coverage;
+ u64 start;
struct annotation_options *options;
struct annotation_line **offsets;
int nr_events;
@@ -297,8 +300,8 @@ struct LOCKABLE annotation {
u8 max_addr;
u8 max_ins_name;
} widths;
- bool have_cycles;
struct annotated_source *src;
+ struct annotated_branch *branch;
};
static inline void annotation__init(struct annotation *notes __maybe_unused)
@@ -312,10 +315,10 @@ bool annotation__trylock(struct annotation *notes) EXCLUSIVE_TRYLOCK_FUNCTION(tr
static inline int annotation__cycles_width(struct annotation *notes)
{
- if (notes->have_cycles && notes->options->show_minmax_cycle)
+ if (notes->branch && notes->options->show_minmax_cycle)
return ANNOTATION__IPC_WIDTH + ANNOTATION__MINMAX_CYCLES_WIDTH;
- return notes->have_cycles ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
+ return notes->branch ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
}
static inline int annotation__pcnt_width(struct annotation *notes)
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 591fc1edd385..08f82c1f166c 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -129,9 +129,9 @@ int block_info__process_sym(struct hist_entry *he, struct block_hist *bh,
al.sym = he->ms.sym;
notes = symbol__annotation(he->ms.sym);
- if (!notes || !notes->src || !notes->src->cycles_hist)
+ if (!notes || !notes->branch || !notes->branch->cycles_hist)
return 0;
- ch = notes->src->cycles_hist;
+ ch = notes->branch->cycles_hist;
for (unsigned int i = 0; i < symbol__size(he->ms.sym); i++) {
if (ch[i].num_aggr) {
struct block_info *bi;
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 80e4f6132740..27b123ccd2d1 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -583,21 +583,21 @@ static int hist_entry__sym_ipc_snprintf(struct hist_entry *he, char *bf,
{
struct symbol *sym = he->ms.sym;
- struct annotation *notes;
+ struct annotated_branch *branch;
double ipc = 0.0, coverage = 0.0;
char tmp[64];
if (!sym)
return repsep_snprintf(bf, size, "%-*s", width, "-");
- notes = symbol__annotation(sym);
+ branch = symbol__annotation(sym)->branch;
- 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);
}
snprintf(tmp, sizeof(tmp), "%-5.2f [%5.1f%%]", ipc, coverage);
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] perf annotate: Split struct annotated_branch
2023-11-02 22:26 ` [PATCH 2/5] perf annotate: Split struct annotated_branch Namhyung Kim
@ 2023-11-02 22:58 ` Ian Rogers
2023-11-03 18:49 ` Namhyung Kim
0 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2023-11-02 22:58 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
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
> 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.
Thanks,
Ian
> u64 hit_cycles;
> u64 hit_insn;
> unsigned int total_insn;
> unsigned int cover_insn;
> + struct cyc_hist *cycles_hist;
> +};
> +
> +struct LOCKABLE annotation {
> + u64 max_coverage;
> + u64 start;
> struct annotation_options *options;
> struct annotation_line **offsets;
> int nr_events;
> @@ -297,8 +300,8 @@ struct LOCKABLE annotation {
> u8 max_addr;
> u8 max_ins_name;
> } widths;
> - bool have_cycles;
> struct annotated_source *src;
> + struct annotated_branch *branch;
> };
>
> static inline void annotation__init(struct annotation *notes __maybe_unused)
> @@ -312,10 +315,10 @@ bool annotation__trylock(struct annotation *notes) EXCLUSIVE_TRYLOCK_FUNCTION(tr
>
> static inline int annotation__cycles_width(struct annotation *notes)
> {
> - if (notes->have_cycles && notes->options->show_minmax_cycle)
> + if (notes->branch && notes->options->show_minmax_cycle)
> return ANNOTATION__IPC_WIDTH + ANNOTATION__MINMAX_CYCLES_WIDTH;
>
> - return notes->have_cycles ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
> + return notes->branch ? ANNOTATION__IPC_WIDTH + ANNOTATION__CYCLES_WIDTH : 0;
> }
>
> static inline int annotation__pcnt_width(struct annotation *notes)
> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
> index 591fc1edd385..08f82c1f166c 100644
> --- a/tools/perf/util/block-info.c
> +++ b/tools/perf/util/block-info.c
> @@ -129,9 +129,9 @@ int block_info__process_sym(struct hist_entry *he, struct block_hist *bh,
> al.sym = he->ms.sym;
>
> notes = symbol__annotation(he->ms.sym);
> - if (!notes || !notes->src || !notes->src->cycles_hist)
> + if (!notes || !notes->branch || !notes->branch->cycles_hist)
> return 0;
> - ch = notes->src->cycles_hist;
> + ch = notes->branch->cycles_hist;
> for (unsigned int i = 0; i < symbol__size(he->ms.sym); i++) {
> if (ch[i].num_aggr) {
> struct block_info *bi;
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 80e4f6132740..27b123ccd2d1 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -583,21 +583,21 @@ static int hist_entry__sym_ipc_snprintf(struct hist_entry *he, char *bf,
> {
>
> struct symbol *sym = he->ms.sym;
> - struct annotation *notes;
> + struct annotated_branch *branch;
> double ipc = 0.0, coverage = 0.0;
> char tmp[64];
>
> if (!sym)
> return repsep_snprintf(bf, size, "%-*s", width, "-");
>
> - notes = symbol__annotation(sym);
> + branch = symbol__annotation(sym)->branch;
>
> - 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);
> }
>
> snprintf(tmp, sizeof(tmp), "%-5.2f [%5.1f%%]", ipc, coverage);
> --
> 2.42.0.869.gea05f2083d-goog
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] perf annotate: Split struct annotated_branch
2023-11-02 22:58 ` Ian Rogers
@ 2023-11-03 18:49 ` Namhyung Kim
0 siblings, 0 replies; 16+ messages in thread
From: Namhyung Kim @ 2023-11-03 18:49 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
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;
> > +};
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] perf annotate: Move max_coverage to annotated_branch
2023-11-02 22:26 [PATCHSET 0/5] perf annotate: Reduce memory footprint (v1) Namhyung Kim
2023-11-02 22:26 ` [PATCH 1/5] perf annotate: Split struct cycles_info Namhyung Kim
2023-11-02 22:26 ` [PATCH 2/5] perf annotate: Split struct annotated_branch Namhyung Kim
@ 2023-11-02 22:26 ` Namhyung Kim
2023-11-02 22:59 ` Ian Rogers
2023-11-02 22:26 ` [PATCH 4/5] perf annotate: Move some fields to annotated_source Namhyung Kim
2023-11-02 22:26 ` [PATCH 5/5] perf annotate: Move offsets " Namhyung Kim
4 siblings, 1 reply; 16+ messages in thread
From: Namhyung Kim @ 2023-11-02 22:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
The max_coverage is only used when branch stack info is available so
it'd be natural to move to the annotated_branch.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-annotate.c | 7 +++++--
tools/perf/util/annotate.c | 2 +-
tools/perf/util/annotate.h | 4 +++-
tools/perf/util/block-range.c | 7 ++++++-
4 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index aeeb801f1ed7..a9129b51d511 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -94,6 +94,7 @@ static void process_basic_block(struct addr_map_symbol *start,
struct annotation *notes = sym ? symbol__annotation(sym) : NULL;
struct block_range_iter iter;
struct block_range *entry;
+ struct annotated_branch *branch;
/*
* Sanity; NULL isn't executable and the CPU cannot execute backwards
@@ -105,6 +106,8 @@ static void process_basic_block(struct addr_map_symbol *start,
if (!block_range_iter__valid(&iter))
return;
+ branch = annotation__get_branch(notes);
+
/*
* First block in range is a branch target.
*/
@@ -118,8 +121,8 @@ static void process_basic_block(struct addr_map_symbol *start,
entry->coverage++;
entry->sym = sym;
- if (notes)
- notes->max_coverage = max(notes->max_coverage, entry->coverage);
+ if (branch)
+ branch->max_coverage = max(branch->max_coverage, entry->coverage);
} while (block_range_iter__next(&iter));
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 2fa1ce3a0858..92a9adf9d5eb 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -946,7 +946,7 @@ static int __symbol__inc_addr_samples(struct map_symbol *ms,
return 0;
}
-static struct annotated_branch *annotation__get_branch(struct annotation *notes)
+struct annotated_branch *annotation__get_branch(struct annotation *notes)
{
if (notes == NULL)
return NULL;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 9c199629305d..d8a221591926 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -280,10 +280,10 @@ struct annotated_branch {
unsigned int total_insn;
unsigned int cover_insn;
struct cyc_hist *cycles_hist;
+ u64 max_coverage;
};
struct LOCKABLE annotation {
- u64 max_coverage;
u64 start;
struct annotation_options *options;
struct annotation_line **offsets;
@@ -356,6 +356,8 @@ static inline struct annotation *symbol__annotation(struct symbol *sym)
int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
struct evsel *evsel);
+struct annotated_branch *annotation__get_branch(struct annotation *notes);
+
int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
struct addr_map_symbol *start,
unsigned cycles);
diff --git a/tools/perf/util/block-range.c b/tools/perf/util/block-range.c
index 680e92774d0c..15c42196c24c 100644
--- a/tools/perf/util/block-range.c
+++ b/tools/perf/util/block-range.c
@@ -311,6 +311,7 @@ struct block_range_iter block_range__create(u64 start, u64 end)
double block_range__coverage(struct block_range *br)
{
struct symbol *sym;
+ struct annotated_branch *branch;
if (!br) {
if (block_ranges.blocks)
@@ -323,5 +324,9 @@ double block_range__coverage(struct block_range *br)
if (!sym)
return -1;
- return (double)br->coverage / symbol__annotation(sym)->max_coverage;
+ branch = symbol__annotation(sym)->branch;
+ if (!branch)
+ return -1;
+
+ return (double)br->coverage / branch->max_coverage;
}
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/5] perf annotate: Move max_coverage to annotated_branch
2023-11-02 22:26 ` [PATCH 3/5] perf annotate: Move max_coverage to annotated_branch Namhyung Kim
@ 2023-11-02 22:59 ` Ian Rogers
0 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2023-11-02 22:59 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Thu, Nov 2, 2023 at 3:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The max_coverage is only used when branch stack info is available so
> it'd be natural to move to the annotated_branch.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/builtin-annotate.c | 7 +++++--
> tools/perf/util/annotate.c | 2 +-
> tools/perf/util/annotate.h | 4 +++-
> tools/perf/util/block-range.c | 7 ++++++-
> 4 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index aeeb801f1ed7..a9129b51d511 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -94,6 +94,7 @@ static void process_basic_block(struct addr_map_symbol *start,
> struct annotation *notes = sym ? symbol__annotation(sym) : NULL;
> struct block_range_iter iter;
> struct block_range *entry;
> + struct annotated_branch *branch;
>
> /*
> * Sanity; NULL isn't executable and the CPU cannot execute backwards
> @@ -105,6 +106,8 @@ static void process_basic_block(struct addr_map_symbol *start,
> if (!block_range_iter__valid(&iter))
> return;
>
> + branch = annotation__get_branch(notes);
> +
> /*
> * First block in range is a branch target.
> */
> @@ -118,8 +121,8 @@ static void process_basic_block(struct addr_map_symbol *start,
> entry->coverage++;
> entry->sym = sym;
>
> - if (notes)
> - notes->max_coverage = max(notes->max_coverage, entry->coverage);
> + if (branch)
> + branch->max_coverage = max(branch->max_coverage, entry->coverage);
>
> } while (block_range_iter__next(&iter));
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 2fa1ce3a0858..92a9adf9d5eb 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -946,7 +946,7 @@ static int __symbol__inc_addr_samples(struct map_symbol *ms,
> return 0;
> }
>
> -static struct annotated_branch *annotation__get_branch(struct annotation *notes)
> +struct annotated_branch *annotation__get_branch(struct annotation *notes)
> {
> if (notes == NULL)
> return NULL;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 9c199629305d..d8a221591926 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -280,10 +280,10 @@ struct annotated_branch {
> unsigned int total_insn;
> unsigned int cover_insn;
> struct cyc_hist *cycles_hist;
> + u64 max_coverage;
> };
>
> struct LOCKABLE annotation {
> - u64 max_coverage;
> u64 start;
> struct annotation_options *options;
> struct annotation_line **offsets;
> @@ -356,6 +356,8 @@ static inline struct annotation *symbol__annotation(struct symbol *sym)
> int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
> struct evsel *evsel);
>
> +struct annotated_branch *annotation__get_branch(struct annotation *notes);
> +
> int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
> struct addr_map_symbol *start,
> unsigned cycles);
> diff --git a/tools/perf/util/block-range.c b/tools/perf/util/block-range.c
> index 680e92774d0c..15c42196c24c 100644
> --- a/tools/perf/util/block-range.c
> +++ b/tools/perf/util/block-range.c
> @@ -311,6 +311,7 @@ struct block_range_iter block_range__create(u64 start, u64 end)
> double block_range__coverage(struct block_range *br)
> {
> struct symbol *sym;
> + struct annotated_branch *branch;
>
> if (!br) {
> if (block_ranges.blocks)
> @@ -323,5 +324,9 @@ double block_range__coverage(struct block_range *br)
> if (!sym)
> return -1;
>
> - return (double)br->coverage / symbol__annotation(sym)->max_coverage;
> + branch = symbol__annotation(sym)->branch;
> + if (!branch)
> + return -1;
> +
> + return (double)br->coverage / branch->max_coverage;
> }
> --
> 2.42.0.869.gea05f2083d-goog
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/5] perf annotate: Move some fields to annotated_source
2023-11-02 22:26 [PATCHSET 0/5] perf annotate: Reduce memory footprint (v1) Namhyung Kim
` (2 preceding siblings ...)
2023-11-02 22:26 ` [PATCH 3/5] perf annotate: Move max_coverage to annotated_branch Namhyung Kim
@ 2023-11-02 22:26 ` Namhyung Kim
2023-11-02 23:00 ` Ian Rogers
2023-11-03 5:41 ` Christophe JAILLET
2023-11-02 22:26 ` [PATCH 5/5] perf annotate: Move offsets " Namhyung Kim
4 siblings, 2 replies; 16+ messages in thread
From: Namhyung Kim @ 2023-11-02 22:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Some fields in the struct annotation are only used with annotated_source
so better to be moved there in order to reduce memory consumption for
other symbols.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 12 ++++++------
tools/perf/util/annotate.c | 17 +++++++++--------
tools/perf/util/annotate.h | 14 +++++++-------
3 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d2470f87344d..1b42db70c998 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -384,7 +384,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
if (al->idx_asm < offset)
offset = al->idx;
- browser->b.nr_entries = notes->nr_entries;
+ browser->b.nr_entries = notes->src->nr_entries;
notes->options->hide_src_code = false;
browser->b.seek(&browser->b, -offset, SEEK_CUR);
browser->b.top_idx = al->idx - offset;
@@ -402,7 +402,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
if (al->idx_asm < offset)
offset = al->idx_asm;
- browser->b.nr_entries = notes->nr_asm_entries;
+ browser->b.nr_entries = notes->src->nr_asm_entries;
notes->options->hide_src_code = true;
browser->b.seek(&browser->b, -offset, SEEK_CUR);
browser->b.top_idx = al->idx_asm - offset;
@@ -435,7 +435,7 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
{
struct annotation *notes = browser__annotation(browser);
ui_browser__reset_index(browser);
- browser->nr_entries = notes->nr_asm_entries;
+ browser->nr_entries = notes->src->nr_asm_entries;
}
static int sym_title(struct symbol *sym, struct map *map, char *title,
@@ -860,7 +860,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
browser->b.height,
browser->b.index,
browser->b.top_idx,
- notes->nr_asm_entries);
+ notes->src->nr_asm_entries);
}
continue;
case K_ENTER:
@@ -991,8 +991,8 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
ui_helpline__push("Press ESC to exit");
- browser.b.width = notes->max_line_len;
- browser.b.nr_entries = notes->nr_entries;
+ browser.b.width = notes->src->max_line_len;
+ browser.b.nr_entries = notes->src->nr_entries;
browser.b.entries = ¬es->src->source,
browser.b.width += 18; /* Percentage */
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 92a9adf9d5eb..ee7b8e1848a8 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2808,19 +2808,20 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
void annotation__set_offsets(struct annotation *notes, s64 size)
{
struct annotation_line *al;
+ struct annotated_source *src = notes->src;
- notes->max_line_len = 0;
- notes->nr_entries = 0;
- notes->nr_asm_entries = 0;
+ src->max_line_len = 0;
+ src->nr_entries = 0;
+ src->nr_asm_entries = 0;
- list_for_each_entry(al, ¬es->src->source, node) {
+ list_for_each_entry(al, &src->source, node) {
size_t line_len = strlen(al->line);
- if (notes->max_line_len < line_len)
- notes->max_line_len = line_len;
- al->idx = notes->nr_entries++;
+ if (src->max_line_len < line_len)
+ src->max_line_len = line_len;
+ al->idx = src->nr_entries++;
if (al->offset != -1) {
- al->idx_asm = notes->nr_asm_entries++;
+ al->idx_asm = src->nr_asm_entries++;
/*
* FIXME: short term bandaid to cope with assembly
* routines that comes with labels in the same column
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index d8a221591926..c51ceb857bd6 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -268,10 +268,13 @@ struct cyc_hist {
* returns.
*/
struct annotated_source {
- struct list_head source;
- int nr_histograms;
- size_t sizeof_sym_hist;
- struct sym_hist *histograms;
+ struct list_head source;
+ int nr_histograms;
+ size_t sizeof_sym_hist;
+ struct sym_hist *histograms;
+ int nr_entries;
+ int nr_asm_entries;
+ u16 max_line_len;
};
struct annotated_branch {
@@ -289,9 +292,6 @@ struct LOCKABLE annotation {
struct annotation_line **offsets;
int nr_events;
int max_jump_sources;
- int nr_entries;
- int nr_asm_entries;
- u16 max_line_len;
struct {
u8 addr;
u8 jumps;
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] perf annotate: Move some fields to annotated_source
2023-11-02 22:26 ` [PATCH 4/5] perf annotate: Move some fields to annotated_source Namhyung Kim
@ 2023-11-02 23:00 ` Ian Rogers
2023-11-03 5:41 ` Christophe JAILLET
1 sibling, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2023-11-02 23:00 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Thu, Nov 2, 2023 at 3:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Some fields in the struct annotation are only used with annotated_source
> so better to be moved there in order to reduce memory consumption for
> other symbols.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/ui/browsers/annotate.c | 12 ++++++------
> tools/perf/util/annotate.c | 17 +++++++++--------
> tools/perf/util/annotate.h | 14 +++++++-------
> 3 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index d2470f87344d..1b42db70c998 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -384,7 +384,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> if (al->idx_asm < offset)
> offset = al->idx;
>
> - browser->b.nr_entries = notes->nr_entries;
> + browser->b.nr_entries = notes->src->nr_entries;
> notes->options->hide_src_code = false;
> browser->b.seek(&browser->b, -offset, SEEK_CUR);
> browser->b.top_idx = al->idx - offset;
> @@ -402,7 +402,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> if (al->idx_asm < offset)
> offset = al->idx_asm;
>
> - browser->b.nr_entries = notes->nr_asm_entries;
> + browser->b.nr_entries = notes->src->nr_asm_entries;
> notes->options->hide_src_code = true;
> browser->b.seek(&browser->b, -offset, SEEK_CUR);
> browser->b.top_idx = al->idx_asm - offset;
> @@ -435,7 +435,7 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
> {
> struct annotation *notes = browser__annotation(browser);
> ui_browser__reset_index(browser);
> - browser->nr_entries = notes->nr_asm_entries;
> + browser->nr_entries = notes->src->nr_asm_entries;
> }
>
> static int sym_title(struct symbol *sym, struct map *map, char *title,
> @@ -860,7 +860,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> browser->b.height,
> browser->b.index,
> browser->b.top_idx,
> - notes->nr_asm_entries);
> + notes->src->nr_asm_entries);
> }
> continue;
> case K_ENTER:
> @@ -991,8 +991,8 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>
> ui_helpline__push("Press ESC to exit");
>
> - browser.b.width = notes->max_line_len;
> - browser.b.nr_entries = notes->nr_entries;
> + browser.b.width = notes->src->max_line_len;
> + browser.b.nr_entries = notes->src->nr_entries;
> browser.b.entries = ¬es->src->source,
> browser.b.width += 18; /* Percentage */
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 92a9adf9d5eb..ee7b8e1848a8 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2808,19 +2808,20 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
> void annotation__set_offsets(struct annotation *notes, s64 size)
> {
> struct annotation_line *al;
> + struct annotated_source *src = notes->src;
>
> - notes->max_line_len = 0;
> - notes->nr_entries = 0;
> - notes->nr_asm_entries = 0;
> + src->max_line_len = 0;
> + src->nr_entries = 0;
> + src->nr_asm_entries = 0;
>
> - list_for_each_entry(al, ¬es->src->source, node) {
> + list_for_each_entry(al, &src->source, node) {
> size_t line_len = strlen(al->line);
>
> - if (notes->max_line_len < line_len)
> - notes->max_line_len = line_len;
> - al->idx = notes->nr_entries++;
> + if (src->max_line_len < line_len)
> + src->max_line_len = line_len;
> + al->idx = src->nr_entries++;
> if (al->offset != -1) {
> - al->idx_asm = notes->nr_asm_entries++;
> + al->idx_asm = src->nr_asm_entries++;
> /*
> * FIXME: short term bandaid to cope with assembly
> * routines that comes with labels in the same column
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index d8a221591926..c51ceb857bd6 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -268,10 +268,13 @@ struct cyc_hist {
> * returns.
> */
> struct annotated_source {
> - struct list_head source;
> - int nr_histograms;
> - size_t sizeof_sym_hist;
> - struct sym_hist *histograms;
> + struct list_head source;
> + int nr_histograms;
> + size_t sizeof_sym_hist;
> + struct sym_hist *histograms;
> + int nr_entries;
> + int nr_asm_entries;
> + u16 max_line_len;
> };
>
> struct annotated_branch {
> @@ -289,9 +292,6 @@ struct LOCKABLE annotation {
> struct annotation_line **offsets;
> int nr_events;
> int max_jump_sources;
> - int nr_entries;
> - int nr_asm_entries;
> - u16 max_line_len;
> struct {
> u8 addr;
> u8 jumps;
> --
> 2.42.0.869.gea05f2083d-goog
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] perf annotate: Move some fields to annotated_source
2023-11-02 22:26 ` [PATCH 4/5] perf annotate: Move some fields to annotated_source Namhyung Kim
2023-11-02 23:00 ` Ian Rogers
@ 2023-11-03 5:41 ` Christophe JAILLET
2023-11-03 18:58 ` Namhyung Kim
1 sibling, 1 reply; 16+ messages in thread
From: Christophe JAILLET @ 2023-11-03 5:41 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Le 02/11/2023 à 23:26, Namhyung Kim a écrit :
> Some fields in the struct annotation are only used with annotated_source
> so better to be moved there in order to reduce memory consumption for
> other symbols.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/ui/browsers/annotate.c | 12 ++++++------
> tools/perf/util/annotate.c | 17 +++++++++--------
> tools/perf/util/annotate.h | 14 +++++++-------
> 3 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index d2470f87344d..1b42db70c998 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -384,7 +384,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> if (al->idx_asm < offset)
> offset = al->idx;
>
> - browser->b.nr_entries = notes->nr_entries;
> + browser->b.nr_entries = notes->src->nr_entries;
> notes->options->hide_src_code = false;
> browser->b.seek(&browser->b, -offset, SEEK_CUR);
> browser->b.top_idx = al->idx - offset;
> @@ -402,7 +402,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> if (al->idx_asm < offset)
> offset = al->idx_asm;
>
> - browser->b.nr_entries = notes->nr_asm_entries;
> + browser->b.nr_entries = notes->src->nr_asm_entries;
> notes->options->hide_src_code = true;
> browser->b.seek(&browser->b, -offset, SEEK_CUR);
> browser->b.top_idx = al->idx_asm - offset;
> @@ -435,7 +435,7 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
> {
> struct annotation *notes = browser__annotation(browser);
> ui_browser__reset_index(browser);
> - browser->nr_entries = notes->nr_asm_entries;
> + browser->nr_entries = notes->src->nr_asm_entries;
> }
>
> static int sym_title(struct symbol *sym, struct map *map, char *title,
> @@ -860,7 +860,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> browser->b.height,
> browser->b.index,
> browser->b.top_idx,
> - notes->nr_asm_entries);
> + notes->src->nr_asm_entries);
> }
> continue;
> case K_ENTER:
> @@ -991,8 +991,8 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>
> ui_helpline__push("Press ESC to exit");
>
> - browser.b.width = notes->max_line_len;
> - browser.b.nr_entries = notes->nr_entries;
> + browser.b.width = notes->src->max_line_len;
> + browser.b.nr_entries = notes->src->nr_entries;
> browser.b.entries = ¬es->src->source,
> browser.b.width += 18; /* Percentage */
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 92a9adf9d5eb..ee7b8e1848a8 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2808,19 +2808,20 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
> void annotation__set_offsets(struct annotation *notes, s64 size)
> {
> struct annotation_line *al;
> + struct annotated_source *src = notes->src;
>
> - notes->max_line_len = 0;
> - notes->nr_entries = 0;
> - notes->nr_asm_entries = 0;
> + src->max_line_len = 0;
> + src->nr_entries = 0;
> + src->nr_asm_entries = 0;
>
> - list_for_each_entry(al, ¬es->src->source, node) {
> + list_for_each_entry(al, &src->source, node) {
> size_t line_len = strlen(al->line);
>
> - if (notes->max_line_len < line_len)
> - notes->max_line_len = line_len;
> - al->idx = notes->nr_entries++;
> + if (src->max_line_len < line_len)
> + src->max_line_len = line_len;
> + al->idx = src->nr_entries++;
> if (al->offset != -1) {
> - al->idx_asm = notes->nr_asm_entries++;
> + al->idx_asm = src->nr_asm_entries++;
> /*
> * FIXME: short term bandaid to cope with assembly
> * routines that comes with labels in the same column
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index d8a221591926..c51ceb857bd6 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -268,10 +268,13 @@ struct cyc_hist {
> * returns.
> */
> struct annotated_source {
> - struct list_head source;
> - int nr_histograms;
> - size_t sizeof_sym_hist;
> - struct sym_hist *histograms;
> + struct list_head source;
> + int nr_histograms;
If this int...
> + size_t sizeof_sym_hist;
> + struct sym_hist *histograms;
> + int nr_entries;
> + int nr_asm_entries;
> + u16 max_line_len;
... and these int and u16 were grouped, you would also save a hole in
the struct and reduce padding.
On x86_64, after patch 4/5:
struct annotated_source {
struct list_head source; /* 0 16 */
int nr_histograms; /* 16 4 */
/* XXX 4 bytes hole, try to pack */ <====
size_t sizeof_sym_hist; /* 24 8 */
struct sym_hist * histograms; /* 32 8 */
int nr_entries; /* 40 4 */
int nr_asm_entries; /* 44 4 */
u16 max_line_len; /* 48 2 */
/* size: 56, cachelines: 1, members: 7 */
/* sum members: 46, holes: 1, sum holes: 4 */
/* padding: 6 */ <====
/* last cacheline: 56 bytes */
};
After patch 5/5, the struct would be just 64 bytes. If fields are
re-ordered, it would be 56 bytes.
Because of rounding in memory allocations, 56 or 64 shouldn't change
anything. But it would be more future proof, should something else be
added here in the future.
CJ
> };
>
> struct annotated_branch {
> @@ -289,9 +292,6 @@ struct LOCKABLE annotation {
> struct annotation_line **offsets;
> int nr_events;
> int max_jump_sources;
> - int nr_entries;
> - int nr_asm_entries;
> - u16 max_line_len;
> struct {
> u8 addr;
> u8 jumps;
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] perf annotate: Move some fields to annotated_source
2023-11-03 5:41 ` Christophe JAILLET
@ 2023-11-03 18:58 ` Namhyung Kim
0 siblings, 0 replies; 16+ messages in thread
From: Namhyung Kim @ 2023-11-03 18:58 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
Hello,
On Thu, Nov 2, 2023 at 10:41 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 02/11/2023 à 23:26, Namhyung Kim a écrit :
> > Some fields in the struct annotation are only used with annotated_source
> > so better to be moved there in order to reduce memory consumption for
> > other symbols.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/ui/browsers/annotate.c | 12 ++++++------
> > tools/perf/util/annotate.c | 17 +++++++++--------
> > tools/perf/util/annotate.h | 14 +++++++-------
> > 3 files changed, 22 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index d2470f87344d..1b42db70c998 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -384,7 +384,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> > if (al->idx_asm < offset)
> > offset = al->idx;
> >
> > - browser->b.nr_entries = notes->nr_entries;
> > + browser->b.nr_entries = notes->src->nr_entries;
> > notes->options->hide_src_code = false;
> > browser->b.seek(&browser->b, -offset, SEEK_CUR);
> > browser->b.top_idx = al->idx - offset;
> > @@ -402,7 +402,7 @@ static bool annotate_browser__toggle_source(struct annotate_browser *browser)
> > if (al->idx_asm < offset)
> > offset = al->idx_asm;
> >
> > - browser->b.nr_entries = notes->nr_asm_entries;
> > + browser->b.nr_entries = notes->src->nr_asm_entries;
> > notes->options->hide_src_code = true;
> > browser->b.seek(&browser->b, -offset, SEEK_CUR);
> > browser->b.top_idx = al->idx_asm - offset;
> > @@ -435,7 +435,7 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
> > {
> > struct annotation *notes = browser__annotation(browser);
> > ui_browser__reset_index(browser);
> > - browser->nr_entries = notes->nr_asm_entries;
> > + browser->nr_entries = notes->src->nr_asm_entries;
> > }
> >
> > static int sym_title(struct symbol *sym, struct map *map, char *title,
> > @@ -860,7 +860,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> > browser->b.height,
> > browser->b.index,
> > browser->b.top_idx,
> > - notes->nr_asm_entries);
> > + notes->src->nr_asm_entries);
> > }
> > continue;
> > case K_ENTER:
> > @@ -991,8 +991,8 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> >
> > ui_helpline__push("Press ESC to exit");
> >
> > - browser.b.width = notes->max_line_len;
> > - browser.b.nr_entries = notes->nr_entries;
> > + browser.b.width = notes->src->max_line_len;
> > + browser.b.nr_entries = notes->src->nr_entries;
> > browser.b.entries = ¬es->src->source,
> > browser.b.width += 18; /* Percentage */
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 92a9adf9d5eb..ee7b8e1848a8 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -2808,19 +2808,20 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
> > void annotation__set_offsets(struct annotation *notes, s64 size)
> > {
> > struct annotation_line *al;
> > + struct annotated_source *src = notes->src;
> >
> > - notes->max_line_len = 0;
> > - notes->nr_entries = 0;
> > - notes->nr_asm_entries = 0;
> > + src->max_line_len = 0;
> > + src->nr_entries = 0;
> > + src->nr_asm_entries = 0;
> >
> > - list_for_each_entry(al, ¬es->src->source, node) {
> > + list_for_each_entry(al, &src->source, node) {
> > size_t line_len = strlen(al->line);
> >
> > - if (notes->max_line_len < line_len)
> > - notes->max_line_len = line_len;
> > - al->idx = notes->nr_entries++;
> > + if (src->max_line_len < line_len)
> > + src->max_line_len = line_len;
> > + al->idx = src->nr_entries++;
> > if (al->offset != -1) {
> > - al->idx_asm = notes->nr_asm_entries++;
> > + al->idx_asm = src->nr_asm_entries++;
> > /*
> > * FIXME: short term bandaid to cope with assembly
> > * routines that comes with labels in the same column
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index d8a221591926..c51ceb857bd6 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -268,10 +268,13 @@ struct cyc_hist {
> > * returns.
> > */
> > struct annotated_source {
> > - struct list_head source;
> > - int nr_histograms;
> > - size_t sizeof_sym_hist;
> > - struct sym_hist *histograms;
> > + struct list_head source;
> > + int nr_histograms;
>
> If this int...
>
> > + size_t sizeof_sym_hist;
> > + struct sym_hist *histograms;
> > + int nr_entries;
> > + int nr_asm_entries;
> > + u16 max_line_len;
>
> ... and these int and u16 were grouped, you would also save a hole in
> the struct and reduce padding.
>
>
> On x86_64, after patch 4/5:
> struct annotated_source {
> struct list_head source; /* 0 16 */
> int nr_histograms; /* 16 4 */
>
> /* XXX 4 bytes hole, try to pack */ <====
>
> size_t sizeof_sym_hist; /* 24 8 */
> struct sym_hist * histograms; /* 32 8 */
> int nr_entries; /* 40 4 */
> int nr_asm_entries; /* 44 4 */
> u16 max_line_len; /* 48 2 */
>
> /* size: 56, cachelines: 1, members: 7 */
> /* sum members: 46, holes: 1, sum holes: 4 */
> /* padding: 6 */ <====
> /* last cacheline: 56 bytes */
> };
>
> After patch 5/5, the struct would be just 64 bytes. If fields are
> re-ordered, it would be 56 bytes.
>
> Because of rounding in memory allocations, 56 or 64 shouldn't change
> anything. But it would be more future proof, should something else be
> added here in the future.
Thanks for looking at this. I agree with your analysis and I will
reorder the struct. Actually I plan to make more changes here
so it's better to make it smaller.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/5] perf annotate: Move offsets to annotated_source
2023-11-02 22:26 [PATCHSET 0/5] perf annotate: Reduce memory footprint (v1) Namhyung Kim
` (3 preceding siblings ...)
2023-11-02 22:26 ` [PATCH 4/5] perf annotate: Move some fields to annotated_source Namhyung Kim
@ 2023-11-02 22:26 ` Namhyung Kim
2023-11-02 23:00 ` Ian Rogers
4 siblings, 1 reply; 16+ messages in thread
From: Namhyung Kim @ 2023-11-02 22:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ian Rogers, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
The offsets array keeps pointers to struct annotation_line which is
available in the annotated_source. Let's move it to there.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 4 ++--
tools/perf/util/annotate.c | 18 +++++++++---------
tools/perf/util/annotate.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 1b42db70c998..163f916fff68 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -188,7 +188,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
* name right after the '<' token and probably treating this like a
* 'call' instruction.
*/
- target = notes->offsets[cursor->ops.target.offset];
+ target = notes->src->offsets[cursor->ops.target.offset];
if (target == NULL) {
ui_helpline__printf("WARN: jump target inconsistency, press 'o', notes->offsets[%#x] = NULL\n",
cursor->ops.target.offset);
@@ -1006,6 +1006,6 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
out_free_offsets:
if(not_annotated)
- zfree(¬es->offsets);
+ zfree(¬es->src->offsets);
return ret;
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ee7b8e1848a8..8ab2e1cf63ea 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1075,7 +1075,7 @@ static unsigned annotation__count_insn(struct annotation *notes, u64 start, u64
u64 offset;
for (offset = start; offset <= end; offset++) {
- if (notes->offsets[offset])
+ if (notes->src->offsets[offset])
n_insn++;
}
return n_insn;
@@ -1105,7 +1105,7 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
return;
for (offset = start; offset <= end; offset++) {
- struct annotation_line *al = notes->offsets[offset];
+ struct annotation_line *al = notes->src->offsets[offset];
if (al && al->cycles && al->cycles->ipc == 0.0) {
al->cycles->ipc = ipc;
@@ -1142,7 +1142,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
if (ch && ch->cycles) {
struct annotation_line *al;
- al = notes->offsets[offset];
+ al = notes->src->offsets[offset];
if (al && al->cycles == NULL) {
al->cycles = zalloc(sizeof(*al->cycles));
if (al->cycles == NULL)
@@ -2783,7 +2783,7 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
return;
for (offset = 0; offset < size; ++offset) {
- struct annotation_line *al = notes->offsets[offset];
+ struct annotation_line *al = notes->src->offsets[offset];
struct disasm_line *dl;
dl = disasm_line(al);
@@ -2791,7 +2791,7 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
if (!disasm_line__is_valid_local_jump(dl, sym))
continue;
- al = notes->offsets[dl->ops.target.offset];
+ al = notes->src->offsets[dl->ops.target.offset];
/*
* FIXME: Oops, no jump target? Buggy disassembler? Or do we
@@ -2830,7 +2830,7 @@ void annotation__set_offsets(struct annotation *notes, s64 size)
* E.g. copy_user_generic_unrolled
*/
if (al->offset < size)
- notes->offsets[al->offset] = al;
+ notes->src->offsets[al->offset] = al;
} else
al->idx_asm = -1;
}
@@ -3263,8 +3263,8 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
size_t size = symbol__size(sym);
int nr_pcnt = 1, err;
- notes->offsets = zalloc(size * sizeof(struct annotation_line *));
- if (notes->offsets == NULL)
+ notes->src->offsets = zalloc(size * sizeof(struct annotation_line *));
+ if (notes->src->offsets == NULL)
return ENOMEM;
if (evsel__is_group_event(evsel))
@@ -3290,7 +3290,7 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
return 0;
out_free_offsets:
- zfree(¬es->offsets);
+ zfree(¬es->src->offsets);
return err;
}
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index c51ceb857bd6..44af7e71a204 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -272,6 +272,7 @@ struct annotated_source {
int nr_histograms;
size_t sizeof_sym_hist;
struct sym_hist *histograms;
+ struct annotation_line **offsets;
int nr_entries;
int nr_asm_entries;
u16 max_line_len;
@@ -289,7 +290,6 @@ struct annotated_branch {
struct LOCKABLE annotation {
u64 start;
struct annotation_options *options;
- struct annotation_line **offsets;
int nr_events;
int max_jump_sources;
struct {
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] perf annotate: Move offsets to annotated_source
2023-11-02 22:26 ` [PATCH 5/5] perf annotate: Move offsets " Namhyung Kim
@ 2023-11-02 23:00 ` Ian Rogers
0 siblings, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2023-11-02 23:00 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Thu, Nov 2, 2023 at 3:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The offsets array keeps pointers to struct annotation_line which is
> available in the annotated_source. Let's move it to there.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/ui/browsers/annotate.c | 4 ++--
> tools/perf/util/annotate.c | 18 +++++++++---------
> tools/perf/util/annotate.h | 2 +-
> 3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 1b42db70c998..163f916fff68 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -188,7 +188,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
> * name right after the '<' token and probably treating this like a
> * 'call' instruction.
> */
> - target = notes->offsets[cursor->ops.target.offset];
> + target = notes->src->offsets[cursor->ops.target.offset];
> if (target == NULL) {
> ui_helpline__printf("WARN: jump target inconsistency, press 'o', notes->offsets[%#x] = NULL\n",
> cursor->ops.target.offset);
> @@ -1006,6 +1006,6 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>
> out_free_offsets:
> if(not_annotated)
> - zfree(¬es->offsets);
> + zfree(¬es->src->offsets);
> return ret;
> }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index ee7b8e1848a8..8ab2e1cf63ea 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1075,7 +1075,7 @@ static unsigned annotation__count_insn(struct annotation *notes, u64 start, u64
> u64 offset;
>
> for (offset = start; offset <= end; offset++) {
> - if (notes->offsets[offset])
> + if (notes->src->offsets[offset])
> n_insn++;
> }
> return n_insn;
> @@ -1105,7 +1105,7 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
> return;
>
> for (offset = start; offset <= end; offset++) {
> - struct annotation_line *al = notes->offsets[offset];
> + struct annotation_line *al = notes->src->offsets[offset];
>
> if (al && al->cycles && al->cycles->ipc == 0.0) {
> al->cycles->ipc = ipc;
> @@ -1142,7 +1142,7 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
> if (ch && ch->cycles) {
> struct annotation_line *al;
>
> - al = notes->offsets[offset];
> + al = notes->src->offsets[offset];
> if (al && al->cycles == NULL) {
> al->cycles = zalloc(sizeof(*al->cycles));
> if (al->cycles == NULL)
> @@ -2783,7 +2783,7 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
> return;
>
> for (offset = 0; offset < size; ++offset) {
> - struct annotation_line *al = notes->offsets[offset];
> + struct annotation_line *al = notes->src->offsets[offset];
> struct disasm_line *dl;
>
> dl = disasm_line(al);
> @@ -2791,7 +2791,7 @@ void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym)
> if (!disasm_line__is_valid_local_jump(dl, sym))
> continue;
>
> - al = notes->offsets[dl->ops.target.offset];
> + al = notes->src->offsets[dl->ops.target.offset];
>
> /*
> * FIXME: Oops, no jump target? Buggy disassembler? Or do we
> @@ -2830,7 +2830,7 @@ void annotation__set_offsets(struct annotation *notes, s64 size)
> * E.g. copy_user_generic_unrolled
> */
> if (al->offset < size)
> - notes->offsets[al->offset] = al;
> + notes->src->offsets[al->offset] = al;
> } else
> al->idx_asm = -1;
> }
> @@ -3263,8 +3263,8 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
> size_t size = symbol__size(sym);
> int nr_pcnt = 1, err;
>
> - notes->offsets = zalloc(size * sizeof(struct annotation_line *));
> - if (notes->offsets == NULL)
> + notes->src->offsets = zalloc(size * sizeof(struct annotation_line *));
> + if (notes->src->offsets == NULL)
> return ENOMEM;
>
> if (evsel__is_group_event(evsel))
> @@ -3290,7 +3290,7 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
> return 0;
>
> out_free_offsets:
> - zfree(¬es->offsets);
> + zfree(¬es->src->offsets);
> return err;
> }
>
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index c51ceb857bd6..44af7e71a204 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -272,6 +272,7 @@ struct annotated_source {
> int nr_histograms;
> size_t sizeof_sym_hist;
> struct sym_hist *histograms;
> + struct annotation_line **offsets;
> int nr_entries;
> int nr_asm_entries;
> u16 max_line_len;
> @@ -289,7 +290,6 @@ struct annotated_branch {
> struct LOCKABLE annotation {
> u64 start;
> struct annotation_options *options;
> - struct annotation_line **offsets;
> int nr_events;
> int max_jump_sources;
> struct {
> --
> 2.42.0.869.gea05f2083d-goog
>
^ permalink raw reply [flat|nested] 16+ messages in thread