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

From: Jeff King <peff@peff.net>
Date: Dec 14, 2021, 15:08 UTC
Message-ID: <YbizfdGq+RSu9BGe@coredump.intra.peff.net>
In-Reply-To: <211214.86y24nbd46.gmgdl@evledraar.gmail.com>

On Tue, Dec 14, 2021 at 01:08:55PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

> > +#ifndef UPLOAD_PACK_BUFFER_SIZE
> > +#define UPLOAD_PACK_BUFFER_SIZE 8192
> > +#endif
> > +
> >  /* Enum for allowed unadvertised object request (UOR) */
> >  enum allow_uor {
> >  	/* Allow specifying sha1 if it is a ref tip. */
> > @@ -194,7 +198,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> >  }
> >  
> >  struct output_state {
> > -	char buffer[8193];
> > +	char buffer[UPLOAD_PACK_BUFFER_SIZE+1];
> >  	int used;
> >  	unsigned packfile_uris_started : 1;
> >  	unsigned packfile_started : 1;
> 
> Making this configurable obviousl has big impact in some cases, but I'm
> a bit iffy on the faciltity to do so + it not being documented.
> 
> I don't think that the "static buffer" part here is important to anyone,
> but the write() size is clearly important.
> 
> So doesn't it make more sense to have a uploadPack.bufferSize=8k
> variable we can tweak, just make this "buffer" a "struct strbuf" instead
> (i.e. it'll by dynamically grown), and then just flush it whenever we
> hit the configured buffer size?

I don't think we want to grow dynamically, because we don't want to hold arbitrary amounts of data in memory. We're just passing it through. But if there were a run-time config option, we could easily heap-allocate the buffer up front.

That may be overkill, though I do agree this is kind of weirdly undocumented. There are two other subtleties I notice:

  - This output_state struct does go on the stack, so something big like
    64k is questionable there (though on Linux, without recursion, it's
    usually OK).

  - we're relaying the data into pkt-lines. Do we run into problems when
    the buffer is larger than a packet? I think in send_sideband() we'll
    break it up as appropriate. But before we hit the PACK header, we
    send out packfile-uris directly with packet_write_fmt(). Those
    aren't likely to be long, but if they are, we'd die() in
    format_packet(). So something around LARGE_PACKET_DATA_MAX is
    probably the highest you'd want to set it anyway (and the most
    performant, since otherwise you have to write out extra partial
    packets).

So I kind of wonder if there is any real _harm_ in just always using bigger packets, even if it does not always help. Given the subtle rules about packet-max above, then we could just use that optimal value and not worry about configurability.

-Peff