Re: [PATCH] setup: avoid unconditional open with write flags

[Taylor Blau <me@xxxxxxxxxxxx>]

On Mon, Dec 05, 2022 at 08:00:19PM +0100, Christian Göttsche wrote:
> @@ -1669,7 +1669,14 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
>  /* if any standard file descriptor is missing open it to /dev/null */
>  void sanitize_stdfds(void)
>  {
> -	int fd = xopen("/dev/null", O_RDWR);

So before we would do one syscall here unconditionally. Then in the
common case, we'll do another syscall to close the descriptor we just
opened. IOW, ordinarily we expect this function to execute two syscalls.

> +	int fd;
> +
> +	if (fcntl(0, F_GETFD) != -1 &&
> +	    fcntl(1, F_GETFD) != -1 &&
> +	    fcntl(2, F_GETFD) != -1)
> +		return;

But under the same circumstances (i.e., where all three descriptors are
already valid), we now have to make three syscalls to determine the same
fact.

...So I'm not sure that I agree with brian's "this isn't making anything
worse" statement earlier in the thread.

In practice, however, it appears to be basically undetectable. Here,
'git.old' is the pre-image of this patch, and 'git.new' is the
post-image. I figure that running 'git -h' is about the fastest thing I
could do:

    $ hyperfine -N -L V old,new './git.{V} -h'
    Benchmark 1: ./git.old -h
      Time (mean ± σ):       1.6 ms ±   0.2 ms    [User: 1.1 ms, System: 0.4 ms]
      Range (min … max):     0.8 ms …   2.2 ms    1589 runs

    Benchmark 2: ./git.new -h
      Time (mean ± σ):       1.6 ms ±   0.2 ms    [User: 1.1 ms, System: 0.4 ms]
      Range (min … max):     0.9 ms …   2.2 ms    1746 runs

    Summary
      './git.old -h' ran
        1.00 ± 0.14 times faster than './git.new -h'

So it appears that the old version is ever-so-slightly faster than the
new one. But it's so noisy, and the regression is so small that it's
hard to notice it at all.

So I wouldn't strongly oppose the patch based on those numbers, but in
principle it seems flawed.

Thanks,
Taylor