Re: [PATCH v6 5/7] perf diff: Link same basic blocks among different data

[Arnaldo Carvalho de Melo]

Em Fri, Jun 28, 2019 at 05:23:02PM +0800, Jin Yao escreveu:
> The target is to compare the performance difference (cycles
> diff) for the same basic blocks in different data files.
> 
> The same basic block means same function, same start address
> and same end address. This patch finds the same basic blocks
> from different data files and link them together and resort
> by the cycles diff.
> 
>  v3:
>  ---
>  The block stuffs are maintained by new structure 'block_hist',
>  so this patch is update accordingly.
> 
>  v2:
>  ---
>  Since now the basic block hists is changed to per symbol,
>  the patch only links the basic block hists for the same
>  symbol in different data files.
> 
> Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/builtin-diff.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 83b8c0f..823f162 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -641,6 +641,85 @@ static int process_block_per_sym(struct hist_entry *he)
>  	return 0;
>  }
>  
> +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;
> +
> +	if (bi_a->sym->name && bi_b->sym->name) {
> +		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;
> +		}


builtin-diff.c:658:17: error: address of array 'bi_a->sym->name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
        if (bi_a->sym->name && bi_b->sym->name) {
            ~~~~~~~~~~~^~~~ ~~
builtin-diff.c:658:36: error: address of array 'bi_b->sym->name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
        if (bi_a->sym->name && bi_b->sym->name) {

Because:

struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *name)
{
        size_t namelen = strlen(name) + 1;
        struct symbol *sym = calloc(1, (symbol_conf.priv_size +
                                        sizeof(*sym) + namelen));


So it will be at least a strlen(sym->name) == 0, i.e. we can use it
without checking anything.

I'm chanign it to do the cmp straight away


> +	}
> +
> +	return -1;
> +}
> +
> +static struct hist_entry *get_block_pair(struct hist_entry *he,
> +					 struct hists *hists_pair)
> +{
> +	struct rb_root_cached *root = hists_pair->entries_in;
> +	struct rb_node *next = rb_first_cached(root);
> +	int cmp;
> +
> +	while (next != NULL) {
> +		struct hist_entry *he_pair = rb_entry(next, struct hist_entry,
> +						      rb_node_in);
> +
> +		next = rb_next(&he_pair->rb_node_in);
> +
> +		cmp = block_pair_cmp(he_pair, he);
> +		if (!cmp)
> +			return he_pair;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void compute_cycles_diff(struct hist_entry *he,
> +				struct hist_entry *pair)
> +{
> +	pair->diff.computed = true;
> +	if (pair->block_info->num && he->block_info->num) {
> +		pair->diff.cycles =
> +			pair->block_info->cycles_aggr / pair->block_info->num_aggr -
> +			he->block_info->cycles_aggr / he->block_info->num_aggr;
> +	}
> +}
> +
> +static void block_hists_match(struct hists *hists_base,
> +			      struct hists *hists_pair)
> +{
> +	struct rb_root_cached *root = hists_base->entries_in;
> +	struct rb_node *next = rb_first_cached(root);
> +
> +	while (next != NULL) {
> +		struct hist_entry *he = rb_entry(next, struct hist_entry,
> +						 rb_node_in);
> +		struct hist_entry *pair = get_block_pair(he, hists_pair);
> +
> +		next = rb_next(&he->rb_node_in);
> +
> +		if (pair) {
> +			hist_entry__add_pair(pair, he);
> +			compute_cycles_diff(he, pair);
> +		}
> +	}
> +}
> +
> +static int filter_cb(struct hist_entry *he, void *arg __maybe_unused)
> +{
> +	/* Skip the calculation of column length in output_resort */
> +	he->filtered = true;
> +	return 0;
> +}
> +
>  static void hists__precompute(struct hists *hists)
>  {
>  	struct rb_root_cached *root;
> @@ -653,6 +732,7 @@ static void hists__precompute(struct hists *hists)
>  
>  	next = rb_first_cached(root);
>  	while (next != NULL) {
> +		struct block_hist *bh, *pair_bh;
>  		struct hist_entry *he, *pair;
>  		struct data__file *d;
>  		int i;
> @@ -681,6 +761,16 @@ static void hists__precompute(struct hists *hists)
>  				break;
>  			case COMPUTE_CYCLES:
>  				process_block_per_sym(pair);
> +				bh = container_of(he, struct block_hist, he);
> +				pair_bh = container_of(pair, struct block_hist,
> +						       he);
> +
> +				if (bh->valid && pair_bh->valid) {
> +					block_hists_match(&bh->block_hists,
> +							  &pair_bh->block_hists);
> +					hists__output_resort_cb(&pair_bh->block_hists,
> +								NULL, filter_cb);
> +				}
>  				break;
>  			default:
>  				BUG_ON(1);
> -- 
> 2.7.4

-- 

- Arnaldo