linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf: replace automatic const char[] variables by statics
@ 2018-11-02 23:06 Rasmus Villemoes
  2018-11-04 19:26 ` Jiri Olsa
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2018-11-02 23:06 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim
  Cc: Rasmus Villemoes, linux-kernel

An automatic const char[] variable gets initialized at runtime, just
like any other automatic variable. For long strings, that uses a lot of
stack and wastes time building the string; e.g. for the "No %s
allocation events..." case one has

  444516:       48 b8 4e 6f 20 25 73 20 61 6c   movabs $0x6c61207325206f4e,%rax # "No %s al"
  ...
  444674:       48 89 45 80                     mov    %rax,-0x80(%rbp)
  444678:       48 b8 6c 6f 63 61 74 69 6f 6e   movabs $0x6e6f697461636f6c,%rax # "location"
  444682:       48 89 45 88                     mov    %rax,-0x78(%rbp)
  444686:       48 b8 20 65 76 65 6e 74 73 20   movabs $0x2073746e65766520,%rax # " events "
  444690:       66 44 89 55 c4                  mov    %r10w,-0x3c(%rbp)
  444695:       48 89 45 90                     mov    %rax,-0x70(%rbp)
  444699:       48 b8 66 6f 75 6e 64 2e 20 20   movabs $0x20202e646e756f66,%rax

Make them all static so that the compiler just references objects in .rodata.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 tools/perf/builtin-c2c.c        | 4 ++--
 tools/perf/builtin-kmem.c       | 4 ++--
 tools/perf/builtin-report.c     | 6 +++---
 tools/perf/builtin-sched.c      | 2 +-
 tools/perf/ui/browsers/header.c | 2 +-
 tools/perf/ui/browsers/hists.c  | 4 ++--
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index f3aa9d02a5ab..ab6a89e5393c 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2343,7 +2343,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	struct c2c_cacheline_browser *cl_browser;
 	struct hist_browser *browser;
 	int key = -1;
-	const char help[] =
+	static const char help[] =
 	" ENTER         Toggle callchains (if present) \n"
 	" n             Toggle Node details info \n"
 	" s             Toggle full length of symbol and source line columns \n"
@@ -2424,7 +2424,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
 {
 	struct hist_browser *browser;
 	int key = -1;
-	const char help[] =
+	static const char help[] =
 	" d             Display cacheline details \n"
 	" ENTER         Toggle callchains (if present) \n"
 	" q             Quit \n";
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index b63bca4b0c2a..088705c167bf 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -334,7 +334,7 @@ static int build_alloc_func_list(void)
 	struct alloc_func *func;
 	struct machine *machine = &kmem_session->machines.host;
 	regex_t alloc_func_regex;
-	const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
+	static const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
 
 	ret = regcomp(&alloc_func_regex, pattern, REG_EXTENDED);
 	if (ret) {
@@ -1924,7 +1924,7 @@ int cmd_kmem(int argc, const char **argv)
 		NULL
 	};
 	struct perf_session *session;
-	const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
+	static const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
 	int ret = perf_config(kmem_config, NULL);
 
 	if (ret)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 257c9c18cb7e..ff615c624784 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -955,9 +955,9 @@ int cmd_report(int argc, const char **argv)
 	int branch_mode = -1;
 	bool branch_call_mode = false;
 #define CALLCHAIN_DEFAULT_OPT  "graph,0.5,caller,function,percent"
-	const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
-					     CALLCHAIN_REPORT_HELP
-					     "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
+	static const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
+						    CALLCHAIN_REPORT_HELP
+						    "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
 	char callchain_default_opt[] = CALLCHAIN_DEFAULT_OPT;
 	const char * const report_usage[] = {
 		"perf report [<options>]",
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index cbf39dab19c1..2e0f0c65964a 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3336,7 +3336,7 @@ static int __cmd_record(int argc, const char **argv)
 
 int cmd_sched(int argc, const char **argv)
 {
-	const char default_sort_order[] = "avg, max, switch, runtime";
+	static const char default_sort_order[] = "avg, max, switch, runtime";
 	struct perf_sched sched = {
 		.tool = {
 			.sample		 = perf_sched__process_tracepoint_sample,
diff --git a/tools/perf/ui/browsers/header.c b/tools/perf/ui/browsers/header.c
index d75492189acb..5aeb663dd184 100644
--- a/tools/perf/ui/browsers/header.c
+++ b/tools/perf/ui/browsers/header.c
@@ -35,7 +35,7 @@ static int list_menu__run(struct ui_browser *menu)
 {
 	int key;
 	unsigned long offset;
-	const char help[] =
+	static const char help[] =
 	"h/?/F1        Show this window\n"
 	"UP/DOWN/PGUP\n"
 	"PGDN/SPACE\n"
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a96f62ca984a..eab9dc025b3a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2737,7 +2737,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	"S             Zoom into current Processor Socket\n"		\
 
 	/* help messages are sorted by lexical order of the hotkey */
-	const char report_help[] = HIST_BROWSER_HELP_COMMON
+	static const char report_help[] = HIST_BROWSER_HELP_COMMON
 	"i             Show header information\n"
 	"P             Print histograms to perf.hist.N\n"
 	"r             Run available scripts\n"
@@ -2745,7 +2745,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	"t             Zoom into current Thread\n"
 	"V             Verbose (DSO names in callchains, etc)\n"
 	"/             Filter symbol by name";
-	const char top_help[] = HIST_BROWSER_HELP_COMMON
+	static const char top_help[] = HIST_BROWSER_HELP_COMMON
 	"P             Print histograms to perf.hist.N\n"
 	"t             Zoom into current Thread\n"
 	"V             Verbose (DSO names in callchains, etc)\n"
-- 
2.19.1.6.gbde171bbf5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: replace automatic const char[] variables by statics
  2018-11-02 23:06 [PATCH] perf: replace automatic const char[] variables by statics Rasmus Villemoes
@ 2018-11-04 19:26 ` Jiri Olsa
  2019-01-10 21:12   ` Rasmus Villemoes
  2018-11-04 21:52 ` [PATCH] checkpatch: Warn on const char foo[] = "bar"; declarations Joe Perches
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2018-11-04 19:26 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Namhyung Kim, linux-kernel

