[PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patches only put a single null byte at the beginning of each
temporary buffers line[], offs[], file[] instead of filling their full
contents with null bytes.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 tools/perf/util/probe-event.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index adc2620..e453f13 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1058,9 +1058,11 @@ error:
 static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
 {
 	char *buf, *tmp;
-	char offs[32] = "", line[32] = "", file[32] = "";
+	char offs[32], line[32], file[32];
 	int ret, len;
 
+	offs[0] = line[0] = file[0] = '\0';
+
 	buf = zalloc(MAX_CMDLEN);
 	if (buf == NULL) {
 		ret = -ENOMEM;
-- 
1.7.3.2

Re: [PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[Masami Hiramatsu]

(2010/12/24 0:27), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> This patches only put a single null byte at the beginning of each
> temporary buffers line[], offs[], file[] instead of filling their full
> contents with null bytes.

Hmm, sorry but NAK it.

IMHO, with modern chips, the original code has no problem from the
viewpoint of memory access (all are cached and no need to access just
one byte) nor a bottleneck.
I'd rather use '= ""' style initialization for local variables from the
viewpoint of readability.

Anyway, thank you for looking into the code :-)

Thanks,

> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  tools/perf/util/probe-event.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index adc2620..e453f13 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1058,9 +1058,11 @@ error:
>  static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
>  {
>  	char *buf, *tmp;
> -	char offs[32] = "", line[32] = "", file[32] = "";
> +	char offs[32], line[32], file[32];
>  	int ret, len;
>  
> +	offs[0] = line[0] = file[0] = '\0';
> +
>  	buf = zalloc(MAX_CMDLEN);
>  	if (buf == NULL) {
>  		ret = -ENOMEM;


-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[Arnaldo Carvalho de Melo]

Em Fri, Dec 24, 2010 at 01:46:09PM +0900, Masami Hiramatsu escreveu:
> (2010/12/24 0:27), Franck Bui-Huu wrote:
> > This patches only put a single null byte at the beginning of each
> > temporary buffers line[], offs[], file[] instead of filling their
> > full contents with null bytes.

> Hmm, sorry but NAK it.

> IMHO, with modern chips, the original code has no problem from the
> viewpoint of memory access (all are cached and no need to access just
> one byte) nor a bottleneck.
> I'd rather use '= ""' style initialization for local variables from the
> viewpoint of readability.

No strong feelings here, not really a fast path, just learned something
new, I thought that that kind of initialization would be equivalent to
what Franck proposed, but gcc really uses the most efficient way of
zeroing the whole string (movq for things like that, and rep stos for
bigger arrays, etc).
 
> Anyway, thank you for looking into the code :-)

Yes, please continue sending your improvements and fixes!

Masami, what about the fixes Franck sent, clould you please send ACK or
NACKs for those?

- Arnaldo

Re: [PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[Franck Bui-Huu]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:

> (2010/12/24 0:27), Franck Bui-Huu wrote:
>> From: Franck Bui-Huu <fbuihuu@gmail.com>
>> 
>> This patches only put a single null byte at the beginning of each
>> temporary buffers line[], offs[], file[] instead of filling their full
>> contents with null bytes.
>
> Hmm, sorry but NAK it.
>

No problem :)

>
> IMHO, with modern chips, the original code has no problem from the
> viewpoint of memory access (all are cached and no need to access just
> one byte) nor a bottleneck.

I'm not sure to understand this.

But my point is that you're clearing the whole buffers with 0 although
you just need to initialize them with the null string (a single null
byte at the beginning).

So you're doing useless memory accesses (cached or not).

I agree with you that it won't make any speed improvements though, but
it was just clearer for me, since what you want are null strings and not
a char arrays fill with 0.

Thanks.
-- 
		Franck

Re: [PATCH] perf-probe: no need to initialize the entire temporary buffers in synthesize_perf_probe_point()

[Franck Bui-Huu]

Arnaldo Carvalho de Melo <acme@ghostprotocols.net> writes:

> Em Fri, Dec 24, 2010 at 01:46:09PM +0900, Masami Hiramatsu escreveu:
>> (2010/12/24 0:27), Franck Bui-Huu wrote:
>> > This patches only put a single null byte at the beginning of each
>> > temporary buffers line[], offs[], file[] instead of filling their
>> > full contents with null bytes.
>
>> Hmm, sorry but NAK it.
>
>> IMHO, with modern chips, the original code has no problem from the
>> viewpoint of memory access (all are cached and no need to access just
>> one byte) nor a bottleneck.
>> I'd rather use '= ""' style initialization for local variables from the
>> viewpoint of readability.
>
> No strong feelings here, not really a fast path, just learned something
> new, I thought that that kind of initialization would be equivalent to
> what Franck proposed, but gcc really uses the most efficient way of
> zeroing the whole string (movq for things like that, and rep stos for
> bigger arrays, etc).
> 

gcc has no other choice than clearing the whole buffers here since it's
how C initialisation works.

Thanks
-- 
		Franck