Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes

From: Jacob Vosmaer <jacob@gitlab.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>,
	Git Mailing List <git@vger.kernel.org>,
	ps@pks.im
Subject: Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes
Date: Thu, 26 Aug 2021 12:02:26 +0200	[thread overview]
Message-ID: <CADMWQoMpURczcnZne=0cr2vavoLm_VT5eEMg4FCu3VeSg_UJaQ@mail.gmail.com> (raw)
In-Reply-To: <YSWSd5fMbSD5duOU@coredump.intra.peff.net>

> Are there any consumers that rely on having information early (where
> buffering would be a detriment to them)?

I think not.

> Hmm. Seeing a reduction in CPU time is surprising to me: do you have an
> explanation for why the time-savings isn't coming purely from "system"
> (i.e., any blocking I/O)?

Pipe writes only block in the IO sense if the pipe buffer is full.
Otherwise they seem to spend their time in spinlocks and copying into
the page buffer. In my benchmark, I don't believe the pipe buffer was
ever full.

I'm not an expert on the Linux kernel; you can see CPU flame graphs in
https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1257.

> Yeah, I had the same thought. It also feels like this is a problem
> already solved by stdio. I.e., a lot of the packet_* functions can
> handle descriptors or strbufs. Why not "FILE *" handles?

My colleague Patrick Steinhardt (cc) made the same suggestion
off-list. I'll post an alternative patch set to this thread.

Jacob

On Wed, Aug 25, 2021 at 2:44 AM Jeff King <peff@peff.net> wrote:
>
> On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote:
>
> > > None of this looks wrong to me, but it might be nice to teach the
> > > packet writer a buffered mode that would handle this for us. It would be
> > > especially nice to bolt the final flush onto packet_flush(), since it is
> > > otherwise easy to forget to do.
> >
> > FWIW, the packet-line part of the system was from the beginning
> > written with an eye to allow buffering until _flush() comes; we may
> > have added some buggy conversation path that deadlocks if we make
> > the non-flush packets fully buffered, so there may need some fixes,
> > but I do not expect the fallout would be too hard to diagnose.
> >
> > It may be worth trying that avenue first before piling on the user
> > level buffering like this patch does.
>
> Yeah, I had the same thought. It also feels like this is a problem
> already solved by stdio. I.e., a lot of the packet_* functions can
> handle descriptors or strbufs. Why not "FILE *" handles?
>
> It would probably involve using the original descriptor _and_ the
> filehandle in some cases (e.g., ref advertisement over the handle, and
> then muxing pack-objects output straight to the descriptor). But that's
> OK as long we are sensible about flushing at the right moments.
>
> It may not be much less complex than just implementing buffering in the
> packet_* interfaces, though. The tricky part is likely to be the
> interface (not itself, but avoiding repetition between all the
> fd/strbuf/buffered variants).
>
> -Peff