Messages in this thread | | | Date | Mon, 12 Mar 2012 11:21:00 -0700 | From | Arun Sharma <> | Subject | Re: [PATCH 0/2] perf: add sort by inclusive time functionality (v2) |
| |
On 3/12/12 12:43 AM, Namhyung Kim wrote:
>> Known bugs: >> >> total_period computation is broken for order=callee >> > > I'd like to add two more :). > > * If perf record misses callchain info, perf report will get stuck. > * If it's used with "symbol" sort order, it'll get stuck too. >
Can you post the command lines you used to reproduce this? A backtrace on where perf report is getting stuck would be useful as well.
Since I didn't make any changes to the perf record or perf report code paths that don't involve "-s inclusive", I wonder if you see the same issues without my patch.
For missing callchain info cases, I'd expect cursor->nr to be zero. So loops such as:
+ for (i = 0; i < cursor->nr; i++) {
should terminate immediately.
> > BTW, I don't like the name 'inclusive' as a sort key. If it cares about > time, IMHO, the name should contain 'time' - something like 'itime' or > 'inctime'?
The existing sort orders: pid, comm, dso, symbol, parent -- all care about time, but none of them have time in their name?
> > Furthermore, I don't think it is a sort key. As it doesn't sort > anything, and only affects the way calculating symbol's period value, > wouldn't it be better making it a separate switch rather than a sort > key? Plus, checking whether it has callchain data and symbol sort key > might be added also.
Yes, we're still sorting by period, but a redefined meaning of period (please see my other mail about period vs period_self). Previous iterations of this code did use a top level flag, but that namespace is getting a bit crowded.
-Arun
| |