Re: [PATCH v3 0/2] send_ref buffering

From: Jeff King <peff@peff.net>
Date: Aug 31, 2021, 10:25 UTC
Message-ID: <YS4DmlJdjxeo+QI0@coredump.intra.peff.net>
In-Reply-To: <20210831093444.28199-1-jacob@gitlab.com>

On Tue, Aug 31, 2021 at 11:34:42AM +0200, Jacob Vosmaer wrote:

> Thanks for the reactions everyone. I agree that packet_fwrite_fmt
> simplifies the patch nicely. Jeff, I hope I have given you credit
> in an appropriate way, let me know if you want me to change something
> there.

What you did looks fine.

Overall the series looks much nicer, and I don't have any real complaints. I do think it would be nice to take the packet_writer interface further (letting it replace the static buf, and use stdio handles, and using it throughout upload-pack). But this is a strict improvement, so we can do that other refactoring later.

Show 8 quoted lines

> Regarding setvbuf: I have found out that GNU coreutils has a utility
> called stdbuf that lets you modify the stdout buffer size at runtime
> using some LD_PRELOAD hack so we can use that in Gitaly. I don't
> think this is the best outcome for users, we ought to give them a
> good default instead of expecting them to invoke git-upload-pack
> as 'stdbuf -o 64K git-upload-pack'. But I can't judge the impact
> of globally changing the stdout buffer size for Git so I'll settle
> for having to use stdbuf.

Does the 64k buffer actually improve things? Here are the timings I get on a repo with ~1M refs (it's linux.git with one ref per commit). "git" is current unbuffered version, and "git.compile" is master with your patches on top:

  $ hyperfine -i 'git upload-pack .' 'git.compile upload-pack .' 'stdbuf -o 64K git.compile upload-pack .'
  Benchmark #1: git upload-pack .
    Time (mean ± σ):     948.6 ms ±   7.3 ms    [User: 840.8 ms, System: 107.8 ms]
    Range (min … max):   937.7 ms … 961.1 ms    10 runs
   
    Warning: Ignoring non-zero exit code.
   
  Benchmark #2: git.compile upload-pack .
    Time (mean ± σ):     867.3 ms ±   6.8 ms    [User: 821.5 ms, System: 45.7 ms]
    Range (min … max):   859.7 ms … 883.0 ms    10 runs
   
    Warning: Ignoring non-zero exit code.
   
  Benchmark #3: stdbuf -o 64K git.compile upload-pack .
    Time (mean ± σ):     861.1 ms ±   8.2 ms    [User: 815.5 ms, System: 45.6 ms]
    Range (min … max):   846.1 ms … 872.0 ms    10 runs
   
    Warning: Ignoring non-zero exit code.
   
  Summary
    'stdbuf -o 64K git.compile upload-pack .' ran
      1.01 ± 0.01 times faster than 'git.compile upload-pack .'
      1.10 ± 0.01 times faster than 'git upload-pack .'

This is on a glibc system, so the default buffers should be 4k. It doesn't appear to make any difference (there's a slight improvement, but well within the noise, and I had other runs where it did worse).

By the way, if you really want to speed things up, try this:

  $ hyperfine -i 'git.compile upload-pack .' 'GIT_REF_PARANOIA=1 git.compile upload-pack .'
  Benchmark #1: git.compile upload-pack .
    Time (mean ± σ):     855.4 ms ±   5.8 ms    [User: 803.4 ms, System: 52.0 ms]
    Range (min … max):   848.7 ms … 869.5 ms    10 runs
   
    Warning: Ignoring non-zero exit code.
   
  Benchmark #2: GIT_REF_PARANOIA=1 git.compile upload-pack .
    Time (mean ± σ):     394.4 ms ±   3.0 ms    [User: 357.9 ms, System: 36.4 ms]
    Range (min … max):   390.6 ms … 400.3 ms    10 runs
   
    Warning: Ignoring non-zero exit code.
   
  Summary
    'GIT_REF_PARANOIA=1 git.compile upload-pack .' ran
      2.17 ± 0.02 times faster than 'git.compile upload-pack .'

It's not exactly the intended use of that environment variable, but its side effect is that we do not call has_object_file() on each ref tip.

-Peff