[PATCH] gfs2: better code for translating characters

[PATCH] gfs2: better code for translating characters

[Denis Cheng]

the original code could work, but I think this code could work better.

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 fs/gfs2/ops_fstype.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index cf5aa50..b9a7759 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -145,7 +145,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
 	snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
 	snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
 
-	while ((table = strchr(sdp->sd_table_name, '/')))
+	table = sdp->sd_table_name;
+	while ((table = strchr(table, '/')))
 		*table = '_';
 
 out:
-- 
1.5.2.2

Re: [PATCH] gfs2: better code for translating characters

[rae l]

On 8/13/07, Denis Cheng <crquan@gmail.com> wrote:
> the original code could work, but I think this code could work better.
>
> Signed-off-by: Denis Cheng <crquan@gmail.com>
> ---
>  fs/gfs2/ops_fstype.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index cf5aa50..b9a7759 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -145,7 +145,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
>         snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
>         snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
>
> -       while ((table = strchr(sdp->sd_table_name, '/')))
> +       table = sdp->sd_table_name;
> +       while ((table = strchr(table, '/')))
>                 *table = '_';
Sorry, I don't know what the while loop really means, what's the
common case that slash character exists? if the '/' appears multiple,
the latter code would be better; however, if slash appears rarely, the
original would still be better.

>
>  out:
> --
> 1.5.2.2
>
>


-- 
Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
 - Ken Thompson.

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[H. Peter Anvin]

rae l wrote:
> On 8/13/07, Denis Cheng <crquan@gmail.com> wrote:
>> the original code could work, but I think this code could work better.
>>
>> Signed-off-by: Denis Cheng <crquan@gmail.com>
>> ---
>>  fs/gfs2/ops_fstype.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
>> index cf5aa50..b9a7759 100644
>> --- a/fs/gfs2/ops_fstype.c
>> +++ b/fs/gfs2/ops_fstype.c
>> @@ -145,7 +145,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
>>         snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
>>         snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
>>
>> -       while ((table = strchr(sdp->sd_table_name, '/')))
>> +       table = sdp->sd_table_name;
>> +       while ((table = strchr(table, '/')))
>>                 *table = '_';
> Sorry, I don't know what the while loop really means, what's the
> common case that slash character exists? if the '/' appears multiple,
> the latter code would be better; however, if slash appears rarely, the
> original would still be better.
> 

Only if the compiler is stupid.

	-hpa

Re: [PATCH] gfs2: better code for translating characters

[H. Peter Anvin]

rae l wrote:
> On 8/13/07, Denis Cheng <crquan@gmail.com> wrote:
>> the original code could work, but I think this code could work better.
>>
>> Signed-off-by: Denis Cheng <crquan@gmail.com>
>> ---
>>  fs/gfs2/ops_fstype.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
>> index cf5aa50..b9a7759 100644
>> --- a/fs/gfs2/ops_fstype.c
>> +++ b/fs/gfs2/ops_fstype.c
>> @@ -145,7 +145,8 @@ static int init_names(struct gfs2_sbd *sdp, int silent)
>>         snprintf(sdp->sd_proto_name, GFS2_FSNAME_LEN, "%s", proto);
>>         snprintf(sdp->sd_table_name, GFS2_FSNAME_LEN, "%s", table);
>>
>> -       while ((table = strchr(sdp->sd_table_name, '/')))
>> +       table = sdp->sd_table_name;
>> +       while ((table = strchr(table, '/')))
>>                 *table = '_';
> Sorry, I don't know what the while loop really means, what's the
> common case that slash character exists? if the '/' appears multiple,
> the latter code would be better; however, if slash appears rarely, the
> original would still be better.
> 

Only if the compiler is stupid.

	-hpa

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[H. Peter Anvin]

rae l wrote:
>>>
>> Only if the compiler is stupid.
> What? Did you know I really say? Could you tell a little more clear?
> 
> if the string in sdp->sd_table_name has many '/' chars, the latter
> algorithm will be better.
> but if there's no '/' char, one assignment will be wasted.
> 

You seem to have confused modern compiled C with an old BASIC interpreter.

Consider the code in point:

-       while ((table = strchr(sdp->sd_table_name, '/')))
+       table = sdp->sd_table_name;
+       while ((table = strchr(table, '/')))
  		*table = '_';

sdp->sd_table_name refers to a memory location, and will have to be
loaded from memory into a register before it can be transmitted to the
strchr() function.  In the latter case, we call this register "table";
since the value is immediately killed after the function call, there is
no reason for the compiler to carry it across the function.

Consider x86-64 as an example:

	# Assume sdp is held in %r15 at this point, and assume
	# the offset of sd_table_name is 0x30.

	# First case
