Re: [PATCH 12/18] perf ui/hist: Add support for event group view
Hi Arnaldo, On Tue, 4 Dec 2012 10:50:49 -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Dec 04, 2012 at 06:16:59PM +0900, Namhyung Kim escreveu: >> On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo wrote: > >> Ah, I missed your point. Just got it now, will try this approach. So >> you want to see no "0.00%" for a dummy entry, right? > > That wasn't the point, and perhaps printing 0.00% pollutes the screen > needlessly or may be a help in seeing the columns more clearly, no > strong opinion at this point, please experiment. > > The point was that there seems to be no need for the temporary array. Yeah, it shows my really bad writing skill. :( I meant I got your point of not using a temporary array. And asked additional question of your preference in a dummy entry. Sorry for confusing you. Btw, I found that printing 0.00% is useful for a case of field_sep and GTK+ output. So unless someone objects, I'll keep it. 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/
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
Em Tue, Dec 04, 2012 at 06:16:59PM +0900, Namhyung Kim escreveu: > On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo wrote: > Ah, I missed your point. Just got it now, will try this approach. So > you want to see no "0.00%" for a dummy entry, right? That wasn't the point, and perhaps printing 0.00% pollutes the screen needlessly or may be a help in seeing the columns more clearly, no strong opinion at this point, please experiment. The point was that there seems to be no need for the temporary array. - Arnaldo -- 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/
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo
wrote:
> Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
>> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)
>> > \
>> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry
>> > *he) \
>> > +{
>> > \
>> > + int ret;
>> > \
>> > + double percent = 0.0;
>> > \
>> > + struct hists *hists = he->hists;
>> > \
>> > +
>> > \
>> > + if (hists->stats.total_period)
>> > \
>> > + percent = 100.0 * he->stat._field / hists->stats.total_period;
>> > \
>> > +
>> > \
>> > + ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%",
>> > percent); \
>> > +
>> > \
>> > + if (symbol_conf.event_group) {
>> > \
>> > + int i;
>> > \
>> > + struct perf_evsel *evsel = hists_to_evsel(hists);
>> > \
>> > + struct hist_entry *pair;
>> > \
>> > + int nr_members = evsel->nr_members;
>> > \
>> > + double *percents;
>> > \
>> > +
>> > \
>> > + if (nr_members <= 1)
>> > \
>> > + return ret;
>> > \
>> > +
>> > \
>> > + percents = zalloc(sizeof(*percents) * nr_members);
>> > \
>> > + if (percents == NULL) {
>> > \
>> > + pr_warning("Not enough memory!\n");
>> > \
>> > + return ret;
>> > \
>> > + }
>> > \
>
> Why do we need to zalloc this percents array?
>
>> > + list_for_each_entry(pair, &he->pairs.head, pairs.node) {
>> > \
>> > + u64 period = pair->stat._field;
>> > \
>> > + u64 total = pair->hists->stats.total_period;
>> > \
>> > +
>> > \
>> > + if (!total)
>> > \
>> > + continue;
>> > \
>> > +
>> > \
>> > + evsel = hists_to_evsel(pair->hists);
>> > \
>> > + i = perf_evsel__group_idx(evsel);
>> > \
>> > + percents[i] = 100.0 * period / total;
>> > \
>
> Why not use percent_color_snprintf here, using some "%*s" thing that
> uses i to position the output in the right column? This way the
> following loop can be ditched as well. No?
>
>> > + }
>> > \
>> > +
>> > \
>> > + for (i = 1; i < nr_members; i++) {
>> > \
>> > + ret += percent_color_snprintf(hpp->buf + ret,
>> > \
>> > + hpp->size - ret,
>> > \
>> > + " %6.2f%%",
>> > percents[i]); \
>> > + }
>> > \
>> > + free(percents);
>> > \
>> > + }
>> > \
>> > + return ret;
>> > \
>
> - Arnaldo
Ah, I missed your point. Just got it now, will try this approach. So
you want to see no "0.00%" for a dummy entry, right?
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 t
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Mon, 3 Dec 2012 16:57:36 +0100, Jiri Olsa wrote: > On Mon, Dec 03, 2012 at 07:39:31PM +0900, Namhyung Kim wrote: >> On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote: >> > On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote: >> >> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa wrote: >> >> > ok, so this is the part thats common for both multi diff and group >> >> > report and hugely depends on how we link matching hist_entry > > SNIP > >> > we discussed with Arnaldo and decided to stay with current approach and >> > make the change later if needed.. to be able to see/meassure the benefit >> > >> > I made some initial attempt to workaround this and it appears to be >> > not that bad ;) I'll repost my changes shortly.. >> >> Hmm.. so are you OK with this patch now? > > yep, well except for following macros: > > __HPP_COLOR_PERCENT_FN > __HPP_ENTRY_PERCENT_FN > __HPP_ENTRY_RAW_FN > > being so long.. any chance the main part of it being in function? > > Seems like '_type' is just in function name, but the '_field' might > be the killer ;) maybe it could be 'used' in such function via PERF_HPP__* > enums instead..? Okay, I'll re-think about it tomorrow. 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/
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Mon, Dec 03, 2012 at 07:39:31PM +0900, Namhyung Kim wrote: > On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote: > > On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote: > >> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa wrote: > >> > ok, so this is the part thats common for both multi diff and group > >> > report and hugely depends on how we link matching hist_entry SNIP > > we discussed with Arnaldo and decided to stay with current approach and > > make the change later if needed.. to be able to see/meassure the benefit > > > > I made some initial attempt to workaround this and it appears to be > > not that bad ;) I'll repost my changes shortly.. > > Hmm.. so are you OK with this patch now? yep, well except for following macros: __HPP_COLOR_PERCENT_FN __HPP_ENTRY_PERCENT_FN __HPP_ENTRY_RAW_FN being so long.. any chance the main part of it being in function? Seems like '_type' is just in function name, but the '_field' might be the killer ;) maybe it could be 'used' in such function via PERF_HPP__* enums instead..? thanks, jirka -- 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/
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Mon, 3 Dec 2012 11:23:27 +0100, Jiri Olsa wrote: > On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote: >> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa wrote: >> > ok, so this is the part thats common for both multi diff and group >> > report and hugely depends on how we link matching hist_entry >> > >> > To sum to what group report does here: >> > >> > 1) starting with event group >> > 2) the function 'he' belongs to leader hists >> > 3) display leaders data value >> > 4) if 'symbol_conf.event_group' is enabled, we want to display all >> >group members data values in a column >> > 5) say we have group of 3 events 'e1,e2,e3' and e1 being the leader, >> >we have following possibilities: >> >- e1 have no pairs >> >- e1 is paired with e2 >> >- e1 is paired with e3 >> >- e1 is paired with e2 and e3 (e1 could also be dummy one) >> > 6) need to display 3 values wrt to e1,e2,e3 output order >> > 7) because events belongs to a group, each evsel carries group idx >> > 8) so we walk all pairs and compute the eX value and put it >> >into temporary array based on its group idx >> > 9) finally we display all temporary array values >> > >> > >> > To sum up what multi diff needs to do here: >> > >> > 1) starting with 3 separate matching events from different >> >evlists(sessions) >> > 2 - 5) are similar >> > 6) need to display single diff value of hist_entry that >> >belongs to evsel, that belongs to a column we are just >> >displaying value for >> > 7) events are not part of group; based on >> >[PATCH 02/14] perf tool: Add struct perf_hpp_fmt into hpp callbacks >> >we can tell what column we are in -> session -> evlist >> > 8) we need to walk all pairs and for each hist_entry: >> >- compare all evsels (from point 7 evlist) >> > and match hists pointer >> >- when found, we have a matching hist_entry for this column >> > 9) print out the value of matching hist_entry >> > >> > >> > I think both this processing could be simplified by having hist_entry >> > pairs connected via array and not linked list. >> > >> > >> > For group report, each leader hist_entry would have 'pairs' array >> > with the size of event group. Then we could omit the temporary array >> > creation by: >> > - walking the leaders pairs >> > - when pair is found -> compute data -> display >> > - when pair is NULL -> display 0 >> > >> > >> > For multi diff, each baseline hist_entry would have 'pairs' array >> > with the size equal to number of data files on diff command input. >> > This way we could use the data from point 7 to directly access >> > related hist_entry. >> > >> > >> > ufff... thoughts? ;-) >> >> Nice summary, really! :) >> >> Yeah, I do think it's better to use array for this. If Arnaldo has no >> objection to this approach, I'll convert my code to use the array. > > we discussed with Arnaldo and decided to stay with current approach and > make the change later if needed.. to be able to see/meassure the benefit > > I made some initial attempt to workaround this and it appears to be > not that bad ;) I'll repost my changes shortly.. Hmm.. so are you OK with this patch now? 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/
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Mon, Dec 03, 2012 at 10:56:28AM +0900, Namhyung Kim wrote:
> Hi Jiri,
>
> On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa wrote:
> > On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
> >
> > SNIP
> >
> >> +#define __HPP_COLOR_PERCENT_FN(_type, _field)
> >>\
> >> +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry
> >> *he) \
> >> +{
> >>\
> >> + int ret;
> >>\
> >> + double percent = 0.0;
> >>\
> >> + struct hists *hists = he->hists;
> >>\
> >> +
> >>\
> >> + if (hists->stats.total_period)
> >>\
> >> + percent = 100.0 * he->stat._field /
> >> hists->stats.total_period; \
> >> +
> >>\
> >> + ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%",
> >> percent); \
> >> +
> >>\
> >> + if (symbol_conf.event_group) {
> >>\
> >> + int i;
> >>\
> >> + struct perf_evsel *evsel = hists_to_evsel(hists);
> >>\
> >> + struct hist_entry *pair;
> >>\
> >> + int nr_members = evsel->nr_members;
> >>\
> >> + double *percents;
> >>\
> >> +
> >>\
> >> + if (nr_members <= 1)
> >>\
> >> + return ret;
> >>\
> >> +
> >>\
> >> + percents = zalloc(sizeof(*percents) * nr_members);
> >>\
> >> + if (percents == NULL) {
> >>\
> >> + pr_warning("Not enough memory!\n");
> >>\
> >> + return ret;
> >>\
> >> + }
> >>\
> >> +
> >>\
> >> + list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> >>\
> >> + u64 period = pair->stat._field;
> >>\
> >> + u64 total = pair->hists->stats.total_period;
> >>\
> >> +
> >>\
> >> + if (!total)
> >>\
> >> + continue;
> >>\
> >> +
> >>\
> >> + evsel = hists_to_evsel(pair->hists);
> >>\
> >> + i = perf_evsel__group_idx(evsel);
> >>\
> >> + percents[i] = 100.0 * period / total;
> >>\
> >> + }
> >>\
> >> +
> >>\
> >> + for (i = 1; i < nr_members; i++) {
> >>\
> >> + ret += percent_color_snprintf(hpp->buf + ret,
> >>\
> >> + hpp->size - ret,
> >>\
> >> + " %6.2f%%",
> >> percents[i]); \
> >> + }
> >>\
> >> + free(percents);
> >>\
> >> + }
> >>\
> >> + return ret;
> >>\
> >> +}
> >
> > ok, so this is the part thats common for both multi diff and group
> > report and hugely depends on how we link matching hist_entry
> >
> > To sum to what group report does here:
> >
> > 1) starting with event group
> > 2) the function 'he' belongs to leader hists
> > 3) display leaders data value
> > 4) if 'symbol_conf.event_group' is enabled, we want to display all
> >group members dat
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
Hi Jiri,
On Fri, Nov 30, 2012 at 10:29 PM, Jiri Olsa wrote:
> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> +#define __HPP_COLOR_PERCENT_FN(_type, _field)
>> \
>> +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he)
>> \
>> +{
>> \
>> + int ret;
>> \
>> + double percent = 0.0;
>> \
>> + struct hists *hists = he->hists;
>> \
>> +
>> \
>> + if (hists->stats.total_period)
>> \
>> + percent = 100.0 * he->stat._field / hists->stats.total_period;
>> \
>> +
>> \
>> + ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%",
>> percent); \
>> +
>> \
>> + if (symbol_conf.event_group) {
>> \
>> + int i;
>> \
>> + struct perf_evsel *evsel = hists_to_evsel(hists);
>> \
>> + struct hist_entry *pair;
>> \
>> + int nr_members = evsel->nr_members;
>> \
>> + double *percents;
>> \
>> +
>> \
>> + if (nr_members <= 1)
>> \
>> + return ret;
>> \
>> +
>> \
>> + percents = zalloc(sizeof(*percents) * nr_members);
>> \
>> + if (percents == NULL) {
>> \
>> + pr_warning("Not enough memory!\n");
>> \
>> + return ret;
>> \
>> + }
>> \
>> +
>> \
>> + list_for_each_entry(pair, &he->pairs.head, pairs.node) {
>> \
>> + u64 period = pair->stat._field;
>> \
>> + u64 total = pair->hists->stats.total_period;
>> \
>> +
>> \
>> + if (!total)
>> \
>> + continue;
>> \
>> +
>> \
>> + evsel = hists_to_evsel(pair->hists);
>> \
>> + i = perf_evsel__group_idx(evsel);
>> \
>> + percents[i] = 100.0 * period / total;
>> \
>> + }
>> \
>> +
>> \
>> + for (i = 1; i < nr_members; i++) {
>> \
>> + ret += percent_color_snprintf(hpp->buf + ret,
>> \
>> + hpp->size - ret,
>> \
>> + " %6.2f%%",
>> percents[i]); \
>> + }
>> \
>> + free(percents);
>> \
>> + }
>> \
>> + return ret;
>> \
>> +}
>
> ok, so this is the part thats common for both multi diff and group
> report and hugely depends on how we link matching hist_entry
>
> To sum to what group report does here:
>
> 1) starting with event group
> 2) the function 'he' belongs to leader hists
> 3) display leaders data value
> 4) if 'symbol_conf.event_group' is enabled, we want to display all
>group members data values in a column
> 5) say we have group of 3 events 'e1,e2,e3' and e1 being the leader,
>we have following possibilities:
>- e1 have no pairs
>- e1 is paired with e2
>- e1 is paired with e3
>- e1 is paired with e2 and e3 (e1 could also be dummy one)
> 6) need to displ
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
On Fri, 30 Nov 2012 10:52:15 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
>> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
>> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)
>> > \
>> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry
>> > *he)\
>> > +{
>> > \
>> > + int ret;
>> > \
>> > + double percent = 0.0;
>> > \
>> > + struct hists *hists = he->hists;
>> > \
>> > +
>> > \
>> > + if (hists->stats.total_period)
>> > \
>> > + percent = 100.0 * he->stat._field / hists->stats.total_period;
>> > \
>> > +
>> > \
>> > + ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);
>> > \
>> > +
>> > \
>> > + if (symbol_conf.event_group) {
>> > \
>> > + int i;
>> > \
>> > + struct perf_evsel *evsel = hists_to_evsel(hists);
>> > \
>> > + struct hist_entry *pair;
>> > \
>> > + int nr_members = evsel->nr_members;
>> > \
>> > + double *percents;
>> > \
>> > +
>> > \
>> > + if (nr_members <= 1)
>> > \
>> > + return ret;
>> > \
>> > +
>> > \
>> > + percents = zalloc(sizeof(*percents) * nr_members);
>> > \
>> > + if (percents == NULL) {
>> > \
>> > + pr_warning("Not enough memory!\n");
>> > \
>> > + return ret;
>> > \
>> > + }
>> > \
>
> Why do we need to zalloc this percents array?
>
>> > + list_for_each_entry(pair, &he->pairs.head, pairs.node) {
>> > \
>> > + u64 period = pair->stat._field;
>> > \
>> > + u64 total = pair->hists->stats.total_period;
>> > \
>> > +
>> > \
>> > + if (!total)
>> > \
>> > + continue;
>> > \
>> > +
>> > \
>> > + evsel = hists_to_evsel(pair->hists);
>> > \
>> > + i = perf_evsel__group_idx(evsel);
>> > \
>> > + percents[i] = 100.0 * period / total;
>> > \
>
> Why not use percent_color_snprintf here, using some "%*s" thing that
> uses i to position the output in the right column? This way the
> following loop can be ditched as well. No?
It's because it's possible that an entry didn't have pairs for every
group member. Say, there's a group consists of 3 members (1 leader + 2
member). It's quite legitimate that a leader hist entry has just one
pair for a member and miss another. So I allocated a zero-filled array,
and filled what it has, and print them all in the for loop below.
Thanks,
Namhyung
>
>> > + }
>> > \
>> > +
>> > \
>> > + for (i = 1; i < nr_members; i++) {
>> > \
>> > + ret += percent_color_snprintf(hpp->buf + ret,
>> > \
>> > +hpp->size - ret,
>> > \
>> > +" %6.2f%%", percents[i]);
>> > \
>> > + }
>> > \
>> > + free(percents);
>> > \
>> > + }
>> > \
>> > + return ret;
>> > \
>
> - Arnaldo
--
To unsubscribe from this
Re: [PATCH 12/18] perf ui/hist: Add support for event group view
Em Fri, Nov 30, 2012 at 02:29:43PM +0100, Jiri Olsa escreveu:
> On Thu, Nov 29, 2012 at 03:38:40PM +0900, Namhyung Kim wrote:
> > +#define __HPP_COLOR_PERCENT_FN(_type, _field)
> > \
> > +static int hpp__color_##_type(struct perf_hpp *hpp, struct hist_entry *he)
> > \
> > +{
> > \
> > + int ret;
> > \
> > + double percent = 0.0;
> > \
> > + struct hists *hists = he->hists;
> > \
> > +
> > \
> > + if (hists->stats.total_period)
> > \
> > + percent = 100.0 * he->stat._field / hists->stats.total_period;
> > \
> > +
> > \
> > + ret = percent_color_snprintf(hpp->buf, hpp->size, " %6.2f%%", percent);
> > \
> > +
> > \
> > + if (symbol_conf.event_group) {
> > \
> > + int i;
> > \
> > + struct perf_evsel *evsel = hists_to_evsel(hists);
> > \
> > + struct hist_entry *pair;
> > \
> > + int nr_members = evsel->nr_members;
> > \
> > + double *percents;
> > \
> > +
> > \
> > + if (nr_members <= 1)
> > \
> > + return ret;
> > \
> > +
> > \
> > + percents = zalloc(sizeof(*percents) * nr_members);
> > \
> > + if (percents == NULL) {
> > \
> > + pr_warning("Not enough memory!\n");
> > \
> > + return ret;
> > \
> > + }
> > \
Why do we need to zalloc this percents array?
> > + list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> > \
> > + u64 period = pair->stat._field;
> > \
> > + u64 total = pair->hists->stats.total_period;
> > \
> > +
> > \
> > + if (!total)
> > \
> > + continue;
> > \
> > +
> > \
> > + evsel = hists_to_evsel(pair->hists);
> > \
> > + i = perf_evsel__group_idx(evsel);
> > \
> > + percents[i] = 100.0 * period / total;
> > \
Why not use percent_color_snprintf here, using some "%*s" thing that
uses i to position the output in the right column? This way the
following loop can be ditched as well. No?
> > + }
> > \
> > +
> > \
> > + for (i = 1; i < nr_members; i++) {
> > \
> > + ret += percent_color_snprintf(hpp->buf + ret,
> > \
> > + hpp->size - ret,
> > \
> > + " %6.2f%%", percents[i]);
> > \
> > + }
> > \
> > + free(percents);
> > \
> > + }
> > \
> > + return ret;
> > \
- Arnaldo
--
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/
