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
---
tools/perf/util/hist.c | 104 +++++++++++++++++------------------------
tools/perf/util/hist.h | 2 -
2 files changed, 44 insertions(+), 62 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index fff1345658016..8e4e844425370 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,27 @@ 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)
{
+ typedef int64_t (*fn_t)(struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *);
struct hists *hists = left->hists;
struct perf_hpp_fmt *fmt;
int64_t cmp = 0;
+ fn_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 = *(fn_t *)(((char *)fmt) + fn_offset);
+ cmp = fn(fmt, left, right);
if (cmp)
break;
}
@@ -1313,23 +1324,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 +1524,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 +1744,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 +2445,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 +2511,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..6cff03eb043b7 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);
--
2.47.1.613.gc27f4b7a9f-googHello,
On Wed, Jan 08, 2025 at 08:36:53AM +0100, Dmitry Vyukov wrote:
> Application of cmp/sort/collapse fmt callbacks is duplicated 6 times.
> Factor it into a common helper function. NFC.
It's interesting to use offsetof and cast it to functions. :)
>
> 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
> ---
> tools/perf/util/hist.c | 104 +++++++++++++++++------------------------
> tools/perf/util/hist.h | 2 -
> 2 files changed, 44 insertions(+), 62 deletions(-)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index fff1345658016..8e4e844425370 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,27 @@ 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)
> {
> + typedef int64_t (*fn_t)(struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *);
Probably better to put it in util/hist.h and each members (cmp, collapse,
sort) use the typedef to make sure they remain in the same.
> struct hists *hists = left->hists;
> struct perf_hpp_fmt *fmt;
> int64_t cmp = 0;
> + fn_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 = *(fn_t *)(((char *)fmt) + fn_offset);
Doesn't just below work?
fn = (void *)fmt + fn_offset;
Thanks,
Namhyung
> + cmp = fn(fmt, left, right);
> if (cmp)
> break;
> }
> @@ -1313,23 +1324,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 +1524,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 +1744,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 +2445,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 +2511,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..6cff03eb043b7 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);
> --
> 2.47.1.613.gc27f4b7a9f-goog
>
On Thu, 9 Jan 2025 at 22:43, Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Wed, Jan 08, 2025 at 08:36:53AM +0100, Dmitry Vyukov wrote:
> > Application of cmp/sort/collapse fmt callbacks is duplicated 6 times.
> > Factor it into a common helper function. NFC.
>
> It's interesting to use offsetof and cast it to functions. :)
Is there a better way to do this?
Intuitively I reached for member pointers, but then realized it's not
a thing in C...
> > 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
> > ---
> > tools/perf/util/hist.c | 104 +++++++++++++++++------------------------
> > tools/perf/util/hist.h | 2 -
> > 2 files changed, 44 insertions(+), 62 deletions(-)
> >
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index fff1345658016..8e4e844425370 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,27 @@ 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)
> > {
> > + typedef int64_t (*fn_t)(struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *);
>
> Probably better to put it in util/hist.h and each members (cmp, collapse,
> sort) use the typedef to make sure they remain in the same.
Noted. But let's decide on the "perf hist: Fix bogus profiles when
filters are enabled" first, that was the motivation for this
refactoring.
> > struct hists *hists = left->hists;
> > struct perf_hpp_fmt *fmt;
> > int64_t cmp = 0;
> > + fn_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 = *(fn_t *)(((char *)fmt) + fn_offset);
>
> Doesn't just below work?
>
> fn = (void *)fmt + fn_offset;
>
> Thanks,
> Namhyung
>
>
> > + cmp = fn(fmt, left, right);
> > if (cmp)
> > break;
> > }
> > @@ -1313,23 +1324,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 +1524,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 +1744,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 +2445,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 +2511,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..6cff03eb043b7 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);
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
> >
On Fri, 10 Jan 2025 at 14:01, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Thu, 9 Jan 2025 at 22:43, Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Wed, Jan 08, 2025 at 08:36:53AM +0100, Dmitry Vyukov wrote:
> > > Application of cmp/sort/collapse fmt callbacks is duplicated 6 times.
> > > Factor it into a common helper function. NFC.
> >
> > It's interesting to use offsetof and cast it to functions. :)
>
> Is there a better way to do this?
> Intuitively I reached for member pointers, but then realized it's not
> a thing in C...
>
>
> > > 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
> > > ---
> > > tools/perf/util/hist.c | 104 +++++++++++++++++------------------------
> > > tools/perf/util/hist.h | 2 -
> > > 2 files changed, 44 insertions(+), 62 deletions(-)
> > >
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index fff1345658016..8e4e844425370 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,27 @@ 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)
> > > {
> > > + typedef int64_t (*fn_t)(struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *);
> >
> > Probably better to put it in util/hist.h and each members (cmp, collapse,
> > sort) use the typedef to make sure they remain in the same.
Good idea!. Done in v2
> Noted. But let's decide on the "perf hist: Fix bogus profiles when
> filters are enabled" first, that was the motivation for this
> refactoring.
>
>
> > > struct hists *hists = left->hists;
> > > struct perf_hpp_fmt *fmt;
> > > int64_t cmp = 0;
> > > + fn_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 = *(fn_t *)(((char *)fmt) + fn_offset);
> >
> > Doesn't just below work?
> >
> > fn = (void *)fmt + fn_offset;
No, this just calculates a pointer to the field that contains the
function pointer. But we also need to dereference that field and get
the function pointer.
> > Thanks,
> > Namhyung
> >
> >
> > > + cmp = fn(fmt, left, right);
> > > if (cmp)
> > > break;
> > > }
> > > @@ -1313,23 +1324,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 +1524,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 +1744,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 +2445,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 +2511,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..6cff03eb043b7 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);
> > > --
> > > 2.47.1.613.gc27f4b7a9f-goog
> > >
© 2016 - 2025 Red Hat, Inc.