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

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 3, 2026, 09:31 UTC
Message-ID: <aaaqgrmOBj-Ly1Vx@pks.im>
In-Reply-To: <20260302182023.GG28275@coredump.intra.peff.net>

On Mon, Mar 02, 2026 at 01:20:23PM -0500, Jeff King wrote:

Show 28 quoted lines

> On Mon, Mar 02, 2026 at 01:12:07PM +0100, Patrick Steinhardt wrote:
> 
> > > 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.

I've got a patch for that. The problem can even trigger right now as we already do buffer some of the data, and that may cause the keepalives to be missed. But this only happens initially in our current infrastructure, before we see the "PACK" signature, so it's unlikely to be a problem in practice.

Show 18 quoted lines

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

We would likely hit this issue if we insist on the buffer being completely filled before sending it out. But that's why I adapted the logic to say that we send out once we've filled it at least 2/3rds of the pktline limit. So in your case above we wouldn't face an issue as we'd already send the first 50kB, as it is smaller than 2/3rds of the maximum length (~42kB).

That being said, you'll still be able to construct cases where we have weird edge cases. For example if you consistently send one byte less than 2/3rds of the capacity.

Patrick