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

[Namhyung Kim <namhyung@kernel.org>]

2014-01-08 (수), 15:59 -0300, 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.

Yeah, the above is the reason why I wrote this firstly.  And then I
thought it should be applied to the command line filter options too.

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

One thing related to it is when --children option is used.  Since total
period is added only for a real sample, if the sample is filtered but
the parents are not, the parents might have more than 100% overhead.

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

Hmm.. do you really want two columns instead of single column and a
switch/option?  Then the (second) %filtered column will be shown up only
if filtering is enabled.  Isn't it annoying for a dynamic filtering
(i.e. '/' key on TUI)?

> 
> So we would have new field: hists->stats.total_filtered_period, and
> hists->stats.nr_filtered_entries, for this, etc.
> 
> What do you think?

I'm fine with it if we decide to support two columns at the same time.

Thanks,
Namhyung