[PATCH v1 00/10] cat-file speedups

[PATCH v1 00/10] cat-file speedups

[Eric Wong]

This continues the work of Jeff King and my initial work to
speed up cat-file --batch(-contents)? users in
https://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/

There's more speedups I'm working on, but this series touches
on the work Jeff and I have already published.

I've started putting some Perl5 + Inline::C benchmarks with
several knobs up at: git clone https://80x24.org/misc-git-benchmarks.git

I've found it necessary to use schedtool(1) on Linux to pin all
processes to a single CPU on multicore systems.

Some patches make more sense for largish objects, some for
smaller objects.  Small objects (several KB) were my main focus,
but I figure 5/10 could help with some pathological big cases
and also open the door to expanding the use of caching down the
line.

10/10 actually ended up being more significant than I originally
anticipated for repeat lookups of the same objects (common for
web frontends getting hammered).

Jeff: I started writing commit messages for your patches (1 and
2), but there's probably better explanations you could do :>

Eric Wong (8):
  packfile: fix off-by-one in content_limit comparison
  packfile: inline cache_or_unpack_entry
  cat-file: use delta_base_cache entries directly
  packfile: packed_object_info avoids packed_to_object_type
  object_info: content_limit only applies to blobs
  cat-file: batch-command uses content_limit
  cat-file: batch_write: use size_t for length
  cat-file: use writev(2) if available

Jeff King (2):
  packfile: move sizep computation
  packfile: allow content-limit for cat-file

 Makefile            |   3 ++
 builtin/cat-file.c  | 124 +++++++++++++++++++++++++++++++-------------
 config.mak.uname    |   5 ++
 git-compat-util.h   |  10 ++++
 object-file.c       |  12 +++++
 object-store-ll.h   |   8 +++
 packfile.c          | 120 ++++++++++++++++++++++++++----------------
 packfile.h          |   4 ++
 t/t1006-cat-file.sh |  19 +++++--
 wrapper.c           |  18 +++++++
 wrapper.h           |   1 +
 write-or-die.c      |  66 +++++++++++++++++++++++
 write-or-die.h      |   2 +
 13 files changed, 308 insertions(+), 84 deletions(-)

[PATCH v1 01/10] packfile: move sizep computation

[Eric Wong]

From: Jeff King <peff@peff.net>

This makes the next commit to avoid redundant object info
lookups easier to understand.

[ew: commit message]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/packfile.c b/packfile.c
index 813584646f..e547522e3d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1527,7 +1527,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 
 	/*
 	 * We always get the representation type, but only convert it to
-	 * a "real" type later if the caller is interested.
+	 * a "real" type later if the caller is interested. Likewise...
+	 * tbd.
 	 */
 	if (oi->contentp) {
 		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
@@ -1536,24 +1537,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			type = OBJ_BAD;
 	} else {
 		type = unpack_object_header(p, &w_curs, &curpos, &size);
-	}
 
-	if (!oi->contentp && oi->sizep) {
-		if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
-			off_t tmp_pos = curpos;
-			off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
-							   type, obj_offset);
-			if (!base_offset) {
-				type = OBJ_BAD;
-				goto out;
+		if (oi->sizep) {
+			if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
+				off_t tmp_pos = curpos;
+				off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
+								   type, obj_offset);
+				if (!base_offset) {
+					type = OBJ_BAD;
+					goto out;
+				}
+				*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);
+				if (*oi->sizep == 0) {
+					type = OBJ_BAD;
+					goto out;
+				}
+			} else {
+				*oi->sizep = size;
 			}
-			*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);
-			if (*oi->sizep == 0) {
-				type = OBJ_BAD;
-				goto out;
-			}
-		} else {
-			*oi->sizep = size;
 		}
 	}
 

Re: [PATCH v1 01/10] packfile: move sizep computation

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Mon, Jul 15, 2024 at 12:35:10AM +0000, Eric Wong wrote:
> From: Jeff King <peff@peff.net>
> 
> This makes the next commit to avoid redundant object info

Starting with "this" without mentioning what "this" is in the commit
subject reads a bit weird. I know you mention what you do in the commit
title, but we usually use fully self-contained commit messages here.

> lookups easier to understand.
> 
> [ew: commit message]
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  packfile.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 813584646f..e547522e3d 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1527,7 +1527,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  
>  	/*
>  	 * We always get the representation type, but only convert it to
> -	 * a "real" type later if the caller is interested.
> +	 * a "real" type later if the caller is interested. Likewise...
> +	 * tbd.

This comment gets addressed in the next commit, so this should likely
not be changed here?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 02/10] packfile: allow content-limit for cat-file

[Eric Wong]

From: Jeff King <peff@peff.net>

This avoids unnecessary round trips to the object store to speed
up cat-file contents retrievals.  The majority of packed objects
don't benefit from the streaming interface at all and we end up
having to load them in core anyways to satisfy our streaming
API.

This drops the runtime of
`git cat-file --batch-all-objects --unordered --batch' from
~7.1s to ~6.1s on Jeff's machine.

[ew: commit message]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 17 +++++++++++++++--
 object-file.c      |  6 ++++++
 object-store-ll.h  |  1 +
 packfile.c         | 13 ++++++++++++-
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 18fe58d6b8..bc4bb89610 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -280,6 +280,7 @@ struct expand_data {
 	off_t disk_size;
 	const char *rest;
 	struct object_id delta_base_oid;
+	void *content;
 
 	/*
 	 * If mark_query is true, we do not expand anything, but rather
@@ -383,7 +384,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 
 	assert(data->info.typep);
 
-	if (data->type == OBJ_BLOB) {
+	if (data->content) {
+		batch_write(opt, data->content, data->size);
+		FREE_AND_NULL(data->content);
+	} else if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
 		if (opt->transform_mode) {
@@ -801,9 +805,18 @@ static int batch_objects(struct batch_options *opt)
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
+	 *
+	 * Likewise, grab the content in the initial request if it's small
+	 * and we're not planning to filter it.
 	 */
-	if (opt->batch_mode == BATCH_MODE_CONTENTS)
+	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
 		data.info.typep = &data.type;
