Re: [PATCH v2] perf diff: Report noisy for cycles diff

[Jin, Yao]

On 8/6/2019 4:34 PM, Jiri Olsa wrote:
> On Thu, Jul 25, 2019 at 06:14:32AM +0800, Jin Yao wrote:
>
> SNIP
>
> >   static int cycles_printf(struct hist_entry *he, struct hist_entry *pair,
> > -			 struct perf_hpp *hpp, int width)
> > +			 struct perf_hpp *hpp, int width __maybe_unused)
> >   {
> >   	struct block_hist *bh = container_of(he, struct block_hist, he);
> >   	struct block_hist *bh_pair = container_of(pair, struct block_hist, he);
> >   	struct hist_entry *block_he;
> >   	struct block_info *bi;
> > -	char buf[128];
> > +	char buf[128], spark[32];
> >   	char *start_line, *end_line;
> > +	int ret = 0, pad;
> > +	char pfmt[20] = " ";
> > +	double d;
> >   
> >   	block_he = hists__get_entry(&bh_pair->block_hists, bh->block_idx);
> >   	if (!block_he) {
> > @@ -1350,18 +1375,56 @@ static int cycles_printf(struct hist_entry *he, struct hist_entry *pair,
> >   	end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
> >   				he->ms.sym);
> >   
> > -	if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > -		scnprintf(buf, sizeof(buf), "[%s -> %s] %4ld",
> > -			  start_line, end_line, block_he->diff.cycles);
> > +	if (show_noisy) {
> > +		ret = print_stat_spark(spark, sizeof(spark),
> > +				       &block_he->diff.stats);
> > +		d = rel_stddev_stats(stddev_stats(&block_he->diff.stats),
> > +				     avg_stats(&block_he->diff.stats));
> > +
> > +		if ((start_line != SRCLINE_UNKNOWN) &&
> > +		    (end_line != SRCLINE_UNKNOWN)) {
> > +			scnprintf(buf, sizeof(buf),
> > +				  "[%s -> %s] %4ld  %s%5.1f%% %s",
> > +				  start_line, end_line, block_he->diff.cycles,
> > +				  "\u00B1", d, spark);
> > +		} else {
> > +			scnprintf(buf, sizeof(buf),
> > +				  "[%7lx -> %7lx] %4ld  %s%5.1f%% %s",
> > +				  bi->start, bi->end, block_he->diff.cycles,
> > +				  "\u00B1", d, spark);
> > +		}
> > +
> > +		if (ret > 0) {
> > +			pad = 8 - ((ret - 1) / 3);
> > +			scnprintf(pfmt, 20, "%%%ds",
> > +				  81 + (2 * ((ret - 1) / 3)) - pad);
> > +			ret = scnprintf(hpp->buf, hpp->size, pfmt, buf);
> > +			if (pad > 0) {
> > +				ret += scnprintf(hpp->buf + ret,
> > +						 hpp->size - ret,
> > +						 "%-*s", pad, " ");
> > +			}
> > +		} else {
> > +			ret = scnprintf(hpp->buf, hpp->size, "%73s", buf);
> > +			ret += scnprintf(hpp->buf + ret, hpp->size - ret,
> > +					 "%-*s", 8, " ");
> > +		}
>
>
> hum, why isn't the histogram in the separate column?
> looks like there's lot of duplicated code in here
>
> thanks,
> jirka

Yes, it'd better add the histogram in a separate column. But it's not 
very easy to add that. Anyway let me double check if I can find a less 
complicated method for that.

Thanks
Jin Yao


> >   	} else {
> > -		scnprintf(buf, sizeof(buf), "[%7lx -> %7lx] %4ld",
> > -			  bi->start, bi->end, block_he->diff.cycles);
> > +		if ((start_line != SRCLINE_UNKNOWN) &&
> > +		    (end_line != SRCLINE_UNKNOWN)) {
> > +			scnprintf(buf, sizeof(buf), "[%s -> %s] %4ld",
> > +				  start_line, end_line, block_he->diff.cycles);
> > +		} else {
> > +			scnprintf(buf, sizeof(buf), "[%7lx -> %7lx] %4ld",
> > +				  bi->start, bi->end, block_he->diff.cycles);
> > +		}
> > +
> > +		ret = scnprintf(hpp->buf, hpp->size, "%*s", width, buf);
> >   	}
> >   
> >   	free_srcline(start_line);
> >   	free_srcline(end_line);
> > -
> > -	return scnprintf(hpp->buf, hpp->size, "%*s", width, buf);
> > +	return ret;
> >   }
>
> SNIP