Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data

From: Jeff King <peff@peff.net>
Date: Mar 2, 2026, 18:20 UTC
Message-ID: <20260302182023.GG28275@coredump.intra.peff.net>
In-Reply-To: <aaV-l_NyWpkKDDp6@pks.im>

On Mon, Mar 02, 2026 at 01:12:07PM +0100, Patrick Steinhardt wrote:

Show 11 quoted lines

> > Rather than buffering in upload-pack, would it not be simpler to just
> > increase the write size from pack-objects? Then we do not have to worry
> > about disrupting upload-pack's keepalive timeouts. And as a bonus, if
> > you are worried about the system-wide number of calls, you will likewise
> > be reducing the number of read() and write() calls over the pipe between
> > pack-objects and upload-pack.
> 
> We can do that. But we also have to keep in mind that downstream in the
> pipe may be a process that's not even git-pack-objects(1) in the first
> place because of "uploadpack.packObjectsHook". So maybe we should have a
> look at doing both.

True, though I think that whatever is producing gobs of output from that hook should consider using a buffer size close to a pktline. In many cases it will be pack-objects itself (just wrapped with some extra magic), but I guess you may have some kind of caching layer at GitLab (we did at GitHub).

That is getting specialized enough that I don't feel too bad suggesting that authors of those tools should consider buffer sizes.

As far as doing both, I'm not sure if it's worth it. My two concerns are:

  1. It re-opens the question of whether upload-pack might stall waiting
     to fill its buffer and fail to produce keepalives correctly.

  2. I wonder if we could get some weird interactions between the two
     buffer sizes. E.g., if pack-objects sends 50k bytes at a time, but
     upload-pack wants to wait for 51k. So we read 50k then wait for the
     next chunk. Either:

       a. we read 50k again, pull off 13k of it to make a full pktline,
          and then memcpy around the other 37k to await more data.
	  There's a bunch of extra copying as our buffer sizes don't
	  line up.

       b. we read 13k (to fill up the pkt we're trying to send), send
          that, and then the next read gets a partial read(). So we end
	  up issuing more reads, although sometimes pack-objects might
	  catch up and fill the pipe buffer, and we'd get a full packet
	  in one go. But depending on the timing, I wonder if things
	  could get choppy and we'd end up issuing a bunch of extra
	  reads (and possibly extra writes on the pack-objects side if
	  it's waiting on us to create more space in the pipe).

-Peff