Re: [PATCH v2 02/14] pkt-line: promote static buffer in packet_write_gently() to callers
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 2, 2021, 22:54 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2102022340460.54@tvgsbejvaqbjf.bet>
- In-Reply-To
- <YBkeYSA5UfQP1m/x@coredump.intra.peff.net>
Hi Peff,
On Tue, 2 Feb 2021, Jeff King wrote:
Show 20 quoted lines
> On Mon, Feb 01, 2021 at 07:45:35PM +0000, Jeff Hostetler via GitGitGadget wrote:
>
> > [in packet_write_gently]
> > - set_packet_header(packet_write_buffer, packet_size);
> > - memcpy(packet_write_buffer + 4, buf, size);
> > - if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
> > +
> > + set_packet_header(scratch->buffer, packet_size);
> > + memcpy(scratch->buffer + 4, buf, size);
> > +
> > + if (write_in_full(fd_out, scratch->buffer, packet_size) < 0)
> > return error(_("packet write failed"));
>
> Would it really be so bad to do:
>
> char header[4];
> set_packet_header(header, packet_size);
> if (write_in_full(fd_out, header, 4) < 0 ||
> write_in_full(fd_out, buf, size) < 0)
> return error(...);There must have been a reason why the original code went out of its way to copy the data. At least that's what I _assume_.
I could see, for example, that these extra round-trips just for the header, really have a negative impact on network operations.
> I doubt that two syscalls is breaking the bank here, but if people are > really concerned, using writev() would be a much better solution.
No, because there is no equivalent for that on Windows. And since Windows is the primary target of our Simple IPC/FSMonitor work, that would break the bank.
Show 14 quoted lines
> Obviously we can't rely on it being available everywhere, but it's quite
> easy to emulate with a wrapper (and I'd be happy punt on any writev
> stuff until somebody actually measures a difference).
>
> The other direction is that callers could be using a correctly-sized
> buffer in the first place. I.e., something like:
>
> struct packet_buffer {
> char full_packet[LARGE_PACKET_MAX];
> };
> static inline char *packet_data(struct packet_buffer *pb)
> {
> return pb->full_packet + 4;
> }Or we change it to
struct packet_buffer {
char count[4];
char payload[LARGE_PACKET_MAX - 4];
};and then ask the callers to allocate one of those beauties Not sure how well we can guarantee that the compiler won't pad this, though.
And then there is `write_packetized_from_buf()` whose `src` parameter can come from `convert_to_git()` that _definitely_ would not be of the desired form.
So I guess if we can get away with the 2-syscall version, that's kind of better than that.
Ciao, Dscho
Show 16 quoted lines
> > That lets people work with the oversized buffer in a natural-ish way > that would be hard to get wrong, like: > > memcpy(packet_data(pb), some_other_buf, len); > > (though if we wanted to go even further, we could provide accessors that > actually do the writing and sanity-check the lengths; the downside is > that I'm not sure how callers typically get the bytes into these bufs in > the first place). > > That's a much bigger change, of course, and I'd guess you'd much prefer > to focus on the actual point of your series. ;) > > -Peff >