.L1:
	movq	30(%r15), %rdi	# First argument register
	movb	'/', %sil	# Second argument register
	call	strchr
	testq	%rax, %rax	# Result register
	jz	.L2
	movb	'_', (%rax)
	jmp	.L1
.L2:

	# Second case
	movq	30(%r15), %rdi
.L1:
	movb	'/', %sil
	call	strchr
	testq	%rax, %rax
	jz	.L2
	movq	%rax, %rdi
	movb	'_', (%rax)
	jmp	.L1


As you can see, in the zero case, the instruction sequence is exactly
the same, whereas in the nonzero case, we have replaced a memory load
with a register-register copy.  On most architectures (x86-64, Alpha and
MIPS are the oddballs here) we wouldn't even need the copy.

	-hpa

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[rae l]

On 8/13/07, H. Peter Anvin <hpa@zytor.com> wrote:
> You seem to have confused modern compiled C with an old BASIC interpreter.
>
> Consider the code in point:
>
> -       while ((table = strchr(sdp->sd_table_name, '/')))
> +       table = sdp->sd_table_name;
> +       while ((table = strchr(table, '/')))
>                 *table = '_';
Sorry, I just mean for call to strchr, things are different,
especially for multiple '/' chars appeared.

The while loop's purpose is to translate all '/' chars appeared in
sdp->sd_table_name to '_' chars, consider the string:
'a////aa/a/a/...'
if strchr called with sdp->sd_table_name, every strchr would begun at
index 0 of the string, but if called with table, every strchr begun at
the last searched position.

So I wonder the common case is no existence of '/', or just one or
multiple existence? Things are different for these cases.

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[H. Peter Anvin]

rae l wrote:
> On 8/13/07, H. Peter Anvin <hpa@zytor.com> wrote:
>> You seem to have confused modern compiled C with an old BASIC interpreter.
>>
>> Consider the code in point:
>>
>> -       while ((table = strchr(sdp->sd_table_name, '/')))
>> +       table = sdp->sd_table_name;
>> +       while ((table = strchr(table, '/')))
>>                 *table = '_';
> Sorry, I just mean for call to strchr, things are different,
> especially for multiple '/' chars appeared.

Things are different only for multiple '/' chars.  In that case, the
latter form is better, in the former case, there is no difference.

So it doesn't matter.  One is unambiguously better.

	-hpa

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[Steven Whitehouse]

Hi,

On Mon, 2007-08-13 at 09:35 -0700, H. Peter Anvin wrote:
> rae l wrote:
> > On 8/13/07, H. Peter Anvin <hpa@zytor.com> wrote:
> >> You seem to have confused modern compiled C with an old BASIC interpreter.
> >>
> >> Consider the code in point:
> >>
> >> -       while ((table = strchr(sdp->sd_table_name, '/')))
> >> +       table = sdp->sd_table_name;
> >> +       while ((table = strchr(table, '/')))
> >>                 *table = '_';
> > Sorry, I just mean for call to strchr, things are different,
> > especially for multiple '/' chars appeared.
> 
> Things are different only for multiple '/' chars.  In that case, the
> latter form is better, in the former case, there is no difference.
> 
> So it doesn't matter.  One is unambiguously better.
> 
> 	-hpa

I agree, so I've applied the patch. Thanks Denis,

Steve.

[Cluster-devel] Re: [PATCH] gfs2: better code for translating characters

[Steven Whitehouse]

Hi,

On Mon, 2007-08-13 at 17:19 +0800, rae l wrote:
> On 8/13/07, H. Peter Anvin <hpa@zytor.com> wrote:
> > You seem to have confused modern compiled C with an old BASIC interpreter.
> >
> > Consider the code in point:
> >
> > -       while ((table = strchr(sdp->sd_table_name, '/')))
> > +       table = sdp->sd_table_name;
> > +       while ((table = strchr(table, '/')))
> >                 *table = '_';
> Sorry, I just mean for call to strchr, things are different,
> especially for multiple '/' chars appeared.
> 
> The while loop's purpose is to translate all '/' chars appeared in
> sdp->sd_table_name to '_' chars, consider the string:
> 'a////aa/a/a/...'
> if strchr called with sdp->sd_table_name, every strchr would begun at
> index 0 of the string, but if called with table, every strchr begun at
> the last searched position.
> 
> So I wonder the common case is no existence of '/', or just one or
> multiple existence? Things are different for these cases.
The common case is for there to be a few '/' characters since the table
name is often the name of the device upon which the fs is mounted,
e.g. /dev/sda so it general its not going to be a long string or have
many '/' in it,

Steve.