[PATCHSET 0/5] perf annotate: Reduce memory footprint (v2)

[PATCHSET 0/5] perf annotate: Reduce memory footprint (v2)

[Namhyung Kim]

Hello,

This is a part of my work to improve perf annotate.  At first, I'd
like reduce the size of struct annotation which will be allocated
together with struct symbol in some cases.  In fact, it doesn't use
most of them so it needs to slim down and lazy-allocate used part.

* v2 changes)
 - fix typo
 - reorder struct annotated_source
 - add Ian's Reviewed-by tags
 
With this applied, size of the struct goes down from 96 to 48.

The code is available at perf/annotate-diet-v2 branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (5):
  perf annotate: Split struct cycles_info
  perf annotate: Split struct annotated_branch
  perf annotate: Move max_coverage to annotated_branch
  perf annotate: Move some fields to annotated_source
  perf annotate: Move offsets to annotated_source

 tools/perf/builtin-annotate.c     |   7 +-
 tools/perf/ui/browsers/annotate.c |  18 ++--
 tools/perf/util/annotate.c        | 162 ++++++++++++++++--------------
 tools/perf/util/annotate.h        |  49 +++++----
 tools/perf/util/block-info.c      |   4 +-
 tools/perf/util/block-range.c     |   7 +-
 tools/perf/util/sort.c            |  14 +--
 7 files changed, 147 insertions(+), 114 deletions(-)

-- 
2.42.0.869.gea05f2083d-goog

[PATCH 1/5] perf annotate: Split struct cycles_info

[Namhyung Kim]

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

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

[Namhyung Kim]

The cycles info is only meaningful 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 = &notes->src->cycles_hist[offset];
+		ch = &notes->branch->cycles_hist[offset];
 		if (ch && ch->cycles) {
 			struct annotation_line *al;
 
@@ -1147,7 +1155,6 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
 				al->cycles->max = ch->cycles_max;
 				al->cycles->min = ch->cycles_min;
 			}
-			notes->have_cycles = true;
 		}
 	}
 	annotation__unlock(notes);
@@ -1305,6 +1312,7 @@ int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool r
 void annotation__exit(struct annotation *notes)
 {
 	annotated_source__delete(notes->src);
+	annotated_branch__delete(notes->branch);
 }
 
 static struct sharded_mutex *sharded_mutex;