On Sat, Nov 03, 2018 at 12:06:23AM +0100, Rasmus Villemoes wrote:
> An automatic const char[] variable gets initialized at runtime, just
> like any other automatic variable. For long strings, that uses a lot of
> stack and wastes time building the string; e.g. for the "No %s
> allocation events..." case one has
> 
>   444516:       48 b8 4e 6f 20 25 73 20 61 6c   movabs $0x6c61207325206f4e,%rax # "No %s al"
>   ...
>   444674:       48 89 45 80                     mov    %rax,-0x80(%rbp)
>   444678:       48 b8 6c 6f 63 61 74 69 6f 6e   movabs $0x6e6f697461636f6c,%rax # "location"
>   444682:       48 89 45 88                     mov    %rax,-0x78(%rbp)
>   444686:       48 b8 20 65 76 65 6e 74 73 20   movabs $0x2073746e65766520,%rax # " events "
>   444690:       66 44 89 55 c4                  mov    %r10w,-0x3c(%rbp)
>   444695:       48 89 45 90                     mov    %rax,-0x70(%rbp)
>   444699:       48 b8 66 6f 75 6e 64 2e 20 20   movabs $0x20202e646e756f66,%rax
> 
> Make them all static so that the compiler just references objects in .rodata.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

sounds good

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/builtin-c2c.c        | 4 ++--
>  tools/perf/builtin-kmem.c       | 4 ++--
>  tools/perf/builtin-report.c     | 6 +++---
>  tools/perf/builtin-sched.c      | 2 +-
>  tools/perf/ui/browsers/header.c | 2 +-
>  tools/perf/ui/browsers/hists.c  | 4 ++--
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index f3aa9d02a5ab..ab6a89e5393c 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -2343,7 +2343,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
>  	struct c2c_cacheline_browser *cl_browser;
>  	struct hist_browser *browser;
>  	int key = -1;
> -	const char help[] =
> +	static const char help[] =
>  	" ENTER         Toggle callchains (if present) \n"
>  	" n             Toggle Node details info \n"
>  	" s             Toggle full length of symbol and source line columns \n"
> @@ -2424,7 +2424,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
>  {
>  	struct hist_browser *browser;
>  	int key = -1;
> -	const char help[] =
> +	static const char help[] =
>  	" d             Display cacheline details \n"
>  	" ENTER         Toggle callchains (if present) \n"
>  	" q             Quit \n";
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index b63bca4b0c2a..088705c167bf 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -334,7 +334,7 @@ static int build_alloc_func_list(void)
>  	struct alloc_func *func;
>  	struct machine *machine = &kmem_session->machines.host;
>  	regex_t alloc_func_regex;
> -	const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
> +	static const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
>  
>  	ret = regcomp(&alloc_func_regex, pattern, REG_EXTENDED);
>  	if (ret) {
> @@ -1924,7 +1924,7 @@ int cmd_kmem(int argc, const char **argv)
>  		NULL
>  	};
>  	struct perf_session *session;
> -	const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
> +	static const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
>  	int ret = perf_config(kmem_config, NULL);
>  
>  	if (ret)
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 257c9c18cb7e..ff615c624784 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -955,9 +955,9 @@ int cmd_report(int argc, const char **argv)
>  	int branch_mode = -1;
>  	bool branch_call_mode = false;
>  #define CALLCHAIN_DEFAULT_OPT  "graph,0.5,caller,function,percent"
> -	const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
> -					     CALLCHAIN_REPORT_HELP
> -					     "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
> +	static const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
> +						    CALLCHAIN_REPORT_HELP
> +						    "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
>  	char callchain_default_opt[] = CALLCHAIN_DEFAULT_OPT;
>  	const char * const report_usage[] = {
>  		"perf report [<options>]",
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index cbf39dab19c1..2e0f0c65964a 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3336,7 +3336,7 @@ static int __cmd_record(int argc, const char **argv)
>  
>  int cmd_sched(int argc, const char **argv)
>  {
> -	const char default_sort_order[] = "avg, max, switch, runtime";
> +	static const char default_sort_order[] = "avg, max, switch, runtime";
>  	struct perf_sched sched = {
>  		.tool = {
>  			.sample		 = perf_sched__process_tracepoint_sample,
> diff --git a/tools/perf/ui/browsers/header.c b/tools/perf/ui/browsers/header.c
> index d75492189acb..5aeb663dd184 100644
> --- a/tools/perf/ui/browsers/header.c
> +++ b/tools/perf/ui/browsers/header.c
> @@ -35,7 +35,7 @@ static int list_menu__run(struct ui_browser *menu)
>  {
>  	int key;
>  	unsigned long offset;
> -	const char help[] =
> +	static const char help[] =
>  	"h/?/F1        Show this window\n"
>  	"UP/DOWN/PGUP\n"
>  	"PGDN/SPACE\n"
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index a96f62ca984a..eab9dc025b3a 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2737,7 +2737,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  	"S             Zoom into current Processor Socket\n"		\
>  
>  	/* help messages are sorted by lexical order of the hotkey */
> -	const char report_help[] = HIST_BROWSER_HELP_COMMON
> +	static const char report_help[] = HIST_BROWSER_HELP_COMMON
>  	"i             Show header information\n"
>  	"P             Print histograms to perf.hist.N\n"
>  	"r             Run available scripts\n"
> @@ -2745,7 +2745,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  	"t             Zoom into current Thread\n"
>  	"V             Verbose (DSO names in callchains, etc)\n"
>  	"/             Filter symbol by name";
> -	const char top_help[] = HIST_BROWSER_HELP_COMMON
> +	static const char top_help[] = HIST_BROWSER_HELP_COMMON
>  	"P             Print histograms to perf.hist.N\n"
>  	"t             Zoom into current Thread\n"
>  	"V             Verbose (DSO names in callchains, etc)\n"
> -- 
> 2.19.1.6.gbde171bbf5
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] checkpatch: Warn on const char foo[]  = "bar"; declarations
  2018-11-02 23:06 [PATCH] perf: replace automatic const char[] variables by statics Rasmus Villemoes
  2018-11-04 19:26 ` Jiri Olsa
@ 2018-11-04 21:52 ` Joe Perches
  2019-01-11 13:32 ` [PATCH] perf: replace automatic const char[] variables by statics Arnaldo Carvalho de Melo
  2019-01-22 10:09 ` [tip:perf/core] perf tools: Replace " tip-bot for Rasmus Villemoes
  3 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2018-11-04 21:52 UTC (permalink / raw)
  To: Andrew Morton, Andy Whitcroft; +Cc: Rasmus Villemoes, linux-kernel

These declarations should generally be static const to avoid
poor compilation and runtime performance where compilers
tend to initialize the const declaration for every call
instead of using .rodata for the string.

Miscellanea:

o Convert spaces to tabs for indentation in 2 adjacent checks

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c883ec55654f..7789e13dac9d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3869,14 +3869,23 @@ sub process {
 			WARN("STATIC_CONST_CHAR_ARRAY",
 			     "static const char * array should probably be static const char * const\n" .
 				$herecurr);
-               }
+		}
+
+# check for initialized const char arrays that should be static const
+		if ($line =~ /^\+\s*const\s+(char|unsigned\s+char|_*u8|(?:[us]_)?int8_t)\s+\w+\s*\[\s*(?:\w+\s*)?\]\s*=\s*"/) {
+			if (WARN("STATIC_CONST_CHAR_ARRAY",
+				 "const array should probably be static const\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/(^.\s*)const\b/${1}static const/;
+			}
+		}
 
 # check for static char foo[] = "bar" declarations.
 		if ($line =~ /\bstatic\s+char\s+(\w+)\s*\[\s*\]\s*=\s*"/) {
 			WARN("STATIC_CONST_CHAR_ARRAY",
 			     "static char array declaration should probably be static const char\n" .
 				$herecurr);
-               }
+		}
 
 # check for const <foo> const where <foo> is not a pointer or array type
 		if ($sline =~ /\bconst\s+($BasicType)\s+const\b/) {


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: replace automatic const char[] variables by statics
  2018-11-04 19:26 ` Jiri Olsa
@ 2019-01-10 21:12   ` Rasmus Villemoes
  0 siblings, 0 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2019-01-10 21:12 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Namhyung Kim, linux-kernel

ping

On Sun, 4 Nov 2018 at 20:26, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Sat, Nov 03, 2018 at 12:06:23AM +0100, Rasmus Villemoes wrote:
> > An automatic const char[] variable gets initialized at runtime, just
> > like any other automatic variable. For long strings, that uses a lot of
> > stack and wastes time building the string; e.g. for the "No %s
> > allocation events..." case one has
> >
> >   444516:       48 b8 4e 6f 20 25 73 20 61 6c   movabs $0x6c61207325206f4e,%rax # "No %s al"
> >   ...
> >   444674:       48 89 45 80                     mov    %rax,-0x80(%rbp)
> >   444678:       48 b8 6c 6f 63 61 74 69 6f 6e   movabs $0x6e6f697461636f6c,%rax # "location"
> >   444682:       48 89 45 88                     mov    %rax,-0x78(%rbp)
> >   444686:       48 b8 20 65 76 65 6e 74 73 20   movabs $0x2073746e65766520,%rax # " events "
> >   444690:       66 44 89 55 c4                  mov    %r10w,-0x3c(%rbp)
> >   444695:       48 89 45 90                     mov    %rax,-0x70(%rbp)
> >   444699:       48 b8 66 6f 75 6e 64 2e 20 20   movabs $0x20202e646e756f66,%rax
> >
> > Make them all static so that the compiler just references objects in .rodata.
> >
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> sounds good
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
> > ---
> >  tools/perf/builtin-c2c.c        | 4 ++--
> >  tools/perf/builtin-kmem.c       | 4 ++--
> >  tools/perf/builtin-report.c     | 6 +++---
> >  tools/perf/builtin-sched.c      | 2 +-
> >  tools/perf/ui/browsers/header.c | 2 +-
> >  tools/perf/ui/browsers/hists.c  | 4 ++--
> >  6 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index f3aa9d02a5ab..ab6a89e5393c 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -2343,7 +2343,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
> >       struct c2c_cacheline_browser *cl_browser;
> >       struct hist_browser *browser;
> >       int key = -1;
> > -     const char help[] =
> > +     static const char help[] =
> >       " ENTER         Toggle callchains (if present) \n"
> >       " n             Toggle Node details info \n"
> >       " s             Toggle full length of symbol and source line columns \n"
> > @@ -2424,7 +2424,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
> >  {
> >       struct hist_browser *browser;
> >       int key = -1;
> > -     const char help[] =
> > +     static const char help[] =
> >       " d             Display cacheline details \n"
> >       " ENTER         Toggle callchains (if present) \n"
> >       " q             Quit \n";
> > diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> > index b63bca4b0c2a..088705c167bf 100644
> > --- a/tools/perf/builtin-kmem.c
> > +++ b/tools/perf/builtin-kmem.c
> > @@ -334,7 +334,7 @@ static int build_alloc_func_list(void)
> >       struct alloc_func *func;
> >       struct machine *machine = &kmem_session->machines.host;
> >       regex_t alloc_func_regex;
> > -     const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
> > +     static const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
> >
> >       ret = regcomp(&alloc_func_regex, pattern, REG_EXTENDED);
> >       if (ret) {
> > @@ -1924,7 +1924,7 @@ int cmd_kmem(int argc, const char **argv)
> >               NULL
> >       };
> >       struct perf_session *session;
> > -     const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
> > +     static const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
> >       int ret = perf_config(kmem_config, NULL);
> >
> >       if (ret)
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 257c9c18cb7e..ff615c624784 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -955,9 +955,9 @@ int cmd_report(int argc, const char **argv)
> >       int branch_mode = -1;
> >       bool branch_call_mode = false;
> >  #define CALLCHAIN_DEFAULT_OPT  "graph,0.5,caller,function,percent"
> > -     const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
> > -                                          CALLCHAIN_REPORT_HELP
> > -                                          "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
> > +     static const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
> > +                                                 CALLCHAIN_REPORT_HELP
> > +                                                 "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
> >       char callchain_default_opt[] = CALLCHAIN_DEFAULT_OPT;
> >       const char * const report_usage[] = {
> >               "perf report [<options>]",
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index cbf39dab19c1..2e0f0c65964a 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -3336,7 +3336,7 @@ static int __cmd_record(int argc, const char **argv)
> >
> >  int cmd_sched(int argc, const char **argv)
> >  {
> > -     const char default_sort_order[] = "avg, max, switch, runtime";
> > +     static const char default_sort_order[] = "avg, max, switch, runtime";
> >       struct perf_sched sched = {
> >               .tool = {
> >                       .sample          = perf_sched__process_tracepoint_sample,
> > diff --git a/tools/perf/ui/browsers/header.c b/tools/perf/ui/browsers/header.c
> > index d75492189acb..5aeb663dd184 100644
> > --- a/tools/perf/ui/browsers/header.c
> > +++ b/tools/perf/ui/browsers/header.c
> > @@ -35,7 +35,7 @@ static int list_menu__run(struct ui_browser *menu)
> >  {
> >       int key;
> >       unsigned long offset;
> > -     const char help[] =
> > +     static const char help[] =
> >       "h/?/F1        Show this window\n"
> >       "UP/DOWN/PGUP\n"
> >       "PGDN/SPACE\n"
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index a96f62ca984a..eab9dc025b3a 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2737,7 +2737,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
> >       "S             Zoom into current Processor Socket\n"            \
> >
> >       /* help messages are sorted by lexical order of the hotkey */
> > -     const char report_help[] = HIST_BROWSER_HELP_COMMON
> > +     static const char report_help[] = HIST_BROWSER_HELP_COMMON
> >       "i             Show header information\n"
> >       "P             Print histograms to perf.hist.N\n"
> >       "r             Run available scripts\n"
> > @@ -2745,7 +2745,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
> >       "t             Zoom into current Thread\n"
> >       "V             Verbose (DSO names in callchains, etc)\n"
> >       "/             Filter symbol by name";
> > -     const char top_help[] = HIST_BROWSER_HELP_COMMON
> > +     static const char top_help[] = HIST_BROWSER_HELP_COMMON
> >       "P             Print histograms to perf.hist.N\n"
> >       "t             Zoom into current Thread\n"
> >       "V             Verbose (DSO names in callchains, etc)\n"
> > --
> > 2.19.1.6.gbde171bbf5
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: replace automatic const char[] variables by statics
  2018-11-02 23:06 [PATCH] perf: replace automatic const char[] variables by statics Rasmus Villemoes
  2018-11-04 19:26 ` Jiri Olsa
  2018-11-04 21:52 ` [PATCH] checkpatch: Warn on const char foo[] = "bar"; declarations Joe Perches
@ 2019-01-11 13:32 ` Arnaldo Carvalho de Melo
  2019-01-22 10:09 ` [tip:perf/core] perf tools: Replace " tip-bot for Rasmus Villemoes
  3 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-01-11 13:32 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, linux-kernel

Em Sat, Nov 03, 2018 at 12:06:23AM +0100, Rasmus Villemoes escreveu:
> An automatic const char[] variable gets initialized at runtime, just
> like any other automatic variable. For long strings, that uses a lot of
> stack and wastes time building the string; e.g. for the "No %s
> allocation events..." case one has
> 
>   444516:       48 b8 4e 6f 20 25 73 20 61 6c   movabs $0x6c61207325206f4e,%rax # "No %s al"
>   ...
>   444674:       48 89 45 80                     mov    %rax,-0x80(%rbp)
>   444678:       48 b8 6c 6f 63 61 74 69 6f 6e   movabs $0x6e6f697461636f6c,%rax # "location"
>   444682:       48 89 45 88                     mov    %rax,-0x78(%rbp)
>   444686:       48 b8 20 65 76 65 6e 74 73 20   movabs $0x2073746e65766520,%rax # " events "
>   444690:       66 44 89 55 c4                  mov    %r10w,-0x3c(%rbp)
>   444695:       48 89 45 90                     mov    %rax,-0x70(%rbp)
>   444699:       48 b8 66 6f 75 6e 64 2e 20 20   movabs $0x20202e646e756f66,%rax
> 
> Make them all static so that the compiler just references objects in .rodata.
> 

Ok, using dwarves's codiff tool:

  $ codiff --functions /tmp/perf.before ~/bin/perf
builtin-sched.c:
  cmd_sched                 |  -48
 1 function changed, 48 bytes removed, diff: -48

builtin-report.c:
  cmd_report                |  -32
 1 function changed, 32 bytes removed, diff: -32

builtin-kmem.c:
  cmd_kmem                  |  -64
  build_alloc_func_list     |  -50
 2 functions changed, 114 bytes removed, diff: -114

builtin-c2c.c:
  perf_c2c__report          | -390
 1 function changed, 390 bytes removed, diff: -390

ui/browsers/header.c:
  tui__header_window        | -104
 1 function changed, 104 bytes removed, diff: -104

/home/acme/bin/perf:
 9 functions changed, 688 bytes removed, diff: -688

Thanks, applied.

- Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:perf/core] perf tools: Replace automatic const char[] variables by statics
  2018-11-02 23:06 [PATCH] perf: replace automatic const char[] variables by statics Rasmus Villemoes
                   ` (2 preceding siblings ...)
  2019-01-11 13:32 ` [PATCH] perf: replace automatic const char[] variables by statics Arnaldo Carvalho de Melo
@ 2019-01-22 10:09 ` tip-bot for Rasmus Villemoes
  3 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Rasmus Villemoes @ 2019-01-22 10:09 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, peterz, jolsa, hpa, namhyung, mingo, linux, linux-kernel,
	alexander.shishkin, tglx

Commit-ID:  49b8e2beceda79ae25938faf68fad0ad62534852
Gitweb:     https://git.kernel.org/tip/49b8e2beceda79ae25938faf68fad0ad62534852
Author:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
AuthorDate: Sat, 3 Nov 2018 00:06:23 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 21 Jan 2019 15:15:57 -0300

perf tools: Replace automatic const char[] variables by statics

An automatic const char[] variable gets initialized at runtime, just
like any other automatic variable. For long strings, that uses a lot of
stack and wastes time building the string; e.g. for the "No %s
allocation events..." case one has:

  444516:       48 b8 4e 6f 20 25 73 20 61 6c   movabs $0x6c61207325206f4e,%rax # "No %s al"
  ...
  444674:       48 89 45 80                     mov    %rax,-0x80(%rbp)
  444678:       48 b8 6c 6f 63 61 74 69 6f 6e   movabs $0x6e6f697461636f6c,%rax # "location"
  444682:       48 89 45 88                     mov    %rax,-0x78(%rbp)
  444686:       48 b8 20 65 76 65 6e 74 73 20   movabs $0x2073746e65766520,%rax # " events "
  444690:       66 44 89 55 c4                  mov    %r10w,-0x3c(%rbp)
  444695:       48 89 45 90                     mov    %rax,-0x70(%rbp)
  444699:       48 b8 66 6f 75 6e 64 2e 20 20   movabs $0x20202e646e756f66,%rax

Make them all static so that the compiler just references objects in .rodata.

Committer testing:

Ok, using dwarves's codiff tool:

    $ codiff --functions /tmp/perf.before ~/bin/perf
  builtin-sched.c:
    cmd_sched                 |  -48
   1 function changed, 48 bytes removed, diff: -48

  builtin-report.c:
    cmd_report                |  -32
   1 function changed, 32 bytes removed, diff: -32

  builtin-kmem.c:
    cmd_kmem                  |  -64
    build_alloc_func_list     |  -50
   2 functions changed, 114 bytes removed, diff: -114

  builtin-c2c.c:
    perf_c2c__report          | -390
   1 function changed, 390 bytes removed, diff: -390

  ui/browsers/header.c:
    tui__header_window        | -104
   1 function changed, 104 bytes removed, diff: -104

  /home/acme/bin/perf:
   9 functions changed, 688 bytes removed, diff: -688

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20181102230624.20064-1-linux@rasmusvillemoes.dk
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c        | 4 ++--
 tools/perf/builtin-kmem.c       | 4 ++--
 tools/perf/builtin-report.c     | 6 +++---
 tools/perf/builtin-sched.c      | 2 +-
 tools/perf/ui/browsers/header.c | 2 +-
 tools/perf/ui/browsers/hists.c  | 4 ++--
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index d340d2e42776..f2863496dede 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2343,7 +2343,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	struct c2c_cacheline_browser *cl_browser;
 	struct hist_browser *browser;
 	int key = -1;
-	const char help[] =
+	static const char help[] =
 	" ENTER         Toggle callchains (if present) \n"
 	" n             Toggle Node details info \n"
 	" s             Toggle full length of symbol and source line columns \n"
@@ -2424,7 +2424,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
 {
 	struct hist_browser *browser;
 	int key = -1;
-	const char help[] =
+	static const char help[] =
 	" d             Display cacheline details \n"
 	" ENTER         Toggle callchains (if present) \n"
 	" q             Quit \n";
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index b63bca4b0c2a..088705c167bf 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -334,7 +334,7 @@ static int build_alloc_func_list(void)
 	struct alloc_func *func;
 	struct machine *machine = &kmem_session->machines.host;
 	regex_t alloc_func_regex;
-	const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
+	static const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
 
 	ret = regcomp(&alloc_func_regex, pattern, REG_EXTENDED);
 	if (ret) {
@@ -1924,7 +1924,7 @@ int cmd_kmem(int argc, const char **argv)
 		NULL
 	};
 	struct perf_session *session;
-	const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
+	static const char errmsg[] = "No %s allocation events found.  Have you run 'perf kmem record --%s'?\n";
 	int ret = perf_config(kmem_config, NULL);
 
 	if (ret)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4958095be4fc..794a0de5ac1d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -956,9 +956,9 @@ int cmd_report(int argc, const char **argv)
 	int branch_mode = -1;
 	bool branch_call_mode = false;
 #define CALLCHAIN_DEFAULT_OPT  "graph,0.5,caller,function,percent"
-	const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
-					     CALLCHAIN_REPORT_HELP
-					     "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
+	static const char report_callchain_help[] = "Display call graph (stack chain/backtrace):\n\n"
+						    CALLCHAIN_REPORT_HELP
+						    "\n\t\t\t\tDefault: " CALLCHAIN_DEFAULT_OPT;
 	char callchain_default_opt[] = CALLCHAIN_DEFAULT_OPT;
 	const char * const report_usage[] = {
 		"perf report [<options>]",
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index cbf39dab19c1..2e0f0c65964a 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3336,7 +3336,7 @@ static int __cmd_record(int argc, const char **argv)
 
 int cmd_sched(int argc, const char **argv)
 {
-	const char default_sort_order[] = "avg, max, switch, runtime";
+	static const char default_sort_order[] = "avg, max, switch, runtime";
 	struct perf_sched sched = {
 		.tool = {
 			.sample		 = perf_sched__process_tracepoint_sample,
diff --git a/tools/perf/ui/browsers/header.c b/tools/perf/ui/browsers/header.c
index d75492189acb..5aeb663dd184 100644
--- a/tools/perf/ui/browsers/header.c
+++ b/tools/perf/ui/browsers/header.c
@@ -35,7 +35,7 @@ static int list_menu__run(struct ui_browser *menu)
 {
 	int key;
 	unsigned long offset;
-	const char help[] =
+	static const char help[] =
 	"h/?/F1        Show this window\n"
 	"UP/DOWN/PGUP\n"
 	"PGDN/SPACE\n"
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ffac1d54a3d4..b32f505a7608 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2748,7 +2748,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	"S             Zoom into current Processor Socket\n"		\
 
 	/* help messages are sorted by lexical order of the hotkey */
-	const char report_help[] = HIST_BROWSER_HELP_COMMON
+	static const char report_help[] = HIST_BROWSER_HELP_COMMON
 	"i             Show header information\n"
 	"P             Print histograms to perf.hist.N\n"
 	"r             Run available scripts\n"
@@ -2756,7 +2756,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	"t             Zoom into current Thread\n"
 	"V             Verbose (DSO names in callchains, etc)\n"
 	"/             Filter symbol by name";
-	const char top_help[] = HIST_BROWSER_HELP_COMMON
+	static const char top_help[] = HIST_BROWSER_HELP_COMMON
 	"P             Print histograms to perf.hist.N\n"
 	"t             Zoom into current Thread\n"
 	"V             Verbose (DSO names in callchains, etc)\n"

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-22 10:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-02 23:06 [PATCH] perf: replace automatic const char[] variables by statics Rasmus Villemoes
2018-11-04 19:26 ` Jiri Olsa
2019-01-10 21:12   ` Rasmus Villemoes
2018-11-04 21:52 ` [PATCH] checkpatch: Warn on const char foo[] = "bar"; declarations Joe Perches
2019-01-11 13:32 ` [PATCH] perf: replace automatic const char[] variables by statics Arnaldo Carvalho de Melo
2019-01-22 10:09 ` [tip:perf/core] perf tools: Replace " tip-bot for Rasmus Villemoes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).