Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable

From: Jacob Vosmaer <jacob@gitlab.com>
Date: Dec 14, 2021, 20:04 UTC
Message-ID: <CADMWQoOaPQaCQa5V+1Dujc6A=+f8rnv7MG+fP1mZi1kwm=d3fQ@mail.gmail.com>
In-Reply-To: <Ybi6SwndUHLs27bO@coredump.intra.peff.net>

On Tue, Dec 14, 2021 at 4:37 PM Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

>
> It gets about 2.3GB/s with the tip of 'master' and 3.2GB/s with the
> equivalent of your patch (using LARGE_PACKET_DATA_MAX). So definitely an
> improvement.
>
> Without the cached case (so actually running pack-objects, albeit a
> pretty quick one because of bitmaps and pack-reuse), the timings are
> about the same (171MB/s versus 174MB/s, but really it's just pegging a
> CPU running pack-objects). So it would be fine to just do this
> unconditionally, I think.

Thank you for validating this!

Show 5 quoted lines

> Looking at strace, the other thing I notice is that we write() the
> packet header separately in send_sideband(), which doubles the number of
> syscalls. I hackily re-wrote this to use writev() instead (patch below),
> but it doesn't seem to actually help much (maybe a curiosity to explore
> further, but definitely not something to hold up your patch).

Those double writes bug me too. Getting rid of them was one of the things I liked about using stdio here. Curious now if writev will make an impact in my benchmarks. As your experiments indicate, the double writes may not be a problem in real life. Either way, I also feel it is not something to rope into this patch set.