by Namhyung Kim

[permalink] [raw]

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
<[email protected]> 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 <[email protected]>
> > ---
> > 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.

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