From: Namhyung Kim <namhyung@kernel.org>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: irogers@google.com, linux-perf-users@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code
Date: Tue, 14 Jan 2025 17:07:02 -0800 [thread overview]
Message-ID: <Z4cKNjudLDJBrlM_@google.com> (raw)
In-Reply-To: <52f5a4dcc10ecb00fdf78750d1b32dabcbaf2a31.1736775109.git.dvyukov@google.com>
On Mon, Jan 13, 2025 at 02:34:14PM +0100, Dmitry Vyukov wrote:
> Application of cmp/sort/collapse fmt callbacks is duplicated 6 times.
> Factor it into a common helper function. NFC.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: linux-perf-users@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> ---
>
> Changes in v2:
> - add perf_hpp_fmt_cmp_t typedef to util/hist.h
> - use the typedef for perf_hpp_fmt::{cmp,collapse,sort}
> ---
> tools/perf/util/hist.c | 103 +++++++++++++++++------------------------
> tools/perf/util/hist.h | 14 +++---
> 2 files changed, 49 insertions(+), 68 deletions(-)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index fff1345658016..a4b8ec3805565 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -32,6 +32,9 @@
> #include <linux/time64.h>
> #include <linux/zalloc.h>
>
> +static int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right);
> +static int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right);
> +
> static bool hists__filter_entry_by_dso(struct hists *hists,
> struct hist_entry *he);
> static bool hists__filter_entry_by_thread(struct hists *hists,
> @@ -1292,19 +1295,26 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
> return err;
> }
>
> -int64_t
> -hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
> +static int64_t
> +hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left,
> + struct hist_entry *right, unsigned long fn_offset,
> + bool ignore_dynamic, bool ignore_skipped)
Maybe it's a matter of tastes, but I think it could be simpler like
below.
> {
> struct hists *hists = left->hists;
> struct perf_hpp_fmt *fmt;
> int64_t cmp = 0;
> + perf_hpp_fmt_cmp_t fn;
perf_hpp_fmt_cmp_t *fn;
>
> - hists__for_each_sort_list(hists, fmt) {
> - if (perf_hpp__is_dynamic_entry(fmt) &&
> + perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
> + if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) &&
> !perf_hpp__defined_dynamic_entry(fmt, hists))
> continue;
>
> - cmp = fmt->cmp(fmt, left, right);
> + if (ignore_skipped && perf_hpp__should_skip(fmt, hists))
> + continue;
> +
> + fn = *(perf_hpp_fmt_cmp_t *)(((char *)fmt) + fn_offset);
> + cmp = fn(fmt, left, right);
fn = (void *)fmt + fn_offset;
cmp = (*fn)(fmt, left, right);
Otherwise, looks good to me.
Thanks,
Namhyung
> if (cmp)
> break;
> }
> @@ -1313,23 +1323,33 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
> }
>
> int64_t
> -hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
> +hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
> {
> - struct hists *hists = left->hists;
> - struct perf_hpp_fmt *fmt;
> - int64_t cmp = 0;
> + return hist_entry__cmp_impl(left->hists->hpp_list, left, right,
> + offsetof(struct perf_hpp_fmt, cmp), true, false);
> +}
>
> - hists__for_each_sort_list(hists, fmt) {
> - if (perf_hpp__is_dynamic_entry(fmt) &&
> - !perf_hpp__defined_dynamic_entry(fmt, hists))
> - continue;
> +static int64_t
> +hist_entry__sort(struct hist_entry *left, struct hist_entry *right)
> +{
> + return hist_entry__cmp_impl(left->hists->hpp_list, left, right,
> + offsetof(struct perf_hpp_fmt, sort), false, true);
> +}
>
> - cmp = fmt->collapse(fmt, left, right);
> - if (cmp)
> - break;
> - }
> +int64_t
> +hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
> +{
> + return hist_entry__cmp_impl(left->hists->hpp_list, left, right,
> + offsetof(struct perf_hpp_fmt, collapse), true, false);
> +}
>
> - return cmp;
> +static int64_t
> +hist_entry__collapse_hierarchy(struct perf_hpp_list *hpp_list,
> + struct hist_entry *left,
> + struct hist_entry *right)
> +{
> + return hist_entry__cmp_impl(hpp_list, left, right,
> + offsetof(struct perf_hpp_fmt, collapse), false, false);
> }
>
> void hist_entry__delete(struct hist_entry *he)
> @@ -1503,14 +1523,7 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
> while (*p != NULL) {
> parent = *p;
> iter = rb_entry(parent, struct hist_entry, rb_node_in);
> -
> - cmp = 0;
> - perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
> - cmp = fmt->collapse(fmt, iter, he);
> - if (cmp)
> - break;
> - }
> -
> + cmp = hist_entry__collapse_hierarchy(hpp_list, iter, he);
> if (!cmp) {
> he_stat__add_stat(&iter->stat, &he->stat);
> return iter;
> @@ -1730,24 +1743,6 @@ int hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
> return 0;
> }
>
> -static int64_t hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
> -{
> - struct hists *hists = a->hists;
> - struct perf_hpp_fmt *fmt;
> - int64_t cmp = 0;
> -
> - hists__for_each_sort_list(hists, fmt) {
> - if (perf_hpp__should_skip(fmt, a->hists))
> - continue;
> -
> - cmp = fmt->sort(fmt, a, b);
> - if (cmp)
> - break;
> - }
> -
> - return cmp;
> -}
> -
> static void hists__reset_filter_stats(struct hists *hists)
> {
> hists->nr_non_filtered_entries = 0;
> @@ -2449,21 +2444,15 @@ static struct hist_entry *add_dummy_hierarchy_entry(struct hists *hists,
> struct rb_node **p;
> struct rb_node *parent = NULL;
> struct hist_entry *he;
> - struct perf_hpp_fmt *fmt;
> bool leftmost = true;
>
> p = &root->rb_root.rb_node;
> while (*p != NULL) {
> - int64_t cmp = 0;
> + int64_t cmp;
>
> parent = *p;
> he = rb_entry(parent, struct hist_entry, rb_node_in);
> -
> - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
> - cmp = fmt->collapse(fmt, he, pair);
> - if (cmp)
> - break;
> - }
> + cmp = hist_entry__collapse_hierarchy(he->hpp_list, he, pair);
> if (!cmp)
> goto out;
>
> @@ -2521,16 +2510,10 @@ static struct hist_entry *hists__find_hierarchy_entry(struct rb_root_cached *roo
>
> while (n) {
> struct hist_entry *iter;
> - struct perf_hpp_fmt *fmt;
> - int64_t cmp = 0;
> + int64_t cmp;
>
> iter = rb_entry(n, struct hist_entry, rb_node_in);
> - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
> - cmp = fmt->collapse(fmt, iter, he);
> - if (cmp)
> - break;
> - }
> -
> + cmp = hist_entry__collapse_hierarchy(he->hpp_list, iter, he);
> if (cmp < 0)
> n = n->rb_left;
> else if (cmp > 0)
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 1131056924d9c..46c8373e31465 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -342,8 +342,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
> struct perf_hpp;
> struct perf_hpp_fmt;
>
> -int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right);
> -int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right);
> int hist_entry__transaction_len(void);
> int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size,
> struct hists *hists);
> @@ -452,6 +450,9 @@ struct perf_hpp {
> bool skip;
> };
>
> +typedef int64_t (*perf_hpp_fmt_cmp_t)(
> + struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *);
> +
> struct perf_hpp_fmt {
> const char *name;
> int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
> @@ -463,12 +464,9 @@ struct perf_hpp_fmt {
> struct hist_entry *he);
> int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
> struct hist_entry *he);
> - int64_t (*cmp)(struct perf_hpp_fmt *fmt,
> - struct hist_entry *a, struct hist_entry *b);
> - int64_t (*collapse)(struct perf_hpp_fmt *fmt,
> - struct hist_entry *a, struct hist_entry *b);
> - int64_t (*sort)(struct perf_hpp_fmt *fmt,
> - struct hist_entry *a, struct hist_entry *b);
> + perf_hpp_fmt_cmp_t cmp;
> + perf_hpp_fmt_cmp_t collapse;
> + perf_hpp_fmt_cmp_t sort;
> bool (*equal)(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
> void (*free)(struct perf_hpp_fmt *fmt);
>
> --
> 2.47.1.688.g23fc6f90ad-goog
>
next prev parent reply other threads:[~2025-01-15 1:07 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-13 13:34 [PATCH v2 0/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov
2025-01-13 13:34 ` [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code Dmitry Vyukov
2025-01-15 1:07 ` Namhyung Kim [this message]
2025-01-15 8:02 ` Dmitry Vyukov
2025-01-13 13:34 ` [PATCH v2 2/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z4cKNjudLDJBrlM_@google.com \
--to=namhyung@kernel.org \
--cc=dvyukov@google.com \
--cc=irogers@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.