Re: [PATCH v2 10/10] cat-file: use writev(2) if available

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

Eric Wong <e@80x24.org> writes:

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index bf81054662..016b7d26a7 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -280,7 +280,7 @@ struct expand_data {
>  	off_t disk_size;
>  	const char *rest;
>  	struct object_id delta_base_oid;
> -	void *content;
> +	struct git_iovec iov[3];


The earlier content pointer hinted that the caller that obtained
data into this structure from the object layer can use it for any
purpose that suits it, but using git_iovec structure screams that
"we are going to write this thing out!".  As "expand_data" is about
what we are going to write out from cat-file anyway, is probably OK.

Having said that ...

> -static void print_object_or_die(struct batch_options *opt, struct expand_data *data)
> +static void batch_writev(struct batch_options *opt, struct expand_data *data,
> +			const struct strbuf *hdr, size_t size)
> +{
> +	data->iov[0].iov_base = hdr->buf;
> +	data->iov[0].iov_len = hdr->len;
> +	data->iov[1].iov_len = size;
> +
> +	/*
> +	 * Copying a (8|16)-byte iovec for a single byte is gross, but my
> +	 * attempt to stuff output_delim into the trailing NUL byte of
> +	 * iov[1].iov_base (and restoring it after writev(2) for the
> +	 * OI_DBCACHED case) to drop iovcnt from 3->2 wasn't faster.
> +	 */
> +	data->iov[2].iov_base = &opt->output_delim;
> +	data->iov[2].iov_len = 1;
> +	if (opt->buffer_output)
> +		fwritev_or_die(stdout, data->iov, 3);
> +	else
> +		writev_or_die(1, data->iov, 3);
> +
> +	/* writev_or_die may move iov[1].iov_base, so it's invalid */
> +	data->iov[1].iov_base = NULL;
> +}

... the above made me read it twice, wondering "where does
iov[1].iov_base comes from???"  The location of the git_iovec
structure in the expand_data forces this rather unnatural calling
convention where the iovec is passed by address (as part of the
expand_data structure), with only one of six slots filled, and the
other five slots are filled by this function from the parameters
passed to it.

I wonder if we can rework the data structure to

 - Not embed git_iovec iov[] in expand_data;

 - Keep "void *content" instead there;

 - Define an on-stack "struct git_iovec iov[3]" local to this function;

 - Pass "void *content" from the caller to this function;

 - Populate iov[] fully from hdr->{buf,len}, content, size, and
   opt->output_delim and consume it in this function by either
   calling fwritev_or_die() or writev_or_die().

That way, the caller does not have to use data->iov[1].iov_base in
place of data->content, which is the source of "Huh?  Why is the 2nd
element of the 3-element array so special?" puzzlement readers would
feel while reading the caller---after all, the fact that we are
using writev with three chunks is an implementation detail that the
caller does not have to know to correctly use this helper function.

Or am I missing something?

> +static void print_object_or_die(struct batch_options *opt,
> +				struct expand_data *data, struct strbuf *hdr)
>  {
>  	const struct object_id *oid = &data->oid;
>  
>  	assert(data->info.typep);
>  
> -	if (data->content) {
> -		void *content = data->content;
> +	if (data->iov[1].iov_base) {
> +		void *content = data->iov[1].iov_base;
>  		unsigned long size = data->size;
>  
> -		data->content = NULL;
>  		if (use_mailmap && (data->type == OBJ_COMMIT ||
>  					data->type == OBJ_TAG)) {
>  			size_t s = size;
> @@ -399,10 +424,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  			}
>  
>  			content = replace_idents_using_mailmap(content, &s);
> +			data->iov[1].iov_base = content;
>  			size = cast_size_t_to_ulong(s);
>  		}
> -
> -		batch_write(opt, content, size);
> +		batch_writev(opt, data, hdr, size);
>  		switch (data->info.whence) {
>  		case OI_CACHED:
>  			/*

And with the "let's make iov[3] a local implementation detail of
batch_writev()" approach, the above two hunks would shrink and
essentialy we'd replace batch_write() with batch_writev() (with
adjusted parameters).

> @@ -419,8 +444,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  		}
>  	} else {
>  		assert(data->type == OBJ_BLOB);
> -		if (opt->buffer_output)
> -			fflush(stdout);

We used to fflush whatever we have written before entering this
"else" clause.  We no longer do so

>  		if (opt->transform_mode) {
>  			char *contents;
>  			unsigned long size;
> @@ -447,10 +470,15 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  					    oid_to_hex(oid), data->rest);
>  			} else
>  				BUG("invalid transform_mode: %c", opt->transform_mode);
> -			batch_write(opt, contents, size);
> +			data->iov[1].iov_base = contents;
> +			batch_writev(opt, data, hdr, size);

And in the buffer_output mode, batch_writev() ends up calling
fwritev_or_die(), which is merely a series of fwrite() calls.  And
the removed fflush() earlier is perfectly fine, as it was solely
because we wanted to make sure fflush() before going down to direct
file descriptor access with write(2) and we are now still going
through stdio layer.

>  			free(contents);
>  		} else {
> +			batch_write(opt, hdr->buf, hdr->len);
> +			if (opt->buffer_output)
> +				fflush(stdout);

The bigger else clause is entered with potentially unflushed bytes
in the stdio buffer, as that was why we first fflush().  Then we do
batch_write() here, which uses fwrite() in the buffer_output mode,
without having to fflush().  But before doing stream_blob() below,
we do need to fflush().  Makes sense.

>  static void batch_one_object(const char *obj_name,
> @@ -666,7 +692,7 @@ static void parse_cmd_contents(struct batch_options *opt,
>  			     struct expand_data *data)
>  {
>  	opt->batch_mode = BATCH_MODE_CONTENTS;
> -	data->info.contentp = &data->content;
> +	data->info.contentp = &data->iov[1].iov_base;
>  	batch_one_object(line, output, opt, data);
>  }
>  
> @@ -823,7 +849,7 @@ static int batch_objects(struct batch_options *opt)
>  		data.info.typep = &data.type;
>  		if (!opt->transform_mode) {
>  			data.info.sizep = &data.size;
> -			data.info.contentp = &data.content;
> +			data.info.contentp = &data.iov[1].iov_base;
>  			data.info.content_limit = big_file_threshold;
>  			data.info.direct_cache = 1;
>  		}

If we do the "let's not leak the iov[3] implementation detail from
batch_writev()" update, the above two hunks can be eliminated.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index ca7678a379..afde8abc99 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -388,6 +388,16 @@ static inline int git_setitimer(int which UNUSED,
>  #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)
>  #endif
>  
> +#ifdef HAVE_WRITEV
> +#include <sys/uio.h>
> +#define git_iovec iovec
> +#else /* !HAVE_WRITEV */
> +struct git_iovec {
> +	void *iov_base;
> +	size_t iov_len;
> +};
> +#endif /* !HAVE_WRITEV */

OK.

> diff --git a/write-or-die.c b/write-or-die.c
> index 01a9a51fa2..227b051165 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -107,3 +107,69 @@ void fflush_or_die(FILE *f)
>  	if (fflush(f))
>  		die_errno("fflush error");
>  }
> +
> +void fwritev_or_die(FILE *fp, const struct git_iovec *iov, int iovcnt)
> +{
> +	int i;
> +
> +	for (i = 0; i < iovcnt; i++) {
> +		size_t n = iov[i].iov_len;
> +
> +		if (fwrite(iov[i].iov_base, 1, n, fp) != n)
> +			die_errno("unable to write to FD=%d", fileno(fp));
> +	}
> +}

OK.