Re: [RFC PATCH] micro-optimize __procfdname

[Rich Felker <dalias@libc.org>]

On Sun, Feb 21, 2016 at 02:41:21PM +0300, Alexander Monakov wrote:
> Hello,
> 
> I've noticed that internal function __procfdname can be slightly cleaned up by
> filling the supplied buffer right-to-left and returning the last filled
> position.  The patch below implements what I have in mind, and changes one
> call site to demonstrate (I'll be happy to submit a patch that converts all
> calls, if overall this change is desirable).

I really doubt this makes any major improvement, but it might help
size a bit and it might be cleaner/more readable, so it's interesting.

> diff --git a/src/internal/procfdname.c b/src/internal/procfdname.c
> index 697e0bd..cfb3f90 100644
> --- a/src/internal/procfdname.c
> +++ b/src/internal/procfdname.c
> @@ -1,13 +1,9 @@
> -void __procfdname(char *buf, unsigned fd)
> +char *__procfdname_impl(char *buf, unsigned fd)
>  {
> -	unsigned i, j;
> -	for (i=0; (buf[i] = "/proc/self/fd/"[i]); i++);
> -	if (!fd) {
> -		buf[i] = '0';
> -		buf[i+1] = 0;
> -		return;
> -	}
> -	for (j=fd; j; j/=10, i++);
> -	buf[i] = 0;
> -	for (; fd; fd/=10) buf[--i] = '0' + fd%10;
> +	*buf = 0;
> +	do *--buf = '0' + fd % 10;
> +	while (fd /= 10);
> +	for (int i = 13; i >= 0; i--)
> +		*--buf = "/proc/self/fd/"[i];
> +	return buf;
>  }
> diff --git a/src/internal/procfdname.h b/src/internal/procfdname.h
> index e69de29..6d3c6e2 100644
> --- a/src/internal/procfdname.h
> +++ b/src/internal/procfdname.h
> @@ -0,0 +1,9 @@
> +#ifndef PROCFDNAME_H
> +#define PROCFDNAME_H
> +
> +char *__procfdname_impl(char *, unsigned);
> +
> +#define procfdbufsize sizeof "/proc/self/fd/0123456789" + (3 * (sizeof(int)-4))

What is the motivation behind changing the size expression to use the
"012...9" part? It's nonobvious to me.

> +#define procfdname(buf, fd) __procfdname_impl(buf + procfdbufsize - 1, fd)

I suppose the idea of putting the offset to the end in a macro in the
header rather than in the callee is both optimization and allowing the
compiler to detect out-of-bounds pointer arithmetic?

> +
> +#endif
> diff --git a/src/process/fexecve.c b/src/process/fexecve.c
> index 6507b42..88e6b9d 100644
> --- a/src/process/fexecve.c
> +++ b/src/process/fexecve.c
> @@ -1,13 +1,11 @@
>  #include <unistd.h>
>  #include <errno.h>
> -
> -void __procfdname(char *, unsigned);
> +#include "procfdname.h"
>  
>  int fexecve(int fd, char *const argv[], char *const envp[])
>  {
> -	char buf[15 + 3*sizeof(int)];
> -	__procfdname(buf, fd);
> -	execve(buf, argv, envp);
> +	char buf[procfdbufsize];
> +	execve(procfdname(buf, fd), argv, envp);
>  	if (errno == ENOENT) errno = EBADF;
>  	return -1;
>  }

Here using the return value directly is nice but at some other call
points might we need to introduce a pointer variable to store the
pointer returned? I haven't checked yet.

Rich