Received-SPF: pass (google.com: domain of linux-kernel+bounces-69275-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240213075256.1983638-1-namhyung@kernel.org> <20240213075256.1983638-4-namhyung@kernel.org>
 <CAP-5=fVK3vvNoaGHPep8NCweGw9cztKBGQh5+0bVX91PhWp5Eg@mail.gmail.com>
 <CAM9d7ci=A9rwZxEYYQRi-Cncs7NSpRG+TaH5knTdEPZYxJWp9Q@mail.gmail.com> <CAP-5=fWwRjnv=BSdz2GV+EfKUrtzMCSvheR38hkbFadKpvW2eQ@mail.gmail.com>
In-Reply-To: <CAP-5=fWwRjnv=BSdz2GV+EfKUrtzMCSvheR38hkbFadKpvW2eQ@mail.gmail.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 16 Feb 2024 12:08:47 -0800
Message-ID: <CAM9d7cg6c7t7zAgDEJP=+D1qM16SQuWN05fn4T-m1WPfXjaZvA@mail.gmail.com>
Subject: Re: [PATCH 3/4] perf hist: Do not use event index in hpp__fmt()
To: Ian Rogers <irogers@google.com>
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

On Wed, Feb 14, 2024 at 11:54=E2=80=AFPM Ian Rogers <irogers@google.com> wr=
ote:
>
> On Wed, Feb 14, 2024 at 9:26=E2=80=AFPM Namhyung Kim <namhyung@kernel.org=
> wrote:
> >
> > On Wed, Feb 14, 2024 at 4:08=E2=80=AFPM Ian Rogers <irogers@google.com>=
 wrote:
> > >
> > > On Mon, Feb 12, 2024 at 11:52=E2=80=AFPM Namhyung Kim <namhyung@kerne=
l.org> wrote:
> > > >
> > > > The __hpp__fmt() is to print period values in a hist entry.  It han=
dles
> > > > event groups using linked pair entries.  Until now, it used event i=
ndex
> > > > to print values of group members.  But we want to make it more robu=
st
> > > > and support groups even if some members in the group were removed.
> > >
> > > I'm unclear how it breaks currently. The evsel idx is set the evlist
> > > nr_entries on creation and not updated by a remove. A remove may
> > > change a groups leader, should the remove also make the next entry's
> > > index idx that of the previous group leader?
> >
> > The evsel__group_idx() returns evsel->idx - leader->idx.
> > If it has a group event {A, B, C} then the index would be 0, 1, 2.
> > If it removes B, the group would be {A, C} with index 0 and 2.
> > The nr_members is 2 now so it cannot use index 2 for C.
> >
> > Note that we cannot change the index of C because some information
> > like annotation histogram relies on the index.
>
> Ugh, the whole index thing is just messy - perhaps these days we could
> have a hashmap with the evsel as a key instead. I remember that I also
> forced the idx here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/=
tree/tools/perf/util/parse-events.c?h=3Dperf-tools-next#n2049
> If it were invariant that the idx were always the position of an event
> in the evlist then I think life would be easier, but that won't help
> for the arrays of counters that need the index to be constant. I guess
> this is why the previous work to do this skipped evsels rather than
> removed them.

Actually I have a patch series to convert the annotation histogram
to a hash map.  It'd reduce memory usage as well.  Will post.

I think removing evsel is not a common operation and should be
done with care.  In this patchset, it removed (dummy) events after
processing all samples.  I can make the code to skip those event
when printing the result but it'd be much easier if it can remove the
unnecessary events.

Thanks,
Namhyung