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

From: Christophe JAILLET
Date: Fri Nov 03 2023 - 01:41:43 EST

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@xxxxxxxxxx>
---
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..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;