Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 3 Nov 2023 11:58:31 -0700 | Subject | Re: [PATCH 4/5] perf annotate: Move some fields to annotated_source |
| |
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
| |