Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default
From: Ian Rogers
Date: Fri May 17 2024 - 23:19:16 EST
On Fri, May 17, 2024 at 6:25 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > Instead of decaying histograms over time change it so that they are
> > zero-ed on each perf top refresh. Previously the option '-z', or
> > pressing 'z' in tui mode, would enable this behavior. Decaying samples
> > is non-intuitive as it isn't how "top" works. Make zeroing on refresh
> > the default and rename the command line options from 'z' to 'Z' and
> > 'zero' to 'decay'.
>
> While it may make more sense, I'm afraid of changing the default
> behavior. I think we can add a config option for this.
I don't think the config option would clear up the confusion. I think
zero should be the default given it matches "top". The problem is
worse with filtering as samples will decay and disappear faster when
there are more of them. We could keep a -z option that does nothing,
for the sake of command line compatibility. I don't see the -z option
documented on the wiki, or Brendan Gregg's tutorials so my guess is
that people don't know it exists (I didn't) and they aren't confused
as cycles:P without filtering looks similar with zero or with
decaying.
Thanks,
Ian
> Also instead of adding a new option, it should be able to use the
> existing --no-zero option.
>
> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> > tools/perf/Documentation/perf-top.txt | 6 +++---
> > tools/perf/builtin-top.c | 23 +++++++++++++----------
> > tools/perf/ui/browsers/hists.c | 7 ++++---
> > tools/perf/util/top.h | 2 +-
> > 4 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
> > index 667e5102075e..f1524cc0d409 100644
> > --- a/tools/perf/Documentation/perf-top.txt
> > +++ b/tools/perf/Documentation/perf-top.txt
> > @@ -124,9 +124,9 @@ Default is to monitor all CPUS.
> > --verbose::
> > Be more verbose (show counter open errors, etc).
> >
> > --z::
> > ---zero::
> > - Zero history across display updates.
> > +-Z::
> > +--decay::
> > + Decay rather than zero history across display updates.
> >
> > -s::
> > --sort::
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index e8cbbf10d361..8f06635cb7cd 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -266,10 +266,10 @@ static void perf_top__show_details(struct perf_top *top)
> > more = symbol__annotate_printf(&he->ms, top->sym_evsel);
> >
> > if (top->evlist->enabled) {
> > - if (top->zero)
> > - symbol__annotate_zero_histogram(symbol, top->sym_evsel->core.idx);
> > - else
> > + if (top->decay_samples)
> > symbol__annotate_decay_histogram(symbol, top->sym_evsel->core.idx);
> > + else
> > + symbol__annotate_zero_histogram(symbol, top->sym_evsel->core.idx);
> > }
> > if (more != 0)
> > printf("%d lines not displayed, maybe increase display entries [e]\n", more);
> > @@ -292,11 +292,11 @@ static void perf_top__resort_hists(struct perf_top *t)
> > hists__unlink(hists);
> >
> > if (evlist->enabled) {
> > - if (t->zero) {
> > - hists__delete_entries(hists);
> > - } else {
> > + if (t->decay_samples) {
> > hists__decay_entries(hists, t->hide_user_symbols,
> > t->hide_kernel_symbols);
> > + } else {
> > + hists__delete_entries(hists);
> > }
> > }
> >
> > @@ -460,7 +460,9 @@ static void perf_top__print_mapped_keys(struct perf_top *top)
> > fprintf(stdout,
> > "\t[U] hide user symbols. \t(%s)\n",
> > top->hide_user_symbols ? "yes" : "no");
> > - fprintf(stdout, "\t[z] toggle sample zeroing. \t(%d)\n", top->zero ? 1 : 0);
> > + fprintf(stdout,
> > + "\t[z] toggle sample zeroing/decaying. \t(%s)\n",
> > + top->decay_samples ? "decay" : "zero");
> > fprintf(stdout, "\t[qQ] quit.\n");
> > }
> >
> > @@ -583,7 +585,7 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
> > top->hide_user_symbols = !top->hide_user_symbols;
> > break;
> > case 'z':
> > - top->zero = !top->zero;
> > + top->decay_samples = !top->decay_samples;
> > break;
> > default:
> > break;
> > @@ -648,7 +650,7 @@ static void *display_thread_tui(void *arg)
> > ret = evlist__tui_browse_hists(top->evlist, help, &hbt, top->min_percent,
> > &top->session->header.env, !top->record_opts.overwrite);
> > if (ret == K_RELOAD) {
> > - top->zero = true;
> > + top->decay_samples = false;
> > goto repeat;
> > } else
> > stop_top();
> > @@ -1502,7 +1504,8 @@ int cmd_top(int argc, const char **argv)
> > "child tasks do not inherit counters"),
> > OPT_STRING(0, "sym-annotate", &top.sym_filter, "symbol name",
> > "symbol to annotate"),
> > - OPT_BOOLEAN('z', "zero", &top.zero, "zero history across updates"),
> > + OPT_BOOLEAN('Z', "decay", &top.decay_samples,
> > + "decay rather than zero history across updates"),
> > OPT_CALLBACK('F', "freq", &top.record_opts, "freq or 'max'",
> > "profile at this frequency",
> > record__parse_freq),
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index b7219df51236..bcc4720f8198 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2305,8 +2305,8 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
> > " drop: %" PRIu64 "/%" PRIu64,
> > top->drop, top->drop_total);
> >
> > - if (top->zero)
> > - printed += scnprintf(bf + printed, size - printed, " [z]");
> > + if (top->decay_samples)
> > + printed += scnprintf(bf + printed, size - printed, " [decay]");
> >
> > perf_top__reset_sample_counters(top);
> > }
> > @@ -3209,9 +3209,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> > continue;
> > case 'z':
> > if (!is_report_browser(hbt)) {
> > + /* Toggle between zeroing and decaying samples. */
> > struct perf_top *top = hbt->arg;
> >
> > - top->zero = !top->zero;
> > + top->decay_samples = !top->decay_samples;
> > }
> > continue;
> > case 'L':
> > diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
> > index 4c5588dbb131..b2c199925b36 100644
> > --- a/tools/perf/util/top.h
> > +++ b/tools/perf/util/top.h
> > @@ -32,7 +32,7 @@ struct perf_top {
> > u64 guest_us_samples, guest_kernel_samples;
> > int print_entries, count_filter, delay_secs;
> > int max_stack;
> > - bool hide_kernel_symbols, hide_user_symbols, zero;
> > + bool hide_kernel_symbols, hide_user_symbols, decay_samples;
> > #ifdef HAVE_SLANG_SUPPORT
> > bool use_tui;
> > #endif
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >