Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231128175441.721579-1-namhyung@kernel.org>
In-Reply-To: <20231128175441.721579-1-namhyung@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 28 Nov 2023 11:13:55 -0800
Message-ID: <CAP-5=fWfKqgT60TFRALw8vTDQT7VFV+0+eo1rFSSH3eVrjzPmA@mail.gmail.com>
Subject: Re: [PATCHSET 0/8] perf annotate: Make annotation_options global (v1)
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 28, 2023 at 9:54=E2=80=AFAM 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. 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

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.

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.

Thanks,
Ian


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