[v5,01/12] pkt-line: eliminate the need for static buffer in packet_write_gently()

Commit Message

Jeff Hostetler March 9, 2021, 3:02 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Teach `packet_write_gently()` to write the pkt-line header and the actual
buffer in 2 separate calls to `write_in_full()` and avoid the need for a
static buffer, thread-safe scratch space, or an excessively large stack
buffer.

Change `write_packetized_from_fd()` to allocate a temporary buffer rather
than using a static buffer to avoid similar issues here.

These changes are intended to make it easier to use pkt-line routines in
a multi-threaded context with multiple concurrent writers writing to
different streams.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 pkt-line.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Junio C Hamano March 9, 2021, 11:48 p.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

> +	/*
> +	 * Write the header and the buffer in 2 parts so that we do not need
> +	 * to allocate a buffer or rely on a static buffer.  This avoids perf
> +	 * and multi-threading issues.
> +	 */

I understand "multi-threading issues" (i.e. let's not have too much
stuff on the stack), but what issue around "perf" are we worried
about?

Even though we eliminate memcpy() from the original buffer to our
temporary, this doubles the number of write(2) system calls used to
write out packetised data, by the way.  I do not know if this results
in measurable performance degradation, but hopefully we can fix it
locally if it turns out to be a real problem later.

> +	if (write_in_full(fd_out, header, 4) < 0 ||
> +	    write_in_full(fd_out, buf, size) < 0)
>  		return error(_("packet write failed"));
>  	return 0;
>  }
> @@ -244,20 +252,23 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
>  
>  int write_packetized_from_fd(int fd_in, int fd_out)
>  {
> -	static char buf[LARGE_PACKET_DATA_MAX];
> +	char *buf = xmalloc(LARGE_PACKET_DATA_MAX);
>  	int err = 0;
>  	ssize_t bytes_to_write;
>  
>  	while (!err) {
> -		bytes_to_write = xread(fd_in, buf, sizeof(buf));
> -		if (bytes_to_write < 0)
> +		bytes_to_write = xread(fd_in, buf, LARGE_PACKET_DATA_MAX);
> +		if (bytes_to_write < 0) {
> +			free(buf);
>  			return COPY_READ_ERROR;
> +		}
>  		if (bytes_to_write == 0)
>  			break;
>  		err = packet_write_gently(fd_out, buf, bytes_to_write);
>  	}
>  	if (!err)
>  		err = packet_flush_gently(fd_out);
> +	free(buf);
>  	return err;
>  }

Jeff King March 11, 2021, 7:29 p.m. UTC | #2

On Tue, Mar 09, 2021 at 03:48:40PM -0800, Junio C Hamano wrote:

> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> 
> > +	/*
> > +	 * Write the header and the buffer in 2 parts so that we do not need
> > +	 * to allocate a buffer or rely on a static buffer.  This avoids perf
> > +	 * and multi-threading issues.
> > +	 */
> 
> I understand "multi-threading issues" (i.e. let's not have too much
> stuff on the stack), but what issue around "perf" are we worried
> about?
> 
> Even though we eliminate memcpy() from the original buffer to our
> temporary, this doubles the number of write(2) system calls used to
> write out packetised data, by the way.  I do not know if this results
> in measurable performance degradation, but hopefully we can fix it
> locally if it turns out to be a real problem later.

Yeah, this came from my suggestion. My gut feeling is that it isn't
likely to matter, but I'd much rather solve any performance problem we
find using writev(), which would be pretty easy to emulate with a
wrapper for systems that lack it.

-Peff

Junio C Hamano March 11, 2021, 8:32 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> On Tue, Mar 09, 2021 at 03:48:40PM -0800, Junio C Hamano wrote:
>
>> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
>> 
>> > +	/*
>> > +	 * Write the header and the buffer in 2 parts so that we do not need
>> > +	 * to allocate a buffer or rely on a static buffer.  This avoids perf
>> > +	 * and multi-threading issues.
>> > +	 */
>> 
>> I understand "multi-threading issues" (i.e. let's not have too much
>> stuff on the stack), but what issue around "perf" are we worried
>> about?
>>  ...
> Yeah, this came from my suggestion. My gut feeling is that it isn't
> likely to matter, but I'd much rather solve any performance problem we
> find using writev(), which would be pretty easy to emulate with a
> wrapper for systems that lack it.

I too had writev() in mind when I said "can fix it locally", so we
are on the same page, which is good.

So "this avoid multi-threading issues" without mentioning "perf and"
would be more appropriate?

Thanks.

Jeff King March 11, 2021, 8:53 p.m. UTC | #4

On Thu, Mar 11, 2021 at 12:32:29PM -0800, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > On Tue, Mar 09, 2021 at 03:48:40PM -0800, Junio C Hamano wrote:
> >
> >> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> >> 
> >> > +	/*
> >> > +	 * Write the header and the buffer in 2 parts so that we do not need
> >> > +	 * to allocate a buffer or rely on a static buffer.  This avoids perf
> >> > +	 * and multi-threading issues.
> >> > +	 */
> >> 
> >> I understand "multi-threading issues" (i.e. let's not have too much
> >> stuff on the stack), but what issue around "perf" are we worried
> >> about?
> >>  ...
> > Yeah, this came from my suggestion. My gut feeling is that it isn't
> > likely to matter, but I'd much rather solve any performance problem we
> > find using writev(), which would be pretty easy to emulate with a
> > wrapper for systems that lack it.
> 
> I too had writev() in mind when I said "can fix it locally", so we
> are on the same page, which is good.
> 
> So "this avoid multi-threading issues" without mentioning "perf and"
> would be more appropriate?

IMHO yes. I think "avoid perf issues" is probably answering the "why not
just heap-allocate the buffer" question. But that makes sense in the
commit message, not in a comment.

-Peff

Patch

diff --git a/pkt-line.c b/pkt-line.c
index d633005ef746..8b3512190442 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -196,17 +196,25 @@  int packet_write_fmt_gently(int fd, const char *fmt, ...)
 
 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
-	static char packet_write_buffer[LARGE_PACKET_MAX];
+	char header[4];
 	size_t packet_size;
 
-	if (size > sizeof(packet_write_buffer) - 4)
+	if (size > LARGE_PACKET_DATA_MAX)
 		return error(_("packet write failed - data exceeds max packet size"));
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
-	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(header, packet_size);
+
+	/*
+	 * Write the header and the buffer in 2 parts so that we do not need
+	 * to allocate a buffer or rely on a static buffer.  This avoids perf
+	 * and multi-threading issues.
+	 */
+
+	if (write_in_full(fd_out, header, 4) < 0 ||
+	    write_in_full(fd_out, buf, size) < 0)
 		return error(_("packet write failed"));
 	return 0;
 }
@@ -244,20 +252,23 @@  void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
 
 int write_packetized_from_fd(int fd_in, int fd_out)
 {
-	static char buf[LARGE_PACKET_DATA_MAX];
+	char *buf = xmalloc(LARGE_PACKET_DATA_MAX);
 	int err = 0;
 	ssize_t bytes_to_write;
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, buf, sizeof(buf));
-		if (bytes_to_write < 0)
+		bytes_to_write = xread(fd_in, buf, LARGE_PACKET_DATA_MAX);
+		if (bytes_to_write < 0) {
+			free(buf);
 			return COPY_READ_ERROR;
+		}
 		if (bytes_to_write == 0)
 			break;
 		err = packet_write_gently(fd_out, buf, bytes_to_write);
 	}
 	if (!err)
 		err = packet_flush_gently(fd_out);
+	free(buf);
 	return err;
 }