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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Feb 27, 2026 at 01:04:05PM +0000, brian m. carlson wrote:
> On 2026-02-27 at 11:23:01, Patrick Steinhardt wrote:
> > diff --git a/upload-pack.c b/upload-pack.c
> > index c2643c0295..f8ba245616 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Make sure that we buffer some data before sending it to the client.
> > +	 * This significantly reduces the number of write(3p) syscalls.
> > +	 */
> > +	if (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))
> > +		return readsz;
> > +
> >  	if (os->used > 1) {
> >  		send_client_data(1, os->buffer, os->used - 1, use_sideband);
> >  		os->buffer[0] = os->buffer[os->used - 1];
> 
> This seems mostly reasonable and well-explained.  The one question I
> have is this: how does this work when packfile generation is actually
> very slow (or when the connection is slow) and we need to send data
> every so often to keep the connection alive?
> 
> I just want to make sure we're not breaking the keepalive sideband case
> when that's necessary, but of course I have no objections to improving
> performance and reducing overhead.

Right. `relay_pack_data()` is handling the logic to soak up data from
git-pack-objects(1). The outer loop is in `create_pack_file()`, and
there we already have a timeout configured. If data is produced too
slow, then we'd eventually land in there and send the keepalive packet.

I guess there is an interesting edge case here: if git-pack-objects(1)
creates bytes fast enough to not hit the 1 second timeout, but slow
enough to basically never fill the buffer, then we could potentially run
into a timeout eventually.

I'm not sure whether this is likely to happen, and whether it's
something we want to address. Maybe we should extend the logic so that
we also send the keepalive pkt-line in case we have only been buffering
data for the last 1 second?

Basically, we'd reset a timestamp every time data was sent. If we see
that we received data without writing it out, and if the current
time is one second beyond the last time we reset, then we might want to
send out the keepalive.

Patrick