+		if (!opt->transform_mode) {
+			data.info.sizep = &data.size;
+			data.info.contentp = &data.content;
+			data.info.content_limit = big_file_threshold;
+		}
+	}
 
 	if (opt->all_objects) {
 		struct object_cb_data cb;
diff --git a/object-file.c b/object-file.c
index 065103be3e..1cc29c3c58 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,
 
 		if (!oi->contentp)
 			break;
+		if (oi->content_limit && *oi->sizep > oi->content_limit) {
+			git_inflate_end(&stream);
+			oi->contentp = NULL;
+			goto cleanup;
+		}
+
 		*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
 		if (*oi->contentp)
 			goto cleanup;
diff --git a/object-store-ll.h b/object-store-ll.h
index c5f2bb2fc2..b71a15f590 100644
--- a/object-store-ll.h
+++ b/object-store-ll.h
@@ -289,6 +289,7 @@ struct object_info {
 	struct object_id *delta_base_oid;
 	struct strbuf *type_name;
 	void **contentp;
+	size_t content_limit;
 
 	/* Response */
 	enum {
diff --git a/packfile.c b/packfile.c
index e547522e3d..54b9d46928 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1530,7 +1530,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	 * a "real" type later if the caller is interested. Likewise...
 	 * tbd.
 	 */
-	if (oi->contentp) {
+	if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
 						      &type);
 		if (!*oi->contentp)
@@ -1556,6 +1556,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 				*oi->sizep = size;
 			}
 		}
+
+		if (oi->contentp) {
+			if (oi->sizep && *oi->sizep < oi->content_limit) {
+				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
+								      oi->sizep, &type);
+				if (!*oi->contentp)
+					type = OBJ_BAD;
+			} else {
+				*oi->contentp = NULL;
+			}
+		}
 	}
 
 	if (oi->disk_sizep) {

Re: [PATCH v1 02/10] packfile: allow content-limit for cat-file

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Jul 15, 2024 at 12:35:11AM +0000, Eric Wong wrote:
> From: Jeff King <peff@peff.net>
> 
> This avoids unnecessary round trips to the object store to speed

Same comment regarding "this". Despite not being self-contained, I also
think that the commit message could do a better job of explaining what
the problem is that you're fixing in the first place. Right now, I'm
left second-guessing what the idea is that this patch has to make
git-cat-file(1) faster.

> up cat-file contents retrievals.  The majority of packed objects
> don't benefit from the streaming interface at all and we end up
> having to load them in core anyways to satisfy our streaming
> API.
> 
> This drops the runtime of
> `git cat-file --batch-all-objects --unordered --batch' from
> ~7.1s to ~6.1s on Jeff's machine.

It would be nice to get some more context here for the benchmark. Most
importantly, what kind of repository did this run in? Otherwise it is
going to be next to impossible to get remotely comparable results.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 02/10] packfile: allow content-limit for cat-file

[Eric Wong]

Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Jul 15, 2024 at 12:35:11AM +0000, Eric Wong wrote:
> > From: Jeff King <peff@peff.net>
> > 
> > This avoids unnecessary round trips to the object store to speed
> 
> Same comment regarding "this". Despite not being self-contained, I also
> think that the commit message could do a better job of explaining what
> the problem is that you're fixing in the first place. Right now, I'm
> left second-guessing what the idea is that this patch has to make
> git-cat-file(1) faster.

I was hoping Jeff would flesh out the commit messages for the
changes he authored.  I'll take a closer look and update the
messages if he's too busy.

> > up cat-file contents retrievals.  The majority of packed objects
> > don't benefit from the streaming interface at all and we end up
> > having to load them in core anyways to satisfy our streaming
> > API.
> > 
> > This drops the runtime of
> > `git cat-file --batch-all-objects --unordered --batch' from
> > ~7.1s to ~6.1s on Jeff's machine.
> 
> It would be nice to get some more context here for the benchmark. Most
> importantly, what kind of repository did this run in? Otherwise it is
> going to be next to impossible to get remotely comparable results.

Oops, that was for git.git
<https://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/>

[PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison

[Eric Wong]

object-file.c::loose_object_info() accepts objects matching
content_limit exactly, so it follows packfile handling allows
slurping objects which match loose object handling and slurp
objects with size matching the content_limit exactly.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packfile.c b/packfile.c
index 54b9d46928..371da96cdb 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1558,7 +1558,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		}
 
 		if (oi->contentp) {
-			if (oi->sizep && *oi->sizep < oi->content_limit) {
+			if (oi->sizep && *oi->sizep <= oi->content_limit) {
 				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
 								      oi->sizep, &type);
 				if (!*oi->contentp)

Re: [PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On Mon, Jul 15, 2024 at 12:35:12AM +0000, Eric Wong wrote:
> object-file.c::loose_object_info() accepts objects matching
> content_limit exactly, so it follows packfile handling allows
> slurping objects which match loose object handling and slurp
> objects with size matching the content_limit exactly.
> 
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  packfile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 54b9d46928..371da96cdb 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1558,7 +1558,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  		}
>  
>  		if (oi->contentp) {
> -			if (oi->sizep && *oi->sizep < oi->content_limit) {
> +			if (oi->sizep && *oi->sizep <= oi->content_limit) {
>  				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
>  								      oi->sizep, &type);
>  				if (!*oi->contentp)

In practice this doesn't really fix a user-visible bug, right? The only
difference before and after is that we now start to stream contents
earlier? And that's why we cannot have a test for this.

If so, I'd recommend to explain this in the commit message.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison

[Eric Wong]

Patrick Steinhardt <ps@pks.im> wrote:
> In practice this doesn't really fix a user-visible bug, right? The only
> difference before and after is that we now start to stream contents
> earlier? And that's why we cannot have a test for this.
> 
> If so, I'd recommend to explain this in the commit message.

Right, it's just for consistency with the rest of the code base.
Will update the message for v2.

[PATCH v1 04/10] packfile: inline cache_or_unpack_entry

[Eric Wong]

We need to check delta_base_cache anyways to fill in the
`whence' field in `struct object_info'.  Inlining
cache_or_unpack_entry() makes it easier to only do the hashmap
lookup once and avoid a redundant lookup later on.

This code reorganization will also make an optimization to
use the cache entry directly easier to implement.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/packfile.c b/packfile.c
index 371da96cdb..1a409ec142 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1444,23 +1444,6 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
 	free(ent);
 }
 
-static void *cache_or_unpack_entry(struct repository *r, struct packed_git *p,
-				   off_t base_offset, unsigned long *base_size,
-				   enum object_type *type)
-{
-	struct delta_base_cache_entry *ent;
-
-	ent = get_delta_base_cache_entry(p, base_offset);
-	if (!ent)
-		return unpack_entry(r, p, base_offset, type, base_size);
-
-	if (type)
-		*type = ent->type;
-	if (base_size)
-		*base_size = ent->size;
-	return xmemdupz(ent->data, ent->size);
-}
-
 static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 {
 	free(ent->data);
@@ -1521,21 +1504,36 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		       off_t obj_offset, struct object_info *oi)
 {
 	struct pack_window *w_curs = NULL;
-	unsigned long size;
 	off_t curpos = obj_offset;
 	enum object_type type;
+	struct delta_base_cache_entry *ent;
 
 	/*
 	 * We always get the representation type, but only convert it to
 	 * a "real" type later if the caller is interested. Likewise...
 	 * tbd.
 	 */
-	if (oi->contentp && !oi->content_limit) {
-		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
-						      &type);
+	oi->whence = OI_PACKED;
+	ent = get_delta_base_cache_entry(p, obj_offset);
+	if (ent) {
+		oi->whence = OI_DBCACHED;
+		type = ent->type;
+		if (oi->sizep)
+			*oi->sizep = ent->size;
+		if (oi->contentp) {
+			if (!oi->content_limit ||
+					ent->size <= oi->content_limit)
+				*oi->contentp = xmemdupz(ent->data, ent->size);
+			else
+				*oi->contentp = NULL; /* caller must stream */
+		}
+	} else if (oi->contentp && !oi->content_limit) {
+		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
+						oi->sizep);
 		if (!*oi->contentp)
 			type = OBJ_BAD;
 	} else {
+		unsigned long size;
 		type = unpack_object_header(p, &w_curs, &curpos, &size);
 
 		if (oi->sizep) {
@@ -1559,8 +1557,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 
 		if (oi->contentp) {
 			if (oi->sizep && *oi->sizep <= oi->content_limit) {
-				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
-								      oi->sizep, &type);
+				*oi->contentp = unpack_entry(r, p, obj_offset,
+							&type, oi->sizep);
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {
@@ -1609,10 +1607,6 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		} else
 			oidclr(oi->delta_base_oid, the_repository->hash_algo);
 	}
-
-	oi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :
-							  OI_PACKED;
-
 out:
 	unuse_pack(&w_curs);
 	return type;

[PATCH v1 05/10] cat-file: use delta_base_cache entries directly

[Eric Wong]

For objects already in the delta_base_cache, we can safely use
them directly to avoid the malloc+memcpy+free overhead.

While only 2-7% of objects are delta bases in repos I've looked
at, this avoids up to 96MB of duplicated memory in the worst
case with the default git config.  For a more reasonable 1MB
delta base object, this eliminates the speed penalty of
duplicating large objects into memory and speeds up those 1MB
delta base cached content retrievals by roughly 30%.

The new delta_base_cache_lock is a simple single-threaded
assertion to ensure cat-file is the exclusive user of the
delta_base_cache.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 15 ++++++++++++++-
 object-file.c      |  5 +++++
 object-store-ll.h  |  7 +++++++
 packfile.c         | 28 +++++++++++++++++++++++++---
 packfile.h         |  4 ++++
 5 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bc4bb89610..769c8b48d2 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -24,6 +24,7 @@
 #include "promisor-remote.h"
 #include "mailmap.h"
 #include "write-or-die.h"
+#define USE_DIRECT_CACHE 1
 
 enum batch_mode {
 	BATCH_MODE_CONTENTS,
@@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 
 	if (data->content) {
 		batch_write(opt, data->content, data->size);
-		FREE_AND_NULL(data->content);
+		switch (data->info.whence) {
+		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
+		case OI_LOOSE:
+		case OI_PACKED:
+			FREE_AND_NULL(data->content);
+			break;
+		case OI_DBCACHED:
+			if (USE_DIRECT_CACHE)
+				unlock_delta_base_cache();
+			else
+				FREE_AND_NULL(data->content);
+		}
 	} else if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
@@ -815,6 +827,7 @@ static int batch_objects(struct batch_options *opt)
 			data.info.sizep = &data.size;
 			data.info.contentp = &data.content;
 			data.info.content_limit = big_file_threshold;
+			data.info.direct_cache = USE_DIRECT_CACHE;
 		}
 	}
 
diff --git a/object-file.c b/object-file.c
index 1cc29c3c58..19100e823d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,
 			oidclr(oi->delta_base_oid, the_repository->hash_algo);
 		if (oi->type_name)
 			strbuf_addstr(oi->type_name, type_name(co->type));
+		/*
+		 * Currently `blame' is the only command which creates
+		 * OI_CACHED, and direct_cache is only used by `cat-file'.
+		 */
+		assert(!oi->direct_cache);
 		if (oi->contentp)
 			*oi->contentp = xmemdupz(co->buf, co->size);
 		oi->whence = OI_CACHED;
diff --git a/object-store-ll.h b/object-store-ll.h
index b71a15f590..50c5219308 100644
--- a/object-store-ll.h
+++ b/object-store-ll.h
@@ -298,6 +298,13 @@ struct object_info {
 		OI_PACKED,
 		OI_DBCACHED
 	} whence;
+
+	/*
+	 * set if caller is able to use OI_DBCACHED entries without copying
+	 * TODO OI_CACHED if its use goes beyond blame
+	 */
+	unsigned direct_cache:1;
+
 	union {
 		/*
 		 * struct {
diff --git a/packfile.c b/packfile.c
index 1a409ec142..b2660e14f9 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,
 static struct hashmap delta_base_cache;
 static size_t delta_base_cached;
 
+/* ensures oi->direct_cache is used properly */
+static int delta_base_cache_lock;
+
 static LIST_HEAD(delta_base_cache_lru);
 
 struct delta_base_cache_key {
@@ -1444,6 +1447,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
 	free(ent);
 }
 
+static void lock_delta_base_cache(void)
+{
+	delta_base_cache_lock++;
+	assert(delta_base_cache_lock == 1);
+}
+
+void unlock_delta_base_cache(void)
+{
+	delta_base_cache_lock--;
+	assert(delta_base_cache_lock == 0);
+}
+
 static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 {
 	free(ent->data);
@@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 void clear_delta_base_cache(void)
 {
 	struct list_head *lru, *tmp;
+	assert(!delta_base_cache_lock);
 	list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
 		struct delta_base_cache_entry *entry =
 			list_entry(lru, struct delta_base_cache_entry, lru);
@@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
 	struct delta_base_cache_entry *ent;
 	struct list_head *lru, *tmp;
 
+	assert(!delta_base_cache_lock);
 	/*
 	 * Check required to avoid redundant entries when more than one thread
 	 * is unpacking the same object, in unpack_entry() (since its phases I
@@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		if (oi->sizep)
 			*oi->sizep = ent->size;
 		if (oi->contentp) {
-			if (!oi->content_limit ||
-					ent->size <= oi->content_limit)
+			/* ignore content_limit if avoiding copy from cache */
+			if (oi->direct_cache) {
+				lock_delta_base_cache();
+				*oi->contentp = ent->data;
+			} else if (!oi->content_limit ||
+					ent->size <= oi->content_limit) {
 				*oi->contentp = xmemdupz(ent->data, ent->size);
-			else
+			} else {
 				*oi->contentp = NULL; /* caller must stream */
+			}
 		}
 	} else if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
diff --git a/packfile.h b/packfile.h
index eb18ec15db..94941bbe80 100644
--- a/packfile.h
+++ b/packfile.h
@@ -210,4 +210,8 @@ int is_promisor_object(const struct object_id *oid);
 int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
 	     size_t idx_size, struct packed_git *p);
 
+/*
+ * release lock acquired via oi->direct_cache
+ */
+void unlock_delta_base_cache(void);
 #endif

Re: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6381 bytes --]

On Mon, Jul 15, 2024 at 12:35:14AM +0000, Eric Wong wrote:
> For objects already in the delta_base_cache, we can safely use
> them directly to avoid the malloc+memcpy+free overhead.

Same here, I feel like you need to explain a bit more in depth what the
actual idea behind your patch is to help reviewers.

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index bc4bb89610..769c8b48d2 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -24,6 +24,7 @@
>  #include "promisor-remote.h"
>  #include "mailmap.h"
>  #include "write-or-die.h"
> +#define USE_DIRECT_CACHE 1

I'm confused by this. Why do we introduce a macro that is always defined
to a trueish value? Why don't we just remove the code guarded by this?

>  enum batch_mode {
>  	BATCH_MODE_CONTENTS,
> @@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  
>  	if (data->content) {
>  		batch_write(opt, data->content, data->size);
> -		FREE_AND_NULL(data->content);
> +		switch (data->info.whence) {
> +		case OI_CACHED: BUG("FIXME OI_CACHED support not done");

Is this something that will get addressed in a subsequent patch? If so,
the commit message and the message here should likely mention this. If
not, we should have a comment here saying why this is fine to be kept.

> +		case OI_LOOSE:
> +		case OI_PACKED:
> +			FREE_AND_NULL(data->content);
> +			break;
> +		case OI_DBCACHED:
> +			if (USE_DIRECT_CACHE)
> +				unlock_delta_base_cache();
> +			else
> +				FREE_AND_NULL(data->content);
> +		}
>  	} else if (data->type == OBJ_BLOB) {
>  		if (opt->buffer_output)
>  			fflush(stdout);
> @@ -815,6 +827,7 @@ static int batch_objects(struct batch_options *opt)
>  			data.info.sizep = &data.size;
>  			data.info.contentp = &data.content;
>  			data.info.content_limit = big_file_threshold;
> +			data.info.direct_cache = USE_DIRECT_CACHE;
>  		}
>  	}
>  
> diff --git a/object-file.c b/object-file.c
> index 1cc29c3c58..19100e823d 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,
>  			oidclr(oi->delta_base_oid, the_repository->hash_algo);
>  		if (oi->type_name)
>  			strbuf_addstr(oi->type_name, type_name(co->type));
> +		/*
> +		 * Currently `blame' is the only command which creates
> +		 * OI_CACHED, and direct_cache is only used by `cat-file'.
> +		 */
> +		assert(!oi->direct_cache);

We shouldn't use asserts, but rather use `BUG()` statements in our
codebase. `assert()`s don't help users that run production builds.

>  		if (oi->contentp)
>  			*oi->contentp = xmemdupz(co->buf, co->size);
>  		oi->whence = OI_CACHED;
> diff --git a/object-store-ll.h b/object-store-ll.h
> index b71a15f590..50c5219308 100644
> --- a/object-store-ll.h
> +++ b/object-store-ll.h
> @@ -298,6 +298,13 @@ struct object_info {
>  		OI_PACKED,
>  		OI_DBCACHED
>  	} whence;
> +
> +	/*
> +	 * set if caller is able to use OI_DBCACHED entries without copying
> +	 * TODO OI_CACHED if its use goes beyond blame
> +	 */
> +	unsigned direct_cache:1;
> +

This comment looks unfinished to me.

>  	union {
>  		/*
>  		 * struct {
> diff --git a/packfile.c b/packfile.c
> index 1a409ec142..b2660e14f9 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,
>  static struct hashmap delta_base_cache;
>  static size_t delta_base_cached;
>  
> +/* ensures oi->direct_cache is used properly */
> +static int delta_base_cache_lock;
> +

How exactly does it ensure it? What is the intent of this variable and
how would it be used correctly?

>  static LIST_HEAD(delta_base_cache_lru);
>  
>  struct delta_base_cache_key {
> @@ -1444,6 +1447,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
>  	free(ent);
>  }
>  
> +static void lock_delta_base_cache(void)
> +{
> +	delta_base_cache_lock++;
> +	assert(delta_base_cache_lock == 1);
> +}
> +
> +void unlock_delta_base_cache(void)
> +{
> +	delta_base_cache_lock--;
> +	assert(delta_base_cache_lock == 0);
> +}

Hum. So this looks like a pseudo-mutex to me? Are there any code paths
where this may be used in a threaded context? I assume not in the
current state of affairs as we only use it in git-cat-file(1).

>  static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
>  {
>  	free(ent->data);
> @@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
>  void clear_delta_base_cache(void)
>  {
>  	struct list_head *lru, *tmp;
> +	assert(!delta_base_cache_lock);
>  	list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
>  		struct delta_base_cache_entry *entry =
>  			list_entry(lru, struct delta_base_cache_entry, lru);
> @@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
>  	struct delta_base_cache_entry *ent;
>  	struct list_head *lru, *tmp;
>  
> +	assert(!delta_base_cache_lock);
>  	/*
>  	 * Check required to avoid redundant entries when more than one thread
>  	 * is unpacking the same object, in unpack_entry() (since its phases I
> @@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  		if (oi->sizep)
>  			*oi->sizep = ent->size;
>  		if (oi->contentp) {
> -			if (!oi->content_limit ||
> -					ent->size <= oi->content_limit)
> +			/* ignore content_limit if avoiding copy from cache */
> +			if (oi->direct_cache) {
> +				lock_delta_base_cache();
> +				*oi->contentp = ent->data;
> +			} else if (!oi->content_limit ||
> +					ent->size <= oi->content_limit) {
>  				*oi->contentp = xmemdupz(ent->data, ent->size);
> -			else
> +			} else {
>  				*oi->contentp = NULL; /* caller must stream */
> +			}
>  		}
>  	} else if (oi->contentp && !oi->content_limit) {
>  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,

Okay, this hunk is the gist of this patch. Instead of copying over the
delta base, we simply take its data pointer as the content pointer. All
the other infra that you're adding is mostly only added as a safeguard
to make sure that we don't discard the delta base while the object is
getting accessed.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly

[Eric Wong]

Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Jul 15, 2024 at 12:35:14AM +0000, Eric Wong wrote:
> > For objects already in the delta_base_cache, we can safely use
> > them directly to avoid the malloc+memcpy+free overhead.
> 
> Same here, I feel like you need to explain a bit more in depth what the
> actual idea behind your patch is to help reviewers.

I elaborated more on the speedup gained in the second paragraph
of the commit message:

	... this avoids up to 96MB of duplicated memory in the worst
	case with the default git config.  For a more reasonable 1MB
	delta base object, this eliminates the speed penalty of
	duplicating large objects into memory and speeds up those 1MB
	delta base cached content retrievals by roughly 30%.

> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index bc4bb89610..769c8b48d2 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -24,6 +24,7 @@
> >  #include "promisor-remote.h"
> >  #include "mailmap.h"
> >  #include "write-or-die.h"
> > +#define USE_DIRECT_CACHE 1
> 
> I'm confused by this. Why do we introduce a macro that is always defined
> to a trueish value? Why don't we just remove the code guarded by this?

I wanted to be able to toggle the feature for comparison during
development.  I can eliminate it for v2.

> >  enum batch_mode {
> >  	BATCH_MODE_CONTENTS,
> > @@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
> >  
> >  	if (data->content) {
> >  		batch_write(opt, data->content, data->size);
> > -		FREE_AND_NULL(data->content);
> > +		switch (data->info.whence) {
> > +		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
> 
> Is this something that will get addressed in a subsequent patch? If so,
> the commit message and the message here should likely mention this. If
> not, we should have a comment here saying why this is fine to be kept.

Not in this series.  I'm not sure if we'll ever need OI_CACHED
support, here.  However, I've been considering an new cache
that can be shared across multiple cat-file processes, but
that'll be a separate series.

> > diff --git a/object-file.c b/object-file.c
> > index 1cc29c3c58..19100e823d 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,
> >  			oidclr(oi->delta_base_oid, the_repository->hash_algo);
> >  		if (oi->type_name)
> >  			strbuf_addstr(oi->type_name, type_name(co->type));
> > +		/*
> > +		 * Currently `blame' is the only command which creates
> > +		 * OI_CACHED, and direct_cache is only used by `cat-file'.
> > +		 */
> > +		assert(!oi->direct_cache);
> 
> We shouldn't use asserts, but rather use `BUG()` statements in our
> codebase. `assert()`s don't help users that run production builds.

OK.

> >  		if (oi->contentp)
> >  			*oi->contentp = xmemdupz(co->buf, co->size);
> >  		oi->whence = OI_CACHED;
> > diff --git a/object-store-ll.h b/object-store-ll.h
> > index b71a15f590..50c5219308 100644
> > --- a/object-store-ll.h
> > +++ b/object-store-ll.h
> > @@ -298,6 +298,13 @@ struct object_info {
> >  		OI_PACKED,
> >  		OI_DBCACHED
> >  	} whence;
> > +
> > +	/*
> > +	 * set if caller is able to use OI_DBCACHED entries without copying
> > +	 * TODO OI_CACHED if its use goes beyond blame
> > +	 */
> > +	unsigned direct_cache:1;
> > +
> 
> This comment looks unfinished to me.

Yeah.  I'll elaborate on it's only intended for cat-file atm and
would break if blame (or other callers) used it.

> >  	union {
> >  		/*
> >  		 * struct {
> > diff --git a/packfile.c b/packfile.c
> > index 1a409ec142..b2660e14f9 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,
> >  static struct hashmap delta_base_cache;
> >  static size_t delta_base_cached;
> >  
> > +/* ensures oi->direct_cache is used properly */
> > +static int delta_base_cache_lock;
> > +
> 
> How exactly does it ensure it? What is the intent of this variable and
> how would it be used correctly?

It prevents multiple cache entries from being acquired at once.

> > +static void lock_delta_base_cache(void)
> > +{
> > +	delta_base_cache_lock++;
> > +	assert(delta_base_cache_lock == 1);
> > +}
> > +
> > +void unlock_delta_base_cache(void)
> > +{
> > +	delta_base_cache_lock--;
> > +	assert(delta_base_cache_lock == 0);
> > +}
> 
> Hum. So this looks like a pseudo-mutex to me? Are there any code paths
> where this may be used in a threaded context? I assume not in the
> current state of affairs as we only use it in git-cat-file(1).

No parallelism or threads at all.  It's to ensure callers can't
load multiple entries at the same time since retrieving a delta
base cache entry could invalidate an entry that's already
acquired for use.

> >  static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
> >  {
> >  	free(ent->data);
> > @@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
> >  void clear_delta_base_cache(void)
> >  {
> >  	struct list_head *lru, *tmp;
> > +	assert(!delta_base_cache_lock);
> >  	list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
> >  		struct delta_base_cache_entry *entry =
> >  			list_entry(lru, struct delta_base_cache_entry, lru);
> > @@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
> >  	struct delta_base_cache_entry *ent;
> >  	struct list_head *lru, *tmp;
> >  
> > +	assert(!delta_base_cache_lock);
> >  	/*
> >  	 * Check required to avoid redundant entries when more than one thread
> >  	 * is unpacking the same object, in unpack_entry() (since its phases I
> > @@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  		if (oi->sizep)
> >  			*oi->sizep = ent->size;
> >  		if (oi->contentp) {
> > -			if (!oi->content_limit ||
> > -					ent->size <= oi->content_limit)
> > +			/* ignore content_limit if avoiding copy from cache */
> > +			if (oi->direct_cache) {
> > +				lock_delta_base_cache();
> > +				*oi->contentp = ent->data;
> > +			} else if (!oi->content_limit ||
> > +					ent->size <= oi->content_limit) {
> >  				*oi->contentp = xmemdupz(ent->data, ent->size);
> > -			else
> > +			} else {
> >  				*oi->contentp = NULL; /* caller must stream */
> > +			}
> >  		}
> >  	} else if (oi->contentp && !oi->content_limit) {
> >  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
> 
> Okay, this hunk is the gist of this patch. Instead of copying over the
> delta base, we simply take its data pointer as the content pointer. All
> the other infra that you're adding is mostly only added as a safeguard
> to make sure that we don't discard the delta base while the object is
> getting accessed.

Right.  I'll switch the asserts to BUG calls for v2.

assert vs BUG [was: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly]

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Patrick Steinhardt <ps@pks.im> wrote:
> > We shouldn't use asserts, but rather use `BUG()` statements in our
> > codebase. `assert()`s don't help users that run production builds.
> 
> OK.

Thinking about this more, I still favor assert() in common code
paths since it's only meant to be used during development and
later removed or neutralized (via -DNDEBUG).

IOW, I treat assert() as scaffolding that can/should later be
removed once the code is proven to work well.  We also have
plenty of existing asserts in our codebase.

Furthermore, assert() is also a well known API which reduces the
learning curve for drive-by hackers (I still consider myself
a drive-by since my I do minimal C).

Re: assert vs BUG [was: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly]

[Junio C Hamano]

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

> Eric Wong <e@80x24.org> wrote:
>> Patrick Steinhardt <ps@pks.im> wrote:
>> > We shouldn't use asserts, but rather use `BUG()` statements in our
>> > codebase. `assert()`s don't help users that run production builds.
>> 
>> OK.
>
> Thinking about this more, I still favor assert() in common code
> paths since it's only meant to be used during development and
> later removed or neutralized (via -DNDEBUG).

I have a mixed feeling.

I agree that assert() is only meant to be used during development,
but we only want code that is polished enough to be added to the
system, so there is no place for assert() in the code in 'master'.

The point of BUG() is that it is not easily "neutralized", so it can
help safeguarding the production code from harming the end-user data
due to doing nonsense things without noticing that the precondition
is not satisfied for it to perform correctly.  It makes sense to
have it in both during development and after deployment.

[PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type

[Eric Wong]

For calls the delta base cache, packed_to_object_type calls
can be omitted.  This prepares us to bypass content_limit for
non-blob types in the following commit.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/packfile.c b/packfile.c
index b2660e14f9..c2ba6ab203 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 {
 	struct pack_window *w_curs = NULL;
 	off_t curpos = obj_offset;
-	enum object_type type;
+	enum object_type type, final_type = OBJ_BAD;
 	struct delta_base_cache_entry *ent;
 
 	/*
@@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	ent = get_delta_base_cache_entry(p, obj_offset);
 	if (ent) {
 		oi->whence = OI_DBCACHED;
-		type = ent->type;
+		final_type = type = ent->type;
 		if (oi->sizep)
 			*oi->sizep = ent->size;
 		if (oi->contentp) {
@@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	} else if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
 						oi->sizep);
+		final_type = type;
 		if (!*oi->contentp)
 			type = OBJ_BAD;
 	} else {
@@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			if (oi->sizep && *oi->sizep <= oi->content_limit) {
 				*oi->contentp = unpack_entry(r, p, obj_offset,
 							&type, oi->sizep);
+				final_type = type;
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {
@@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	}
 
 	if (oi->typep || oi->type_name) {
-		enum object_type ptot;
-		ptot = packed_to_object_type(r, p, obj_offset,
-					     type, &w_curs, curpos);
+		if (final_type < 0)
+			final_type = packed_to_object_type(r, p, obj_offset,
+						     type, &w_curs, curpos);
 		if (oi->typep)
-			*oi->typep = ptot;
+			*oi->typep = final_type;
 		if (oi->type_name) {
-			const char *tn = type_name(ptot);
+			const char *tn = type_name(final_type);
 			if (tn)
 				strbuf_addstr(oi->type_name, tn);
 		}
-		if (ptot < 0) {
+		if (final_type < 0) {
 			type = OBJ_BAD;
 			goto out;
 		}

Re: [PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

On Mon, Jul 15, 2024 at 12:35:15AM +0000, Eric Wong wrote:
> For calls the delta base cache, packed_to_object_type calls
> can be omitted.  This prepares us to bypass content_limit for
> non-blob types in the following commit.
> 
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  packfile.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/packfile.c b/packfile.c
> index b2660e14f9..c2ba6ab203 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  {
>  	struct pack_window *w_curs = NULL;
>  	off_t curpos = obj_offset;
> -	enum object_type type;
> +	enum object_type type, final_type = OBJ_BAD;
>  	struct delta_base_cache_entry *ent;

I think it might help this patch to move `type` to the scopes where it's
used to demonstrate that all code paths set `final_type` as expected.

>  	/*
> @@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	ent = get_delta_base_cache_entry(p, obj_offset);
>  	if (ent) {
>  		oi->whence = OI_DBCACHED;
> -		type = ent->type;
> +		final_type = type = ent->type;
>  		if (oi->sizep)
>  			*oi->sizep = ent->size;
>  		if (oi->contentp) {
> @@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	} else if (oi->contentp && !oi->content_limit) {
>  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
>  						oi->sizep);
> +		final_type = type;
>  		if (!*oi->contentp)
>  			type = OBJ_BAD;
>  	} else {
> @@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  			if (oi->sizep && *oi->sizep <= oi->content_limit) {
>  				*oi->contentp = unpack_entry(r, p, obj_offset,
>  							&type, oi->sizep);
> +				final_type = type;
>  				if (!*oi->contentp)
>  					type = OBJ_BAD;
>  			} else {
> @@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	}
>  
>  	if (oi->typep || oi->type_name) {
> -		enum object_type ptot;
> -		ptot = packed_to_object_type(r, p, obj_offset,
> -					     type, &w_curs, curpos);
> +		if (final_type < 0)
> +			final_type = packed_to_object_type(r, p, obj_offset,
> +						     type, &w_curs, curpos);

So this is the actual change we're interested in, right? Instead of
unconditionally calling `packed_to_object_type()`, we skip that call in
case we know that we have already figured out the correct object type.

Wouldn't it be easier to manage this with a single `type` variable,
only, and then conditionally call `packed_to_object_type()` only in the
cases where `type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA`? Not sure
whether that would be all that useful though given that the function
already knows to exit without doing anything in case the type is already
properly resolved. So maybe the next patch will enlighten me.

Patrick

>  		if (oi->typep)
> -			*oi->typep = ptot;
> +			*oi->typep = final_type;
>  		if (oi->type_name) {
> -			const char *tn = type_name(ptot);
> +			const char *tn = type_name(final_type);
>  			if (tn)
>  				strbuf_addstr(oi->type_name, tn);
>  		}
> -		if (ptot < 0) {
> +		if (final_type < 0) {
>  			type = OBJ_BAD;
>  			goto out;
>  		}
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type

[Eric Wong]

Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Jul 15, 2024 at 12:35:15AM +0000, Eric Wong wrote:
> > For calls the delta base cache, packed_to_object_type calls
> > can be omitted.  This prepares us to bypass content_limit for
> > non-blob types in the following commit.
> > 
> > Signed-off-by: Eric Wong <e@80x24.org>
> > ---
> >  packfile.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/packfile.c b/packfile.c
> > index b2660e14f9..c2ba6ab203 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  {
> >  	struct pack_window *w_curs = NULL;
> >  	off_t curpos = obj_offset;
> > -	enum object_type type;
> > +	enum object_type type, final_type = OBJ_BAD;
> >  	struct delta_base_cache_entry *ent;
> 
> I think it might help this patch to move `type` to the scopes where it's
> used to demonstrate that all code paths set `final_type` as expected.

The condition at the end of packed_object_info() requires the original
`type' to keep its top-level scope:

        if (oi->delta_base_oid) {
                if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {

But yeah, the whole function is huge and remains a bit convoluted.
Inlining cache_or_unpack_entry in 4/10 helped some, I think.

> >  	/*
> > @@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	ent = get_delta_base_cache_entry(p, obj_offset);
> >  	if (ent) {
> >  		oi->whence = OI_DBCACHED;
> > -		type = ent->type;
> > +		final_type = type = ent->type;
> >  		if (oi->sizep)
> >  			*oi->sizep = ent->size;
> >  		if (oi->contentp) {
> > @@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	} else if (oi->contentp && !oi->content_limit) {
> >  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
> >  						oi->sizep);
> > +		final_type = type;
> >  		if (!*oi->contentp)
> >  			type = OBJ_BAD;
> >  	} else {
> > @@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  			if (oi->sizep && *oi->sizep <= oi->content_limit) {
> >  				*oi->contentp = unpack_entry(r, p, obj_offset,
> >  							&type, oi->sizep);
> > +				final_type = type;
> >  				if (!*oi->contentp)
> >  					type = OBJ_BAD;
> >  			} else {
> > @@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	}
> >  
> >  	if (oi->typep || oi->type_name) {
> > -		enum object_type ptot;
> > -		ptot = packed_to_object_type(r, p, obj_offset,
> > -					     type, &w_curs, curpos);
> > +		if (final_type < 0)
> > +			final_type = packed_to_object_type(r, p, obj_offset,
> > +						     type, &w_curs, curpos);
> 
> So this is the actual change we're interested in, right? Instead of
> unconditionally calling `packed_to_object_type()`, we skip that call in
> case we know that we have already figured out the correct object type.
> 
> Wouldn't it be easier to manage this with a single `type` variable,
> only, and then conditionally call `packed_to_object_type()` only in the
> cases where `type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA`? Not sure
> whether that would be all that useful though given that the function
> already knows to exit without doing anything in case the type is already
> properly resolved. So maybe the next patch will enlighten me.

As I mentioned above, I think the `type' var remains necessary.

[PATCH v1 07/10] object_info: content_limit only applies to blobs

[Eric Wong]

Streaming is only supported for blobs, so we'd end up having to
slurp all the other object types into memory regardless.  So
slurp all the non-blob types up front when requesting content
since we always handle them in-core, anyways.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 51 +++++++++++++++++++++-------------------------
 object-file.c      |  3 ++-
 packfile.c         |  8 +++++---
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 769c8b48d2..0752ff7a74 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -386,20 +386,39 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	assert(data->info.typep);
 
 	if (data->content) {
-		batch_write(opt, data->content, data->size);
+		void *content = data->content;
+		unsigned long size = data->size;
+
+		data->content = NULL;
+		if (use_mailmap && (data->type == OBJ_COMMIT ||
+					data->type == OBJ_TAG)) {
+			size_t s = size;
+
+			if (USE_DIRECT_CACHE &&
+					data->info.whence == OI_DBCACHED) {
+				content = xmemdupz(content, s);
+				data->info.whence = OI_PACKED;
+			}
+
+			content = replace_idents_using_mailmap(content, &s);
+			size = cast_size_t_to_ulong(s);
+		}
+
+		batch_write(opt, content, size);
 		switch (data->info.whence) {
 		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
 		case OI_LOOSE:
 		case OI_PACKED:
-			FREE_AND_NULL(data->content);
+			free(content);
 			break;
 		case OI_DBCACHED:
 			if (USE_DIRECT_CACHE)
 				unlock_delta_base_cache();
 			else
-				FREE_AND_NULL(data->content);
+				free(content);
 		}
-	} else if (data->type == OBJ_BLOB) {
+	} else {
+		assert(data->type == OBJ_BLOB);
 		if (opt->buffer_output)
 			fflush(stdout);
 		if (opt->transform_mode) {
@@ -434,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 			stream_blob(oid);
 		}
 	}
-	else {
-		enum object_type type;
-		unsigned long size;
-		void *contents;
-
-		contents = repo_read_object_file(the_repository, oid, &type,
-						 &size);
-		if (!contents)
-			die("object %s disappeared", oid_to_hex(oid));
-
-		if (use_mailmap) {
-			size_t s = size;
-			contents = replace_idents_using_mailmap(contents, &s);
-			size = cast_size_t_to_ulong(s);
-		}
-
-		if (type != data->type)
-			die("object %s changed type!?", oid_to_hex(oid));
-		if (data->info.sizep && size != data->size && !use_mailmap)
-			die("object %s changed size!?", oid_to_hex(oid));
-
-		batch_write(opt, contents, size);
-		free(contents);
-	}
 }
 
 static void print_default_format(struct strbuf *scratch, struct expand_data *data,
diff --git a/object-file.c b/object-file.c
index 19100e823d..59842cfe1b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,
 
 		if (!oi->contentp)
 			break;
-		if (oi->content_limit && *oi->sizep > oi->content_limit) {
+		if (oi->content_limit && *oi->typep == OBJ_BLOB &&
+				*oi->sizep > oi->content_limit) {
 			git_inflate_end(&stream);
 			oi->contentp = NULL;
 			goto cleanup;
diff --git a/packfile.c b/packfile.c
index c2ba6ab203..01ce3a49db 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1542,7 +1542,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			if (oi->direct_cache) {
 				lock_delta_base_cache();
 				*oi->contentp = ent->data;
-			} else if (!oi->content_limit ||
+			} else if (type != OBJ_BLOB || !oi->content_limit ||
 					ent->size <= oi->content_limit) {
 				*oi->contentp = xmemdupz(ent->data, ent->size);
 			} else {
@@ -1579,10 +1579,12 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		}
 
 		if (oi->contentp) {
-			if (oi->sizep && *oi->sizep <= oi->content_limit) {
+			final_type = packed_to_object_type(r, p, obj_offset,
+						     type, &w_curs, curpos);
+			if (final_type != OBJ_BLOB || (oi->sizep &&
+					*oi->sizep <= oi->content_limit)) {
 				*oi->contentp = unpack_entry(r, p, obj_offset,
 							&type, oi->sizep);
-				final_type = type;
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {

[PATCH v1 08/10] cat-file: batch-command uses content_limit

[Eric Wong]

As with the normal `--batch' mode, we can use the content_limit
round trip optimization to avoid a redundant lookup.  The only
tricky thing here is we need to enable/disable setting the
object_info.contentp field depending on whether we hit an `info'
or `contents' command.

t1006 is updated to ensure we can switch back and forth between
`info' and `contents' commands without problems.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c  |  5 ++++-
 t/t1006-cat-file.sh | 19 ++++++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0752ff7a74..c4c28236db 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -666,6 +666,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;
 	batch_one_object(line, output, opt, data);
 }
 
@@ -675,6 +676,7 @@ static void parse_cmd_info(struct batch_options *opt,
 			   struct expand_data *data)
 {
 	opt->batch_mode = BATCH_MODE_INFO;
+	data->info.contentp = NULL;
 	batch_one_object(line, output, opt, data);
 }
 
@@ -816,7 +818,8 @@ static int batch_objects(struct batch_options *opt)
 	 * Likewise, grab the content in the initial request if it's small
 	 * and we're not planning to filter it.
 	 */
-	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
+	if ((opt->batch_mode == BATCH_MODE_CONTENTS) ||
+			(opt->batch_mode == BATCH_MODE_QUEUE_AND_DISPATCH)) {
 		data.info.typep = &data.type;
 		if (!opt->transform_mode) {
 			data.info.sizep = &data.size;
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index ff9bf213aa..841e8567e9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -622,20 +622,33 @@ test_expect_success 'confirm that neither loose blob is a delta' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup delta base tests' '
+	foo="$(git rev-parse HEAD:foo)" &&
+	foo_plus="$(git rev-parse HEAD:foo-plus)" &&
+	git repack -ad
+'
+
 # To avoid relying too much on the current delta heuristics,
 # we will check only that one of the two objects is a delta
 # against the other, but not the order. We can do so by just
 # asking for the base of both, and checking whether either
 # oid appears in the output.
 test_expect_success '%(deltabase) reports packed delta bases' '
-	git repack -ad &&
 	git cat-file --batch-check="%(deltabase)" <blobs >actual &&
 	{
-		grep "$(git rev-parse HEAD:foo)" actual ||
-		grep "$(git rev-parse HEAD:foo-plus)" actual
+		grep "$foo" actual || grep "$foo_plus" actual
 	}
 '
 
+test_expect_success 'delta base direct cache use succeeds w/o asserting' '
+	commands="info $foo
+info $foo_plus
+contents $foo_plus
+contents $foo" &&
+	echo "$commands" >in &&
+	git cat-file --batch-command <in >out
+'
+
 test_expect_success 'setup bogus data' '
 	bogus_short_type="bogus" &&
 	bogus_short_content="bogus" &&

[PATCH v1 09/10] cat-file: batch_write: use size_t for length

[Eric Wong]

fwrite(3) and write(2), and all of our wrappers for them use
size_t while object size is `unsigned long', so there's no
excuse to use a potentially smaller representation.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index c4c28236db..efc0df760c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -370,7 +370,7 @@ static void expand_format(struct strbuf *sb, const char *start,
 	}
 }
 
-static void batch_write(struct batch_options *opt, const void *data, int len)
+static void batch_write(struct batch_options *opt, const void *data, size_t len)
 {
 	if (opt->buffer_output) {
 		if (fwrite(data, 1, len, stdout) != len)

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

[Eric Wong]

Using writev here is can be 20-40% faster than three write
syscalls in succession for smaller (1-10k) objects in the delta
base cache.  This advantage decreases as object sizes approach
pipe size (64k on Linux).  This reduces wakeups and syscalls on
the read side, as well, especially if the reader is relying on
non-blocking I/O.

Unfortunately, this turns into a small (1-3%) slowdown for
gigantic objects of a megabyte or more even with after
increasing pipe size to 1MB via the F_SETPIPE_SZ fcntl(2) op.
This slowdown is acceptable to me since the vast majority of
objects are 64K or less for projects I've looked at.

Relying on stdio buffering and fflush(3) after each response was
considered for users without --buffer, but historically cat-file
defaults to being compatible with non-blocking stdout and able
to poll(2) after hitting EAGAIN on write(2).  Using stdio on
files with the O_NONBLOCK flag is (AFAIK) unspecified and likely
subject to portability problems.

Signed-off-by: Eric Wong <e@80x24.org>
---
 Makefile           |  3 +++
 builtin/cat-file.c | 62 ++++++++++++++++++++++++++++++-------------
 config.mak.uname   |  5 ++++
 git-compat-util.h  | 10 +++++++
 wrapper.c          | 18 +++++++++++++
 wrapper.h          |  1 +
 write-or-die.c     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 write-or-die.h     |  2 ++
 8 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index 3eab701b10..c7a062de00 100644
--- a/Makefile
+++ b/Makefile
@@ -1844,6 +1844,9 @@ ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
 endif
+ifdef HAVE_WRITEV
+	COMPAT_CFLAGS += -DHAVE_WRITEV
+endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
 endif
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index efc0df760c..0a448e82a7 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -281,7 +281,7 @@ struct expand_data {
 	off_t disk_size;
 	const char *rest;
 	struct object_id delta_base_oid;
-	void *content;
+	struct git_iovec iov[3];
 
 	/*
 	 * If mark_query is true, we do not expand anything, but rather
@@ -379,17 +379,42 @@ static void batch_write(struct batch_options *opt, const void *data, size_t len)
 		write_or_die(1, data, len);
 }
 
-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;
+}
+
+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;
@@ -401,10 +426,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: BUG("FIXME OI_CACHED support not done");
 		case OI_LOOSE:
@@ -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);
 		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);
 			free(contents);
 		} else {
+			batch_write(opt, hdr->buf, hdr->len);
+			if (opt->buffer_output)
+				fflush(stdout);
 			stream_blob(oid);
+			batch_write(opt, &opt->output_delim, 1);
 		}
 	}
 }
@@ -519,12 +547,10 @@ static void batch_object_write(const char *obj_name,
 		strbuf_addch(scratch, opt->output_delim);
 	}
 
-	batch_write(opt, scratch->buf, scratch->len);
-
-	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
-		print_object_or_die(opt, data);
-		batch_write(opt, &opt->output_delim, 1);
-	}
+	if (opt->batch_mode == BATCH_MODE_CONTENTS)
+		print_object_or_die(opt, data, scratch);
+	else
+		batch_write(opt, scratch->buf, scratch->len);
 }
 
 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 = USE_DIRECT_CACHE;
 		}
diff --git a/config.mak.uname b/config.mak.uname
index 85d63821ec..8ce8776657 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -69,6 +69,7 @@ ifeq ($(uname_S),Linux)
 		BASIC_CFLAGS += -std=c99
         endif
 	LINK_FUZZ_PROGRAMS = YesPlease
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
@@ -77,6 +78,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 	LIBC_CONTAINS_LIBINTL = YesPlease
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -292,6 +294,7 @@ ifeq ($(uname_S),FreeBSD)
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 	FILENO_IS_A_MACRO = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
@@ -307,6 +310,7 @@ ifeq ($(uname_S),OpenBSD)
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/file
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 	FILENO_IS_A_MACRO = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),MirBSD)
 	NO_STRCASESTR = YesPlease
@@ -329,6 +333,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	CSPRNG_METHOD = arc4random
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),AIX)
 	DEFAULT_PAGER = more
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 */
+
 #ifndef NO_LIBGEN_H
 #include <libgen.h>
 #else
diff --git a/wrapper.c b/wrapper.c
index f87d90bf57..066c772145 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -262,6 +262,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+#ifdef HAVE_WRITEV
+ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt)
+{
+	while (1) {
+		ssize_t nr = writev(fd, iov, iovcnt);
+
+		if (nr < 0) {
+			if (errno == EINTR)
+				continue;
+			if (handle_nonblock(fd, POLLOUT, errno))
+				continue;
+		}
+
+		return nr;
+	}
+}
+#endif /* !HAVE_WRITEV */
+
 /*
  * xpread() is the same as pread(), but it automatically restarts pread()
  * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES
diff --git a/wrapper.h b/wrapper.h
index 1b2b047ea0..3d33c63d4f 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);
 ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, const struct git_iovec *, int iovcnt);
 ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 int xdup(int fd);
 FILE *xfopen(const char *path, const char *mode);
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));
+	}
+}
+
+/*
+ * note: we don't care about atomicity from writev(2) right now.
+ * The goal is to avoid allocations+copies in the writer and
+ * reduce wakeups+syscalls in the reader.
+ * n.b. @iov is not const since we modify it to avoid allocating
+ * on partial write.
+ */
+#ifdef HAVE_WRITEV
+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)
+{
+	int i;
+
+	while (iovcnt > 0) {
+		ssize_t n = xwritev(fd, iov, iovcnt);
+
+		/* EINVAL happens when sum of iov_len exceeds SSIZE_MAX */
+		if (n < 0 && errno == EINVAL)
+			n = xwrite(fd, iov[0].iov_base, iov[0].iov_len);
+		if (n < 0) {
+			check_pipe(errno);
+			die_errno("writev error");
+		} else if (!n) {
+			errno = ENOSPC;
+			die_errno("writev_error");
+		}
+		/* skip fully written iovs, retry from the first partial iov */
+		for (i = 0; i < iovcnt; i++) {
+			if (n >= iov[i].iov_len) {
+				n -= iov[i].iov_len;
+			} else {
+				iov[i].iov_len -= n;
+				iov[i].iov_base = (char *)iov[i].iov_base + n;
+				break;
+			}
+		}
+		iovcnt -= i;
+		iov += i;
+	}
+}
+#else /* !HAVE_WRITEV */
+
+/*
+ * n.b. don't use stdio fwrite here even if it's faster, @fd may be
+ * non-blocking and stdio isn't equipped for EAGAIN
+ */
+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)
+{
+	int i;
+
+	for (i = 0; i < iovcnt; i++)
+		write_or_die(fd, iov[i].iov_base, iov[i].iov_len);
+}
+#endif /* !HAVE_WRITEV */
diff --git a/write-or-die.h b/write-or-die.h
index 65a5c42a47..20abec211c 100644
--- a/write-or-die.h
+++ b/write-or-die.h
@@ -7,6 +7,8 @@ void fprintf_or_die(FILE *, const char *fmt, ...);
 void fwrite_or_die(FILE *f, const void *buf, size_t count);
 void fflush_or_die(FILE *f);
 void write_or_die(int fd, const void *buf, size_t count);
+void writev_or_die(int fd, struct git_iovec *, int iovcnt);
+void fwritev_or_die(FILE *, const struct git_iovec *, int iovcnt);
 
 /*
  * These values are used to help identify parts of a repository to fsync.

Re: [PATCH v1 00/10] cat-file speedups

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Mon, Jul 15, 2024 at 12:35:09AM +0000, Eric Wong wrote:
> This continues the work of Jeff King and my initial work to
> speed up cat-file --batch(-contents)? users in
> https://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/
> 
> There's more speedups I'm working on, but this series touches
> on the work Jeff and I have already published.
> 
> I've started putting some Perl5 + Inline::C benchmarks with
> several knobs up at: git clone https://80x24.org/misc-git-benchmarks.git
> 
> I've found it necessary to use schedtool(1) on Linux to pin all
> processes to a single CPU on multicore systems.
> 
> Some patches make more sense for largish objects, some for
> smaller objects.  Small objects (several KB) were my main focus,
> but I figure 5/10 could help with some pathological big cases
> and also open the door to expanding the use of caching down the
> line.
> 
> 10/10 actually ended up being more significant than I originally
> anticipated for repeat lookups of the same objects (common for
> web frontends getting hammered).
> 
> Jeff: I started writing commit messages for your patches (1 and
> 2), but there's probably better explanations you could do :>

I definitely think that most of the commit messages could use some
deeper explanations. I had quite a hard time to figure out the idea
behind the commits because the messages only really talk about what they
are doing, but don't mention why they are doing it or why the
transformations are safe.

It might also help with attracting more folks to review this patch
series if things have better explanations :)

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 00/10] cat-file speedups

[Eric Wong]

This continues the work of Jeff King and my initial work to
speed up cat-file --batch(-contents)? users in
https://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/

v1 is here:
https://lore.kernel.org/git/20240715003519.2671385-1-e@80x24.org/T/

v2 changes:

- attempts to improve various commit messages
  (the human language part of my brain has been pretty broken
  for a few years, now :<)
- expand comments around delta_base_cache_lock
- remove DIRECT_CACHE knob since it's always on
- move `else' arm removal in print_object_or_die from 7/10 to
  8/10 to fix t1006 under bisect

I've kept the assert() calls rather than using BUG() since they're
in easily tested code paths and the tests they perform aren't
useful in release builds.  The assertions should remain useful for
future development if we introduce more caching.

Thanks to Patrick for reviewing v1 and Jeff for the
content_limit work.

Eric Wong (8):
  packfile: fix off-by-one in content_limit comparison
  packfile: inline cache_or_unpack_entry
  cat-file: use delta_base_cache entries directly
  packfile: packed_object_info avoids packed_to_object_type
  object_info: content_limit only applies to blobs
  cat-file: batch-command uses content_limit
  cat-file: batch_write: use size_t for length
  cat-file: use writev(2) if available

Jeff King (2):
  packfile: move sizep computation
  packfile: allow content-limit for cat-file

 Makefile            |   3 ++
 builtin/cat-file.c  | 124 +++++++++++++++++++++++++++++++-------------
 config.mak.uname    |   5 ++
 git-compat-util.h   |  10 ++++
 object-file.c       |  12 +++++
 object-store-ll.h   |   9 ++++
 packfile.c          | 122 ++++++++++++++++++++++++++++---------------
 packfile.h          |   4 ++
 t/t1006-cat-file.sh |  19 +++++--
 wrapper.c           |  18 +++++++
 wrapper.h           |   1 +
 write-or-die.c      |  66 +++++++++++++++++++++++
 write-or-die.h      |   2 +
 13 files changed, 312 insertions(+), 83 deletions(-)

Range-diff against v1:
 1:  36b799ab67 !  1:  b4025cee1f packfile: move sizep computation
    @@ Metadata
      ## Commit message ##
         packfile: move sizep computation
     
    -    This makes the next commit to avoid redundant object info
    -    lookups easier to understand.
    +    Moving the sizep computation now makes the next commit to avoid
    +    redundant object info lookups easier to understand.  There is
    +    no user-visible change, here.
     
         [ew: commit message]
     
    @@ Commit message
     
      ## packfile.c ##
     @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,
    - 
    - 	/*
    - 	 * We always get the representation type, but only convert it to
    --	 * a "real" type later if the caller is interested.
    -+	 * a "real" type later if the caller is interested. Likewise...
    -+	 * tbd.
    - 	 */
    - 	if (oi->contentp) {
    - 		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
    -@@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,
      			type = OBJ_BAD;
      	} else {
      		type = unpack_object_header(p, &w_curs, &curpos, &size);
 2:  50f576ab16 !  2:  bdf6f57fae packfile: allow content-limit for cat-file
    @@ Metadata
      ## Commit message ##
         packfile: allow content-limit for cat-file
     
    -    This avoids unnecessary round trips to the object store to speed
    +    Avoid unnecessary round trips to the object store to speed
         up cat-file contents retrievals.  The majority of packed objects
         don't benefit from the streaming interface at all and we end up
         having to load them in core anyways to satisfy our streaming
         API.
     
         This drops the runtime of
    -    `git cat-file --batch-all-objects --unordered --batch' from
    -    ~7.1s to ~6.1s on Jeff's machine.
    +    `git cat-file --batch-all-objects --unordered --batch' on
    +    git.git from ~7.1s to ~6.1s on Jeff's machine.
     
         [ew: commit message]
     
    @@ object-store-ll.h: struct object_info {
     
      ## packfile.c ##
     @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,
    - 	 * a "real" type later if the caller is interested. Likewise...
    - 	 * tbd.
    + 	 * We always get the representation type, but only convert it to
    + 	 * a "real" type later if the caller is interested.
      	 */
     -	if (oi->contentp) {
     +	if (oi->contentp && !oi->content_limit) {
 3:  6eb732401a !  3:  7e762e3481 packfile: fix off-by-one in content_limit comparison
    @@ Commit message
         slurping objects which match loose object handling and slurp
         objects with size matching the content_limit exactly.
     
    +    This change is merely for consistency with the majority of
    +    existing code and there is no user visible change in nearly all
    +    cases.  The only exception being the corner case when the object
    +    size matches content_limit exactly where users will see a
    +    speedup from avoiding an extra lookup.
    +
         Signed-off-by: Eric Wong <e@80x24.org>
     
      ## packfile.c ##
 4:  9476824ac7 !  4:  a558101b85 packfile: inline cache_or_unpack_entry
    @@ Commit message
         packfile: inline cache_or_unpack_entry
     
         We need to check delta_base_cache anyways to fill in the
    -    `whence' field in `struct object_info'.  Inlining
    -    cache_or_unpack_entry() makes it easier to only do the hashmap
    -    lookup once and avoid a redundant lookup later on.
    +    `whence' field in `struct object_info'.  Inlining (and getting
    +    rid of) cache_or_unpack_entry() makes it easier to only do the
    +    hashmap lookup once and avoid a redundant lookup later on.
     
         This code reorganization will also make an optimization to
    -    use the cache entry directly easier to implement.
    +    use the cache entry directly easier to implement in the next
    +    commit.
     
         Signed-off-by: Eric Wong <e@80x24.org>
     
    @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,
      
      	/*
      	 * We always get the representation type, but only convert it to
    - 	 * a "real" type later if the caller is interested. Likewise...
    - 	 * tbd.
    + 	 * a "real" type later if the caller is interested.
      	 */
     -	if (oi->contentp && !oi->content_limit) {
     -		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
 5:  c99dfb84d4 !  5:  74d21ac89d cat-file: use delta_base_cache entries directly
    @@ Commit message
         cat-file: use delta_base_cache entries directly
     
         For objects already in the delta_base_cache, we can safely use
    -    them directly to avoid the malloc+memcpy+free overhead.
    +    one entry at-a-time directly to avoid the malloc+memcpy+free
    +    overhead.  For a 1MB delta base object, this eliminates the
    +    speed penalty of duplicating large objects into memory and
    +    speeds up those 1MB delta base cached content retrievals by
    +    roughly 30%.
     
         While only 2-7% of objects are delta bases in repos I've looked
         at, this avoids up to 96MB of duplicated memory in the worst
    -    case with the default git config.  For a more reasonable 1MB
    -    delta base object, this eliminates the speed penalty of
    -    duplicating large objects into memory and speeds up those 1MB
    -    delta base cached content retrievals by roughly 30%.
    +    case with the default git config.
     
         The new delta_base_cache_lock is a simple single-threaded
    -    assertion to ensure cat-file is the exclusive user of the
    -    delta_base_cache.
    +    assertion to ensure cat-file (and similar) is the exclusive user
    +    of the delta_base_cache.  In other words, we cannot have diff
    +    or similar commands using two or more entries directly from the
    +    delta base cache.  The new lock has nothing to do with parallel
    +    access via multiple threads at the moment.
     
         Signed-off-by: Eric Wong <e@80x24.org>
     
      ## builtin/cat-file.c ##
    -@@
    - #include "promisor-remote.h"
    - #include "mailmap.h"
    - #include "write-or-die.h"
    -+#define USE_DIRECT_CACHE 1
    - 
    - enum batch_mode {
    - 	BATCH_MODE_CONTENTS,
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
      
      	if (data->content) {
      		batch_write(opt, data->content, data->size);
     -		FREE_AND_NULL(data->content);
     +		switch (data->info.whence) {
    -+		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
    ++		case OI_CACHED:
    ++			/*
    ++			 * only blame uses OI_CACHED atm, so it's unlikely
    ++			 * we'll ever hit this path
    ++			 */
    ++			BUG("TODO OI_CACHED support not done");
     +		case OI_LOOSE:
     +		case OI_PACKED:
     +			FREE_AND_NULL(data->content);
     +			break;
     +		case OI_DBCACHED:
    -+			if (USE_DIRECT_CACHE)
    -+				unlock_delta_base_cache();
    -+			else
    -+				FREE_AND_NULL(data->content);
    ++			unlock_delta_base_cache();
     +		}
      	} else if (data->type == OBJ_BLOB) {
      		if (opt->buffer_output)
    @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
      			data.info.sizep = &data.size;
      			data.info.contentp = &data.content;
      			data.info.content_limit = big_file_threshold;
    -+			data.info.direct_cache = USE_DIRECT_CACHE;
    ++			data.info.direct_cache = 1;
      		}
      	}
      
    @@ object-store-ll.h: struct object_info {
      	} whence;
     +
     +	/*
    -+	 * set if caller is able to use OI_DBCACHED entries without copying
    -+	 * TODO OI_CACHED if its use goes beyond blame
    ++	 * Set if caller is able to use OI_DBCACHED entries without copying.
    ++	 * This only applies to OI_DBCACHED entries at the moment,
    ++	 * not OI_CACHED or any other type of entry.
     +	 */
     +	unsigned direct_cache:1;
     +
    @@ packfile.c: static enum object_type packed_to_object_type(struct repository *r,
      static struct hashmap delta_base_cache;
      static size_t delta_base_cached;
      
    -+/* ensures oi->direct_cache is used properly */
    ++/*
    ++ * Ensures only a single object is used at-a-time via oi->direct_cache.
    ++ * Using two objects directly at once (e.g. diff) would cause corruption
    ++ * since populating the cache may invalidate existing entries.
    ++ * This lock has nothing to do with parallelism at the moment.
    ++ */
     +static int delta_base_cache_lock;
     +
      static LIST_HEAD(delta_base_cache_lru);
 6:  79a84221b2 !  6:  83b6367950 packfile: packed_object_info avoids packed_to_object_type
    @@ Metadata
      ## Commit message ##
         packfile: packed_object_info avoids packed_to_object_type
     
    -    For calls the delta base cache, packed_to_object_type calls
    +    For entries in the delta base cache, packed_to_object_type calls
         can be omitted.  This prepares us to bypass content_limit for
         non-blob types in the following commit.
     
 7:  63b36d759d !  7:  7e0f8c0cf6 object_info: content_limit only applies to blobs
    @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
     +					data->type == OBJ_TAG)) {
     +			size_t s = size;
     +
    -+			if (USE_DIRECT_CACHE &&
    -+					data->info.whence == OI_DBCACHED) {
    ++			if (data->info.whence == OI_DBCACHED) {
     +				content = xmemdupz(content, s);
     +				data->info.whence = OI_PACKED;
     +			}
    @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
     +
     +		batch_write(opt, content, size);
      		switch (data->info.whence) {
    - 		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
    + 		case OI_CACHED:
    + 			/*
    +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
    + 			BUG("TODO OI_CACHED support not done");
      		case OI_LOOSE:
      		case OI_PACKED:
     -			FREE_AND_NULL(data->content);
     +			free(content);
      			break;
      		case OI_DBCACHED:
    - 			if (USE_DIRECT_CACHE)
    - 				unlock_delta_base_cache();
    - 			else
    --				FREE_AND_NULL(data->content);
    -+				free(content);
    - 		}
    --	} else if (data->type == OBJ_BLOB) {
    -+	} else {
    -+		assert(data->type == OBJ_BLOB);
    - 		if (opt->buffer_output)
    - 			fflush(stdout);
    - 		if (opt->transform_mode) {
    -@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
    - 			stream_blob(oid);
    - 		}
    - 	}
    --	else {
    --		enum object_type type;
    --		unsigned long size;
    --		void *contents;
    --
    --		contents = repo_read_object_file(the_repository, oid, &type,
    --						 &size);
    --		if (!contents)
    --			die("object %s disappeared", oid_to_hex(oid));
    --
    --		if (use_mailmap) {
    --			size_t s = size;
    --			contents = replace_idents_using_mailmap(contents, &s);
    --			size = cast_size_t_to_ulong(s);
    --		}
    --
    --		if (type != data->type)
    --			die("object %s changed type!?", oid_to_hex(oid));
    --		if (data->info.sizep && size != data->size && !use_mailmap)
    --			die("object %s changed size!?", oid_to_hex(oid));
    --
    --		batch_write(opt, contents, size);
    --		free(contents);
    --	}
    - }
    - 
    - static void print_default_format(struct strbuf *scratch, struct expand_data *data,
    + 			unlock_delta_base_cache();
     
      ## object-file.c ##
     @@ object-file.c: static int loose_object_info(struct repository *r,
    @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,
      				if (!*oi->contentp)
      					type = OBJ_BAD;
      			} else {
    +
    + ## t/t1006-cat-file.sh ##
    +@@ t/t1006-cat-file.sh: test_expect_success 'confirm that neither loose blob is a delta' '
    + 	test_cmp expect actual
    + '
    + 
    ++test_expect_success 'setup delta base tests' '
    ++	foo="$(git rev-parse HEAD:foo)" &&
    ++	foo_plus="$(git rev-parse HEAD:foo-plus)" &&
    ++	git repack -ad
    ++'
    ++
    + # To avoid relying too much on the current delta heuristics,
    + # we will check only that one of the two objects is a delta
    + # against the other, but not the order. We can do so by just
    + # asking for the base of both, and checking whether either
    + # oid appears in the output.
    + test_expect_success '%(deltabase) reports packed delta bases' '
    +-	git repack -ad &&
    + 	git cat-file --batch-check="%(deltabase)" <blobs >actual &&
    + 	{
    +-		grep "$(git rev-parse HEAD:foo)" actual ||
    +-		grep "$(git rev-parse HEAD:foo-plus)" actual
    ++		grep "$foo" actual || grep "$foo_plus" actual
    + 	}
    + '
    + 
    ++test_expect_success 'delta base direct cache use succeeds w/o asserting' '
    ++	commands="info $foo
    ++info $foo_plus
    ++contents $foo_plus
    ++contents $foo" &&
    ++	echo "$commands" >in &&
    ++	git cat-file --batch-command <in >out
    ++'
    ++
    + test_expect_success 'setup bogus data' '
    + 	bogus_short_type="bogus" &&
    + 	bogus_short_content="bogus" &&
 8:  271f6241bd !  8:  ef83e8b426 cat-file: batch-command uses content_limit
    @@ Commit message
         Signed-off-by: Eric Wong <e@80x24.org>
     
      ## builtin/cat-file.c ##
    +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
    + 		case OI_DBCACHED:
    + 			unlock_delta_base_cache();
    + 		}
    +-	} else if (data->type == OBJ_BLOB) {
    ++	} else {
    ++		assert(data->type == OBJ_BLOB);
    + 		if (opt->buffer_output)
    + 			fflush(stdout);
    + 		if (opt->transform_mode) {
    +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
    + 			stream_blob(oid);
    + 		}
    + 	}
    +-	else {
    +-		enum object_type type;
    +-		unsigned long size;
    +-		void *contents;
    +-
    +-		contents = repo_read_object_file(the_repository, oid, &type,
    +-						 &size);
    +-		if (!contents)
    +-			die("object %s disappeared", oid_to_hex(oid));
    +-
    +-		if (use_mailmap) {
    +-			size_t s = size;
    +-			contents = replace_idents_using_mailmap(contents, &s);
    +-			size = cast_size_t_to_ulong(s);
    +-		}
    +-
    +-		if (type != data->type)
    +-			die("object %s changed type!?", oid_to_hex(oid));
    +-		if (data->info.sizep && size != data->size && !use_mailmap)
    +-			die("object %s changed size!?", oid_to_hex(oid));
    +-
    +-		batch_write(opt, contents, size);
    +-		free(contents);
    +-	}
    + }
    + 
    + static void print_default_format(struct strbuf *scratch, struct expand_data *data,
     @@ builtin/cat-file.c: static void parse_cmd_contents(struct batch_options *opt,
      			     struct expand_data *data)
      {
    @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
      		data.info.typep = &data.type;
      		if (!opt->transform_mode) {
      			data.info.sizep = &data.size;
    -
    - ## t/t1006-cat-file.sh ##
    -@@ t/t1006-cat-file.sh: test_expect_success 'confirm that neither loose blob is a delta' '
    - 	test_cmp expect actual
    - '
    - 
    -+test_expect_success 'setup delta base tests' '
    -+	foo="$(git rev-parse HEAD:foo)" &&
    -+	foo_plus="$(git rev-parse HEAD:foo-plus)" &&
    -+	git repack -ad
    -+'
    -+
    - # To avoid relying too much on the current delta heuristics,
    - # we will check only that one of the two objects is a delta
    - # against the other, but not the order. We can do so by just
    - # asking for the base of both, and checking whether either
    - # oid appears in the output.
    - test_expect_success '%(deltabase) reports packed delta bases' '
    --	git repack -ad &&
    - 	git cat-file --batch-check="%(deltabase)" <blobs >actual &&
    - 	{
    --		grep "$(git rev-parse HEAD:foo)" actual ||
    --		grep "$(git rev-parse HEAD:foo-plus)" actual
    -+		grep "$foo" actual || grep "$foo_plus" actual
    - 	}
    - '
    - 
    -+test_expect_success 'delta base direct cache use succeeds w/o asserting' '
    -+	commands="info $foo
    -+info $foo_plus
    -+contents $foo_plus
    -+contents $foo" &&
    -+	echo "$commands" >in &&
    -+	git cat-file --batch-command <in >out
    -+'
    -+
    - test_expect_success 'setup bogus data' '
    - 	bogus_short_type="bogus" &&
    - 	bogus_short_content="bogus" &&
 9:  d91030b69c =  9:  6a94452e54 cat-file: batch_write: use size_t for length
10:  c356b9e1ce ! 10:  1442e43ec7 cat-file: use writev(2) if available
    @@ Metadata
      ## Commit message ##
         cat-file: use writev(2) if available
     
    -    Using writev here is can be 20-40% faster than three write
    -    syscalls in succession for smaller (1-10k) objects in the delta
    -    base cache.  This advantage decreases as object sizes approach
    -    pipe size (64k on Linux).  This reduces wakeups and syscalls on
    -    the read side, as well, especially if the reader is relying on
    -    non-blocking I/O.
    +    Using writev here is 20-40% faster than three write syscalls in
    +    succession for smaller (1-10k) objects in the delta base cache.
    +    This advantage decreases as object sizes approach pipe size (64k
    +    on Linux).
    +
    +    writev reduces wakeups and syscalls on the read side as well:
    +    each write(2) syscall may trigger one or more corresponding
    +    read(2) syscalls in the reader.  Attempting atomicity in the
    +    writer via writev also reduces the likelyhood of non-blocking
    +    readers failing with EAGAIN and having to call poll||select
    +    before attempting to read again.
     
         Unfortunately, this turns into a small (1-3%) slowdown for
         gigantic objects of a megabyte or more even with after
    @@ Commit message
         defaults to being compatible with non-blocking stdout and able
         to poll(2) after hitting EAGAIN on write(2).  Using stdio on
         files with the O_NONBLOCK flag is (AFAIK) unspecified and likely
    -    subject to portability problems.
    +    subject to portability problems and thus avoided.
     
         Signed-off-by: Eric Wong <e@80x24.org>
     
    @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s
     -		batch_write(opt, content, size);
     +		batch_writev(opt, data, hdr, size);
      		switch (data->info.whence) {
    - 		case OI_CACHED: BUG("FIXME OI_CACHED support not done");
    - 		case OI_LOOSE:
    + 		case OI_CACHED:
    + 			/*
     @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d
      		}
      	} else {
    @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)
     -			data.info.contentp = &data.content;
     +			data.info.contentp = &data.iov[1].iov_base;
      			data.info.content_limit = big_file_threshold;
    - 			data.info.direct_cache = USE_DIRECT_CACHE;
    + 			data.info.direct_cache = 1;
      		}
     
      ## config.mak.uname ##

base-commit: a7dae3bdc8b516d36f630b12bb01e853a667e0d9

[PATCH v2 01/10] packfile: move sizep computation

[Eric Wong]

From: Jeff King <peff@peff.net>

Moving the sizep computation now makes the next commit to avoid
redundant object info lookups easier to understand.  There is
no user-visible change, here.

[ew: commit message]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/packfile.c b/packfile.c
index 813584646f..4028763947 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1536,24 +1536,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			type = OBJ_BAD;
 	} else {
 		type = unpack_object_header(p, &w_curs, &curpos, &size);
-	}
 
-	if (!oi->contentp && oi->sizep) {
-		if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
-			off_t tmp_pos = curpos;
-			off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
-							   type, obj_offset);
-			if (!base_offset) {
-				type = OBJ_BAD;
-				goto out;
+		if (oi->sizep) {
+			if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
+				off_t tmp_pos = curpos;
+				off_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,
+								   type, obj_offset);
+				if (!base_offset) {
+					type = OBJ_BAD;
+					goto out;
+				}
+				*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);
+				if (*oi->sizep == 0) {
+					type = OBJ_BAD;
+					goto out;
+				}
+			} else {
+				*oi->sizep = size;
 			}
-			*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);
-			if (*oi->sizep == 0) {
-				type = OBJ_BAD;
-				goto out;
-			}
-		} else {
-			*oi->sizep = size;
 		}
 	}
 

Re: [PATCH v2 01/10] packfile: move sizep computation

[Taylor Blau]

On Fri, Aug 23, 2024 at 10:46:21PM +0000, Eric Wong wrote:
> From: Jeff King <peff@peff.net>
>
> Moving the sizep computation now makes the next commit to avoid
> redundant object info lookups easier to understand.  There is
> no user-visible change, here.
>
> [ew: commit message]
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  packfile.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 813584646f..4028763947 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1536,24 +1536,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  			type = OBJ_BAD;
>  	} else {
>  		type = unpack_object_header(p, &w_curs, &curpos, &size);
> -	}

Omitted from the context here is that the "if" statement which this
"else" belongs to is "if (oi->contentp)", so placing the "if
(oi->sizep)" conditional within the else block is equivalent to the
pre-image as you suggest.

With that additional detail, this patch looks obviously correct to me.

Thanks,
Taylor

[PATCH v2 02/10] packfile: allow content-limit for cat-file

[Eric Wong]

From: Jeff King <peff@peff.net>

Avoid unnecessary round trips to the object store to speed
up cat-file contents retrievals.  The majority of packed objects
don't benefit from the streaming interface at all and we end up
having to load them in core anyways to satisfy our streaming
API.

This drops the runtime of
`git cat-file --batch-all-objects --unordered --batch' on
git.git from ~7.1s to ~6.1s on Jeff's machine.

[ew: commit message]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 17 +++++++++++++++--
 object-file.c      |  6 ++++++
 object-store-ll.h  |  1 +
 packfile.c         | 13 ++++++++++++-
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 18fe58d6b8..bc4bb89610 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -280,6 +280,7 @@ struct expand_data {
 	off_t disk_size;
 	const char *rest;
 	struct object_id delta_base_oid;
+	void *content;
 
 	/*
 	 * If mark_query is true, we do not expand anything, but rather
@@ -383,7 +384,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 
 	assert(data->info.typep);
 
-	if (data->type == OBJ_BLOB) {
+	if (data->content) {
+		batch_write(opt, data->content, data->size);
+		FREE_AND_NULL(data->content);
+	} else if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
 		if (opt->transform_mode) {
@@ -801,9 +805,18 @@ static int batch_objects(struct batch_options *opt)
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
+	 *
+	 * Likewise, grab the content in the initial request if it's small
+	 * and we're not planning to filter it.
 	 */
-	if (opt->batch_mode == BATCH_MODE_CONTENTS)
+	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
 		data.info.typep = &data.type;
+		if (!opt->transform_mode) {
+			data.info.sizep = &data.size;
+			data.info.contentp = &data.content;
+			data.info.content_limit = big_file_threshold;
+		}
+	}
 
 	if (opt->all_objects) {
 		struct object_cb_data cb;
diff --git a/object-file.c b/object-file.c
index 065103be3e..1cc29c3c58 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,
 
 		if (!oi->contentp)
 			break;
+		if (oi->content_limit && *oi->sizep > oi->content_limit) {
+			git_inflate_end(&stream);
+			oi->contentp = NULL;
+			goto cleanup;
+		}
+
 		*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
 		if (*oi->contentp)
 			goto cleanup;
diff --git a/object-store-ll.h b/object-store-ll.h
index c5f2bb2fc2..b71a15f590 100644
--- a/object-store-ll.h
+++ b/object-store-ll.h
@@ -289,6 +289,7 @@ struct object_info {
 	struct object_id *delta_base_oid;
 	struct strbuf *type_name;
 	void **contentp;
+	size_t content_limit;
 
 	/* Response */
 	enum {
diff --git a/packfile.c b/packfile.c
index 4028763947..c12a0515b3 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	 * We always get the representation type, but only convert it to
 	 * a "real" type later if the caller is interested.
 	 */
-	if (oi->contentp) {
+	if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
 						      &type);
 		if (!*oi->contentp)
@@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 				*oi->sizep = size;
 			}
 		}
+
+		if (oi->contentp) {
+			if (oi->sizep && *oi->sizep < oi->content_limit) {
+				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
+								      oi->sizep, &type);
+				if (!*oi->contentp)
+					type = OBJ_BAD;
+			} else {
+				*oi->contentp = NULL;
+			}
+		}
 	}
 
 	if (oi->disk_sizep) {

Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file

[Junio C Hamano]

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

> From: Jeff King <peff@peff.net>
>
> Avoid unnecessary round trips to the object store to speed
> up cat-file contents retrievals.  The majority of packed objects
> don't benefit from the streaming interface at all and we end up
> having to load them in core anyways to satisfy our streaming
> API.

What I found missing from the description is something like ...

    The new trick used is to teach oid_object_info_extended() that a
    non-NULL oi->contentp that means "grab the contents of the objects
    here" can be told to refrain from grabbing an object that is too
    large.

> diff --git a/object-file.c b/object-file.c
> index 065103be3e..1cc29c3c58 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,
>  
>  		if (!oi->contentp)
>  			break;
> +		if (oi->content_limit && *oi->sizep > oi->content_limit) {

I cannot convince myself enough to say "content limit" is a great
name.  It invites "limited by what?  text files are allowed but
images are not?".

> diff --git a/object-store-ll.h b/object-store-ll.h
> index c5f2bb2fc2..b71a15f590 100644
> --- a/object-store-ll.h
> +++ b/object-store-ll.h
> @@ -289,6 +289,7 @@ struct object_info {
>  	struct object_id *delta_base_oid;
>  	struct strbuf *type_name;
>  	void **contentp;
> +	size_t content_limit;
>  
>  	/* Response */
>  	enum {
> diff --git a/packfile.c b/packfile.c
> index 4028763947..c12a0515b3 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	 * We always get the representation type, but only convert it to
>  	 * a "real" type later if the caller is interested.
>  	 */
> -	if (oi->contentp) {
> +	if (oi->contentp && !oi->content_limit) {
>  		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
>  						      &type);
>  		if (!*oi->contentp)
> @@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  				*oi->sizep = size;
>  			}
>  		}
> +
> +		if (oi->contentp) {
> +			if (oi->sizep && *oi->sizep < oi->content_limit) {

It happens that with the current code structure, at this point,
oi->content_limit is _always_ non-zero.  But it felt somewhat
fragile to rely on it, and I would have appreciated if this was
written with an explicit check for oi->content_limit, just like how
it is done in loose_object_info() function.

Other than that, looking very good.

Thanks.


> +				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
> +								      oi->sizep, &type);
> +				if (!*oi->contentp)
> +					type = OBJ_BAD;
> +			} else {
> +				*oi->contentp = NULL;
> +			}
> +		}
>  	}
>  
>  	if (oi->disk_sizep) {

Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > From: Jeff King <peff@peff.net>
> >
> > Avoid unnecessary round trips to the object store to speed
> > up cat-file contents retrievals.  The majority of packed objects
> > don't benefit from the streaming interface at all and we end up
> > having to load them in core anyways to satisfy our streaming
> > API.
> 
> What I found missing from the description is something like ...
> 
>     The new trick used is to teach oid_object_info_extended() that a
>     non-NULL oi->contentp that means "grab the contents of the objects
>     here" can be told to refrain from grabbing an object that is too
>     large.

OK.

> > diff --git a/object-file.c b/object-file.c
> > index 065103be3e..1cc29c3c58 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,
> >  
> >  		if (!oi->contentp)
> >  			break;
> > +		if (oi->content_limit && *oi->sizep > oi->content_limit) {
> 
> I cannot convince myself enough to say "content limit" is a great
> name.  It invites "limited by what?  text files are allowed but
> images are not?".

Hmm... naming is a most difficult problem :<

->slurp_max?  It could be ->content_slurp_max, but I think
that's too long...

Would welcome other suggestions...

> > diff --git a/object-store-ll.h b/object-store-ll.h
> > index c5f2bb2fc2..b71a15f590 100644
> > --- a/object-store-ll.h
> > +++ b/object-store-ll.h
> > @@ -289,6 +289,7 @@ struct object_info {
> >  	struct object_id *delta_base_oid;
> >  	struct strbuf *type_name;
> >  	void **contentp;
> > +	size_t content_limit;
> >  
> >  	/* Response */
> >  	enum {
> > diff --git a/packfile.c b/packfile.c
> > index 4028763947..c12a0515b3 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  	 * We always get the representation type, but only convert it to
> >  	 * a "real" type later if the caller is interested.
> >  	 */
> > -	if (oi->contentp) {
> > +	if (oi->contentp && !oi->content_limit) {
> >  		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
> >  						      &type);
> >  		if (!*oi->contentp)
> > @@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
> >  				*oi->sizep = size;
> >  			}
> >  		}
> > +
> > +		if (oi->contentp) {
> > +			if (oi->sizep && *oi->sizep < oi->content_limit) {
> 
> It happens that with the current code structure, at this point,
> oi->content_limit is _always_ non-zero.  But it felt somewhat
> fragile to rely on it, and I would have appreciated if this was
> written with an explicit check for oi->content_limit, just like how
> it is done in loose_object_info() function.

Right.  I actually think something like:

		assert(oi->content_limit); /* see `if' above */
		if (oi->sizep && *oi->sizep < oi->content_limit) {

is good for documentation purposes since this is in the `else'
branch of the `if (oi->contentp && !oi->content_limit) {' condition.

Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file

[Taylor Blau]

On Tue, Aug 27, 2024 at 08:23:59PM +0000, Eric Wong wrote:
> > > diff --git a/object-file.c b/object-file.c
> > > index 065103be3e..1cc29c3c58 100644
> > > --- a/object-file.c
> > > +++ b/object-file.c
> > > @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,
> > >
> > >  		if (!oi->contentp)
> > >  			break;
> > > +		if (oi->content_limit && *oi->sizep > oi->content_limit) {
> >
> > I cannot convince myself enough to say "content limit" is a great
> > name.  It invites "limited by what?  text files are allowed but
> > images are not?".
>
> Hmm... naming is a most difficult problem :<
>
> ->slurp_max?  It could be ->content_slurp_max, but I think
> that's too long...
>
> Would welcome other suggestions...

I don't have a huge problem with "content_limit" as a name, but perhaps
"content_size_limit", "streaming_limit", or "streaming_threshold" (with
a vague preference towards the latter) might be more descriptive? I
dunno.

Thanks,
Taylor

Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> I don't have a huge problem with "content_limit" as a name, but perhaps
> "content_size_limit", "streaming_limit", or "streaming_threshold" (with
> a vague preference towards the latter) might be more descriptive? I
> dunno.

Your statement does highlight the second point that should have been
pointed out, but I failed to.  The "limit" is about "maximum content
size" (as opposed to content type or color or smell), but also it is
important to avoid sounding like we are forbidding a blob to exist
if it is larger than that limit.  The limit is only about whether
the contents are prefetched through the contentp pointer (and
anything larger than the limit must be obtained via a separate API
call to obtain the contents).

We can flip the polarity to say "minimum content size to stream"
(i.e., anything larger than this threshold will be streamed out
instead of held in core at once), and it may certainly be a better
way to explain what is going on than "maximum content size to be
held in-core via contentp".

Thanks.

[PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison

[Eric Wong]

object-file.c::loose_object_info() accepts objects matching
content_limit exactly, so it follows packfile handling allows
slurping objects which match loose object handling and slurp
objects with size matching the content_limit exactly.

This change is merely for consistency with the majority of
existing code and there is no user visible change in nearly all
cases.  The only exception being the corner case when the object
size matches content_limit exactly where users will see a
speedup from avoiding an extra lookup.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packfile.c b/packfile.c
index c12a0515b3..8ec86d2d69 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1557,7 +1557,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		}
 
 		if (oi->contentp) {
-			if (oi->sizep && *oi->sizep < oi->content_limit) {
+			if (oi->sizep && *oi->sizep <= oi->content_limit) {
 				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
 								      oi->sizep, &type);
 				if (!*oi->contentp)

Re: [PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison

[Junio C Hamano]

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

> object-file.c::loose_object_info() accepts objects matching
> content_limit exactly, so it follows packfile handling allows
> slurping objects which match loose object handling and slurp
> objects with size matching the content_limit exactly.
>
> This change is merely for consistency with the majority of
> existing code and there is no user visible change in nearly all
> cases.  The only exception being the corner case when the object
> size matches content_limit exactly where users will see a
> speedup from avoiding an extra lookup.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---

I would have preferred to see this (and also "is oi->content_limit
zero?" check I mentioned earlier) as part of the previous step,
which added this comparison that is not consistent with the majority
of existing code.  It's not like importing from an external project
we communicate with only occasionally, in which case we may want to
import "pristine" source and fix it up separetly in order to make it
easier to re-import updated material.

>  packfile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/packfile.c b/packfile.c
> index c12a0515b3..8ec86d2d69 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1557,7 +1557,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  		}
>  
>  		if (oi->contentp) {
> -			if (oi->sizep && *oi->sizep < oi->content_limit) {
> +			if (oi->sizep && *oi->sizep <= oi->content_limit) {
>  				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
>  								      oi->sizep, &type);
>  				if (!*oi->contentp)

Re: [PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison

[Taylor Blau]

On Mon, Aug 26, 2024 at 09:55:13AM -0700, Junio C Hamano wrote:
> Eric Wong <e@80x24.org> writes:
>
> > object-file.c::loose_object_info() accepts objects matching
> > content_limit exactly, so it follows packfile handling allows
> > slurping objects which match loose object handling and slurp
> > objects with size matching the content_limit exactly.
> >
> > This change is merely for consistency with the majority of
> > existing code and there is no user visible change in nearly all
> > cases.  The only exception being the corner case when the object
> > size matches content_limit exactly where users will see a
> > speedup from avoiding an extra lookup.
> >
> > Signed-off-by: Eric Wong <e@80x24.org>
> > ---
>
> I would have preferred to see this (and also "is oi->content_limit
> zero?" check I mentioned earlier) as part of the previous step,
> which added this comparison that is not consistent with the majority
> of existing code.  It's not like importing from an external project
> we communicate with only occasionally, in which case we may want to
> import "pristine" source and fix it up separetly in order to make it
> easier to re-import updated material.

Same here. I don't think there is any reason to split this change out
into a separate patch, but I do not feel strongly about it either way.

Thanks,
Taylor

[PATCH v2 04/10] packfile: inline cache_or_unpack_entry

[Eric Wong]

We need to check delta_base_cache anyways to fill in the
`whence' field in `struct object_info'.  Inlining (and getting
rid of) cache_or_unpack_entry() makes it easier to only do the
hashmap lookup once and avoid a redundant lookup later on.

This code reorganization will also make an optimization to
use the cache entry directly easier to implement in the next
commit.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/packfile.c b/packfile.c
index 8ec86d2d69..0a90a5ed67 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1444,23 +1444,6 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
 	free(ent);
 }
 
-static void *cache_or_unpack_entry(struct repository *r, struct packed_git *p,
-				   off_t base_offset, unsigned long *base_size,
-				   enum object_type *type)
-{
-	struct delta_base_cache_entry *ent;
-
-	ent = get_delta_base_cache_entry(p, base_offset);
-	if (!ent)
-		return unpack_entry(r, p, base_offset, type, base_size);
-
-	if (type)
-		*type = ent->type;
-	if (base_size)
-		*base_size = ent->size;
-	return xmemdupz(ent->data, ent->size);
-}
-
 static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 {
 	free(ent->data);
@@ -1521,20 +1504,35 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		       off_t obj_offset, struct object_info *oi)
 {
 	struct pack_window *w_curs = NULL;
-	unsigned long size;
 	off_t curpos = obj_offset;
 	enum object_type type;
+	struct delta_base_cache_entry *ent;
 
 	/*
 	 * We always get the representation type, but only convert it to
 	 * a "real" type later if the caller is interested.
 	 */
-	if (oi->contentp && !oi->content_limit) {
-		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
-						      &type);
+	oi->whence = OI_PACKED;
+	ent = get_delta_base_cache_entry(p, obj_offset);
+	if (ent) {
+		oi->whence = OI_DBCACHED;
+		type = ent->type;
+		if (oi->sizep)
+			*oi->sizep = ent->size;
+		if (oi->contentp) {
+			if (!oi->content_limit ||
+					ent->size <= oi->content_limit)
+				*oi->contentp = xmemdupz(ent->data, ent->size);
+			else
+				*oi->contentp = NULL; /* caller must stream */
+		}
+	} else if (oi->contentp && !oi->content_limit) {
+		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
+						oi->sizep);
 		if (!*oi->contentp)
 			type = OBJ_BAD;
 	} else {
+		unsigned long size;
 		type = unpack_object_header(p, &w_curs, &curpos, &size);
 
 		if (oi->sizep) {
@@ -1558,8 +1556,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 
 		if (oi->contentp) {
 			if (oi->sizep && *oi->sizep <= oi->content_limit) {
-				*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,
-								      oi->sizep, &type);
+				*oi->contentp = unpack_entry(r, p, obj_offset,
+							&type, oi->sizep);
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {
@@ -1608,10 +1606,6 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		} else
 			oidclr(oi->delta_base_oid, the_repository->hash_algo);
 	}
-
-	oi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :
-							  OI_PACKED;
-
 out:
 	unuse_pack(&w_curs);
 	return type;

Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry

[Junio C Hamano]

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

> We need to check delta_base_cache anyways to fill in the
> `whence' field in `struct object_info'.  Inlining (and getting
> rid of) cache_or_unpack_entry() makes it easier to only do the
> hashmap lookup once and avoid a redundant lookup later on.
>
> This code reorganization will also make an optimization to
> use the cache entry directly easier to implement in the next
> commit.

"cache entry" -> "cached entry"; we tend to use "cache entry"
exclusively to mean an entry in the in-core index structure,
and not the cached objects held in the object layer.

>  {
>  	struct pack_window *w_curs = NULL;
> -	unsigned long size;
>  	off_t curpos = obj_offset;
>  	enum object_type type;
> +	struct delta_base_cache_entry *ent;
>  
>  	/*
>  	 * We always get the representation type, but only convert it to
>  	 * a "real" type later if the caller is interested.
>  	 */
> -	if (oi->contentp && !oi->content_limit) {
> -		*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
> -						      &type);
> +	oi->whence = OI_PACKED;
> +	ent = get_delta_base_cache_entry(p, obj_offset);
> +	if (ent) {
> +		oi->whence = OI_DBCACHED;

OK.  This is very straight-forward.  It is packed but if we grabbed
it from the delta-base-cache, that is the only case we know it is
dbcached.

> +		type = ent->type;
> +		if (oi->sizep)
> +			*oi->sizep = ent->size;
> +		if (oi->contentp) {
> +			if (!oi->content_limit ||
> +					ent->size <= oi->content_limit)
> +				*oi->contentp = xmemdupz(ent->data, ent->size);
> +			else
> +				*oi->contentp = NULL; /* caller must stream */

This assignment of NULL is more explicit than the original; is it
because the original assumed that *(oi->contentp) is initialized to
NULL if oi->contentp asks us to give the contents?

> +	} else if (oi->contentp && !oi->content_limit) {
> +		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
> +						oi->sizep);
>  		if (!*oi->contentp)
>  			type = OBJ_BAD;

Nice.  The code structure is still easy to follow, even though the
if/else cascade here are organized differently with more cases (used
to be "are we peeking the contents, or not?"---now it is "do this if
we can grab from the delta base cache, do one of these other things
if we have go to the packfile").

Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > We need to check delta_base_cache anyways to fill in the
> > `whence' field in `struct object_info'.  Inlining (and getting
> > rid of) cache_or_unpack_entry() makes it easier to only do the
> > hashmap lookup once and avoid a redundant lookup later on.
> >
> > This code reorganization will also make an optimization to
> > use the cache entry directly easier to implement in the next
> > commit.
> 
> "cache entry" -> "cached entry"; we tend to use "cache entry"
> exclusively to mean an entry in the in-core index structure,
> and not the cached objects held in the object layer.

OK, will do for v3 (apologies for the delay, Real Life is awful :<).

> > +		type = ent->type;
> > +		if (oi->sizep)
> > +			*oi->sizep = ent->size;
> > +		if (oi->contentp) {
> > +			if (!oi->content_limit ||
> > +					ent->size <= oi->content_limit)
> > +				*oi->contentp = xmemdupz(ent->data, ent->size);
> > +			else
> > +				*oi->contentp = NULL; /* caller must stream */
> 
> This assignment of NULL is more explicit than the original; is it
> because the original assumed that *(oi->contentp) is initialized to
> NULL if oi->contentp asks us to give the contents?

The original code unconditionally assigned cache_or_unpack_entry() result
to *oi->contentp, and there's a bunch of non-cat-file callers which pass
a non-NULL *oi->contentp and expect packed_object_info() to NULL it.

[PATCH v2 05/10] cat-file: use delta_base_cache entries directly

[Eric Wong]

For objects already in the delta_base_cache, we can safely use
one entry at-a-time directly to avoid the malloc+memcpy+free
overhead.  For a 1MB delta base object, this eliminates the
speed penalty of duplicating large objects into memory and
speeds up those 1MB delta base cached content retrievals by
roughly 30%.

While only 2-7% of objects are delta bases in repos I've looked
at, this avoids up to 96MB of duplicated memory in the worst
case with the default git config.

The new delta_base_cache_lock is a simple single-threaded
assertion to ensure cat-file (and similar) is the exclusive user
of the delta_base_cache.  In other words, we cannot have diff
or similar commands using two or more entries directly from the
delta base cache.  The new lock has nothing to do with parallel
access via multiple threads at the moment.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 16 +++++++++++++++-
 object-file.c      |  5 +++++
 object-store-ll.h  |  8 ++++++++
 packfile.c         | 33 ++++++++++++++++++++++++++++++---
 packfile.h         |  4 ++++
 5 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bc4bb89610..8debcdca3e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -386,7 +386,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 
 	if (data->content) {
 		batch_write(opt, data->content, data->size);
-		FREE_AND_NULL(data->content);
+		switch (data->info.whence) {
+		case OI_CACHED:
+			/*
+			 * only blame uses OI_CACHED atm, so it's unlikely
+			 * we'll ever hit this path
+			 */
+			BUG("TODO OI_CACHED support not done");
+		case OI_LOOSE:
+		case OI_PACKED:
+			FREE_AND_NULL(data->content);
+			break;
+		case OI_DBCACHED:
+			unlock_delta_base_cache();
+		}
 	} else if (data->type == OBJ_BLOB) {
 		if (opt->buffer_output)
 			fflush(stdout);
@@ -815,6 +828,7 @@ static int batch_objects(struct batch_options *opt)
 			data.info.sizep = &data.size;
 			data.info.contentp = &data.content;
 			data.info.content_limit = big_file_threshold;
+			data.info.direct_cache = 1;
 		}
 	}
 
diff --git a/object-file.c b/object-file.c
index 1cc29c3c58..19100e823d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,
 			oidclr(oi->delta_base_oid, the_repository->hash_algo);
 		if (oi->type_name)
 			strbuf_addstr(oi->type_name, type_name(co->type));
+		/*
+		 * Currently `blame' is the only command which creates
+		 * OI_CACHED, and direct_cache is only used by `cat-file'.
+		 */
+		assert(!oi->direct_cache);
 		if (oi->contentp)
 			*oi->contentp = xmemdupz(co->buf, co->size);
 		oi->whence = OI_CACHED;
diff --git a/object-store-ll.h b/object-store-ll.h
index b71a15f590..669bb93784 100644
--- a/object-store-ll.h
+++ b/object-store-ll.h
@@ -298,6 +298,14 @@ struct object_info {
 		OI_PACKED,
 		OI_DBCACHED
 	} whence;
+
+	/*
+	 * Set if caller is able to use OI_DBCACHED entries without copying.
+	 * This only applies to OI_DBCACHED entries at the moment,
+	 * not OI_CACHED or any other type of entry.
+	 */
+	unsigned direct_cache:1;
+
 	union {
 		/*
 		 * struct {
diff --git a/packfile.c b/packfile.c
index 0a90a5ed67..40c6c2e387 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1362,6 +1362,14 @@ static enum object_type packed_to_object_type(struct repository *r,
 static struct hashmap delta_base_cache;
 static size_t delta_base_cached;
 
+/*
+ * Ensures only a single object is used at-a-time via oi->direct_cache.
+ * Using two objects directly at once (e.g. diff) would cause corruption
+ * since populating the cache may invalidate existing entries.
+ * This lock has nothing to do with parallelism at the moment.
+ */
+static int delta_base_cache_lock;
+
 static LIST_HEAD(delta_base_cache_lru);
 
 struct delta_base_cache_key {
@@ -1444,6 +1452,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)
 	free(ent);
 }
 
+static void lock_delta_base_cache(void)
+{
+	delta_base_cache_lock++;
+	assert(delta_base_cache_lock == 1);
+}
+
+void unlock_delta_base_cache(void)
+{
+	delta_base_cache_lock--;
+	assert(delta_base_cache_lock == 0);
+}
+
 static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 {
 	free(ent->data);
@@ -1453,6 +1473,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
 void clear_delta_base_cache(void)
 {
 	struct list_head *lru, *tmp;
+	assert(!delta_base_cache_lock);
 	list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
 		struct delta_base_cache_entry *entry =
 			list_entry(lru, struct delta_base_cache_entry, lru);
@@ -1466,6 +1487,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
 	struct delta_base_cache_entry *ent;
 	struct list_head *lru, *tmp;
 
+	assert(!delta_base_cache_lock);
 	/*
 	 * Check required to avoid redundant entries when more than one thread
 	 * is unpacking the same object, in unpack_entry() (since its phases I
@@ -1520,11 +1542,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		if (oi->sizep)
 			*oi->sizep = ent->size;
 		if (oi->contentp) {
-			if (!oi->content_limit ||
-					ent->size <= oi->content_limit)
+			/* ignore content_limit if avoiding copy from cache */
+			if (oi->direct_cache) {
+				lock_delta_base_cache();
+				*oi->contentp = ent->data;
+			} else if (!oi->content_limit ||
+					ent->size <= oi->content_limit) {
 				*oi->contentp = xmemdupz(ent->data, ent->size);
-			else
+			} else {
 				*oi->contentp = NULL; /* caller must stream */
+			}
 		}
 	} else if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
diff --git a/packfile.h b/packfile.h
index eb18ec15db..94941bbe80 100644
--- a/packfile.h
+++ b/packfile.h
@@ -210,4 +210,8 @@ int is_promisor_object(const struct object_id *oid);
 int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
 	     size_t idx_size, struct packed_git *p);
 
+/*
+ * release lock acquired via oi->direct_cache
+ */
+void unlock_delta_base_cache(void);
 #endif

Re: [PATCH v2 05/10] cat-file: use delta_base_cache entries directly

[Junio C Hamano]

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

> For objects already in the delta_base_cache, we can safely use
> one entry at-a-time directly to avoid the malloc+memcpy+free
> overhead.  For a 1MB delta base object, this eliminates the
> speed penalty of duplicating large objects into memory and
> speeds up those 1MB delta base cached content retrievals by
> roughly 30%.
>
> While only 2-7% of objects are delta bases in repos I've looked
> at, this avoids up to 96MB of duplicated memory in the worst
> case with the default git config.
>
> The new delta_base_cache_lock is a simple single-threaded
> assertion to ensure cat-file (and similar) is the exclusive user
> of the delta_base_cache.  In other words, we cannot have diff
> or similar commands using two or more entries directly from the
> delta base cache.  The new lock has nothing to do with parallel
> access via multiple threads at the moment.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  builtin/cat-file.c | 16 +++++++++++++++-
>  object-file.c      |  5 +++++
>  object-store-ll.h  |  8 ++++++++
>  packfile.c         | 33 ++++++++++++++++++++++++++++++---
>  packfile.h         |  4 ++++
>  5 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index bc4bb89610..8debcdca3e 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -386,7 +386,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  
>  	if (data->content) {
>  		batch_write(opt, data->content, data->size);
> -		FREE_AND_NULL(data->content);
> +		switch (data->info.whence) {
> +		case OI_CACHED:
> +			/*
> +			 * only blame uses OI_CACHED atm, so it's unlikely
> +			 * we'll ever hit this path
> +			 */
> +			BUG("TODO OI_CACHED support not done");
> +		case OI_LOOSE:
> +		case OI_PACKED:
> +			FREE_AND_NULL(data->content);
> +			break;
> +		case OI_DBCACHED:
> +			unlock_delta_base_cache();
> +		}
>  	} else if (data->type == OBJ_BLOB) {
>  		if (opt->buffer_output)
>  			fflush(stdout);
> @@ -815,6 +828,7 @@ static int batch_objects(struct batch_options *opt)
>  			data.info.sizep = &data.size;
>  			data.info.contentp = &data.content;
>  			data.info.content_limit = big_file_threshold;
> +			data.info.direct_cache = 1;
>  		}
>  	}
>  
> diff --git a/object-file.c b/object-file.c
> index 1cc29c3c58..19100e823d 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,
>  			oidclr(oi->delta_base_oid, the_repository->hash_algo);
>  		if (oi->type_name)
>  			strbuf_addstr(oi->type_name, type_name(co->type));
> +		/*
> +		 * Currently `blame' is the only command which creates
> +		 * OI_CACHED, and direct_cache is only used by `cat-file'.
> +		 */
> +		assert(!oi->direct_cache);

If "git cat-file" ever gets enhanced to also use
pretend_object_file(), this assumption gets violated.  

What happens then?  Would we return inconsistent answer to the
caller that may result in garbage output, breaking the downstream
process somehow?  If so, I'd prefer to see this guarded more
explicitly with "if (...)  BUG()" than assert() here.

>  		if (oi->contentp)
>  			*oi->contentp = xmemdupz(co->buf, co->size);

Regardless of the answer of the question above about assert(),
shouldn't the step [02/10] of this series also have been made to pay
attention to oi->content_limit mechanism?  It looks inconsistent.

> diff --git a/object-store-ll.h b/object-store-ll.h
> index b71a15f590..669bb93784 100644
> --- a/object-store-ll.h
> +++ b/object-store-ll.h
> @@ -298,6 +298,14 @@ struct object_info {
>  		OI_PACKED,
>  		OI_DBCACHED
>  	} whence;
> +
> +	/*
> +	 * Set if caller is able to use OI_DBCACHED entries without copying.
> +	 * This only applies to OI_DBCACHED entries at the moment,
> +	 * not OI_CACHED or any other type of entry.
> +	 */
> +	unsigned direct_cache:1;

This is a "response" from the API to the caller, but "Set if ..."
sounds as if you are telling the caller what to do.  Would the
caller set this bit when making a request to say "I am cat-file and
I use entry before making any other requests to the object layer so
I am sure object layer will not evict the entry I am using"?  No,
that is not a "response".

You seem to set this bit in batch_objects(), so it does sound like
that the bit is expected to be set by the caller to tell the API
something.  If that is the case, then (1) move it to "request" part
of the object_info structure, and (2) define what it means to be
"able to use ... without copying".  Mechanically, it may mean
"contentp directly points into the delta base cache", but what
implication does it have to callers?  If the caller obtains such a
pointer in .contentp, what is required for its access pattern to
make accessing the pointer safe?  The caller cannot use the pointed
memory after it accesses another object?  What is the definition of
"access" in the context of this discussion?  Does "checking for
existence of an object" count as an access?

> +static void lock_delta_base_cache(void)
> +{
> +	delta_base_cache_lock++;
> +	assert(delta_base_cache_lock == 1);
> +}
> +
> +void unlock_delta_base_cache(void)
> +{
> +	delta_base_cache_lock--;
> +	assert(delta_base_cache_lock == 0);
> +}

I view assert() validating precondition for the control flow to pass
the point in the code path, so the above looks somewhat surprising.

Shouldn't we instead checking the current value first and then only
if the caller is _allowed_ to lock/unlock from that state (i.e. the
lock is not held/the lock is held), increment/decrement the variable?

> +			/* ignore content_limit if avoiding copy from cache */
> +			if (oi->direct_cache) {
> +				lock_delta_base_cache();
> +				*oi->contentp = ent->data;
> +			} else if (!oi->content_limit ||
> +					ent->size <= oi->content_limit) {
>  				*oi->contentp = xmemdupz(ent->data, ent->size);

This somehow looks making the memory ownership rules confusing.

How does the caller know when it can free(oi->contentp) after a call
returns?  If it sets direct_cache and got a non-NULL *oi->contentp
back, is it always pointing at a borrowed piece memory, so that it
does not have to worry about freeing, but if it didn't set
direct_cache, it can be holding an allocated piece of memory and you
are responsible for freeing it?

The following is a tangent that is not necessary to be addressed in
this series, but since I've spent time to think about it already,
let me record it as #leftoverbits here.

Whatever the answers to the earlier questions on the comment on the
direct_cache member are, I wonder if the access pattern of the
caller that satisfies such requirement allows a lot simpler
solution.  Would it give us similar performance boost by (morally)
reverting 85fe35ab (cache_or_unpack_entry: drop keep_cache
parameter, 2016-08-22) to resurrect the "pass ownership of a delta
base cache entry to the caller" feature?  We give the content to the
caller of object_info, and evict the cached delta base.  Ideally, we
shouldn't have to ask the caller to do anything special, other than
just setting contentp to non-NULL (and optionally setting
content_limit), and the choice of copying or passing the ownership
should be made in the object access layer, which knows how big the
object is (e.g., small things may be easier to recreate), how deep
the delta chain the entry in the delta base cache is (e.g., a delta
base object that is simply deflated can be recreated cheaply, while
one based on top of 100-delta deep chain is very expensive to
recreate), etc.

Re: [PATCH v2 05/10] cat-file: use delta_base_cache entries directly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	/*
>> +	 * Set if caller is able to use OI_DBCACHED entries without copying.
>> +	 * This only applies to OI_DBCACHED entries at the moment,
>> +	 * not OI_CACHED or any other type of entry.
>> +	 */
>> +	unsigned direct_cache:1;
> ...
> You seem to set this bit in batch_objects(), so it does sound like
> that the bit is expected to be set by the caller to tell the API
> something.  If that is the case, then (1) move it to "request" part
> of the object_info structure, and (2) define what it means to be
> "able to use ... without copying".  Mechanically, it may mean
> "contentp directly points into the delta base cache", but what
> implication does it have to callers?  If the caller obtains such a
> pointer in .contentp, what is required for its access pattern to
> make accessing the pointer safe?  The caller cannot use the pointed
> memory after it accesses another object?  What is the definition of
> "access" in the context of this discussion?  Does "checking for
> existence of an object" count as an access?

Another thing that makes me worry about this approach (as opposed to
a much simpler to reason about alternative like "transfer ownership
to the caller") is that it is very hard to guarantee that other
object access that is not under caller's control will never happen,
and it is even harder to make sure that the code will keep giving
such a guarantee.

In other words, the arrangement smells a bit too brittle.

For example, would it be possible for a lazy fetching of (unrelated)
objects triggers after the caller asks about an object and borrows a
pointer into delta base cache, and would it scramble the entries of
delta base cache, silently invalidating the borrowed piece of
memory?  Would it be possible for the textconv and other filtering
mechanism driven by the attribute system trigger access to
configured attribute blob, which has to be lazily fetched from other
place?

Thanks.

[PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type

[Eric Wong]

For entries in the delta base cache, packed_to_object_type calls
can be omitted.  This prepares us to bypass content_limit for
non-blob types in the following commit.

Signed-off-by: Eric Wong <e@80x24.org>
---
 packfile.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/packfile.c b/packfile.c
index 40c6c2e387..94d20034e4 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1527,7 +1527,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 {
 	struct pack_window *w_curs = NULL;
 	off_t curpos = obj_offset;
-	enum object_type type;
+	enum object_type type, final_type = OBJ_BAD;
 	struct delta_base_cache_entry *ent;
 
 	/*
@@ -1538,7 +1538,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	ent = get_delta_base_cache_entry(p, obj_offset);
 	if (ent) {
 		oi->whence = OI_DBCACHED;
-		type = ent->type;
+		final_type = type = ent->type;
 		if (oi->sizep)
 			*oi->sizep = ent->size;
 		if (oi->contentp) {
@@ -1556,6 +1556,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	} else if (oi->contentp && !oi->content_limit) {
 		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
 						oi->sizep);
+		final_type = type;
 		if (!*oi->contentp)
 			type = OBJ_BAD;
 	} else {
@@ -1585,6 +1586,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			if (oi->sizep && *oi->sizep <= oi->content_limit) {
 				*oi->contentp = unpack_entry(r, p, obj_offset,
 							&type, oi->sizep);
+				final_type = type;
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {
@@ -1606,17 +1608,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 	}
 
 	if (oi->typep || oi->type_name) {
-		enum object_type ptot;
-		ptot = packed_to_object_type(r, p, obj_offset,
-					     type, &w_curs, curpos);
+		if (final_type < 0)
+			final_type = packed_to_object_type(r, p, obj_offset,
+						     type, &w_curs, curpos);
 		if (oi->typep)
-			*oi->typep = ptot;
+			*oi->typep = final_type;
 		if (oi->type_name) {
-			const char *tn = type_name(ptot);
+			const char *tn = type_name(final_type);
 			if (tn)
 				strbuf_addstr(oi->type_name, tn);
 		}
-		if (ptot < 0) {
+		if (final_type < 0) {
 			type = OBJ_BAD;
 			goto out;
 		}

Re: [PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type

[Junio C Hamano]

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

> Subject: Re: [PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type

I was confused if "avoids" is talking about the status quo, avoiding
is the bad thing, and this patch is about fixing that so that it
would not avoid.  That is not what this does.

    Subject: packfile: skip packed_to_object_type call in packed_object_info

or something, perhaps?

> For entries in the delta base cache, packed_to_object_type calls
> can be omitted.

"... because an entry in delta base cache knows what the final type
of the object is"?

I had "what the code should do in the error cases" question in a few
places.  Please describe (and justify if needed) the choice of
behaviour in the proposed log message.

> @@ -1538,7 +1538,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	ent = get_delta_base_cache_entry(p, obj_offset);
>  	if (ent) {
>  		oi->whence = OI_DBCACHED;
> -		type = ent->type;
> +		final_type = type = ent->type;
>  		if (oi->sizep)
>  			*oi->sizep = ent->size;
>  		if (oi->contentp) {

OK.

> @@ -1556,6 +1556,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	} else if (oi->contentp && !oi->content_limit) {
>  		*oi->contentp = unpack_entry(r, p, obj_offset, &type,
>  						oi->sizep);
> +		final_type = type;
>  		if (!*oi->contentp)
>  			type = OBJ_BAD;

Do we want to still yield the "type" not "OBJ_BAD" in final_type in
this case?

> @@ -1585,6 +1586,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  			if (oi->sizep && *oi->sizep <= oi->content_limit) {
>  				*oi->contentp = unpack_entry(r, p, obj_offset,
>  							&type, oi->sizep);
> +				final_type = type;
>  				if (!*oi->contentp)
>  					type = OBJ_BAD;

Ditto.

> @@ -1606,17 +1608,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,
>  	}
>  
>  	if (oi->typep || oi->type_name) {
> -		enum object_type ptot;
> -		ptot = packed_to_object_type(r, p, obj_offset,
> -					     type, &w_curs, curpos);
> +		if (final_type < 0)
> +			final_type = packed_to_object_type(r, p, obj_offset,
> +						     type, &w_curs, curpos);
>  		if (oi->typep)
> -			*oi->typep = ptot;
> +			*oi->typep = final_type;
>  		if (oi->type_name) {
> -			const char *tn = type_name(ptot);
> +			const char *tn = type_name(final_type);
>  			if (tn)
>  				strbuf_addstr(oi->type_name, tn);
>  		}
> -		if (ptot < 0) {
> +		if (final_type < 0) {
>  			type = OBJ_BAD;
>  			goto out;
>  		}

[PATCH v2 07/10] object_info: content_limit only applies to blobs

[Eric Wong]

Streaming is only supported for blobs, so we'd end up having to
slurp all the other object types into memory regardless.  So
slurp all the non-blob types up front when requesting content
since we always handle them in-core, anyways.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c  | 21 +++++++++++++++++++--
 object-file.c       |  3 ++-
 packfile.c          |  8 +++++---
 t/t1006-cat-file.sh | 19 ++++++++++++++++---
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8debcdca3e..2aedd62324 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -385,7 +385,24 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 	assert(data->info.typep);
 
 	if (data->content) {
-		batch_write(opt, data->content, data->size);
+		void *content = data->content;
+		unsigned long size = data->size;
+
+		data->content = NULL;
+		if (use_mailmap && (data->type == OBJ_COMMIT ||
+					data->type == OBJ_TAG)) {
+			size_t s = size;
+
+			if (data->info.whence == OI_DBCACHED) {
+				content = xmemdupz(content, s);
+				data->info.whence = OI_PACKED;
+			}
+
+			content = replace_idents_using_mailmap(content, &s);
+			size = cast_size_t_to_ulong(s);
+		}
+
+		batch_write(opt, content, size);
 		switch (data->info.whence) {
 		case OI_CACHED:
 			/*
@@ -395,7 +412,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 			BUG("TODO OI_CACHED support not done");
 		case OI_LOOSE:
 		case OI_PACKED:
-			FREE_AND_NULL(data->content);
+			free(content);
 			break;
 		case OI_DBCACHED:
 			unlock_delta_base_cache();
diff --git a/object-file.c b/object-file.c
index 19100e823d..59842cfe1b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,
 
 		if (!oi->contentp)
 			break;
-		if (oi->content_limit && *oi->sizep > oi->content_limit) {
+		if (oi->content_limit && *oi->typep == OBJ_BLOB &&
+				*oi->sizep > oi->content_limit) {
 			git_inflate_end(&stream);
 			oi->contentp = NULL;
 			goto cleanup;
diff --git a/packfile.c b/packfile.c
index 94d20034e4..a592e0b32c 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1546,7 +1546,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 			if (oi->direct_cache) {
 				lock_delta_base_cache();
 				*oi->contentp = ent->data;
-			} else if (!oi->content_limit ||
+			} else if (type != OBJ_BLOB || !oi->content_limit ||
 					ent->size <= oi->content_limit) {
 				*oi->contentp = xmemdupz(ent->data, ent->size);
 			} else {
@@ -1583,10 +1583,12 @@ int packed_object_info(struct repository *r, struct packed_git *p,
 		}
 
 		if (oi->contentp) {
-			if (oi->sizep && *oi->sizep <= oi->content_limit) {
+			final_type = packed_to_object_type(r, p, obj_offset,
+						     type, &w_curs, curpos);
+			if (final_type != OBJ_BLOB || (oi->sizep &&
+					*oi->sizep <= oi->content_limit)) {
 				*oi->contentp = unpack_entry(r, p, obj_offset,
 							&type, oi->sizep);
-				final_type = type;
 				if (!*oi->contentp)
 					type = OBJ_BAD;
 			} else {
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index ff9bf213aa..841e8567e9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -622,20 +622,33 @@ test_expect_success 'confirm that neither loose blob is a delta' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup delta base tests' '
+	foo="$(git rev-parse HEAD:foo)" &&
+	foo_plus="$(git rev-parse HEAD:foo-plus)" &&
+	git repack -ad
+'
+
 # To avoid relying too much on the current delta heuristics,
 # we will check only that one of the two objects is a delta
 # against the other, but not the order. We can do so by just
 # asking for the base of both, and checking whether either
 # oid appears in the output.
 test_expect_success '%(deltabase) reports packed delta bases' '
-	git repack -ad &&
 	git cat-file --batch-check="%(deltabase)" <blobs >actual &&
 	{
-		grep "$(git rev-parse HEAD:foo)" actual ||
-		grep "$(git rev-parse HEAD:foo-plus)" actual
+		grep "$foo" actual || grep "$foo_plus" actual
 	}
 '
 
+test_expect_success 'delta base direct cache use succeeds w/o asserting' '
+	commands="info $foo
+info $foo_plus
+contents $foo_plus
+contents $foo" &&
+	echo "$commands" >in &&
+	git cat-file --batch-command <in >out
+'
+
 test_expect_success 'setup bogus data' '
 	bogus_short_type="bogus" &&
 	bogus_short_content="bogus" &&

Re: [PATCH v2 07/10] object_info: content_limit only applies to blobs

[Junio C Hamano]

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

> Streaming is only supported for blobs, so we'd end up having to
> slurp all the other object types into memory regardless.  So
> slurp all the non-blob types up front when requesting content
> since we always handle them in-core, anyways.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  builtin/cat-file.c  | 21 +++++++++++++++++++--
>  object-file.c       |  3 ++-
>  packfile.c          |  8 +++++---
>  t/t1006-cat-file.sh | 19 ++++++++++++++++---
>  4 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8debcdca3e..2aedd62324 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -385,7 +385,24 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  	assert(data->info.typep);
>  
>  	if (data->content) {
> -		batch_write(opt, data->content, data->size);
> +		void *content = data->content;
> +		unsigned long size = data->size;
> +
> +		data->content = NULL;
> +		if (use_mailmap && (data->type == OBJ_COMMIT ||
> +					data->type == OBJ_TAG)) {

Line-wrap at higher level in the parse tree, i.e.

		if (use_mailmap &&
		    (data->type == OBJ_COMMIT || data->type == OBJ_TAG)) {

> +			size_t s = size;
> +
> +			if (data->info.whence == OI_DBCACHED) {
> +				content = xmemdupz(content, s);
> +				data->info.whence = OI_PACKED;
> +			}
> +
> +			content = replace_idents_using_mailmap(content, &s);
> +			size = cast_size_t_to_ulong(s);

This piece of code does look good, but it makes me wonder where the
need to duplicate the code comes from.  Before this patch, if
somebody asked use_mailmap, we must have been making the
replace_idents_using_mailmap() call and showing the result elsewhere
in the existing code, and it is curious why that code path is not
removed, or if we can share more common code with that code paths
here.

> +		}
> +
> +		batch_write(opt, content, size);
>  		switch (data->info.whence) {
>  		case OI_CACHED:
>  			/*
> @@ -395,7 +412,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  			BUG("TODO OI_CACHED support not done");
>  		case OI_LOOSE:
>  		case OI_PACKED:
> -			FREE_AND_NULL(data->content);
> +			free(content);

data->content has been nuked earlier, and content that holds the new
copy with replaced one is freed (and the original data->content was
freed by replace_idents_using_mailmap(), so there is no leak or
double-free here.  Good.

> diff --git a/object-file.c b/object-file.c
> index 19100e823d..59842cfe1b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,
>  
>  		if (!oi->contentp)
>  			break;
> -		if (oi->content_limit && *oi->sizep > oi->content_limit) {
> +		if (oi->content_limit && *oi->typep == OBJ_BLOB &&
> +				*oi->sizep > oi->content_limit) {
>  			git_inflate_end(&stream);
>  			oi->contentp = NULL;
>  			goto cleanup;

OK, so non-BLOB objects we would fall through this if/else cascade
and inflate them fully.

[PATCH v2 08/10] cat-file: batch-command uses content_limit

[Eric Wong]

As with the normal `--batch' mode, we can use the content_limit
round trip optimization to avoid a redundant lookup.  The only
tricky thing here is we need to enable/disable setting the
object_info.contentp field depending on whether we hit an `info'
or `contents' command.

t1006 is updated to ensure we can switch back and forth between
`info' and `contents' commands without problems.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2aedd62324..067cdbdbf9 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -417,7 +417,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 		case OI_DBCACHED:
 			unlock_delta_base_cache();
 		}
-	} else if (data->type == OBJ_BLOB) {
+	} else {
+		assert(data->type == OBJ_BLOB);
 		if (opt->buffer_output)
 			fflush(stdout);
 		if (opt->transform_mode) {
@@ -452,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 			stream_blob(oid);
 		}
 	}
-	else {
-		enum object_type type;
-		unsigned long size;
-		void *contents;
-
-		contents = repo_read_object_file(the_repository, oid, &type,
-						 &size);
-		if (!contents)
-			die("object %s disappeared", oid_to_hex(oid));
-
-		if (use_mailmap) {
-			size_t s = size;
-			contents = replace_idents_using_mailmap(contents, &s);
-			size = cast_size_t_to_ulong(s);
-		}
-
-		if (type != data->type)
-			die("object %s changed type!?", oid_to_hex(oid));
-		if (data->info.sizep && size != data->size && !use_mailmap)
-			die("object %s changed size!?", oid_to_hex(oid));
-
-		batch_write(opt, contents, size);
-		free(contents);
-	}
 }
 
 static void print_default_format(struct strbuf *scratch, struct expand_data *data,
@@ -689,6 +666,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;
 	batch_one_object(line, output, opt, data);
 }
 
@@ -698,6 +676,7 @@ static void parse_cmd_info(struct batch_options *opt,
 			   struct expand_data *data)
 {
 	opt->batch_mode = BATCH_MODE_INFO;
+	data->info.contentp = NULL;
 	batch_one_object(line, output, opt, data);
 }
 
@@ -839,7 +818,8 @@ static int batch_objects(struct batch_options *opt)
 	 * Likewise, grab the content in the initial request if it's small
 	 * and we're not planning to filter it.
 	 */
-	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
+	if ((opt->batch_mode == BATCH_MODE_CONTENTS) ||
+			(opt->batch_mode == BATCH_MODE_QUEUE_AND_DISPATCH)) {
 		data.info.typep = &data.type;
 		if (!opt->transform_mode) {
 			data.info.sizep = &data.size;

Re: [PATCH v2 08/10] cat-file: batch-command uses content_limit

[Junio C Hamano]

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

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 2aedd62324..067cdbdbf9 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -417,7 +417,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  		case OI_DBCACHED:
>  			unlock_delta_base_cache();
>  		}
> -	} else if (data->type == OBJ_BLOB) {
> +	} else {
> +		assert(data->type == OBJ_BLOB);
>  		if (opt->buffer_output)
>  			fflush(stdout);
>  		if (opt->transform_mode) {

Hmph, because until this step, the control could reach this function
with data->content NULL for a non-BLOB type, even though we
eliminated that with the previous step for "--batch" command user.
This step covers "--batch-command" user to also force the
data->content to be populated, so we no longer will see anything but
BLOB when data->content is NULL.

That sounds correct with the current code, but it somehow feels a
bit too subtle to my taste.  In addition to an assert(), future
readers of this code would deserve a comment describing why we can
safely assume that blobs are the only types we will see here.  That
way, they can tell when they add another command that ends up calling
this function that they too will need to do the contentp thing.

> @@ -452,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  			stream_blob(oid);
>  		}
>  	}
> -	else {
> -		enum object_type type;
> -		unsigned long size;
> -		void *contents;
> -
> -		contents = repo_read_object_file(the_repository, oid, &type,
> -						 &size);
> -		if (!contents)
> -			die("object %s disappeared", oid_to_hex(oid));
> -
> -		if (use_mailmap) {
> -			size_t s = size;
> -			contents = replace_idents_using_mailmap(contents, &s);
> -			size = cast_size_t_to_ulong(s);
> -		}
> -
> -		if (type != data->type)
> -			die("object %s changed type!?", oid_to_hex(oid));
> -		if (data->info.sizep && size != data->size && !use_mailmap)
> -			die("object %s changed size!?", oid_to_hex(oid));
> -
> -		batch_write(opt, contents, size);
> -		free(contents);
> -	}

And it certainly is nice that we can get rid of this fallback code.

By the way, the previous step sshould rename the .content_limit
member to make it clear that (1) it is about the size limit, and (2)
it only applies to blobs.  Ideally (1) should be done much earlier
in the series, and (2) must be done when the code starts to ignore
the member for non-blob types.

Thanks.

[PATCH v2 09/10] cat-file: batch_write: use size_t for length

[Eric Wong]

fwrite(3) and write(2), and all of our wrappers for them use
size_t while object size is `unsigned long', so there's no
excuse to use a potentially smaller representation.

Signed-off-by: Eric Wong <e@80x24.org>
---
 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 067cdbdbf9..bf81054662 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -369,7 +369,7 @@ static void expand_format(struct strbuf *sb, const char *start,
 	}
 }
 
-static void batch_write(struct batch_options *opt, const void *data, int len)
+static void batch_write(struct batch_options *opt, const void *data, size_t len)
 {
 	if (opt->buffer_output) {
 		if (fwrite(data, 1, len, stdout) != len)

Re: [PATCH v2 09/10] cat-file: batch_write: use size_t for length

[Junio C Hamano]

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

> fwrite(3) and write(2), and all of our wrappers for them use
> size_t while object size is `unsigned long', so there's no
> excuse to use a potentially smaller representation.
>
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
>  builtin/cat-file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Obviously correct.  Thanks.

>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 067cdbdbf9..bf81054662 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -369,7 +369,7 @@ static void expand_format(struct strbuf *sb, const char *start,
>  	}
>  }
>  
> -static void batch_write(struct batch_options *opt, const void *data, int len)
> +static void batch_write(struct batch_options *opt, const void *data, size_t len)
>  {
>  	if (opt->buffer_output) {
>  		if (fwrite(data, 1, len, stdout) != len)

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

[Eric Wong]

Using writev here is 20-40% faster than three write syscalls in
succession for smaller (1-10k) objects in the delta base cache.
This advantage decreases as object sizes approach pipe size (64k
on Linux).

writev reduces wakeups and syscalls on the read side as well:
each write(2) syscall may trigger one or more corresponding
read(2) syscalls in the reader.  Attempting atomicity in the
writer via writev also reduces the likelyhood of non-blocking
readers failing with EAGAIN and having to call poll||select
before attempting to read again.

Unfortunately, this turns into a small (1-3%) slowdown for
gigantic objects of a megabyte or more even with after
increasing pipe size to 1MB via the F_SETPIPE_SZ fcntl(2) op.
This slowdown is acceptable to me since the vast majority of
objects are 64K or less for projects I've looked at.

Relying on stdio buffering and fflush(3) after each response was
considered for users without --buffer, but historically cat-file
defaults to being compatible with non-blocking stdout and able
to poll(2) after hitting EAGAIN on write(2).  Using stdio on
files with the O_NONBLOCK flag is (AFAIK) unspecified and likely
subject to portability problems and thus avoided.

Signed-off-by: Eric Wong <e@80x24.org>
---
 Makefile           |  3 +++
 builtin/cat-file.c | 62 ++++++++++++++++++++++++++++++-------------
 config.mak.uname   |  5 ++++
 git-compat-util.h  | 10 +++++++
 wrapper.c          | 18 +++++++++++++
 wrapper.h          |  1 +
 write-or-die.c     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 write-or-die.h     |  2 ++
 8 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index 3eab701b10..c7a062de00 100644
--- a/Makefile
+++ b/Makefile
@@ -1844,6 +1844,9 @@ ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
 endif
+ifdef HAVE_WRITEV
+	COMPAT_CFLAGS += -DHAVE_WRITEV
+endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
 endif
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];
 
 	/*
 	 * If mark_query is true, we do not expand anything, but rather
@@ -378,17 +378,42 @@ static void batch_write(struct batch_options *opt, const void *data, size_t len)
 		write_or_die(1, data, len);
 }
 
-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;
+}
+
+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:
 			/*
@@ -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);
 		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);
 			free(contents);
 		} else {
+			batch_write(opt, hdr->buf, hdr->len);
+			if (opt->buffer_output)
+				fflush(stdout);
 			stream_blob(oid);
+			batch_write(opt, &opt->output_delim, 1);
 		}
 	}
 }
@@ -519,12 +547,10 @@ static void batch_object_write(const char *obj_name,
 		strbuf_addch(scratch, opt->output_delim);
 	}
 
-	batch_write(opt, scratch->buf, scratch->len);
-
-	if (opt->batch_mode == BATCH_MODE_CONTENTS) {
-		print_object_or_die(opt, data);
-		batch_write(opt, &opt->output_delim, 1);
-	}
+	if (opt->batch_mode == BATCH_MODE_CONTENTS)
+		print_object_or_die(opt, data, scratch);
+	else
+		batch_write(opt, scratch->buf, scratch->len);
 }
 
 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;
 		}
diff --git a/config.mak.uname b/config.mak.uname
index 85d63821ec..8ce8776657 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -69,6 +69,7 @@ ifeq ($(uname_S),Linux)
 		BASIC_CFLAGS += -std=c99
         endif
 	LINK_FUZZ_PROGRAMS = YesPlease
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
@@ -77,6 +78,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 	LIBC_CONTAINS_LIBINTL = YesPlease
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -292,6 +294,7 @@ ifeq ($(uname_S),FreeBSD)
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 	FILENO_IS_A_MACRO = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
@@ -307,6 +310,7 @@ ifeq ($(uname_S),OpenBSD)
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/file
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 	FILENO_IS_A_MACRO = UnfortunatelyYes
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),MirBSD)
 	NO_STRCASESTR = YesPlease
@@ -329,6 +333,7 @@ ifeq ($(uname_S),NetBSD)
 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
 	CSPRNG_METHOD = arc4random
 	PROCFS_EXECUTABLE_PATH = /proc/curproc/exe
+	HAVE_WRITEV = YesPlease
 endif
 ifeq ($(uname_S),AIX)
 	DEFAULT_PAGER = more
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 */
+
 #ifndef NO_LIBGEN_H
 #include <libgen.h>
 #else
diff --git a/wrapper.c b/wrapper.c
index f87d90bf57..066c772145 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -262,6 +262,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+#ifdef HAVE_WRITEV
+ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt)
+{
+	while (1) {
+		ssize_t nr = writev(fd, iov, iovcnt);
+
+		if (nr < 0) {
+			if (errno == EINTR)
+				continue;
+			if (handle_nonblock(fd, POLLOUT, errno))
+				continue;
+		}
+
+		return nr;
+	}
+}
+#endif /* !HAVE_WRITEV */
+
 /*
  * xpread() is the same as pread(), but it automatically restarts pread()
  * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES
diff --git a/wrapper.h b/wrapper.h
index 1b2b047ea0..3d33c63d4f 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);
 ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, const struct git_iovec *, int iovcnt);
 ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 int xdup(int fd);
 FILE *xfopen(const char *path, const char *mode);
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));
+	}
+}
+
+/*
+ * note: we don't care about atomicity from writev(2) right now.
+ * The goal is to avoid allocations+copies in the writer and
+ * reduce wakeups+syscalls in the reader.
+ * n.b. @iov is not const since we modify it to avoid allocating
+ * on partial write.
+ */
+#ifdef HAVE_WRITEV
+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)
+{
+	int i;
+
+	while (iovcnt > 0) {
+		ssize_t n = xwritev(fd, iov, iovcnt);
+
+		/* EINVAL happens when sum of iov_len exceeds SSIZE_MAX */
+		if (n < 0 && errno == EINVAL)
+			n = xwrite(fd, iov[0].iov_base, iov[0].iov_len);
+		if (n < 0) {
+			check_pipe(errno);
+			die_errno("writev error");
+		} else if (!n) {
+			errno = ENOSPC;
+			die_errno("writev_error");
+		}
+		/* skip fully written iovs, retry from the first partial iov */
+		for (i = 0; i < iovcnt; i++) {
+			if (n >= iov[i].iov_len) {
+				n -= iov[i].iov_len;
+			} else {
+				iov[i].iov_len -= n;
+				iov[i].iov_base = (char *)iov[i].iov_base + n;
+				break;
+			}
+		}
+		iovcnt -= i;
+		iov += i;
+	}
+}
+#else /* !HAVE_WRITEV */
+
+/*
+ * n.b. don't use stdio fwrite here even if it's faster, @fd may be
+ * non-blocking and stdio isn't equipped for EAGAIN
+ */
+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)
+{
+	int i;
+
+	for (i = 0; i < iovcnt; i++)
+		write_or_die(fd, iov[i].iov_base, iov[i].iov_len);
+}
+#endif /* !HAVE_WRITEV */
diff --git a/write-or-die.h b/write-or-die.h
index 65a5c42a47..20abec211c 100644
--- a/write-or-die.h
+++ b/write-or-die.h
@@ -7,6 +7,8 @@ void fprintf_or_die(FILE *, const char *fmt, ...);
 void fwrite_or_die(FILE *f, const void *buf, size_t count);
 void fflush_or_die(FILE *f);
 void write_or_die(int fd, const void *buf, size_t count);
+void writev_or_die(int fd, struct git_iovec *, int iovcnt);
+void fwritev_or_die(FILE *, const struct git_iovec *, int iovcnt);
 
 /*
  * These values are used to help identify parts of a repository to fsync.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +static void batch_writev(struct batch_options *opt, struct expand_data *data,
>> +			const struct strbuf *hdr, size_t size)
>> +{
>> ...
>> +}
>
> ... 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?

Additional thought.  Perhaps we can introduce

    static void batch_write(struct batch_options *opt,
			    const void *data, ...);

that is a vararg function that takes <data, len> pairs repeated at
the end, with data==NULL as sentinel.  It may technically need to be
called batch_writel(), but that is a backward compatible interface
for existing batch_write() callers.

Then the use of writev() can be encapsulated inside the updated
batch_write() function.  If you get only a single <data, len> pair,
you would do a single write_or_die() or fwrite_or_die().  Otherwise
you'd do the writev() thing, and the function can stay oblivious to
the meaning of what it is writing out.  There is no need for the
function to know that the payload is "header followed by body
followed by delimiter byte", as that is what the callers express at
the call sites of the function.

Hmm?