@@ -3058,13 +3066,14 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
 static void ipc_coverage_string(char *bf, int size, struct annotation *notes)
 {
 	double ipc = 0.0, coverage = 0.0;
+	struct annotated_branch *branch = annotation__get_branch(notes);
 
-	if (notes->hit_cycles)
-		ipc = notes->hit_insn / ((double)notes->hit_cycles);
+	if (branch && branch->hit_cycles)
+		ipc = branch->hit_insn / ((double)branch->hit_cycles);
 
-	if (notes->total_insn) {
-		coverage = notes->cover_insn * 100.0 /
-			((double)notes->total_insn);
+	if (branch && branch->total_insn) {
+		coverage = branch->cover_insn * 100.0 /
+			((double)branch->total_insn);
 	}
 
 	scnprintf(bf, size, "(Average IPC: %.2f, IPC Coverage: %.1f%%)",
@@ -3089,7 +3098,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
 	int printed;
 
 	if (first_line && (al->offset == -1 || percent_max == 0.0)) {
-		if (notes->have_cycles && al->cycles) {
+		if (notes->branch && al->cycles) {
 			if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
 				show_title = true;
 		} else
@@ -3126,7 +3135,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
 		}
 	}
 
-	if (notes->have_cycles) {
+	if (notes->branch) {
 		if (al->cycles && al->cycles->ipc)
 			obj__printf(obj, "%*.2f ", ANNOTATION__IPC_WIDTH - 1, al->cycles->ipc);
 		else if (!show_title)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 16d27952fd5c..9c199629305d 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -271,17 +271,20 @@ struct annotated_source {
 	struct list_head   source;
 	int    		   nr_histograms;
 	size_t		   sizeof_sym_hist;
-	struct cyc_hist	   *cycles_hist;
 	struct sym_hist	   *histograms;
 };
 
-struct LOCKABLE annotation {
-	u64			max_coverage;
-	u64			start;
+struct annotated_branch {
 	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

[PATCH 3/5] perf annotate: Move max_coverage to annotated_branch

[Namhyung Kim]

The max_coverage is only used when branch stack info is available so
it'd be natural to move to the annotated_branch.

Reviewed-by: Ian Rogers <irogers@google.com>
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

[PATCH 4/5] perf annotate: Move some fields to annotated_source

[Namhyung Kim]

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.

Reviewed-by: Ian Rogers <irogers@google.com>
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 = &notes->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, &notes->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..022fa7ea0f22 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;
+	size_t			sizeof_sym_hist;
+	struct sym_hist		*histograms;
+	int    			nr_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

[PATCH 5/5] perf annotate: Move offsets to annotated_source

[Namhyung Kim]

The offsets array keeps pointers to struct annotation_line which is
available in the annotated_source.  Let's move it to there.

Reviewed-by: Ian Rogers <irogers@google.com>
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(&notes->offsets);
+		zfree(&notes->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(&notes->offsets);
+	zfree(&notes->src->offsets);
 	return err;
 }
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 022fa7ea0f22..ddadece4cd5a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -271,6 +271,7 @@ struct annotated_source {
 	struct list_head	source;
 	size_t			sizeof_sym_hist;
 	struct sym_hist		*histograms;
+	struct annotation_line	**offsets;
 	int    			nr_histograms;
 	int			nr_entries;
 	int			nr_asm_entries;
@@ -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

Re: [PATCH 1/5] perf annotate: Split struct cycles_info

[Arnaldo Carvalho de Melo]

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

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

Re: [PATCH 1/5] perf annotate: Split struct cycles_info

[Arnaldo Carvalho de Melo]

Em Mon, Nov 06, 2023 at 04:34:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> 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@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)
> 
> 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?

I.e. with this folded into this patch, all but one of the callers of
symbol__annotate2() already call a ui error messagem for instance:

                err = symbol__annotate2(ms, evsel, opts, &browser.arch);
                if (err) {
                        char msg[BUFSIZ];
                        dso->annotate_warned = true;
                        symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
                        ui__error("Couldn't annotate %s:\n%s", sym->name, msg);

- Arnaldo

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 3e7f758272703554..99ff3bb9cad8daa6 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1114,12 +1114,13 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
 	}
 }
 
-void annotation__compute_ipc(struct annotation *notes, size_t size)
+static int annotation__compute_ipc(struct annotation *notes, size_t size)
 {
+	int err = 0;
 	s64 offset;
 
 	if (!notes->src || !notes->src->cycles_hist)
-		return;
+		return 0;
 
 	notes->total_insn = annotation__count_insn(notes, 0, size - 1);
 	notes->hit_cycles = 0;
@@ -1137,8 +1138,10 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
 			al = notes->offsets[offset];
 			if (al && al->cycles == NULL) {
 				al->cycles = zalloc(sizeof(*al->cycles));
-				if (al->cycles == NULL)
-					continue;
+				if (al->cycles == NULL) {
+					err = ENOMEM;
+					break;
+				}
 			}
 			if (ch->have_start)
 				annotation__count_and_fill(notes, ch->start, offset, ch);
@@ -1150,7 +1153,21 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
 			notes->have_cycles = true;
 		}
 	}
+
+	if (err) {
+		while (++offset < (s64)size) {
+			struct cyc_hist *ch = &notes->src->cycles_hist[offset];
+
+			if (ch && ch->cycles) {
+				struct annotation_line *al = notes->offsets[offset];
+				if (al)
+					zfree(&al->cycles);
+			}
+		}
+	}
+
 	annotation__unlock(notes);
+	return 0;
 }
 
 int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
@@ -3270,7 +3287,11 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
 
 	annotation__set_offsets(notes, size);
 	annotation__mark_jump_targets(notes, sym);
-	annotation__compute_ipc(notes, size);
+
+	err = annotation__compute_ipc(notes, size);
+	if (err)
+		goto out_free_offsets;
+
 	annotation__init_column_widths(notes, sym);
 	notes->nr_events = nr_pcnt;
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 16d27952fd5c1b47..19bc2f03917575a8 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -329,7 +329,6 @@ static inline bool annotation_line__filter(struct annotation_line *al, struct an
 }
 
 void annotation__set_offsets(struct annotation *notes, s64 size);
-void annotation__compute_ipc(struct annotation *notes, size_t size);
 void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym);
 void annotation__update_column_widths(struct annotation *notes);
 void annotation__init_column_widths(struct annotation *notes, struct symbol *sym);

Re: [PATCH 1/5] perf annotate: Split struct cycles_info

[Namhyung Kim]

Hi Arnaldo,

On Mon, Nov 6, 2023 at 11:53 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Nov 06, 2023 at 04:34:51PM -0300, Arnaldo Carvalho de Melo escreveu:
> > 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@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)
> >
> > 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?
>
> I.e. with this folded into this patch, all but one of the callers of
> symbol__annotate2() already call a ui error messagem for instance:
>
>                 err = symbol__annotate2(ms, evsel, opts, &browser.arch);
>                 if (err) {
>                         char msg[BUFSIZ];
>                         dso->annotate_warned = true;
>                         symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
>                         ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
>
> - Arnaldo

Looks good to me, thanks for doing this!
Namhyung


>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 3e7f758272703554..99ff3bb9cad8daa6 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1114,12 +1114,13 @@ static void annotation__count_and_fill(struct annotation *notes, u64 start, u64
>         }
>  }
>
> -void annotation__compute_ipc(struct annotation *notes, size_t size)
> +static int annotation__compute_ipc(struct annotation *notes, size_t size)
>  {
> +       int err = 0;
>         s64 offset;
>
>         if (!notes->src || !notes->src->cycles_hist)
> -               return;
> +               return 0;
>
>         notes->total_insn = annotation__count_insn(notes, 0, size - 1);
>         notes->hit_cycles = 0;
> @@ -1137,8 +1138,10 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
>                         al = notes->offsets[offset];
>                         if (al && al->cycles == NULL) {
>                                 al->cycles = zalloc(sizeof(*al->cycles));
> -                               if (al->cycles == NULL)
> -                                       continue;
> +                               if (al->cycles == NULL) {
> +                                       err = ENOMEM;
> +                                       break;
> +                               }
>                         }
>                         if (ch->have_start)
>                                 annotation__count_and_fill(notes, ch->start, offset, ch);
> @@ -1150,7 +1153,21 @@ void annotation__compute_ipc(struct annotation *notes, size_t size)
>                         notes->have_cycles = true;
>                 }
>         }
> +
> +       if (err) {
> +               while (++offset < (s64)size) {
> +                       struct cyc_hist *ch = &notes->src->cycles_hist[offset];
> +
> +                       if (ch && ch->cycles) {
> +                               struct annotation_line *al = notes->offsets[offset];
> +                               if (al)
> +                                       zfree(&al->cycles);
> +                       }
> +               }
> +       }
> +
>         annotation__unlock(notes);
> +       return 0;
>  }
>
>  int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
> @@ -3270,7 +3287,11 @@ int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
>
>         annotation__set_offsets(notes, size);
>         annotation__mark_jump_targets(notes, sym);
> -       annotation__compute_ipc(notes, size);
> +
> +       err = annotation__compute_ipc(notes, size);
> +       if (err)
> +               goto out_free_offsets;
> +
>         annotation__init_column_widths(notes, sym);
>         notes->nr_events = nr_pcnt;
>
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 16d27952fd5c1b47..19bc2f03917575a8 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -329,7 +329,6 @@ static inline bool annotation_line__filter(struct annotation_line *al, struct an
>  }
>
>  void annotation__set_offsets(struct annotation *notes, s64 size);
> -void annotation__compute_ipc(struct annotation *notes, size_t size);
>  void annotation__mark_jump_targets(struct annotation *notes, struct symbol *sym);
>  void annotation__update_column_widths(struct annotation *notes);
>  void annotation__init_column_widths(struct annotation *notes, struct symbol *sym);

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

