Re: [PATCHSET 0/8] perf annotate: Make annotation_options global (v1)

From: Ian Rogers <irogers@google.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Jiri Olsa <jolsa@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCHSET 0/8] perf annotate: Make annotation_options global (v1)
Date: Thu, 30 Nov 2023 10:37:43 -0800	[thread overview]
Message-ID: <CAP-5=fUf6R=bsfg7i8atFApJBY-=zWUBMq7inFsCPZhB+w2==Q@mail.gmail.com> (raw)
In-Reply-To: <CAM9d7chKmDETK6Ea2wyR8L21jyHWcPHbKavarnq-JmNA-AoUnQ@mail.gmail.com>

On Wed, Nov 29, 2023 at 3:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > It used to have annotation_options for each command separately (for
> > > example, perf report, annotate, and top), but we can make it global as
> > > they never used together (with different settings).  This would save
> > > some memory for each symbol when annotation is enabled.
> > >
> > > This code is available at 'perf/annotate-option-v1' branch in
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > >
> > > Thanks,
> > > Namhyung
> >
> > Thanks for doing this and I think it is progress. I think there is a
> > common problem with having things be an option rather than say part of
> > session. Having a global variable seems unfortunate but I'm not sure
> > if in all locations we have easy access to the session.
>
> That's not the case when you deal with hist entry or TUI browser.
> I think that's the reason we have the option in the annotation.
>
>
> > The rough
> > structure with annotations as I understand it is:
> >
> > session has machines
> > a machine has dsos
> > a dso has symbols
> > a symbol has an annotation
>
> That's true.  But the annotation struct is used only if
> symbol__annotation_init() is called.

Right. I find this approach likely to lead to errors, such as a symbol
created globally before symbol__annotation_init() was called breaking
the container_of assumptions.

> >
> > Annotation is something of unfortunate abstraction as it covers things
> > like an IPC per symbol (why hard code to just IPC?) and things like
> > source files and line numbers.
>
> Right, that's why I splitted the struct annotated_branch.
>
> >
> > A recent success story where we got rid of a configuration variable
> > was by switching to lazy allocation with sorting by name for symbols
> > within a dso. If we could have a lazy allocation model with
> > annotations then maybe we can do away with large hammers like global
> > options.
>
> Maybe I can move the pointer to option into the annotated_source
> which is allocated lazily.  But I don't think it needs to keep the option
> for each symbol or annotation.  It's usually to control some display
> behaviors in the disasm output globally.  So I think it's better to have
> a global variable.

Sgtm. My point wasn't to criticize, I think this is a good change, I
was just trying to imagine doing things in a way that could overall
reduce complexity

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> >
> > > Namhyung Kim (8):
> > >   perf annotate: Introduce global annotation_options
> > >   perf report: Convert to the global annotation_options
> > >   perf top: Convert to the global annotation_options
> > >   perf annotate: Use global annotation_options
> > >   perf ui/browser/annotate: Use global annotation_options
> > >   perf annotate: Ensure init/exit for global options
> > >   perf annotate: Remove remaining usages of local annotation options
> > >   perf annotate: Get rid of local annotation options
> > >
> > >  tools/perf/builtin-annotate.c     |  43 +++++----
> > >  tools/perf/builtin-report.c       |  37 ++++----
> > >  tools/perf/builtin-top.c          |  45 +++++-----
> > >  tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
> > >  tools/perf/ui/browsers/hists.c    |  34 +++----
> > >  tools/perf/ui/browsers/hists.h    |   2 -
> > >  tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
> > >  tools/perf/util/annotate.h        |  38 ++++----
> > >  tools/perf/util/block-info.c      |   6 +-
> > >  tools/perf/util/block-info.h      |   3 +-
> > >  tools/perf/util/hist.h            |  25 ++----
> > >  tools/perf/util/top.h             |   1 -
> > >  12 files changed, 206 insertions(+), 255 deletions(-)
> > >
> > >
> > > base-commit: 757489991f7c08603395b85037a981c31719c92c
> > > --
> > > 2.43.0.rc1.413.gea7ed67945-goog
> > >