Re: [PATCH v5 1/4] perf util: Move block_pair_cmp to block-info

[Jin, Yao]

On 1/31/2020 4:32 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 28, 2020 at 08:55:53PM +0800, Jin Yao escreveu:
> > block_pair_cmp() is a function which is used to compare
> > two blocks. Moving it from builtin-diff.c to block-info.c
> > to let it can be used by other builtins.
> >
> >   v4/v5:
> >   ------
> >   No change.
> >
> >   v3:
> >   ---
> >   Separate it from original patch for good tracking.
> >
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/builtin-diff.c    | 17 -----------------
> >   tools/perf/util/block-info.c | 17 +++++++++++++++++
> >   tools/perf/util/block-info.h |  2 ++
> >   3 files changed, 19 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> > index f8b6ae557d8b..5ff1e21082cb 100644
> > --- a/tools/perf/builtin-diff.c
> > +++ b/tools/perf/builtin-diff.c
> > @@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
> >   	bh->valid = true;
> >   }
> >   
> > -static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
> > -{
> > -	struct block_info *bi_a = a->block_info;
> > -	struct block_info *bi_b = b->block_info;
> > -	int cmp;
> > -
> > -	if (!bi_a->sym || !bi_b->sym)
> > -		return -1;
> > -
> > -	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> > -
> > -	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> > -		return 0;
> > -
> > -	return -1;
> > -}
> > -
> >   static struct hist_entry *get_block_pair(struct hist_entry *he,
> >   					 struct hists *hists_pair)
> >   {
> > diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
> > index c4b030bf6ec2..f0f38bdd496a 100644
> > --- a/tools/perf/util/block-info.c
> > +++ b/tools/perf/util/block-info.c
> > @@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
> >   
> >   	return 0.0;
> >   }
> > +
> > +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
>
> First thing that came to mind was that hist_entry comparision functions
> had been changed to return int64_t recently, when I went to look at it I
> found this:
>
> tools/perf/util/block-info.c
>
> int64_t block_info__cmp(struct perf_hpp_fmt *fmt __maybe_unused,
>                          struct hist_entry *left, struct hist_entry *right)
> {
>          struct block_info *bi_l = left->block_info;
>          struct block_info *bi_r = right->block_info;
>          int cmp;
> .
> .
> .
>
> Which look a bit more complete, can you check if that can be used
> instead or explain why my quick analysis of this is b0rken?
>
> Perhaps we can have a __block_info__cmp() that doesn't receive the
> perf_hpp_fmt (that isn't even used above) so that the previous use of
> block_pair_cmp() can be replaced with __block_info__cmp() instead?
>
> Thanks,
>
> - Arnaldo

Hi Arnaldo,

I think it's a good idea to use __block_info__cmp() to replace the 
block_pair_cmp(). The block_info__cmp() is more comprehensive than 
block_pair_cmp() and we only need a bit change such as,

--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -74,7 +74,7 @@ int64_t block_info__cmp(struct perf_hpp_fmt *fmt 
__maybe_unused,

        if (!bi_l->sym || !bi_r->sym) {
                if (!bi_l->sym && !bi_r->sym)
-                       return 0;
+                       return -1;
                else if (!bi_l->sym)
                        return -1;
                else

It returns -1 if both syms are NULL.

I will update the patchset.

Thanks
Jin Yao

> > +{
> > +	struct block_info *bi_a = a->block_info;
> > +	struct block_info *bi_b = b->block_info;
> > +	int cmp;
> > +
> > +	if (!bi_a->sym || !bi_b->sym)
> > +		return -1;
> > +
> > +	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> > +
> > +	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> > +		return 0;
> > +
> > +	return -1;
> > +}
> > diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
> > index bef0d75e9819..4fa91eeae92e 100644
> > --- a/tools/perf/util/block-info.h
> > +++ b/tools/perf/util/block-info.h
> > @@ -76,4 +76,6 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
> >   
> >   float block_info__total_cycles_percent(struct hist_entry *he);
> >   
> > +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b);
> > +
> >   #endif /* __PERF_BLOCK_H */
> > --
> > 2.17.1