[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160305052459.GD9349@brightrain.aerifal.cx>
Date: Sat, 5 Mar 2016 00:24:59 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [RFC PATCH] micro-optimize __procfdname
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
