Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752373AbdGYOm5 (ORCPT ); Tue, 25 Jul 2017 10:42:57 -0400 Received: from mail.kernel.org ([198.145.29.99]:52460 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751141AbdGYOm4 (ORCPT ); Tue, 25 Jul 2017 10:42:56 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 583C622B4B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Tue, 25 Jul 2017 11:42:52 -0300 From: Arnaldo Carvalho de Melo To: Taeung Song Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Namhyung Kim , Milian Wolff , Jiri Olsa Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Message-ID: <20170725144252.GC11990@kernel.org> References: <1500500215-16646-1-git-send-email-treeze.taeung@gmail.com> <20170720191934.GD4134@kernel.org> <51c3c55a-d43d-427f-53fb-56b3ab275dd9@gmail.com> <20170721112420.GE4134@kernel.org> <20170724173725.GT4134@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5089 Lines: 138 Em Tue, Jul 25, 2017 at 06:28:42AM +0900, Taeung Song escreveu: > On 07/25/2017 02:37 AM, Arnaldo Carvalho de Melo wrote: > > The one above, look further down, from 896bccd3cb8d to 0321d0281cbb, > > there are some more missing, but the ones below should be ready for > > sending to Ingo, which I plan to do today. > > I didn't see them.. > Thank you for adjusted patchset. > I checked them a while ago. > > Would you add Reported-by: Namhyung Kim > to the commit 0321d0281cbbb404ea73f9e1869ec8db42e8ddfd ? Sure, will do. > And we can handle other patches later, > but how about additionally fixing the header > on the annotate stdio browser ? > (If you don't want to handle additional patches anymore today, > I'll send them with next patchset.) > > "Percent" -> "Event Count" > > Because I think it would be better to show the proper first column. Of course, I was coming to that, trying to figure out if what was left was just one or even more separate patches, see below > Moreover there is the below case that is not aligned due to big period > values. So, that "moreover" means its not just one patch, but at least two, i.e. when one selects show-total-period we better have more space for that column, right? I'll break the patch below accordingly. And even then, there is one question left, see below > perf annotate --stdio -i milian.data --show-total-period > Percent | Source code & Disassembly of test for cycles:ppp (1442 > samples) > ------------------------------------------------------------------------------- > : > : > : > : Disassembly of section .text: > ... > 0 : 40089d: pxor %xmm1,%xmm1 > 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1 > 0 : 4008a6: pxor %xmm5,%xmm5 > > > So, I made a patch like below: > > commit 6b96b9947e83474bd6e6fd09f93c390536bb435b > Author: Taeung Song > Date: Tue Jul 25 06:17:59 2017 +0900 > > perf annotate: Show the proper header when using --show-total-period > > Currently a first column is only "Percent", > so fix it to show correct column name based on given options. > (e.g. if using --show-total-period, show "Event count" as a first > column) > > Reported-by: Milian Wolff > Cc: Namhyung Kim > Cc: Jiri Olsa > Signed-off-by: Taeung Song > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 004072f..0224595 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > color = get_percent_color(percent); > > if (symbol_conf.show_total_period) > - color_fprintf(stdout, color, " %7" PRIu64, > + color_fprintf(stdout, color, " %11" PRIu64, > sample.period); this part will be in a separate patch, i.e. something like: [PATCH] Widen "Period" column when using --show-total-period > else > color_fprintf(stdout, color, " %7.2f", > percent); > @@ -1173,6 +1173,10 @@ static int disasm_line__print(struct disasm_line *dl, > struct symbol *sym, u64 st > if (perf_evsel__is_group_event(evsel)) > width *= evsel->nr_members; > > + if (symbol_conf.show_total_period) > + width += perf_evsel__is_group_event(evsel) ? > + 4 * evsel->nr_members : 4; > + But what about this one? What is that '4' for? Not obvious at first sight, can you elaborate on the need for this specific one? > if (!*dl->line) > printf(" %*s:\n", width, " "); > else > @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym, > struct map *map, > if (perf_evsel__is_group_event(evsel)) > width *= evsel->nr_members; > > + if (symbol_conf.show_total_period) > + width += perf_evsel__is_group_event(evsel) ? > + 4 * evsel->nr_members : 4; What about this one? > + > graph_dotted_len = printf(" %-*.*s| Source code & Disassembly > of %s for %s (%" PRIu64 " samples)\n", > - width, width, "Percent", d_filename, evsel_name, > h->nr_samples); > + width, width, > + symbol_conf.show_total_period ? "Event > count" : "Percent", > + d_filename, evsel_name, h->nr_samples); > this one will be in a separate patch, with the title you chose: [PATCH] perf annotate: Show the proper header when using --show-total-period > printf("%-*.*s----\n", > graph_dotted_len, graph_dotted_len, graph_dotted_line); > - Arnaldo