Re: [PATCH 01/28] perf tools: Insert filtered entries to hists also

[Arnaldo Carvalho de Melo]

Em Wed, Jan 08, 2014 at 05:22:53PM +0100, Jiri Olsa escreveu:
> On Wed, Jan 08, 2014 at 09:41:13AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 08, 2014 at 05:46:06PM +0900, Namhyung Kim escreveu:
> > > Currently if a sample was filtered by command line option, it just
> > > dropped.  But this affects final output in that the percentage can be
> > > different since the filtered entries were not included to the total.
> > > 
> > > For example, if an original output looked like below:
> > 
> > Humm, if one says that he/she is interested on just samples for a and b,
> > the current behaviour will state how many of the filtered samples are
> > for a and b, which is valid.
> > 
> >  I bet the number of samples will reflect that as well, but you filtered
> > it out, yes, it stays there, so the percentages are relative to the
> > number of samples.
> > 
> > So I think this change in behaviour is wrong, no?
> > 
> hi,
> haven't checked the implementation yet, but it kind of does
> what I'd expect for symbol filtering:
> 
> perf report
> ...
>  22.00%  yes  libc-2.17.so       [.] __strlen_sse2
>  11.79%  yes  libc-2.17.so       [.] fputs_unlocked
>   9.65%  yes  libc-2.17.so       [.] __GI___mempcpy
>   1.91%  yes  yes                [.] fputs_unlocked@plt
> ...
> 
> search (press '/') for fputs_unlocked (with Namhyung's change):
>  11.79%  yes  libc-2.17.so  [.] fputs_unlocked
>   1.91%  yes  yes           [.] fputs_unlocked@plt
> 
> while the current one shows:
>  86.08%  yes  libc-2.17.so  [.] fputs_unlocked
>  13.92%  yes  yes           [.] fputs_unlocked@plt
> 
> which annoys me when searching for 'invisible' symbol
> within tons of others.. I had to do that grep thing
> you showed.
> 
> I'd like to have the Namhyung's change behaviour as default,
> but I'll be happy with some switch as well ;-)

I understand the desire for this different mode, looks indeed useful.

So I think that this is a new feature and as so we should provide it as
an option, that may (or not) become the default.

Some concerns I have are that when we go on filtering we have to have
all the things that are zeroed to then get accrued for each hist entry
that matches the filter being applied and now at least a nr_entries
field got out of the if (al.filtered) block, i.e. in the end we will
have the number of hist entries entries filtered but continue having the
total period for all (filtered or not) hist entries.

Having it as a separate feature would allow to have both views:

1. the percentages relative to the filtered samples
2. the percentages relative to all (filtered or not) samples

Being selectable on the command line and also with a hotkey to provide
two columns: %total, %filtered.

So we would have new field: hists->stats.total_filtered_period, and
hists->stats.nr_filtered_entries, for this, etc.

What do you think?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/