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

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 [off-list ref] wrote:

On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote:

quoted

quoted

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