Re: [PATCH v2 3/5] perf report: Sort by sampled cycles percent per block for stdio

[Jin, Yao]

On 10/15/2019 4:41 PM, Jiri Olsa wrote:
On Tue, Oct 15, 2019 at 01:33:48PM +0800, Jin Yao wrote:

SNIP

+enum {
+       PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_COV,
+       PERF_HPP_REPORT__BLOCK_LBR_CYCLES,
+       PERF_HPP_REPORT__BLOCK_CYCLES_PCT,
+       PERF_HPP_REPORT__BLOCK_AVG_CYCLES,
+       PERF_HPP_REPORT__BLOCK_RANGE,
+       PERF_HPP_REPORT__BLOCK_DSO,
+       PERF_HPP_REPORT__BLOCK_MAX_INDEX
+};
+
+static struct block_fmt block_fmts[PERF_HPP_REPORT__BLOCK_MAX_INDEX];
+
+static struct block_header_column{
+       const char *name;
+       int width;
+} block_columns[PERF_HPP_REPORT__BLOCK_MAX_INDEX] = {
+       [PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_COV] = {
+               .name = "Sampled Cycles%",
+               .width = 15,
+       },
+       [PERF_HPP_REPORT__BLOCK_LBR_CYCLES] = {
+               .name = "Sampled Cycles",
+               .width = 14,
+       },
+       [PERF_HPP_REPORT__BLOCK_CYCLES_PCT] = {
+               .name = "Avg Cycles%",
+               .width = 11,
+       },
+       [PERF_HPP_REPORT__BLOCK_AVG_CYCLES] = {
+               .name = "Avg Cycles",
+               .width = 10,
+       },
+       [PERF_HPP_REPORT__BLOCK_RANGE] = {
+               .name = "[Program Block Range]",
+               .width = 70,
+       },
+       [PERF_HPP_REPORT__BLOCK_DSO] = {
+               .name = "Shared Object",
+               .width = 20,
+       }
  };

so we already have support for multiple columns,
why don't you add those as 'struct sort_entry' objects?


For 'struct sort_entry' objects, do you mean I should reuse the 
"sort_dso" which has been implemented yet in util/sort.c?

For other columns, it looks we can't reuse the existing sort_entry objects.

SNIP

+{
+       struct block_hist *bh = &rep->block_hist;
+
+       get_block_hists(hists, bh, rep);
+       symbol_conf.report_individual_block = true;
+       hists__fprintf(&bh->block_hists, true, 0, 0, 0,
+                      stdout, true);
+       hists__delete_entries(&bh->block_hists);
+       return 0;
+}
+
  static int perf_evlist__tty_browse_hists(struct evlist *evlist,
                                         struct report *rep,
                                         const char *help)
@@ -500,6 +900,12 @@ static int perf_evlist__tty_browse_hists(struct evlist 
*evlist,
                        continue;
  
  		hists__fprintf_nr_sample_events(hists, rep, evname, stdout);
+
+               if (rep->total_cycles) {
+                       hists__fprintf_all_blocks(hists, rep);

so this call kicks all the block info setup/count/print, right?


Yes, all in this call.

I thingk it shouldn't be in the output code, but in the code before..
from what I see you could count block_info counts during the sample
processing, no?


In sample processing, we just get all symbols and account the cycles per 
symbol. We need to create/count the block_info at some points after the 
sample processing.

Maybe it's not very good to put block info setup/count/print in a call, 
but it's really not easy to process the block_info during the sample 
processing.

Thanks
Jin Yao

jirka