On Thu, Oct 22, 2015 at 5:02 PM, Ingo Molnar <mi...@kernel.org> wrote:
>
> * Namhyung Kim <namhy...@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
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
