Re: [PATCH 1/3] perf tools: Move callchain help messages to callchain.h

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Oct 22, 2015 at 09:13:49PM +0900, Namhyung Kim escreveu:
> On Thu, Oct 22, 2015 at 5:02 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Namhyung Kim <namhyung@kernel.org> 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

Done.
 
> >
> > 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

These can come on a followup patch, improving the situation, right?
 
> >
> > 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