From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Jacob Vosmaer <jacob@gitlab.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/1] upload-pack.c: make output buffer size configurable
Date: Tue, 14 Dec 2021 10:08:45 -0500 [thread overview]
Message-ID: <YbizfdGq+RSu9BGe@coredump.intra.peff.net> (raw)
In-Reply-To: <211214.86y24nbd46.gmgdl@evledraar.gmail.com>
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
next prev parent reply other threads:[~2021-12-14 15:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-13 13:23 [PATCH 0/1] Make upload-pack pack write size configurable Jacob Vosmaer
2021-12-13 13:23 ` [PATCH 1/1] upload-pack.c: make output buffer " Jacob Vosmaer
2021-12-14 12:08 ` Ævar Arnfjörð Bjarmason
2021-12-14 15:08 ` Jeff King [this message]
2021-12-14 19:46 ` [PATCH v2 0/1] upload-pack.c: increase output buffer size Jacob Vosmaer
2021-12-14 19:46 ` [PATCH v2 1/1] " Jacob Vosmaer
2021-12-14 20:41 ` Ævar Arnfjörð Bjarmason
2021-12-15 16:30 ` Jeff King
2021-12-15 19:50 ` Junio C Hamano
2021-12-15 19:59 ` rsbecker
2021-12-15 20:24 ` Jacob Vosmaer
2021-12-15 20:38 ` rsbecker
2021-12-15 20:45 ` Jacob Vosmaer
2021-12-15 21:34 ` rsbecker
2021-12-14 15:37 ` [PATCH 1/1] upload-pack.c: make output buffer size configurable Jeff King
2021-12-14 20:04 ` Jacob Vosmaer
2021-12-14 15:12 ` [PATCH 0/1] Make upload-pack pack write " Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YbizfdGq+RSu9BGe@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=jacob@gitlab.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).