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

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 5, 2022, 22:59 UTC
Message-ID: <Y453yiYAifv/oV5n@nand.local>
In-Reply-To: <20221205190019.52829-1-cgzones@googlemail.com>

On Mon, Dec 05, 2022 at 08:00:19PM +0100, Christian Göttsche wrote:

Show 5 quoted lines

> @@ -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.

Show 6 quoted lines

> +	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