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

Namhyung Kim
2014-01-09T12:57:50Z

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