On Tue, Dec 14, 2021 at 01:08:55PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > +#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
