Hello,
This work is to make the output compact by removing dummy events in
the output. The dummy events are used to save side-band information
like task creation or memory address space change using mmap(2). But
after collecting these, it's not used because it won't have any
samples.
v2 changes)
* just hide the (dummy) event instead of removing it from evlist
Sometimes users want to run perf report --group to show all recorded
events together but they are not interested in those dummy events.
This just wastes the precious screen space so we want to get rid of
them after use.
perf report already has --skip-empty option to skip 0 result in the
stat output. I think we can extend it to skip empty events that have
no samples.
Example output:
Before)
#
# Samples: 232 of events 'cpu/mem-loads,ldlat=30/P, cpu/mem-stores/P, dummy:u'
# Event count (approx.): 3089861
#
# Overhead Command Shared Object Symbol
# ........................ ........... ................. .....................................
#
9.29% 0.00% 0.00% swapper [kernel.kallsyms] [k] update_blocked_averages
5.26% 0.15% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_se
4.15% 0.00% 0.00% perf-exec [kernel.kallsyms] [k] slab_update_freelist.isra.0
3.87% 0.00% 0.00% perf-exec [kernel.kallsyms] [k] memcg_slab_post_alloc_hook
3.79% 0.17% 0.00% swapper [kernel.kallsyms] [k] enqueue_task_fair
3.63% 0.00% 0.00% sleep [kernel.kallsyms] [k] next_uptodate_page
2.86% 0.00% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_cfs_rq
2.78% 0.00% 0.00% swapper [kernel.kallsyms] [k] __schedule
2.34% 0.00% 0.00% swapper [kernel.kallsyms] [k] intel_idle
2.32% 0.97% 0.00% swapper [kernel.kallsyms] [k] psi_group_change
After)
#
# Samples: 232 of events 'cpu/mem-loads,ldlat=30/P, cpu/mem-stores/P'
# Event count (approx.): 3089861
#
# Overhead Command Shared Object Symbol
# ................ ........... ................. .....................................
#
9.29% 0.00% swapper [kernel.kallsyms] [k] update_blocked_averages
5.26% 0.15% swapper [kernel.kallsyms] [k] __update_load_avg_se
4.15% 0.00% perf-exec [kernel.kallsyms] [k] slab_update_freelist.isra.0
3.87% 0.00% perf-exec [kernel.kallsyms] [k] memcg_slab_post_alloc_hook
3.79% 0.17% swapper [kernel.kallsyms] [k] enqueue_task_fair
3.63% 0.00% sleep [kernel.kallsyms] [k] next_uptodate_page
2.86% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_cfs_rq
2.78% 0.00% swapper [kernel.kallsyms] [k] __schedule
2.34% 0.00% swapper [kernel.kallsyms] [k] intel_idle
2.32% 0.97% swapper [kernel.kallsyms] [k] psi_group_change
Now 'Overhead' column only has two values for mem-loads and mem-stores.
Thanks,
Namhyung
Namhyung Kim (4):
perf hist: Factor out __hpp__fmt_print()
perf hist: Simplify __hpp_fmt() using hpp_fmt_data
perf hist: Add symbol_conf.skip_empty
perf hist: Honor symbol_conf.skip_empty
tools/perf/builtin-annotate.c | 4 +-
tools/perf/builtin-report.c | 12 +--
tools/perf/ui/hist.c | 144 ++++++++++++++++-----------------
tools/perf/ui/stdio/hist.c | 5 +-
tools/perf/util/events_stats.h | 3 +-
tools/perf/util/evsel.c | 13 ++-
tools/perf/util/hist.c | 6 +-
tools/perf/util/hist.h | 3 +-
tools/perf/util/python.c | 3 +
tools/perf/util/session.c | 5 +-
tools/perf/util/session.h | 3 +-
tools/perf/util/symbol_conf.h | 3 +-
12 files changed, 105 insertions(+), 99 deletions(-)
--
2.45.1.288.g0e0cd299f1-goog
Split the logic to print the histogram values according to the format
string. This was used in 3 different places so it's better to move out
the logic into a function.
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/ui/hist.c | 83 +++++++++++++++++++-------------------------
1 file changed, 36 insertions(+), 47 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 685ba2a54fd8..e30fcb1e87e7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -23,35 +23,42 @@
__ret; \
})
-static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
- hpp_field_fn get_field, const char *fmt, int len,
- hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
+static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
+ int nr_samples, const char *fmt, int len,
+ hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
{
- int ret;
- struct hists *hists = he->hists;
- struct evsel *evsel = hists_to_evsel(hists);
- char *buf = hpp->buf;
- size_t size = hpp->size;
-
if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
double percent = 0.0;
u64 total = hists__total_period(hists);
if (total)
- percent = 100.0 * get_field(he) / total;
+ percent = 100.0 * val / total;
- ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
- } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
- double average = 0;
+ return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
+ }
- if (he->stat.nr_events)
- average = 1.0 * get_field(he) / he->stat.nr_events;
+ if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
+ double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
- ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
- } else {
- ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
+ return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
}
+ return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
+}
+
+static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
+ hpp_field_fn get_field, const char *fmt, int len,
+ hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
+{
+ int ret;
+ struct hists *hists = he->hists;
+ struct evsel *evsel = hists_to_evsel(hists);
+ char *buf = hpp->buf;
+ size_t size = hpp->size;
+
+ ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
+ fmt, len, print_fn, fmtype);
+
if (evsel__is_group_event(evsel)) {
int prev_idx, idx_delta;
struct hist_entry *pair;
@@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
while (idx_delta--) {
/*
- * zero-fill group members in the middle which
- * have no sample
+ * zero-fill group members in the middle which have
+ * no samples, pair->hists is not correct but it's
+ * fine since the value is 0.
*/
- if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
- ret += hpp__call_print_fn(hpp, print_fn,
- fmt, len, 0.0);
- } else {
- ret += hpp__call_print_fn(hpp, print_fn,
- fmt, len, 0ULL);
- }
+ ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
+ fmt, len, print_fn, fmtype);
}
- if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
- ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
- 100.0 * period / total);
- } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
- double avg = nr_samples ? (period / nr_samples) : 0;
-
- ret += hpp__call_print_fn(hpp, print_fn, fmt,
- len, avg);
- } else {
- ret += hpp__call_print_fn(hpp, print_fn, fmt,
- len, period);
- }
+ ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
+ fmt, len, print_fn, fmtype);
prev_idx = evsel__group_idx(evsel);
}
@@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
while (idx_delta--) {
/*
- * zero-fill group members at last which have no sample
+ * zero-fill group members at last which have no sample.
+ * the hists is not correct but it's fine like above.
*/
- if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
- ret += hpp__call_print_fn(hpp, print_fn,
- fmt, len, 0.0);
- } else {
- ret += hpp__call_print_fn(hpp, print_fn,
- fmt, len, 0ULL);
- }
+ ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
+ fmt, len, print_fn, fmtype);
}
}
--
2.45.1.288.g0e0cd299f1-goog
The struct hpp_fmt_data is to keep the values for each group members so
it doesn't need to check the event index in the group.
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index e30fcb1e87e7..539978c95cfd 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
}
+struct hpp_fmt_data {
+ struct hists *hists;
+ u64 val;
+ int samples;
+};
+
static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
hpp_field_fn get_field, const char *fmt, int len,
hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
{
- int ret;
+ int ret = 0;
struct hists *hists = he->hists;
struct evsel *evsel = hists_to_evsel(hists);
+ struct evsel *pos;
char *buf = hpp->buf;
size_t size = hpp->size;
+ int i, nr_members = 1;
+ struct hpp_fmt_data *data;
+
+ if (evsel__is_group_event(evsel))
+ nr_members = evsel->core.nr_members;
+
+ data = calloc(nr_members, sizeof(*data));
+ if (data == NULL)
+ return 0;
+
+ i = 0;
+ for_each_group_evsel(pos, evsel)
+ data[i++].hists = evsel__hists(pos);
- ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
- fmt, len, print_fn, fmtype);
+ data[0].val = get_field(he);
+ data[0].samples = he->stat.nr_events;
if (evsel__is_group_event(evsel)) {
- int prev_idx, idx_delta;
struct hist_entry *pair;
- int nr_members = evsel->core.nr_members;
-
- prev_idx = evsel__group_idx(evsel);
list_for_each_entry(pair, &he->pairs.head, pairs.node) {
- u64 period = get_field(pair);
- u64 total = hists__total_period(pair->hists);
- int nr_samples = pair->stat.nr_events;
-
- if (!total)
- continue;
+ for (i = 0; i < nr_members; i++) {
+ if (data[i].hists != pair->hists)
+ continue;
- evsel = hists_to_evsel(pair->hists);
- idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
-
- while (idx_delta--) {
- /*
- * zero-fill group members in the middle which have
- * no samples, pair->hists is not correct but it's
- * fine since the value is 0.
- */
- ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
- fmt, len, print_fn, fmtype);
+ data[i].val = get_field(pair);
+ data[i].samples = pair->stat.nr_events;
+ break;
}
-
- ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
- fmt, len, print_fn, fmtype);
-
- prev_idx = evsel__group_idx(evsel);
}
+ }
- idx_delta = nr_members - prev_idx - 1;
-
- while (idx_delta--) {
- /*
- * zero-fill group members at last which have no sample.
- * the hists is not correct but it's fine like above.
- */
- ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
- fmt, len, print_fn, fmtype);
- }
+ for (i = 0; i < nr_members; i++) {
+ ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
+ data[i].samples, fmt, len,
+ print_fn, fmtype);
}
+ free(data);
+
/*
* Restore original buf and size as it's where caller expects
* the result will be saved.
--
2.45.1.288.g0e0cd299f1-goog
Add the skip_empty flag to symbol_conf and set the value from the report
command to preserve the existing behavior. This makes the code simpler
and will be needed other code which is hard to add a new argument.
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-annotate.c | 4 ++--
tools/perf/builtin-report.c | 12 ++++++------
tools/perf/ui/stdio/hist.c | 5 ++---
tools/perf/util/events_stats.h | 3 +--
tools/perf/util/hist.c | 6 +++---
tools/perf/util/hist.h | 3 +--
tools/perf/util/session.c | 5 ++---
tools/perf/util/session.h | 3 +--
tools/perf/util/symbol_conf.h | 3 ++-
9 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 50d2fb222d48..b10b7f005658 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -571,8 +571,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
goto out;
if (dump_trace) {
- perf_session__fprintf_nr_events(session, stdout, false);
- evlist__fprintf_nr_events(session->evlist, stdout, false);
+ perf_session__fprintf_nr_events(session, stdout);
+ evlist__fprintf_nr_events(session->evlist, stdout);
goto out;
}
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 69618fb0110b..9718770facb5 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -810,8 +810,8 @@ static int stats_print(struct report *rep)
{
struct perf_session *session = rep->session;
- perf_session__fprintf_nr_events(session, stdout, rep->skip_empty);
- evlist__fprintf_nr_events(session->evlist, stdout, rep->skip_empty);
+ perf_session__fprintf_nr_events(session, stdout);
+ evlist__fprintf_nr_events(session->evlist, stdout);
return 0;
}
@@ -1089,10 +1089,8 @@ static int __cmd_report(struct report *rep)
perf_session__fprintf_dsos(session, stdout);
if (dump_trace) {
- perf_session__fprintf_nr_events(session, stdout,
- rep->skip_empty);
- evlist__fprintf_nr_events(session->evlist, stdout,
- rep->skip_empty);
+ perf_session__fprintf_nr_events(session, stdout);
+ evlist__fprintf_nr_events(session->evlist, stdout);
return 0;
}
}
@@ -1562,6 +1560,8 @@ int cmd_report(int argc, const char **argv)
data.path = input_name;
data.force = symbol_conf.force;
+ symbol_conf.skip_empty = report.skip_empty;
+
repeat:
session = perf_session__new(&data, &report.tool);
if (IS_ERR(session)) {
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index b849caace398..9372e8904d22 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -897,8 +897,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
return ret;
}
-size_t events_stats__fprintf(struct events_stats *stats, FILE *fp,
- bool skip_empty)
+size_t events_stats__fprintf(struct events_stats *stats, FILE *fp)
{
int i;
size_t ret = 0;
@@ -910,7 +909,7 @@ size_t events_stats__fprintf(struct events_stats *stats, FILE *fp,
name = perf_event__name(i);
if (!strcmp(name, "UNKNOWN"))
continue;
- if (skip_empty && !stats->nr_events[i])
+ if (symbol_conf.skip_empty && !stats->nr_events[i])
continue;
if (i && total) {
diff --git a/tools/perf/util/events_stats.h b/tools/perf/util/events_stats.h
index 8fecc9fbaecc..f43e5b1a366a 100644
--- a/tools/perf/util/events_stats.h
+++ b/tools/perf/util/events_stats.h
@@ -52,7 +52,6 @@ struct hists_stats {
void events_stats__inc(struct events_stats *stats, u32 type);
-size_t events_stats__fprintf(struct events_stats *stats, FILE *fp,
- bool skip_empty);
+size_t events_stats__fprintf(struct events_stats *stats, FILE *fp);
#endif /* __PERF_EVENTS_STATS_ */
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 2e9e193179dd..f028f113c4fd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2706,8 +2706,7 @@ void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
}
}
-size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp,
- bool skip_empty)
+size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp)
{
struct evsel *pos;
size_t ret = 0;
@@ -2715,7 +2714,8 @@ size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp,
evlist__for_each_entry(evlist, pos) {
struct hists *hists = evsel__hists(pos);
- if (skip_empty && !hists->stats.nr_samples && !hists->stats.nr_lost_samples)
+ if (symbol_conf.skip_empty && !hists->stats.nr_samples &&
+ !hists->stats.nr_lost_samples)
continue;
ret += fprintf(fp, "%s stats:\n", evsel__name(pos));
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 8fb3bdd29188..5273f5c37050 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -375,8 +375,7 @@ void hists__inc_nr_lost_samples(struct hists *hists, u32 lost);
size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
int max_cols, float min_pcnt, FILE *fp,
bool ignore_callchains);
-size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp,
- bool skip_empty);
+size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp);
void hists__filter_by_dso(struct hists *hists);
void hists__filter_by_thread(struct hists *hists);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index a10343b9dcd4..0ec92d47373c 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2696,8 +2696,7 @@ size_t perf_session__fprintf_dsos_buildid(struct perf_session *session, FILE *fp
return machines__fprintf_dsos_buildid(&session->machines, fp, skip, parm);
}
-size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp,
- bool skip_empty)
+size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp)
{
size_t ret;
const char *msg = "";
@@ -2707,7 +2706,7 @@ size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp,
ret = fprintf(fp, "\nAggregated stats:%s\n", msg);
- ret += events_stats__fprintf(&session->evlist->stats, fp, skip_empty);
+ ret += events_stats__fprintf(&session->evlist->stats, fp);
return ret;
}
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 3b0256e977a6..4c29dc86956f 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -130,8 +130,7 @@ size_t perf_session__fprintf_dsos(struct perf_session *session, FILE *fp);
size_t perf_session__fprintf_dsos_buildid(struct perf_session *session, FILE *fp,
bool (fn)(struct dso *dso, int parm), int parm);
-size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp,
- bool skip_empty);
+size_t perf_session__fprintf_nr_events(struct perf_session *session, FILE *fp);
void perf_session__dump_kmaps(struct perf_session *session);
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index c114bbceef40..657cfa5af43c 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -46,7 +46,8 @@ struct symbol_conf {
lazy_load_kernel_maps,
keep_exited_threads,
annotate_data_member,
- annotate_data_sample;
+ annotate_data_sample,
+ skip_empty;
const char *vmlinux_name,
*kallsyms_name,
*source_prefix,
--
2.45.1.288.g0e0cd299f1-goog
So that it can skip events with no sample according to the config value.
This can omit the dummy event in the output of perf report --group.
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/ui/hist.c | 18 ++++++++++++++++--
tools/perf/util/evsel.c | 13 ++++++++++---
tools/perf/util/python.c | 3 +++
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 539978c95cfd..d76062979ab7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -95,6 +95,10 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
}
for (i = 0; i < nr_members; i++) {
+ if (symbol_conf.skip_empty &&
+ data[i].hists->stats.nr_samples == 0)
+ continue;
+
ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
data[i].samples, fmt, len,
print_fn, fmtype);
@@ -296,8 +300,18 @@ static int hpp__width_fn(struct perf_hpp_fmt *fmt,
int len = fmt->user_len ?: fmt->len;
struct evsel *evsel = hists_to_evsel(hists);
- if (symbol_conf.event_group)
- len = max(len, evsel->core.nr_members * fmt->len);
+ if (symbol_conf.event_group) {
+ int nr = 0;
+ struct evsel *pos;
+
+ for_each_group_evsel(pos, evsel) {
+ if (!symbol_conf.skip_empty ||
+ evsel__hists(pos)->stats.nr_samples)
+ nr++;
+ }
+
+ len = max(len, nr * fmt->len);
+ }
if (len < (int)strlen(fmt->name))
len = strlen(fmt->name);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 4f818ab6b662..befb80c272d2 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -53,6 +53,7 @@
#include "../perf-sys.h"
#include "util/parse-branch-options.h"
#include "util/bpf-filter.h"
+#include "util/hist.h"
#include <internal/xyarray.h>
#include <internal/lib.h>
#include <internal/threadmap.h>
@@ -830,16 +831,22 @@ const char *evsel__group_name(struct evsel *evsel)
int evsel__group_desc(struct evsel *evsel, char *buf, size_t size)
{
int ret = 0;
+ bool first = true;
struct evsel *pos;
const char *group_name = evsel__group_name(evsel);
if (!evsel->forced_leader)
ret = scnprintf(buf, size, "%s { ", group_name);
- ret += scnprintf(buf + ret, size - ret, "%s", evsel__name(evsel));
+ for_each_group_evsel(pos, evsel) {
+ if (symbol_conf.skip_empty &&
+ evsel__hists(pos)->stats.nr_samples == 0)
+ continue;
- for_each_group_member(pos, evsel)
- ret += scnprintf(buf + ret, size - ret, ", %s", evsel__name(pos));
+ ret += scnprintf(buf + ret, size - ret, "%s%s",
+ first ? "" : ", ", evsel__name(pos));
+ first = false;
+ }
if (!evsel->forced_leader)
ret += scnprintf(buf + ret, size - ret, " }");
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 0aeb97c11c03..88f98f2772fb 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -23,6 +23,7 @@
#include "util/env.h"
#include "util/pmu.h"
#include "util/pmus.h"
+#include "util/symbol_conf.h"
#include <internal/lib.h>
#include "util.h"
@@ -50,6 +51,8 @@
#define Py_TYPE(ob) (((PyObject*)(ob))->ob_type)
#endif
+struct symbol_conf symbol_conf;
+
/*
* Avoid bringing in event parsing.
*/
--
2.45.1.288.g0e0cd299f1-goog
On Mon, Jun 03, 2024 at 03:44:09PM -0700, Namhyung Kim wrote:
> Split the logic to print the histogram values according to the format
> string. This was used in 3 different places so it's better to move out
> the logic into a function.
But are all really equivalent? I had difficulty following it, perhaps it
would be better to introduce the function and then go on making one by
one use it instead of doing it all at once?
So in the end we have:
static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
int nr_samples, const char *fmt, int len,
hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
{
if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
double percent = 0.0;
u64 total = hists__total_period(hists);
if (total)
percent = 100.0 * val / total;
return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
}
if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
}
return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
}
I.e. we do something with the last arg for hpp__call_print_fn() for the
PERF_HPP_FMT_TYPE__PERCENT and PERF_HPP_FMT_TYPE__AVERAGE fmtype's, but
we don't check for those fmtypes on each of the places that we now call
__hpp__fmt_print(), right?
That while (idx_delta--) cases is only interested in !=PERF_HPP_FMT_TYPE__RAW...
Nah, after trying to describe the confusion I think I clarified it,
probably leave it as you did :-\
Continuing...
- Arnaldo
> +++ b/tools/perf/ui/hist.c
> @@ -23,35 +23,42 @@
> __ret; \
> })
>
> -static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> - hpp_field_fn get_field, const char *fmt, int len,
> - hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> +static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> + int nr_samples, const char *fmt, int len,
> + hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> {
> - int ret;
> - struct hists *hists = he->hists;
> - struct evsel *evsel = hists_to_evsel(hists);
> - char *buf = hpp->buf;
> - size_t size = hpp->size;
> -
> if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> double percent = 0.0;
> u64 total = hists__total_period(hists);
>
> if (total)
> - percent = 100.0 * get_field(he) / total;
> + percent = 100.0 * val / total;
>
> - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> - } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> - double average = 0;
> + return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> + }
>
> - if (he->stat.nr_events)
> - average = 1.0 * get_field(he) / he->stat.nr_events;
> + if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> + double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
>
> - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
> - } else {
> - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
> + return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
> }
>
> + return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> +}
> +
> +static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> + hpp_field_fn get_field, const char *fmt, int len,
> + hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> +{
> + int ret;
> + struct hists *hists = he->hists;
> + struct evsel *evsel = hists_to_evsel(hists);
> + char *buf = hpp->buf;
> + size_t size = hpp->size;
> +
> + ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> + fmt, len, print_fn, fmtype);
> +
> if (evsel__is_group_event(evsel)) {
> int prev_idx, idx_delta;
> struct hist_entry *pair;
> @@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>
> while (idx_delta--) {
> /*
> - * zero-fill group members in the middle which
> - * have no sample
> + * zero-fill group members in the middle which have
> + * no samples, pair->hists is not correct but it's
> + * fine since the value is 0.
> */
> - if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> - ret += hpp__call_print_fn(hpp, print_fn,
> - fmt, len, 0.0);
> - } else {
> - ret += hpp__call_print_fn(hpp, print_fn,
> - fmt, len, 0ULL);
> - }
> + ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> + fmt, len, print_fn, fmtype);
> }
>
> - if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> - ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
> - 100.0 * period / total);
> - } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> - double avg = nr_samples ? (period / nr_samples) : 0;
> -
> - ret += hpp__call_print_fn(hpp, print_fn, fmt,
> - len, avg);
> - } else {
> - ret += hpp__call_print_fn(hpp, print_fn, fmt,
> - len, period);
> - }
> + ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> + fmt, len, print_fn, fmtype);
>
> prev_idx = evsel__group_idx(evsel);
> }
> @@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>
> while (idx_delta--) {
> /*
> - * zero-fill group members at last which have no sample
> + * zero-fill group members at last which have no sample.
> + * the hists is not correct but it's fine like above.
> */
> - if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> - ret += hpp__call_print_fn(hpp, print_fn,
> - fmt, len, 0.0);
> - } else {
> - ret += hpp__call_print_fn(hpp, print_fn,
> - fmt, len, 0ULL);
> - }
> + ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> + fmt, len, print_fn, fmtype);
> }
> }
>
> --
> 2.45.1.288.g0e0cd299f1-goog
On Mon, Jun 03, 2024 at 03:44:10PM -0700, Namhyung Kim wrote:
> The struct hpp_fmt_data is to keep the values for each group members so
> it doesn't need to check the event index in the group.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
> 1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index e30fcb1e87e7..539978c95cfd 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> }
>
> +struct hpp_fmt_data {
> + struct hists *hists;
> + u64 val;
> + int samples;
> +};
Can we try to avoid vague terms like 'data' and use a hopefully more
clear 'hpp_fmt_value' instead?
> static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> hpp_field_fn get_field, const char *fmt, int len,
> hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> {
> - int ret;
> + int ret = 0;
> struct hists *hists = he->hists;
> struct evsel *evsel = hists_to_evsel(hists);
> + struct evsel *pos;
> char *buf = hpp->buf;
> size_t size = hpp->size;
> + int i, nr_members = 1;
> + struct hpp_fmt_data *data;
Here we then use:
struct hpp_fmt_value *values;
> +
> + if (evsel__is_group_event(evsel))
> + nr_members = evsel->core.nr_members;
> +
> + data = calloc(nr_members, sizeof(*data));
> + if (data == NULL)
> + return 0;
> +
> + i = 0;
> + for_each_group_evsel(pos, evsel)
> + data[i++].hists = evsel__hists(pos);
>
> - ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> - fmt, len, print_fn, fmtype);
> + data[0].val = get_field(he);
> + data[0].samples = he->stat.nr_events;
>
> if (evsel__is_group_event(evsel)) {
> - int prev_idx, idx_delta;
> struct hist_entry *pair;
> - int nr_members = evsel->core.nr_members;
> -
> - prev_idx = evsel__group_idx(evsel);
>
> list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> - u64 period = get_field(pair);
> - u64 total = hists__total_period(pair->hists);
> - int nr_samples = pair->stat.nr_events;
> -
> - if (!total)
> - continue;
> + for (i = 0; i < nr_members; i++) {
> + if (data[i].hists != pair->hists)
> + continue;
>
> - evsel = hists_to_evsel(pair->hists);
> - idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
> -
> - while (idx_delta--) {
> - /*
> - * zero-fill group members in the middle which have
> - * no samples, pair->hists is not correct but it's
> - * fine since the value is 0.
> - */
> - ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> - fmt, len, print_fn, fmtype);
> + data[i].val = get_field(pair);
> + data[i].samples = pair->stat.nr_events;
> + break;
> }
> -
> - ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> - fmt, len, print_fn, fmtype);
> -
> - prev_idx = evsel__group_idx(evsel);
> }
> + }
>
> - idx_delta = nr_members - prev_idx - 1;
> -
> - while (idx_delta--) {
> - /*
> - * zero-fill group members at last which have no sample.
> - * the hists is not correct but it's fine like above.
> - */
> - ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> - fmt, len, print_fn, fmtype);
> - }
> + for (i = 0; i < nr_members; i++) {
> + ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
> + data[i].samples, fmt, len,
> + print_fn, fmtype);
> }
>
> + free(data);
> +
> /*
> * Restore original buf and size as it's where caller expects
> * the result will be saved.
> --
> 2.45.1.288.g0e0cd299f1-goog
On Mon, Jun 03, 2024 at 03:44:08PM -0700, Namhyung Kim wrote:
> Hello,
>
> This work is to make the output compact by removing dummy events in
> the output. The dummy events are used to save side-band information
> like task creation or memory address space change using mmap(2). But
> after collecting these, it's not used because it won't have any
> samples.
>
> v2 changes)
> * just hide the (dummy) event instead of removing it from evlist
>
> Sometimes users want to run perf report --group to show all recorded
> events together but they are not interested in those dummy events.
> This just wastes the precious screen space so we want to get rid of
> them after use.
>
> perf report already has --skip-empty option to skip 0 result in the
> stat output. I think we can extend it to skip empty events that have
> no samples.
>
> Example output:
Would be interesting to have this as an example in the cset where this
becomes the norm, together with stating that the 'perf.data' file was
generated with 'perf mem record'.
Apart from that, nice feature:
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
- Arnaldo
> Before)
> #
> # Samples: 232 of events 'cpu/mem-loads,ldlat=30/P, cpu/mem-stores/P, dummy:u'
> # Event count (approx.): 3089861
> #
> # Overhead Command Shared Object Symbol
> # ........................ ........... ................. .....................................
> #
> 9.29% 0.00% 0.00% swapper [kernel.kallsyms] [k] update_blocked_averages
> 5.26% 0.15% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_se
> 4.15% 0.00% 0.00% perf-exec [kernel.kallsyms] [k] slab_update_freelist.isra.0
> 3.87% 0.00% 0.00% perf-exec [kernel.kallsyms] [k] memcg_slab_post_alloc_hook
> 3.79% 0.17% 0.00% swapper [kernel.kallsyms] [k] enqueue_task_fair
> 3.63% 0.00% 0.00% sleep [kernel.kallsyms] [k] next_uptodate_page
> 2.86% 0.00% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_cfs_rq
> 2.78% 0.00% 0.00% swapper [kernel.kallsyms] [k] __schedule
> 2.34% 0.00% 0.00% swapper [kernel.kallsyms] [k] intel_idle
> 2.32% 0.97% 0.00% swapper [kernel.kallsyms] [k] psi_group_change
>
> After)
> #
> # Samples: 232 of events 'cpu/mem-loads,ldlat=30/P, cpu/mem-stores/P'
> # Event count (approx.): 3089861
> #
> # Overhead Command Shared Object Symbol
> # ................ ........... ................. .....................................
> #
> 9.29% 0.00% swapper [kernel.kallsyms] [k] update_blocked_averages
> 5.26% 0.15% swapper [kernel.kallsyms] [k] __update_load_avg_se
> 4.15% 0.00% perf-exec [kernel.kallsyms] [k] slab_update_freelist.isra.0
> 3.87% 0.00% perf-exec [kernel.kallsyms] [k] memcg_slab_post_alloc_hook
> 3.79% 0.17% swapper [kernel.kallsyms] [k] enqueue_task_fair
> 3.63% 0.00% sleep [kernel.kallsyms] [k] next_uptodate_page
> 2.86% 0.00% swapper [kernel.kallsyms] [k] __update_load_avg_cfs_rq
> 2.78% 0.00% swapper [kernel.kallsyms] [k] __schedule
> 2.34% 0.00% swapper [kernel.kallsyms] [k] intel_idle
> 2.32% 0.97% swapper [kernel.kallsyms] [k] psi_group_change
>
> Now 'Overhead' column only has two values for mem-loads and mem-stores.
>
> Thanks,
> Namhyung
>
>
> Namhyung Kim (4):
> perf hist: Factor out __hpp__fmt_print()
> perf hist: Simplify __hpp_fmt() using hpp_fmt_data
> perf hist: Add symbol_conf.skip_empty
> perf hist: Honor symbol_conf.skip_empty
>
> tools/perf/builtin-annotate.c | 4 +-
> tools/perf/builtin-report.c | 12 +--
> tools/perf/ui/hist.c | 144 ++++++++++++++++-----------------
> tools/perf/ui/stdio/hist.c | 5 +-
> tools/perf/util/events_stats.h | 3 +-
> tools/perf/util/evsel.c | 13 ++-
> tools/perf/util/hist.c | 6 +-
> tools/perf/util/hist.h | 3 +-
> tools/perf/util/python.c | 3 +
> tools/perf/util/session.c | 5 +-
> tools/perf/util/session.h | 3 +-
> tools/perf/util/symbol_conf.h | 3 +-
> 12 files changed, 105 insertions(+), 99 deletions(-)
>
> --
> 2.45.1.288.g0e0cd299f1-goog
On Tue, Jun 04, 2024 at 12:07:41PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Jun 03, 2024 at 03:44:09PM -0700, Namhyung Kim wrote:
> > Split the logic to print the histogram values according to the format
> > string. This was used in 3 different places so it's better to move out
> > the logic into a function.
>
> But are all really equivalent? I had difficulty following it, perhaps it
> would be better to introduce the function and then go on making one by
> one use it instead of doing it all at once?
Sorry, I agree the code looks confusing. But they are equivalent and
just to print the value in a different way.
>
> So in the end we have:
>
> static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> int nr_samples, const char *fmt, int len,
> hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> {
> if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> double percent = 0.0;
> u64 total = hists__total_period(hists);
>
> if (total)
> percent = 100.0 * val / total;
>
> return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> }
> if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
>
> return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
> }
>
> return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> }
>
> I.e. we do something with the last arg for hpp__call_print_fn() for the
> PERF_HPP_FMT_TYPE__PERCENT and PERF_HPP_FMT_TYPE__AVERAGE fmtype's, but
> we don't check for those fmtypes on each of the places that we now call
> __hpp__fmt_print(), right?
>
> That while (idx_delta--) cases is only interested in !=PERF_HPP_FMT_TYPE__RAW...
>
> Nah, after trying to describe the confusion I think I clarified it,
> probably leave it as you did :-\
:)
The 'while (idx_delta--)' part is just to fill the gap for entries
without samples. The value is always 0, so it only cares the data type
(double or u64) in the format string. RAW needs u64 while PERCENT and
AVERAGE need double.
Thanks,
Namhyung
> > +++ b/tools/perf/ui/hist.c
> > @@ -23,35 +23,42 @@
> > __ret; \
> > })
> >
> > -static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> > - hpp_field_fn get_field, const char *fmt, int len,
> > - hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> > +static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> > + int nr_samples, const char *fmt, int len,
> > + hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> > {
> > - int ret;
> > - struct hists *hists = he->hists;
> > - struct evsel *evsel = hists_to_evsel(hists);
> > - char *buf = hpp->buf;
> > - size_t size = hpp->size;
> > -
> > if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> > double percent = 0.0;
> > u64 total = hists__total_period(hists);
> >
> > if (total)
> > - percent = 100.0 * get_field(he) / total;
> > + percent = 100.0 * val / total;
> >
> > - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> > - } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > - double average = 0;
> > + return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> > + }
> >
> > - if (he->stat.nr_events)
> > - average = 1.0 * get_field(he) / he->stat.nr_events;
> > + if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > + double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
> >
> > - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
> > - } else {
> > - ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
> > + return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
> > }
> >
> > + return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> > +}
> > +
> > +static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> > + hpp_field_fn get_field, const char *fmt, int len,
> > + hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> > +{
> > + int ret;
> > + struct hists *hists = he->hists;
> > + struct evsel *evsel = hists_to_evsel(hists);
> > + char *buf = hpp->buf;
> > + size_t size = hpp->size;
> > +
> > + ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> > + fmt, len, print_fn, fmtype);
> > +
> > if (evsel__is_group_event(evsel)) {
> > int prev_idx, idx_delta;
> > struct hist_entry *pair;
> > @@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >
> > while (idx_delta--) {
> > /*
> > - * zero-fill group members in the middle which
> > - * have no sample
> > + * zero-fill group members in the middle which have
> > + * no samples, pair->hists is not correct but it's
> > + * fine since the value is 0.
> > */
> > - if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> > - ret += hpp__call_print_fn(hpp, print_fn,
> > - fmt, len, 0.0);
> > - } else {
> > - ret += hpp__call_print_fn(hpp, print_fn,
> > - fmt, len, 0ULL);
> > - }
> > + ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> > + fmt, len, print_fn, fmtype);
> > }
> >
> > - if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> > - ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
> > - 100.0 * period / total);
> > - } else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > - double avg = nr_samples ? (period / nr_samples) : 0;
> > -
> > - ret += hpp__call_print_fn(hpp, print_fn, fmt,
> > - len, avg);
> > - } else {
> > - ret += hpp__call_print_fn(hpp, print_fn, fmt,
> > - len, period);
> > - }
> > + ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> > + fmt, len, print_fn, fmtype);
> >
> > prev_idx = evsel__group_idx(evsel);
> > }
> > @@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >
> > while (idx_delta--) {
> > /*
> > - * zero-fill group members at last which have no sample
> > + * zero-fill group members at last which have no sample.
> > + * the hists is not correct but it's fine like above.
> > */
> > - if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> > - ret += hpp__call_print_fn(hpp, print_fn,
> > - fmt, len, 0.0);
> > - } else {
> > - ret += hpp__call_print_fn(hpp, print_fn,
> > - fmt, len, 0ULL);
> > - }
> > + ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> > + fmt, len, print_fn, fmtype);
> > }
> > }
> >
> > --
> > 2.45.1.288.g0e0cd299f1-goog
On Tue, Jun 04, 2024 at 12:18:47PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Jun 03, 2024 at 03:44:10PM -0700, Namhyung Kim wrote:
> > The struct hpp_fmt_data is to keep the values for each group members so
> > it doesn't need to check the event index in the group.
> >
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
> > 1 file changed, 36 insertions(+), 39 deletions(-)
> >
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index e30fcb1e87e7..539978c95cfd 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> > return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> > }
> >
> > +struct hpp_fmt_data {
> > + struct hists *hists;
> > + u64 val;
> > + int samples;
> > +};
>
> Can we try to avoid vague terms like 'data' and use a hopefully more
> clear 'hpp_fmt_value' instead?
Sure, I can do that. I thought 'data' was better since it contains more
than a value like a pointer to hist and number of samples. But it's not
a big deal, and I can make the change.
>
> > static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> > hpp_field_fn get_field, const char *fmt, int len,
> > hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> > {
> > - int ret;
> > + int ret = 0;
> > struct hists *hists = he->hists;
> > struct evsel *evsel = hists_to_evsel(hists);
> > + struct evsel *pos;
> > char *buf = hpp->buf;
> > size_t size = hpp->size;
> > + int i, nr_members = 1;
> > + struct hpp_fmt_data *data;
>
> Here we then use:
>
> struct hpp_fmt_value *values;
Yep, will change in v3.
Thanks,
Namhyung
>
> > +
> > + if (evsel__is_group_event(evsel))
> > + nr_members = evsel->core.nr_members;
> > +
> > + data = calloc(nr_members, sizeof(*data));
> > + if (data == NULL)
> > + return 0;
>
>
> > +
> > + i = 0;
> > + for_each_group_evsel(pos, evsel)
> > + data[i++].hists = evsel__hists(pos);
> >
> > - ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> > - fmt, len, print_fn, fmtype);
> > + data[0].val = get_field(he);
> > + data[0].samples = he->stat.nr_events;
> >
> > if (evsel__is_group_event(evsel)) {
> > - int prev_idx, idx_delta;
> > struct hist_entry *pair;
> > - int nr_members = evsel->core.nr_members;
> > -
> > - prev_idx = evsel__group_idx(evsel);
> >
> > list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> > - u64 period = get_field(pair);
> > - u64 total = hists__total_period(pair->hists);
> > - int nr_samples = pair->stat.nr_events;
> > -
> > - if (!total)
> > - continue;
> > + for (i = 0; i < nr_members; i++) {
> > + if (data[i].hists != pair->hists)
> > + continue;
> >
> > - evsel = hists_to_evsel(pair->hists);
> > - idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
> > -
> > - while (idx_delta--) {
> > - /*
> > - * zero-fill group members in the middle which have
> > - * no samples, pair->hists is not correct but it's
> > - * fine since the value is 0.
> > - */
> > - ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> > - fmt, len, print_fn, fmtype);
> > + data[i].val = get_field(pair);
> > + data[i].samples = pair->stat.nr_events;
> > + break;
> > }
> > -
> > - ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> > - fmt, len, print_fn, fmtype);
> > -
> > - prev_idx = evsel__group_idx(evsel);
> > }
> > + }
> >
> > - idx_delta = nr_members - prev_idx - 1;
> > -
> > - while (idx_delta--) {
> > - /*
> > - * zero-fill group members at last which have no sample.
> > - * the hists is not correct but it's fine like above.
> > - */
> > - ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> > - fmt, len, print_fn, fmtype);
> > - }
> > + for (i = 0; i < nr_members; i++) {
> > + ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
> > + data[i].samples, fmt, len,
> > + print_fn, fmtype);
> > }
> >
> > + free(data);
> > +
> > /*
> > * Restore original buf and size as it's where caller expects
> > * the result will be saved.
> > --
> > 2.45.1.288.g0e0cd299f1-goog