Re: [PATCH 12/18] perf ui/hist: Add support for event group view

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Nov 30, 2012 at 10:52 PM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> 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