From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757206AbbJVMOL (ORCPT ); Thu, 22 Oct 2015 08:14:11 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:35820 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751554AbbJVMOJ (ORCPT ); Thu, 22 Oct 2015 08:14:09 -0400 MIME-Version: 1.0 In-Reply-To: <20151022080204.GA18916@gmail.com> References: <1445495330-25416-1-git-send-email-namhyung@kernel.org> <20151022080204.GA18916@gmail.com> From: Namhyung Kim Date: Thu, 22 Oct 2015 21:13:49 +0900 X-Google-Sender-Auth: 2fnR3btVKpwtDK4lcs8OBcX9UBw Message-ID: Subject: Re: [PATCH 1/3] perf tools: Move callchain help messages to callchain.h To: Ingo Molnar Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Adrian Hunter , Borislav Petkov , Chandler Carruth , Frederic Weisbecker , Stephane Eranian , Wang Nan , Brendan Gregg Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 22, 2015 at 5:02 PM, Ingo Molnar wrote: > > * Namhyung Kim wrote: > >> +#define CALLCHAIN_HELP "setup and enables call-graph (stack chain/backtrace) recording: " >> + >> +#ifdef HAVE_DWARF_UNWIND_SUPPORT >> +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr" >> +#else >> +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr" >> +#endif > > nano-nit, could we structure such balanced #ifdefs the following way: > > #ifdef HAVE_DWARF_UNWIND_SUPPORT > # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr" > #else > # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr" > #endif > > makes the construct stand out a lot better visually. OK > > I also had another look at the help text: > >> output_type,min_percent[,print_limit],call_order[,branch] > >> +#define CALLCHAIN_REPORT_HELP "output_type (graph, flat, fractal, or none), " \ >> + "min percent threshold, optional print limit, callchain order, " \ >> + "key (function or address), add branches" > > Btw., when I first read this message in the help text yesterday, I had to read the > 'min percent threshold' twice, to realize that the default 0.5 is in units of > percentage - the wording wasn't entirely clear about that. OK > > Also, I had to go into the code to decode the real meaning of all the other > parameters. I'd have expected them to be more obvious from reading the help text. Did you check the man page also? I think we have (short) explanation for each parameter and users should read it first to understand the meaning. But I agree that the help text should also be improved to provide quick reference. > > Wording them the following way would have made things a lot more apparent to me: > > print_style,min_percent[,print_percent],call_order[,key] > > call chain tree printing style (graph|flat|fractal|none) > minimum tree inclusion threshold (percent) > printing threshold (percent) Note that this 'printing threshold' is not percent. It's to limit number of callchain entries printed for each hist entry. However it works for --stdio only probably since it lacks interactive collapse/expand feature. > call chain order (caller|callee) > key (function|address|branch) > > Note that I extended the help text with new options not mentioned in the help text > but present in the current code - such as the 'branch' key. > > Also note that in the code I did not find any trace of the '[,branch]' and > 'add branches' part present in the help text. What we have is a 'branch' > option in the (optional) key parameter. Looking at the document, it seems branch is not a key: branch can be: - branch: include last branch information in callgraph when available. Usually more convenient to use --branch-history for this. Confusingly, it was checked in parse_callchain_sort_key() but does nothing with the sort key IIUC. > > I also made various edits to the help text to make it more consistent and more > self-explanatory. I think we should also put the various options into a new line > in the help screen, not the single line dump of text it is currently. OK > > Btw., we also have a grammar problem with all things call chains: there's 800+ > occurances of 'callchain' in the perf code, and less than 20 spellings of 'call > chain'. But the latter is the correct variant: Google won't even let you search > for 'callchain' by default and corrects it to 'call chain' automatically. > > If you insist on searching for 'callchain', Google finds this number of hits: > > 'code callchain': 54,200 > 'code call chain': 141,000,000 > > I think it's pretty obvious what the dominant spelling is in the industry! ;-) > > So we should probably rename all occurances of 'callchain' to 'call chain' or > 'call_chain'. Not sure about this part. Do you really think it's worth changing? Thanks, Namhyung