mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <>
To: "Christian Göttsche" <>
Subject: Re: [PATCH] setup: avoid unconditional open with write flags
Date: Mon, 5 Dec 2022 17:59:28 -0500	[thread overview]
Message-ID: <Y453yiYAifv/oV5n@nand.local> (raw)
In-Reply-To: <>

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

...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 '' 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: ./ -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

      './git.old -h' ran
        1.00 ± 0.14 times faster than './ -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.


  parent reply	other threads:[~2022-12-05 22:59 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-05 19:00 [PATCH] setup: avoid unconditional open with write flags Christian Göttsche
2022-12-05 22:13 ` brian m. carlson
2022-12-06  1:38   ` Jeff King
2022-12-06 16:15     ` Christian Göttsche
2022-12-05 22:59 ` Taylor Blau [this message]
2022-12-06  0:10   ` Junio C Hamano
2022-12-06  0:31     ` Taylor Blau
2022-12-06  0:40       ` Junio C Hamano
2022-12-06 19:47 ` René Scharfe
2022-12-06 23:39   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y453yiYAifv/oV5n@nand.local \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).