[Arnaldo Carvalho de Melo]

Em Fri, Nov 03, 2023 at 12:19:04PM -0700, Namhyung Kim escreveu:
> The cycles info is only meaningful 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.

<SNIP>

> +static void annotated_branch__delete(struct annotated_branch *branch)
> +{
> +	if (branch) {
> +		free(branch->cycles_hist);

I changed the above free to zfree

> +		free(branch);
> +	}
> +}

Re: [PATCHSET 0/5] perf annotate: Reduce memory footprint (v2)

[Arnaldo Carvalho de Melo]

Em Fri, Nov 03, 2023 at 12:19:02PM -0700, Namhyung Kim escreveu:
> Hello,
> 
> This is a part of my work to improve perf annotate.  At first, I'd
> like reduce the size of struct annotation which will be allocated
> together with struct symbol in some cases.  In fact, it doesn't use
> most of them so it needs to slim down and lazy-allocate used part.
> 
> * v2 changes)
>  - fix typo
>  - reorder struct annotated_source
>  - add Ian's Reviewed-by tags
>  
> With this applied, size of the struct goes down from 96 to 48.
> 
> The code is available at perf/annotate-diet-v2 branch in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Thanks,
> Namhyung
> 

Applied to perf-tools-next, thanks,

- Arnaldo
 
> Namhyung Kim (5):
>   perf annotate: Split struct cycles_info
>   perf annotate: Split struct annotated_branch
>   perf annotate: Move max_coverage to annotated_branch
>   perf annotate: Move some fields to annotated_source
>   perf annotate: Move offsets to annotated_source
> 
>  tools/perf/builtin-annotate.c     |   7 +-
>  tools/perf/ui/browsers/annotate.c |  18 ++--
>  tools/perf/util/annotate.c        | 162 ++++++++++++++++--------------
>  tools/perf/util/annotate.h        |  49 +++++----
>  tools/perf/util/block-info.c      |   4 +-
>  tools/perf/util/block-range.c     |   7 +-
>  tools/perf/util/sort.c            |  14 +--
>  7 files changed, 147 insertions(+), 114 deletions(-)
> 
> -- 
> 2.42.0.869.gea05f2083d-goog
> 
> 

-- 

- Arnaldo