[PATCH v1] perf top: Make zeroing histogram on refresh the default

[PATCH v1] perf top: Make zeroing histogram on refresh the default

[Ian Rogers]

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'.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Namhyung Kim]

On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@google.com> 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.

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@google.com>
> ---
>  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
>

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Ian Rogers]

On Fri, May 17, 2024 at 6:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@google.com> 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@google.com>
> > ---
> >  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
> >

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Arnaldo Carvalho de Melo]

On Fri, May 17, 2024 at 06:25:16PM -0700, Namhyung Kim wrote:
> On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@google.com> 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.
 
> Also instead of adding a new option, it should be able to use the
> existing --no-zero option.

Hi Ingo,

	What do you think? -z and the default to decaying is in 'perf
top' since the very beginning, when it was called kerneltop:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0143bad9dbf2a8fad4c5430562bceba196b66ea

Cheers,

- Arnaldo

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Ian Rogers]

On Tue, May 21, 2024 at 12:46 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, May 17, 2024 at 06:25:16PM -0700, Namhyung Kim wrote:
> > On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@google.com> 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.
>
> > Also instead of adding a new option, it should be able to use the
> > existing --no-zero option.
>
> Hi Ingo,
>
>         What do you think? -z and the default to decaying is in 'perf
> top' since the very beginning, when it was called kerneltop:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0143bad9dbf2a8fad4c5430562bceba196b66ea

Ping, thanks,
Ian

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Ravi Bangoria]

On 5/17/2024 3:51 AM, Ian Rogers 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'.
I've also felt `perf top` decay as non-intuitive. Esp. when system becomes
idle after some heavy workload, even decayed samples are far more compared
to samples from currently running processes and thus `perf top` keeps
showing already finished processes at the top, which is kind of confusing.
fwiw:

Acked-by: Ravi Bangoria <ravi.bangoria@amd.com>

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Arnaldo Carvalho de Melo]

On Thu, Jun 06, 2024 at 10:15:00AM +0530, Ravi Bangoria wrote:
> On 5/17/2024 3:51 AM, Ian Rogers 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'.

> I've also felt `perf top` decay as non-intuitive. Esp. when system becomes
> idle after some heavy workload, even decayed samples are far more compared
> to samples from currently running processes and thus `perf top` keeps
> showing already finished processes at the top, which is kind of confusing.
> fwiw:
 
> Acked-by: Ravi Bangoria <ravi.bangoria@amd.com>

Thanks for voicing your opinion, that is really helpful.

Changing tool behaviour can have unintended consequences even when done
with the best intentions and analysis, that is why I'm wary of doing it.

The --children case generated complaints when we made it the default, so
we ended up with a ~/.perfconfig option to disable it:

root@number:~# perf config top.children=false
root@number:~# perf top -g

Or enable explicitely:

root@number:~# perf config top.children=true
root@number:~# perf top -g

Same thing with the build id cache, where one can disable it using 'perf
config', etc.

So I'd do this initially with a 'perf config top.refresh=zero' instead
of changing something so few people complained as not being intuitive
after all those years of having that default.

- Arnaldo

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Ravi Bangoria]

On 6/6/2024 7:41 PM, Arnaldo Carvalho de Melo wrote:
> On Thu, Jun 06, 2024 at 10:15:00AM +0530, Ravi Bangoria wrote:
>> On 5/17/2024 3:51 AM, Ian Rogers 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'.
> 
>> I've also felt `perf top` decay as non-intuitive. Esp. when system becomes
>> idle after some heavy workload, even decayed samples are far more compared
>> to samples from currently running processes and thus `perf top` keeps
>> showing already finished processes at the top, which is kind of confusing.
>> fwiw:
>  
>> Acked-by: Ravi Bangoria <ravi.bangoria@amd.com>
> 
> Thanks for voicing your opinion, that is really helpful.
> 
> Changing tool behaviour can have unintended consequences even when done
> with the best intentions and analysis, that is why I'm wary of doing it.
> 
> The --children case generated complaints when we made it the default, so
> we ended up with a ~/.perfconfig option to disable it:
> 
> root@number:~# perf config top.children=false
> root@number:~# perf top -g
> 
> Or enable explicitely:
> 
> root@number:~# perf config top.children=true
> root@number:~# perf top -g
> 
> Same thing with the build id cache, where one can disable it using 'perf
> config', etc.
> 
> So I'd do this initially with a 'perf config top.refresh=zero' instead
> of changing something so few people complained as not being intuitive
> after all those years of having that default.

Makes sense. Thanks for the clarification.

Ravi

Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

[Namhyung Kim]

On Thu, Jun 06, 2024 at 11:11:38AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jun 06, 2024 at 10:15:00AM +0530, Ravi Bangoria wrote:
> > On 5/17/2024 3:51 AM, Ian Rogers 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'.
> 
> > I've also felt `perf top` decay as non-intuitive. Esp. when system becomes
> > idle after some heavy workload, even decayed samples are far more compared
> > to samples from currently running processes and thus `perf top` keeps
> > showing already finished processes at the top, which is kind of confusing.
> > fwiw:
>  
> > Acked-by: Ravi Bangoria <ravi.bangoria@amd.com>
> 
> Thanks for voicing your opinion, that is really helpful.
> 
> Changing tool behaviour can have unintended consequences even when done
> with the best intentions and analysis, that is why I'm wary of doing it.
> 
> The --children case generated complaints when we made it the default, so
> we ended up with a ~/.perfconfig option to disable it:
> 
> root@number:~# perf config top.children=false
> root@number:~# perf top -g
> 
> Or enable explicitely:
> 
> root@number:~# perf config top.children=true
> root@number:~# perf top -g
> 
> Same thing with the build id cache, where one can disable it using 'perf
> config', etc.
> 
> So I'd do this initially with a 'perf config top.refresh=zero' instead
> of changing something so few people complained as not being intuitive
> after all those years of having that default.

This kind of thing happens periodically, I guess we can add some message
to help users to set the default config when it's not set.  In TUI, it
can even set the config directly according to the users response.

Thanks,
Namhyung