perf/core improvements and fixes
Jiri Olsa
reviewerhi Ingo,
please consider pulling
thanks,
jirka
The following changes since commit a81fef347b32dea2b31275826afe1c93fa0d2d54:
Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf into perf/core (2014-04-22 20:28:23 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git tags/perf-core-for-mingo
for you to fetch changes up to c3b789527b236873557f53740ceac47747e0e1cb:
perf hists/tui: Count callchain rows separately (2014-04-24 16:34:27 +0200)
----------------------------------------------------------------
perf/core improvements and fixes:
. Factor hists statistics counts processing which in turn also
fixes several bugs in TUI report command (Namhyung Kim)
----------------------------------------------------------------
Namhyung Kim (11):
perf report: Count number of entries separately
perf hists: Rename hists__inc_stats()
perf hists: Move column length calculation out of hists__inc_stats() perf hists: Add a couple of hists stat helper functions
perf hists: Collapse expanded callchains after filter is applied perf tools: Account entry stats when it's added to the output tree perf hists: Add missing update on filtered stats in hists__decay_entries() perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries() perf ui/tui: Rename hist_browser__update_nr_entries()
perf top/tui: Update nr_entries properly after a filter is applied perf hists/tui: Count callchain rows separately
tools/perf/builtin-annotate.c | 3 +- tools/perf/builtin-diff.c | 23 ++++++----- tools/perf/builtin-report.c | 64 ++++++++++++++--------------- tools/perf/ui/browsers/hists.c | 92 +++++++++++++++++++++++++++++------------- tools/perf/util/hist.c | 83 ++++++++++++++++++++++++------------- tools/perf/util/hist.h | 9 ++++- 6 files changed, 171 insertions(+), 103 deletions(-)
Jiri Olsa
reviewerPATCH 1/11 perf report: Count number of entries separately
From: Namhyung Kim <[email protected]>
The hists->nr_entries is counted in multiple places so that they can confuse readers of the code. This is a preparation of later change and do not intend any functional difference.
Note that report__collapse_hists() now changed to return nothing since its return value (nr_samples) is only for checking if there's any data in the input file and this can be acheived by checking ->nr_entries.
--- tools/perf/builtin-report.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
tools/perf/builtin-report.c +22 -20
@@ -57,6 +57,7 @@ struct report {const char *cpu_list;const char *symbol_filter_str;float min_percent; +u64 nr_entries;DECLARE_BITMAP (cpu_bitmap,MAX_NR_CPUS ); };
@@ -75,6 +76,17 @@ static int report__config(const char *var, const char *value, void *cb)return perf_default_config (var, value, cb); } +static void report__inc_stats (struct report *rep,struct hist_entry *he) +{ +/* +* The @he is either of a newly created one or an existing one +* merging current sample. We only want to count a new one so +* checking ->nr_events being 1. +*/ +if (he->stat.nr_events ==1 ) + rep->nr_entries++; +} +static int report__add_mem_hist_entry (struct report *rep,struct addr_location *al,struct perf_sample *sample,struct perf_evsel *evsel) {
@@ -121,6 +133,8 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location * goto out; } +report__inc_stats (rep, he); + evsel->hists.stats.total_period += cost;hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE );if (!he->filtered)
@@ -176,6 +190,8 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio goto out; } +report__inc_stats (rep, he); + evsel->hists.stats.total_period +=1 ;hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE );if (!he->filtered)
@@ -212,6 +228,8 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,if (ui__has_annotation ()) err =hist_entry__inc_addr_samples (he, evsel->idx, al->addr); +report__inc_stats (rep, he); + evsel->hists.stats.total_period += sample->period;if (!he->filtered) evsel->hists.stats.nr_non_filtered_samples++;
@@ -486,24 +504,12 @@ static int report__browse_hists(struct report *rep)return ret; } -static u64 report__collapse_hists (struct report *rep) +static void report__collapse_hists (struct report *rep) {struct ui_progress prog;struct perf_evsel *pos; -u64 nr_samples =0 ; -/* -* Count number of histogram entries to use when showing progress, -* reusing nr_samples variable. -*/ -evlist__for_each (rep->session->evlist, pos) - nr_samples += pos->hists.nr_entries; -ui_progress__init (&prog, nr_samples,"Merging related events..." ); -/* -* Count total number of samples, will be used to check if this -* session had any. -*/ - nr_samples =0 ; +ui_progress__init (&prog, rep->nr_entries,"Merging related events..." );evlist__for_each (rep->session->evlist, pos) {struct hists *hists = &pos->hists;
@@ -512,7 +518,6 @@ static u64 report__collapse_hists(struct report *rep) hists->symbol_filter_str = rep->symbol_filter_str;hists__collapse_resort (hists, &prog); - nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE ];/* Non-group events are considered as leader */ if (symbol_conf.event_group &&
@@ -525,14 +530,11 @@ static u64 report__collapse_hists(struct report *rep) }ui_progress__finish (); - -return nr_samples; }static int __cmd_report (struct report *rep) {int ret; -u64 nr_samples;struct perf_session *session = rep->session;struct perf_evsel *pos;struct perf_data_file *file = session->file;
@@ -572,12 +574,12 @@ static int __cmd_report(struct report *rep) } } - nr_samples =report__collapse_hists (rep); +report__collapse_hists (rep);if (session_done ())return 0 ; -if (nr_samples ==0 ) { +if (rep->nr_entries ==0 ) {ui__error ("The %s file has no samples!\n" , file->path);return 0 ; }
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 2/11 perf hists: Rename hists__inc_stats()
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
The existing hists__inc_nr_entries() is a misnomer as it's not only increasing ->nr_entries but also other stats. So rename it to more general hists__inc_stats().
--- tools/perf/builtin-diff.c | 2 +- tools/perf/util/hist.c | 6 +++--- tools/perf/util/hist.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)
tools/perf/builtin-diff.c +1 -1
@@ -586,7 +586,7 @@ static void hists__compute_resort(struct hists *hists) next =rb_next (&he->rb_node_in);insert_hist_entry_by_compute (&hists->entries, he, compute); -hists__inc_nr_entries (hists, he); +hists__inc_stats (hists, he); } }
tools/perf/util/hist.c +3 -3
@@ -317,7 +317,7 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)return he; } -void hists__inc_nr_entries (struct hists *hists,struct hist_entry *h) +void hists__inc_stats (struct hists *hists,struct hist_entry *h) {if (!h->filtered) {hists__calc_col_len (hists, h);
@@ -686,7 +686,7 @@ void hists__output_resort(struct hists *hists) next =rb_next (&n->rb_node_in);__hists__insert_output_entry (&hists->entries, n, min_callchain_hits); -hists__inc_nr_entries (hists, n); +hists__inc_stats (hists, n); } }
@@ -853,7 +853,7 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists, he->hists = hists;rb_link_node (&he->rb_node_in, parent, p);rb_insert_color (&he->rb_node_in, root); -hists__inc_nr_entries (hists, he); +hists__inc_stats (hists, he); he->dummy = true; } out:
tools/perf/util/hist.h +1 -1
@@ -116,7 +116,7 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);void hists__output_recalc_col_len (struct hists *hists,int max_rows);u64 hists__total_period (struct hists *hists); -void hists__inc_nr_entries (struct hists *hists,struct hist_entry *h); +void hists__inc_stats (struct hists *hists,struct hist_entry *h);void hists__inc_nr_events (struct hists *hists,u32 type);void events_stats__inc (struct events_stats *stats,u32 type);size_t events_stats__fprintf (struct events_stats *stats,FILE *fp);
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 3/11 perf hists: Move column length calculation out of hists__inc_stats()
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
It's not the part of logic of hists__inc_stats() so it'd be better to move it out of the function.
--- tools/perf/builtin-diff.c | 3 +++ tools/perf/util/hist.c | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-)
tools/perf/builtin-diff.c +3 -0
@@ -587,6 +587,9 @@ static void hists__compute_resort(struct hists *hists)insert_hist_entry_by_compute (&hists->entries, he, compute);hists__inc_stats (hists, he); + +if (!he->filtered) +hists__calc_col_len (hists, he); } }
tools/perf/util/hist.c +3 -1
@@ -320,7 +320,6 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)void hists__inc_stats (struct hists *hists,struct hist_entry *h) {if (!h->filtered) { -hists__calc_col_len (hists, h); hists->nr_non_filtered_entries++; hists->stats.total_non_filtered_period += h->stat.period; }
@@ -687,6 +686,9 @@ void hists__output_resort(struct hists *hists)__hists__insert_output_entry (&hists->entries, n, min_callchain_hits);hists__inc_stats (hists, n); + +if (!n->filtered) +hists__calc_col_len (hists, n); } }
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 4/11 perf hists: Add a couple of hists stat helper functions
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
Add hists__{reset,inc}_[filter_]stats() functions to cleanup accesses to hist stats (for output). Note that number of samples in the stat is not handled here since it belongs to the input stage.
--- tools/perf/builtin-diff.c | 5 +--- tools/perf/util/hist.c | 59 ++++++++++++++++++++++++++++++----------------- tools/perf/util/hist.h | 1 + 3 files changed, 40 insertions(+), 25 deletions(-)
tools/perf/builtin-diff.c +1 -4
@@ -573,10 +573,7 @@ static void hists__compute_resort(struct hists *hists) hists->entries =RB_ROOT ; next =rb_first (root); - hists->nr_entries =0 ; - hists->nr_non_filtered_entries =0 ; - hists->stats.total_period =0 ; - hists->stats.total_non_filtered_period =0 ; +hists__reset_stats (hists);hists__reset_col_len (hists);while (next !=NULL ) {
tools/perf/util/hist.c +38 -21
109 lines not on this page · raw message
tools/perf/util/hist.h +1 -0
@@ -116,6 +116,7 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);void hists__output_recalc_col_len (struct hists *hists,int max_rows);u64 hists__total_period (struct hists *hists); +void hists__reset_stats (struct hists *hists);void hists__inc_stats (struct hists *hists,struct hist_entry *h);void hists__inc_nr_events (struct hists *hists,u32 type);void events_stats__inc (struct events_stats *stats,u32 type);
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 5/11 perf hists: Collapse expanded callchains after filter is applied
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
When a filter is applied a hist entry checks whether its callchain was folded and account it to the output stat. But this is rather hacky and only TUI-specific. Simply fold the callchains for the entry looks like a simpler and more generic solution IMHO.
--- tools/perf/util/hist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tools/perf/util/hist.c +2 -2
@@ -716,8 +716,8 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *hif (h->filtered)return ; -if (h->ms.unfolded) - hists->nr_non_filtered_entries += h->nr_rows; +/* force fold unfiltered entry for simplicity */ + h->ms.unfolded = false; h->row_offset =0 ; hists->stats.nr_non_filtered_samples += h->stat.nr_events;
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 6/11 perf tools: Account entry stats when it's added to the output tree
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
Currently, accounting each sample is done in multiple places - once when adding them to the input tree, other when adding them to the output tree. It's not only confusing but also can cause a subtle problem since concurrent processing like in perf top might see the updated stats before adding entries into the output tree - like seeing more (blank) lines at the end and/or slight inaccurate percentage.
To fix this, only account the entries when it's moved into the output tree so that they cannot be seen prematurely. There're some exceptional cases here and there - they should be addressed separately with comments.
--- tools/perf/builtin-annotate.c | 3 +-- tools/perf/builtin-diff.c | 13 +++++++++---- tools/perf/builtin-report.c | 24 ++++++++++-------------- tools/perf/util/hist.c | 1 - 4 files changed, 20 insertions(+), 21 deletions(-)
tools/perf/builtin-annotate.c +1 -2
@@ -46,7 +46,7 @@ struct perf_annotate { };static int perf_evsel__add_sample (struct perf_evsel *evsel, -struct perf_sample *sample, +struct perf_sample *sample__maybe_unused ,struct addr_location *al,struct perf_annotate *ann) {
@@ -70,7 +70,6 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,return -ENOMEM ; ret =hist_entry__inc_addr_samples (he, evsel->idx, al->addr); - evsel->hists.stats.total_period += sample->period;hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE );return ret; }
tools/perf/builtin-diff.c +9 -4
@@ -341,11 +341,16 @@ static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,return -1 ; } -if (al.filtered ==0 ) { - evsel->hists.stats.total_non_filtered_period += sample->period; - evsel->hists.nr_non_filtered_entries++; - } +/* +* The total_period is updated here before going to the output +* tree since normally only the baseline hists will call +* hists__output_resort() and precompute needs the total +* period in order to sort entries by percentage delta. +*/ evsel->hists.stats.total_period += sample->period; +if (!al.filtered) + evsel->hists.stats.total_non_filtered_period += sample->period; +return 0 ; }
tools/perf/builtin-report.c +10 -14
@@ -85,6 +85,16 @@ static void report__inc_stats(struct report *rep, struct hist_entry *he)*/ if (he->stat.nr_events ==1 ) rep->nr_entries++; + +/* +* Only counts number of samples at this stage as it's more +* natural to do it here and non-sample events are also +* counted in perf_session_deliver_event(). The dump_trace +* requires this info is ready before going to the output tree. +*/ +hists__inc_nr_events (he->hists,PERF_RECORD_SAMPLE ); +if (!he->filtered) + he->hists->stats.nr_non_filtered_samples++; } static int report__add_mem_hist_entry(struct report *rep, struct addr_location *al,
@@ -135,10 +145,6 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *report__inc_stats (rep, he); - evsel->hists.stats.total_period += cost; -hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE ); -if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; err =hist_entry__append_callchain (he, sample); out:return err;
@@ -189,13 +195,7 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatioif (err) goto out; } -report__inc_stats (rep, he); - - evsel->hists.stats.total_period +=1 ; -hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE ); -if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; }else goto out; }
@@ -230,10 +230,6 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,report__inc_stats (rep, he); - evsel->hists.stats.total_period += sample->period; -if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; -hists__inc_nr_events (&evsel->hists,PERF_RECORD_SAMPLE ); out:return err; }
tools/perf/util/hist.c +0 -1
@@ -382,7 +382,6 @@ static struct hist_entry *add_hist_entry(struct hists *hists,if (!he)return NULL ; - hists->nr_entries++;rb_link_node (&he->rb_node_in, parent, p);rb_insert_color (&he->rb_node_in, hists->entries_in); out:
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 7/11 perf hists: Add missing update on filtered stats in hists__decay_entries()
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
When a filter is used for perf top, its hists->nr_non_filtered_entries was not updated after it removed an entry in hists__decay_entries(). Also hists->stats.total_non_filtered_period was missed too.
--- tools/perf/util/hist.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
tools/perf/util/hist.c +9 -2
@@ -225,14 +225,18 @@ static void he_stat__decay(struct he_stat *he_stat)static bool hists__decay_entry (struct hists *hists,struct hist_entry *he) {u64 prev_period = he->stat.period; +u64 diff;if (prev_period ==0 )return true;he_stat__decay (&he->stat); + diff = prev_period - he->stat.period; + + hists->stats.total_period -= diff;if (!he->filtered) - hists->stats.total_period -= prev_period - he->stat.period; + hists->stats.total_non_filtered_period -= diff;return he->stat.period ==0 ; }
@@ -259,8 +263,11 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel)if (sort__need_collapse)rb_erase (&n->rb_node_in, &hists->entries_collapsed); -hist_entry__free (n); --hists->nr_entries; +if (!n->filtered) + --hists->nr_non_filtered_entries; + +hist_entry__free (n); } } }
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 8/11 perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
The nr_entries variable is increased inside the loop in the function but it always count the first entry regardless of it's filtered or not; caused an off-by-one error.
It'd become a problem especially there's no entry at all - it'd get a segfault during referencing a NULL pointer.
--- tools/perf/ui/browsers/hists.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/ui/browsers/hists.c +3 -3
@@ -1348,10 +1348,10 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb)u64 nr_entries =0 ;struct rb_node *nd =rb_first (&hb->hists->entries); -while (nd) { +while ((nd =hists__filter_entries (nd, hb->hists, + hb->min_pcnt)) !=NULL ) { nr_entries++; - nd =hists__filter_entries (rb_next (nd), hb->hists, - hb->min_pcnt); + nd =rb_next (nd); } hb->nr_pcnt_entries = nr_entries;
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 9/11 perf ui/tui: Rename hist_browser__update_nr_entries()
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
Rename ->nr_pcnt_entries and hist_browser__update_pcnt_entries() to ->nr_non_filtered_entries and hist_browser__update_nr_entries() since it's now used for filtering as well.
--- tools/perf/ui/browsers/hists.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
tools/perf/ui/browsers/hists.c +8 -9
@@ -26,13 +26,14 @@ struct hist_browser {int print_seq;bool show_dso;float min_pcnt; -u64 nr_pcnt_entries; +u64 nr_non_filtered_entries; };extern void hist_browser__init_hpp (void );static int hists__browser_title (struct hists *hists,char *bf,size_t size,const char *ev_name); +static void hist_browser__update_nr_entries (struct hist_browser *hb);static void hist_browser__refresh_dimensions (struct hist_browser *browser) {
@@ -310,8 +311,6 @@ static void ui_browser__warn_lost_events(struct ui_browser *browser) "Or reduce the sampling frequency."); } -static void hist_browser__update_pcnt_entries (struct hist_browser *hb); -static int hist_browser__run (struct hist_browser *browser,const char *ev_name,struct hist_browser_timer *hbt) {
@@ -322,7 +321,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, browser->b.entries = &browser->hists->entries; browser->b.nr_entries = browser->hists->nr_entries;if (browser->min_pcnt) - browser->b.nr_entries = browser->nr_pcnt_entries; + browser->b.nr_entries = browser->nr_non_filtered_entries;hist_browser__refresh_dimensions (browser);hists__browser_title (browser->hists, title,sizeof (title), ev_name);
@@ -340,8 +339,8 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, hbt->timer (hbt->arg);if (browser->min_pcnt) { -hist_browser__update_pcnt_entries (browser); - nr_entries = browser->nr_pcnt_entries; +hist_browser__update_nr_entries (browser); + nr_entries = browser->nr_non_filtered_entries; }else { nr_entries = browser->hists->nr_entries; }
@@ -1343,7 +1342,7 @@ close_file_and_continue:return ret; } -static void hist_browser__update_pcnt_entries (struct hist_browser *hb) +static void hist_browser__update_nr_entries (struct hist_browser *hb) {u64 nr_entries =0 ;struct rb_node *nd =rb_first (&hb->hists->entries);
@@ -1354,7 +1353,7 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb) nd =rb_next (nd); } - hb->nr_pcnt_entries = nr_entries; + hb->nr_non_filtered_entries = nr_entries; } static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
@@ -1411,7 +1410,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,if (min_pcnt) { browser->min_pcnt = min_pcnt; -hist_browser__update_pcnt_entries (browser); +hist_browser__update_nr_entries (browser); } fstack =pstack__new (2 );
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 10/11 perf top/tui: Update nr_entries properly after a filter is applied
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
The hist_browser__reset() is only called right after a filter is applied so it needs to udpate browser->nr_entries properly. We cannot use hists->nr_non_filtered_entreis directly since it's possible that such entries are also filtered out by minimum percentage limit.
In addition when a filter is used for perf top, hist browser's nr_entries field was not updated after applying the filter. But it needs to be updated as new samples are coming.
--- tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++---- tools/perf/util/hist.h | 6 ++++++ 2 files changed, 22 insertions(+), 4 deletions(-)
tools/perf/ui/browsers/hists.c +16 -4
@@ -35,6 +35,11 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size, const char *ev_name);static void hist_browser__update_nr_entries (struct hist_browser *hb); +static bool hist_browser__has_filter (struct hist_browser *hb) +{ +return hists__has_filter (hb->hists) || hb->min_pcnt; +} +static void hist_browser__refresh_dimensions (struct hist_browser *browser) {/* 3 == +/- toggle symbol before actual hist_entry rendering */
@@ -44,7 +49,8 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser)static void hist_browser__reset (struct hist_browser *browser) { - browser->b.nr_entries = browser->hists->nr_entries; +hist_browser__update_nr_entries (browser); + browser->b.nr_entries = browser->nr_non_filtered_entries;hist_browser__refresh_dimensions (browser);ui_browser__reset_index (&browser->b); }
@@ -319,9 +325,10 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,int delay_secs = hbt ? hbt->refresh :0 ; browser->b.entries = &browser->hists->entries; - browser->b.nr_entries = browser->hists->nr_entries; -if (browser->min_pcnt) +if (hist_browser__has_filter (browser)) browser->b.nr_entries = browser->nr_non_filtered_entries; +else + browser->b.nr_entries = browser->hists->nr_entries;hist_browser__refresh_dimensions (browser);hists__browser_title (browser->hists, title,sizeof (title), ev_name);
@@ -338,7 +345,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,u64 nr_entries; hbt->timer (hbt->arg); -if (browser->min_pcnt) { +if (hist_browser__has_filter (browser)) {hist_browser__update_nr_entries (browser); nr_entries = browser->nr_non_filtered_entries; }else {
@@ -1347,6 +1354,11 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb)u64 nr_entries =0 ;struct rb_node *nd =rb_first (&hb->hists->entries); +if (hb->min_pcnt ==0 ) { + hb->nr_non_filtered_entries = hb->hists->nr_non_filtered_entries; +return ; + } +while ((nd =hists__filter_entries (nd, hb->hists, hb->min_pcnt)) !=NULL ) { nr_entries++;
tools/perf/util/hist.h +6 -0
@@ -129,6 +129,12 @@ void hists__filter_by_dso(struct hists *hists);void hists__filter_by_thread (struct hists *hists);void hists__filter_by_symbol (struct hists *hists); +static inline bool hists__has_filter (struct hists *hists) +{ +return hists->thread_filter || hists->dso_filter || + hists->symbol_filter_str; +} +u16 hists__col_len (struct hists *hists,enum hist_column col);void hists__set_col_len (struct hists *hists,enum hist_column col,u16 len);bool hists__new_col_len (struct hists *hists,enum hist_column col,u16 len);
Signature · 1 line
1.8.3.1
Jiri Olsa
reviewerPATCH 11/11 perf hists/tui: Count callchain rows separately
In reply to Jiri OlsaFrom: Namhyung Kim <[email protected]>
When TUI hist browser expands/collapses callchains it accounted number of callchain nodes into total entries to show. However this code ignores filtering so that it can make the cursor go to out of screen.
Thanks to Jiri Olsa for pointing out a bug (and a fix) in the code.
--- tools/perf/ui/browsers/hists.c | 63 ++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 20 deletions(-)
tools/perf/ui/browsers/hists.c +43 -20
@@ -27,6 +27,7 @@ struct hist_browser {bool show_dso;float min_pcnt;u64 nr_non_filtered_entries; +u64 nr_callchain_rows; };extern void hist_browser__init_hpp (void );
@@ -35,11 +36,27 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size, const char *ev_name);static void hist_browser__update_nr_entries (struct hist_browser *hb); +static struct rb_node *hists__filter_entries (struct rb_node *nd, +struct hists *hists, +float min_pcnt); +static bool hist_browser__has_filter (struct hist_browser *hb) {return hists__has_filter (hb->hists) || hb->min_pcnt; } +static u32 hist_browser__nr_entries (struct hist_browser *hb) +{ +u32 nr_entries; + +if (hist_browser__has_filter (hb)) + nr_entries = hb->nr_non_filtered_entries; +else + nr_entries = hb->hists->nr_entries; + +return nr_entries + hb->nr_callchain_rows; +} +static void hist_browser__refresh_dimensions (struct hist_browser *browser) {/* 3 == +/- toggle symbol before actual hist_entry rendering */
@@ -49,8 +66,14 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser)static void hist_browser__reset (struct hist_browser *browser) { +/* +* The hists__remove_entry_filter() already folds non-filtered +* entries so we can assume it has 0 callchain rows. +*/ + browser->nr_callchain_rows =0 ; +hist_browser__update_nr_entries (browser); - browser->b.nr_entries = browser->nr_non_filtered_entries; + browser->b.nr_entries =hist_browser__nr_entries (browser);hist_browser__refresh_dimensions (browser);ui_browser__reset_index (&browser->b); }
@@ -205,14 +228,16 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)struct hist_entry *he = browser->he_selection;hist_entry__init_have_children (he); - browser->hists->nr_entries -= he->nr_rows; + browser->b.nr_entries -= he->nr_rows; + browser->nr_callchain_rows -= he->nr_rows;if (he->ms.unfolded) he->nr_rows =callchain__count_rows (&he->sorted_chain);else he->nr_rows =0 ; - browser->hists->nr_entries += he->nr_rows; - browser->b.nr_entries = browser->hists->nr_entries; + + browser->b.nr_entries += he->nr_rows; + browser->nr_callchain_rows += he->nr_rows;return true; }
@@ -287,23 +312,27 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold) he->nr_rows =0 ; } -static void hists__set_folding (struct hists *hists,bool unfold) +static void +__hist_browser__set_folding (struct hist_browser *browser,bool unfold) {struct rb_node *nd; +struct hists *hists = browser->hists; - hists->nr_entries =0 ; - -for (nd =rb_first (&hists->entries); nd; nd =rb_next (nd)) { +for (nd =rb_first (&hists->entries); + (nd =hists__filter_entries (nd, hists, browser->min_pcnt)) !=NULL ; + nd =rb_next (nd)) { struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);hist_entry__set_folding (he, unfold); - hists->nr_entries +=1 + he->nr_rows; + browser->nr_callchain_rows += he->nr_rows; } }static void hist_browser__set_folding (struct hist_browser *browser,bool unfold) { -hists__set_folding (browser->hists, unfold); - browser->b.nr_entries = browser->hists->nr_entries; + browser->nr_callchain_rows =0 ; +__hist_browser__set_folding (browser, unfold); + + browser->b.nr_entries =hist_browser__nr_entries (browser);/* Go to the start, we may be way after valid entries after a collapse */ ui_browser__reset_index (&browser->b); }
@@ -325,10 +354,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,int delay_secs = hbt ? hbt->refresh :0 ; browser->b.entries = &browser->hists->entries; -if (hist_browser__has_filter (browser)) - browser->b.nr_entries = browser->nr_non_filtered_entries; -else - browser->b.nr_entries = browser->hists->nr_entries; + browser->b.nr_entries =hist_browser__nr_entries (browser);hist_browser__refresh_dimensions (browser);hists__browser_title (browser->hists, title,sizeof (title), ev_name);
@@ -345,13 +371,10 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,u64 nr_entries; hbt->timer (hbt->arg); -if (hist_browser__has_filter (browser)) { +if (hist_browser__has_filter (browser))hist_browser__update_nr_entries (browser); - nr_entries = browser->nr_non_filtered_entries; - }else { - nr_entries = browser->hists->nr_entries; - } + nr_entries =hist_browser__nr_entries (browser);ui_browser__update_nr_entries (&browser->b, nr_entries); if (browser->hists->stats.nr_lost_warned !=
Signature · 1 line
1.8.3.1
Quoted 42 lines · Jiri Olsa
hi Ingo,
please consider pullingthanks,
jirkaThe following changes since commit
a81fef347b32dea2b31275826afe1c93fa0d2d54:Merge tag 'perf-core-for-mingo' of
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perfinto perf/core (2014-04-22 20:28:23 +0200)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.gittags/perf-core-for-mingofor you to fetch changes up to
c3b789527b236873557f53740ceac47747e0e1cb:perf hists/tui: Count callchain rows separately (2014-04-24 16:34:27 +0200)
----------------------------------------------------------------
perf/core improvements and fixes:. Factor hists statistics counts processing which in turn also
fixes several bugs in TUI report command (Namhyung Kim)Signed-off-by Jiri Olsa <[email protected]>----------------------------------------------------------------
Namhyung Kim (11):
perf report: Count number of entries separately
perf hists: Renamehists__inc_stats()
perf hists: Move column length calculation out ofhists__inc_stats()perf hists: Add a couple of hists stat helper functions
perf hists: Collapse expanded callchains after filter is applied perf tools: Account entry stats when it's added to the output tree perf hists: Add missing update on filtered stats inhists__decay_entries()perf ui/tui: Fix off-by-one inhist_browser__update_nr_entries()perf ui/tui: Renamehist_browser__update_nr_entries()
perf top/tui: Updatenr_entriesproperly after a filter is applied perf hists/tui: Count callchain rows separately
Quoted 7 lines
tools/perf/builtin-annotate.c | 3 +- tools/perf/builtin-diff.c | 23 ++++++----- tools/perf/builtin-report.c | 64 ++++++++++++++--------------- tools/perf/ui/browsers/hists.c | 92 +++++++++++++++++++++++++++++------------- tools/perf/util/hist.c | 83 ++++++++++++++++++++++++------------- tools/perf/util/hist.h | 9 ++++- 6 files changed, 171 insertions(+), 103 deletions(-)
Pulled, thanks a lot Jiri!
Ingo