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

[Junio C Hamano <gitster@pobox.com>]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> 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;
>  }