{"thread":{"id":"61778","subject":"[PATCH v1 01/10] packfile: move sizep computation","startedAt":"2024-07-15T00:35:32Z","lastAt":"2024-10-06T17:40:39Z","messageCount":51,"participants":["Eric Wong","Patrick Steinhardt","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"498696","messageId":"20240715003519.2671385-2-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 01/10] packfile: move sizep computation","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:10Z","receivedAt":"2024-07-15T00:35:32Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Jeff King \n\nThis makes the next commit to avoid redundant object info\nlookups easier to understand.\n\n[ew: commit message]\n\nSigned-off-by: Jeff King \nSigned-off-by: Eric Wong \n---\n packfile.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 813584646f..e547522e3d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1527,7 +1527,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \n \t/*\n \t * We always get the representation type, but only convert it to\n-\t * a \"real\" type later if the caller is interested.\n+\t * a \"real\" type later if the caller is interested. Likewise...\n+\t * tbd.\n \t */\n \tif (oi->contentp) {\n \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n@@ -1536,24 +1537,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\ttype = OBJ_BAD;\n \t} else {\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n-\t}\n \n-\tif (!oi->contentp && oi->sizep) {\n-\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n-\t\t\toff_t tmp_pos = curpos;\n-\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n-\t\t\t\t\t\t\t type, obj_offset);\n-\t\t\tif (!base_offset) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n+\t\tif (oi->sizep) {\n+\t\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n+\t\t\t\toff_t tmp_pos = curpos;\n+\t\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n+\t\t\t\t\t\t\t\t type, obj_offset);\n+\t\t\t\tif (!base_offset) {\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t\t*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n+\t\t\t\tif (*oi->sizep == 0) {\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\t*oi->sizep = size;\n \t\t\t}\n-\t\t\t*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n-\t\t\tif (*oi->sizep == 0) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n-\t\t\t}\n-\t\t} else {\n-\t\t\t*oi->sizep = size;\n \t\t}\n \t}\n \n"},{"id":"498697","messageId":"20240715003519.2671385-3-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 02/10] packfile: allow content-limit for cat-file","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:11Z","receivedAt":"2024-07-15T00:35:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Jeff King \n\nThis avoids unnecessary round trips to the object store to speed\nup cat-file contents retrievals. The majority of packed objects\ndon't benefit from the streaming interface at all and we end up\nhaving to load them in core anyways to satisfy our streaming\nAPI.\n\nThis drops the runtime of\n`git cat-file --batch-all-objects --unordered --batch' from\n~7.1s to ~6.1s on Jeff's machine.\n\n[ew: commit message]\n\nSigned-off-by: Jeff King \nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 17 +++++++++++++++--\n object-file.c | 6 ++++++\n object-store-ll.h | 1 +\n packfile.c | 13 ++++++++++++-\n 4 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8..bc4bb89610 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -280,6 +280,7 @@ struct expand_data {\n \toff_t disk_size;\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n+\tvoid *content;\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -383,7 +384,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \tassert(data->info.typep);\n \n-\tif (data->type == OBJ_BLOB) {\n+\tif (data->content) {\n+\t\tbatch_write(opt, data->content, data->size);\n+\t\tFREE_AND_NULL(data->content);\n+\t} else if (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n@@ -801,9 +805,18 @@ static int batch_objects(struct batch_options *opt)\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\n+\t *\n+\t * Likewise, grab the content in the initial request if it's small\n+\t * and we're not planning to filter it.\n \t */\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS)\n+\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n \t\tdata.info.typep = &data.type;\n+\t\tif (!opt->transform_mode) {\n+\t\t\tdata.info.sizep = &data.size;\n+\t\t\tdata.info.contentp = &data.content;\n+\t\t\tdata.info.content_limit = big_file_threshold;\n+\t\t}\n+\t}\n \n \tif (opt->all_objects) {\n \t\tstruct object_cb_data cb;\ndiff --git a/object-file.c b/object-file.c\nindex 065103be3e..1cc29c3c58 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,\n \n \t\tif (!oi->contentp)\n \t\t\tbreak;\n+\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n+\t\t\tgit_inflate_end(&stream);\n+\t\t\toi->contentp = NULL;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\n \t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n \t\tif (*oi->contentp)\n \t\t\tgoto cleanup;\ndiff --git a/object-store-ll.h b/object-store-ll.h\nindex c5f2bb2fc2..b71a15f590 100644\n--- a/object-store-ll.h\n+++ b/object-store-ll.h\n@@ -289,6 +289,7 @@ struct object_info {\n \tstruct object_id *delta_base_oid;\n \tstruct strbuf *type_name;\n \tvoid **contentp;\n+\tsize_t content_limit;\n \n \t/* Response */\n \tenum {\ndiff --git a/packfile.c b/packfile.c\nindex e547522e3d..54b9d46928 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1530,7 +1530,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t * a \"real\" type later if the caller is interested. Likewise...\n \t * tbd.\n \t */\n-\tif (oi->contentp) {\n+\tif (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n \t\t\t\t\t\t &type);\n \t\tif (!*oi->contentp)\n@@ -1556,6 +1556,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\t\t*oi->sizep = size;\n \t\t\t}\n \t\t}\n+\n+\t\tif (oi->contentp) {\n+\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n+\t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n+\t\t\t\t\t\t\t\t oi->sizep, &type);\n+\t\t\t\tif (!*oi->contentp)\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t} else {\n+\t\t\t\t*oi->contentp = NULL;\n+\t\t\t}\n+\t\t}\n \t}\n \n \tif (oi->disk_sizep) {\n"},{"id":"498698","messageId":"20240715003519.2671385-4-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:12Z","receivedAt":"2024-07-15T00:35:46Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"object-file.c::loose_object_info() accepts objects matching\ncontent_limit exactly, so it follows packfile handling allows\nslurping objects which match loose object handling and slurp\nobjects with size matching the content_limit exactly.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 54b9d46928..371da96cdb 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1558,7 +1558,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t}\n \n \t\tif (oi->contentp) {\n-\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n+\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n \t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t\t oi->sizep, &type);\n \t\t\t\tif (!*oi->contentp)\n"},{"id":"498699","messageId":"20240715003519.2671385-5-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 04/10] packfile: inline cache_or_unpack_entry","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:13Z","receivedAt":"2024-07-15T00:35:53Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"We need to check delta_base_cache anyways to fill in the\n`whence' field in `struct object_info'. Inlining\ncache_or_unpack_entry() makes it easier to only do the hashmap\nlookup once and avoid a redundant lookup later on.\n\nThis code reorganization will also make an optimization to\nuse the cache entry directly easier to implement.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 48 +++++++++++++++++++++---------------------------\n 1 file changed, 21 insertions(+), 27 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 371da96cdb..1a409ec142 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1444,23 +1444,6 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)\n \tfree(ent);\n }\n \n-static void *cache_or_unpack_entry(struct repository *r, struct packed_git *p,\n-\t\t\t\t off_t base_offset, unsigned long *base_size,\n-\t\t\t\t enum object_type *type)\n-{\n-\tstruct delta_base_cache_entry *ent;\n-\n-\tent = get_delta_base_cache_entry(p, base_offset);\n-\tif (!ent)\n-\t\treturn unpack_entry(r, p, base_offset, type, base_size);\n-\n-\tif (type)\n-\t\t*type = ent->type;\n-\tif (base_size)\n-\t\t*base_size = ent->size;\n-\treturn xmemdupz(ent->data, ent->size);\n-}\n-\n static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n {\n \tfree(ent->data);\n@@ -1521,21 +1504,36 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t off_t obj_offset, struct object_info *oi)\n {\n \tstruct pack_window *w_curs = NULL;\n-\tunsigned long size;\n \toff_t curpos = obj_offset;\n \tenum object_type type;\n+\tstruct delta_base_cache_entry *ent;\n \n \t/*\n \t * We always get the representation type, but only convert it to\n \t * a \"real\" type later if the caller is interested. Likewise...\n \t * tbd.\n \t */\n-\tif (oi->contentp && !oi->content_limit) {\n-\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n-\t\t\t\t\t\t &type);\n+\toi->whence = OI_PACKED;\n+\tent = get_delta_base_cache_entry(p, obj_offset);\n+\tif (ent) {\n+\t\toi->whence = OI_DBCACHED;\n+\t\ttype = ent->type;\n+\t\tif (oi->sizep)\n+\t\t\t*oi->sizep = ent->size;\n+\t\tif (oi->contentp) {\n+\t\t\tif (!oi->content_limit ||\n+\t\t\t\t\tent->size <= oi->content_limit)\n+\t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n+\t\t\telse\n+\t\t\t\t*oi->contentp = NULL; /* caller must stream */\n+\t\t}\n+\t} else if (oi->contentp && !oi->content_limit) {\n+\t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n+\t\t\t\t\t\toi->sizep);\n \t\tif (!*oi->contentp)\n \t\t\ttype = OBJ_BAD;\n \t} else {\n+\t\tunsigned long size;\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n \n \t\tif (oi->sizep) {\n@@ -1559,8 +1557,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \n \t\tif (oi->contentp) {\n \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n-\t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n-\t\t\t\t\t\t\t\t oi->sizep, &type);\n+\t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n+\t\t\t\t\t\t\t&type, oi->sizep);\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n@@ -1609,10 +1607,6 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t} else\n \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n \t}\n-\n-\toi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :\n-\t\t\t\t\t\t\t OI_PACKED;\n-\n out:\n \tunuse_pack(&w_curs);\n \treturn type;\n"},{"id":"498700","messageId":"20240715003519.2671385-6-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 05/10] cat-file: use delta_base_cache entries directly","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:14Z","receivedAt":"2024-07-15T00:36:00Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"For objects already in the delta_base_cache, we can safely use\nthem directly to avoid the malloc+memcpy+free overhead.\n\nWhile only 2-7% of objects are delta bases in repos I've looked\nat, this avoids up to 96MB of duplicated memory in the worst\ncase with the default git config. For a more reasonable 1MB\ndelta base object, this eliminates the speed penalty of\nduplicating large objects into memory and speeds up those 1MB\ndelta base cached content retrievals by roughly 30%.\n\nThe new delta_base_cache_lock is a simple single-threaded\nassertion to ensure cat-file is the exclusive user of the\ndelta_base_cache.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 15 ++++++++++++++-\n object-file.c | 5 +++++\n object-store-ll.h | 7 +++++++\n packfile.c | 28 +++++++++++++++++++++++++---\n packfile.h | 4 ++++\n 5 files changed, 55 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex bc4bb89610..769c8b48d2 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -24,6 +24,7 @@\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n #include \"write-or-die.h\"\n+#define USE_DIRECT_CACHE 1\n \n enum batch_mode {\n \tBATCH_MODE_CONTENTS,\n@@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \tif (data->content) {\n \t\tbatch_write(opt, data->content, data->size);\n-\t\tFREE_AND_NULL(data->content);\n+\t\tswitch (data->info.whence) {\n+\t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n+\t\tcase OI_LOOSE:\n+\t\tcase OI_PACKED:\n+\t\t\tFREE_AND_NULL(data->content);\n+\t\t\tbreak;\n+\t\tcase OI_DBCACHED:\n+\t\t\tif (USE_DIRECT_CACHE)\n+\t\t\t\tunlock_delta_base_cache();\n+\t\t\telse\n+\t\t\t\tFREE_AND_NULL(data->content);\n+\t\t}\n \t} else if (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n@@ -815,6 +827,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.info.sizep = &data.size;\n \t\t\tdata.info.contentp = &data.content;\n \t\t\tdata.info.content_limit = big_file_threshold;\n+\t\t\tdata.info.direct_cache = USE_DIRECT_CACHE;\n \t\t}\n \t}\n \ndiff --git a/object-file.c b/object-file.c\nindex 1cc29c3c58..19100e823d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n \t\tif (oi->type_name)\n \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n+\t\t/*\n+\t\t * Currently `blame' is the only command which creates\n+\t\t * OI_CACHED, and direct_cache is only used by `cat-file'.\n+\t\t */\n+\t\tassert(!oi->direct_cache);\n \t\tif (oi->contentp)\n \t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n \t\toi->whence = OI_CACHED;\ndiff --git a/object-store-ll.h b/object-store-ll.h\nindex b71a15f590..50c5219308 100644\n--- a/object-store-ll.h\n+++ b/object-store-ll.h\n@@ -298,6 +298,13 @@ struct object_info {\n \t\tOI_PACKED,\n \t\tOI_DBCACHED\n \t} whence;\n+\n+\t/*\n+\t * set if caller is able to use OI_DBCACHED entries without copying\n+\t * TODO OI_CACHED if its use goes beyond blame\n+\t */\n+\tunsigned direct_cache:1;\n+\n \tunion {\n \t\t/*\n \t\t * struct {\ndiff --git a/packfile.c b/packfile.c\nindex 1a409ec142..b2660e14f9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,\n static struct hashmap delta_base_cache;\n static size_t delta_base_cached;\n \n+/* ensures oi->direct_cache is used properly */\n+static int delta_base_cache_lock;\n+\n static LIST_HEAD(delta_base_cache_lru);\n \n struct delta_base_cache_key {\n@@ -1444,6 +1447,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)\n \tfree(ent);\n }\n \n+static void lock_delta_base_cache(void)\n+{\n+\tdelta_base_cache_lock++;\n+\tassert(delta_base_cache_lock == 1);\n+}\n+\n+void unlock_delta_base_cache(void)\n+{\n+\tdelta_base_cache_lock--;\n+\tassert(delta_base_cache_lock == 0);\n+}\n+\n static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n {\n \tfree(ent->data);\n@@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n void clear_delta_base_cache(void)\n {\n \tstruct list_head *lru, *tmp;\n+\tassert(!delta_base_cache_lock);\n \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n \t\tstruct delta_base_cache_entry *entry =\n \t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n@@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tstruct delta_base_cache_entry *ent;\n \tstruct list_head *lru, *tmp;\n \n+\tassert(!delta_base_cache_lock);\n \t/*\n \t * Check required to avoid redundant entries when more than one thread\n \t * is unpacking the same object, in unpack_entry() (since its phases I\n@@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\tif (oi->sizep)\n \t\t\t*oi->sizep = ent->size;\n \t\tif (oi->contentp) {\n-\t\t\tif (!oi->content_limit ||\n-\t\t\t\t\tent->size <= oi->content_limit)\n+\t\t\t/* ignore content_limit if avoiding copy from cache */\n+\t\t\tif (oi->direct_cache) {\n+\t\t\t\tlock_delta_base_cache();\n+\t\t\t\t*oi->contentp = ent->data;\n+\t\t\t} else if (!oi->content_limit ||\n+\t\t\t\t\tent->size <= oi->content_limit) {\n \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n-\t\t\telse\n+\t\t\t} else {\n \t\t\t\t*oi->contentp = NULL; /* caller must stream */\n+\t\t\t}\n \t\t}\n \t} else if (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\ndiff --git a/packfile.h b/packfile.h\nindex eb18ec15db..94941bbe80 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -210,4 +210,8 @@ int is_promisor_object(const struct object_id *oid);\n int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t size_t idx_size, struct packed_git *p);\n \n+/*\n+ * release lock acquired via oi->direct_cache\n+ */\n+void unlock_delta_base_cache(void);\n #endif\n"},{"id":"498701","messageId":"20240715003519.2671385-7-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:15Z","receivedAt":"2024-07-15T00:36:07Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"For calls the delta base cache, packed_to_object_type calls\ncan be omitted. This prepares us to bypass content_limit for\nnon-blob types in the following commit.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex b2660e14f9..c2ba6ab203 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n {\n \tstruct pack_window *w_curs = NULL;\n \toff_t curpos = obj_offset;\n-\tenum object_type type;\n+\tenum object_type type, final_type = OBJ_BAD;\n \tstruct delta_base_cache_entry *ent;\n \n \t/*\n@@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \tent = get_delta_base_cache_entry(p, obj_offset);\n \tif (ent) {\n \t\toi->whence = OI_DBCACHED;\n-\t\ttype = ent->type;\n+\t\tfinal_type = type = ent->type;\n \t\tif (oi->sizep)\n \t\t\t*oi->sizep = ent->size;\n \t\tif (oi->contentp) {\n@@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t} else if (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n \t\t\t\t\t\toi->sizep);\n+\t\tfinal_type = type;\n \t\tif (!*oi->contentp)\n \t\t\ttype = OBJ_BAD;\n \t} else {\n@@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t&type, oi->sizep);\n+\t\t\t\tfinal_type = type;\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n@@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t}\n \n \tif (oi->typep || oi->type_name) {\n-\t\tenum object_type ptot;\n-\t\tptot = packed_to_object_type(r, p, obj_offset,\n-\t\t\t\t\t type, &w_curs, curpos);\n+\t\tif (final_type < 0)\n+\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n+\t\t\t\t\t\t type, &w_curs, curpos);\n \t\tif (oi->typep)\n-\t\t\t*oi->typep = ptot;\n+\t\t\t*oi->typep = final_type;\n \t\tif (oi->type_name) {\n-\t\t\tconst char *tn = type_name(ptot);\n+\t\t\tconst char *tn = type_name(final_type);\n \t\t\tif (tn)\n \t\t\t\tstrbuf_addstr(oi->type_name, tn);\n \t\t}\n-\t\tif (ptot < 0) {\n+\t\tif (final_type < 0) {\n \t\t\ttype = OBJ_BAD;\n \t\t\tgoto out;\n \t\t}\n"},{"id":"498702","messageId":"20240715003519.2671385-8-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 07/10] object_info: content_limit only applies to blobs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:16Z","receivedAt":"2024-07-15T00:36:15Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Streaming is only supported for blobs, so we'd end up having to\nslurp all the other object types into memory regardless. So\nslurp all the non-blob types up front when requesting content\nsince we always handle them in-core, anyways.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 51 +++++++++++++++++++++-------------------------\n object-file.c | 3 ++-\n packfile.c | 8 +++++---\n 3 files changed, 30 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 769c8b48d2..0752ff7a74 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -386,20 +386,39 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \tassert(data->info.typep);\n \n \tif (data->content) {\n-\t\tbatch_write(opt, data->content, data->size);\n+\t\tvoid *content = data->content;\n+\t\tunsigned long size = data->size;\n+\n+\t\tdata->content = NULL;\n+\t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n+\t\t\t\t\tdata->type == OBJ_TAG)) {\n+\t\t\tsize_t s = size;\n+\n+\t\t\tif (USE_DIRECT_CACHE &&\n+\t\t\t\t\tdata->info.whence == OI_DBCACHED) {\n+\t\t\t\tcontent = xmemdupz(content, s);\n+\t\t\t\tdata->info.whence = OI_PACKED;\n+\t\t\t}\n+\n+\t\t\tcontent = replace_idents_using_mailmap(content, &s);\n+\t\t\tsize = cast_size_t_to_ulong(s);\n+\t\t}\n+\n+\t\tbatch_write(opt, content, size);\n \t\tswitch (data->info.whence) {\n \t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n \t\tcase OI_LOOSE:\n \t\tcase OI_PACKED:\n-\t\t\tFREE_AND_NULL(data->content);\n+\t\t\tfree(content);\n \t\t\tbreak;\n \t\tcase OI_DBCACHED:\n \t\t\tif (USE_DIRECT_CACHE)\n \t\t\t\tunlock_delta_base_cache();\n \t\t\telse\n-\t\t\t\tFREE_AND_NULL(data->content);\n+\t\t\t\tfree(content);\n \t\t}\n-\t} else if (data->type == OBJ_BLOB) {\n+\t} else {\n+\t\tassert(data->type == OBJ_BLOB);\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n@@ -434,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tstream_blob(oid);\n \t\t}\n \t}\n-\telse {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\t\tvoid *contents;\n-\n-\t\tcontents = repo_read_object_file(the_repository, oid, &type,\n-\t\t\t\t\t\t &size);\n-\t\tif (!contents)\n-\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n-\n-\t\tif (use_mailmap) {\n-\t\t\tsize_t s = size;\n-\t\t\tcontents = replace_idents_using_mailmap(contents, &s);\n-\t\t\tsize = cast_size_t_to_ulong(s);\n-\t\t}\n-\n-\t\tif (type != data->type)\n-\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n-\t\tif (data->info.sizep && size != data->size && !use_mailmap)\n-\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n-\n-\t\tbatch_write(opt, contents, size);\n-\t\tfree(contents);\n-\t}\n }\n \n static void print_default_format(struct strbuf *scratch, struct expand_data *data,\ndiff --git a/object-file.c b/object-file.c\nindex 19100e823d..59842cfe1b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,\n \n \t\tif (!oi->contentp)\n \t\t\tbreak;\n-\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n+\t\tif (oi->content_limit && *oi->typep == OBJ_BLOB &&\n+\t\t\t\t*oi->sizep > oi->content_limit) {\n \t\t\tgit_inflate_end(&stream);\n \t\t\toi->contentp = NULL;\n \t\t\tgoto cleanup;\ndiff --git a/packfile.c b/packfile.c\nindex c2ba6ab203..01ce3a49db 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1542,7 +1542,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\tif (oi->direct_cache) {\n \t\t\t\tlock_delta_base_cache();\n \t\t\t\t*oi->contentp = ent->data;\n-\t\t\t} else if (!oi->content_limit ||\n+\t\t\t} else if (type != OBJ_BLOB || !oi->content_limit ||\n \t\t\t\t\tent->size <= oi->content_limit) {\n \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n \t\t\t} else {\n@@ -1579,10 +1579,12 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t}\n \n \t\tif (oi->contentp) {\n-\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n+\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n+\t\t\t\t\t\t type, &w_curs, curpos);\n+\t\t\tif (final_type != OBJ_BLOB || (oi->sizep &&\n+\t\t\t\t\t*oi->sizep <= oi->content_limit)) {\n \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t&type, oi->sizep);\n-\t\t\t\tfinal_type = type;\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n"},{"id":"498703","messageId":"20240715003519.2671385-9-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 08/10] cat-file: batch-command uses content_limit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:17Z","receivedAt":"2024-07-15T00:36:22Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"As with the normal `--batch' mode, we can use the content_limit\nround trip optimization to avoid a redundant lookup. The only\ntricky thing here is we need to enable/disable setting the\nobject_info.contentp field depending on whether we hit an `info'\nor `contents' command.\n\nt1006 is updated to ensure we can switch back and forth between\n`info' and `contents' commands without problems.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 5 ++++-\n t/t1006-cat-file.sh | 19 ++++++++++++++++---\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0752ff7a74..c4c28236db 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -666,6 +666,7 @@ static void parse_cmd_contents(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_CONTENTS;\n+\tdata->info.contentp = &data->content;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -675,6 +676,7 @@ static void parse_cmd_info(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_INFO;\n+\tdata->info.contentp = NULL;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -816,7 +818,8 @@ static int batch_objects(struct batch_options *opt)\n \t * Likewise, grab the content in the initial request if it's small\n \t * and we're not planning to filter it.\n \t */\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n+\tif ((opt->batch_mode == BATCH_MODE_CONTENTS) ||\n+\t\t\t(opt->batch_mode == BATCH_MODE_QUEUE_AND_DISPATCH)) {\n \t\tdata.info.typep = &data.type;\n \t\tif (!opt->transform_mode) {\n \t\t\tdata.info.sizep = &data.size;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ff9bf213aa..841e8567e9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -622,20 +622,33 @@ test_expect_success 'confirm that neither loose blob is a delta' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup delta base tests' '\n+\tfoo=\"$(git rev-parse HEAD:foo)\" &&\n+\tfoo_plus=\"$(git rev-parse HEAD:foo-plus)\" &&\n+\tgit repack -ad\n+'\n+\n # To avoid relying too much on the current delta heuristics,\n # we will check only that one of the two objects is a delta\n # against the other, but not the order. We can do so by just\n # asking for the base of both, and checking whether either\n # oid appears in the output.\n test_expect_success '%(deltabase) reports packed delta bases' '\n-\tgit repack -ad &&\n \tgit cat-file --batch-check=\"%(deltabase)\" actual &&\n \t{\n-\t\tgrep \"$(git rev-parse HEAD:foo)\" actual ||\n-\t\tgrep \"$(git rev-parse HEAD:foo-plus)\" actual\n+\t\tgrep \"$foo\" actual || grep \"$foo_plus\" actual\n \t}\n '\n \n+test_expect_success 'delta base direct cache use succeeds w/o asserting' '\n+\tcommands=\"info $foo\n+info $foo_plus\n+contents $foo_plus\n+contents $foo\" &&\n+\techo \"$commands\" >in &&\n+\tgit cat-file --batch-command out\n+'\n+\n test_expect_success 'setup bogus data' '\n \tbogus_short_type=\"bogus\" &&\n \tbogus_short_content=\"bogus\" &&\n"},{"id":"498704","messageId":"20240715003519.2671385-10-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 09/10] cat-file: batch_write: use size_t for length","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:18Z","receivedAt":"2024-07-15T00:36:28Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"fwrite(3) and write(2), and all of our wrappers for them use\nsize_t while object size is `unsigned long', so there's no\nexcuse to use a potentially smaller representation.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex c4c28236db..efc0df760c 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -370,7 +370,7 @@ static void expand_format(struct strbuf *sb, const char *start,\n \t}\n }\n \n-static void batch_write(struct batch_options *opt, const void *data, int len)\n+static void batch_write(struct batch_options *opt, const void *data, size_t len)\n {\n \tif (opt->buffer_output) {\n \t\tif (fwrite(data, 1, len, stdout) != len)\n"},{"id":"498705","messageId":"20240715003519.2671385-11-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v1 10/10] cat-file: use writev(2) if available","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:19Z","receivedAt":"2024-07-15T00:36:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Using writev here is can be 20-40% faster than three write\nsyscalls in succession for smaller (1-10k) objects in the delta\nbase cache. This advantage decreases as object sizes approach\npipe size (64k on Linux). This reduces wakeups and syscalls on\nthe read side, as well, especially if the reader is relying on\nnon-blocking I/O.\n\nUnfortunately, this turns into a small (1-3%) slowdown for\ngigantic objects of a megabyte or more even with after\nincreasing pipe size to 1MB via the F_SETPIPE_SZ fcntl(2) op.\nThis slowdown is acceptable to me since the vast majority of\nobjects are 64K or less for projects I've looked at.\n\nRelying on stdio buffering and fflush(3) after each response was\nconsidered for users without --buffer, but historically cat-file\ndefaults to being compatible with non-blocking stdout and able\nto poll(2) after hitting EAGAIN on write(2). Using stdio on\nfiles with the O_NONBLOCK flag is (AFAIK) unspecified and likely\nsubject to portability problems.\n\nSigned-off-by: Eric Wong \n---\n Makefile | 3 +++\n builtin/cat-file.c | 62 ++++++++++++++++++++++++++++++-------------\n config.mak.uname | 5 ++++\n git-compat-util.h | 10 +++++++\n wrapper.c | 18 +++++++++++++\n wrapper.h | 1 +\n write-or-die.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n write-or-die.h | 2 ++\n 8 files changed, 149 insertions(+), 18 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3eab701b10..c7a062de00 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1844,6 +1844,9 @@ ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n endif\n+ifdef HAVE_WRITEV\n+\tCOMPAT_CFLAGS += -DHAVE_WRITEV\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex efc0df760c..0a448e82a7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -281,7 +281,7 @@ struct expand_data {\n \toff_t disk_size;\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n-\tvoid *content;\n+\tstruct git_iovec iov[3];\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -379,17 +379,42 @@ static void batch_write(struct batch_options *opt, const void *data, size_t len)\n \t\twrite_or_die(1, data, len);\n }\n \n-static void print_object_or_die(struct batch_options *opt, struct expand_data *data)\n+static void batch_writev(struct batch_options *opt, struct expand_data *data,\n+\t\t\tconst struct strbuf *hdr, size_t size)\n+{\n+\tdata->iov[0].iov_base = hdr->buf;\n+\tdata->iov[0].iov_len = hdr->len;\n+\tdata->iov[1].iov_len = size;\n+\n+\t/*\n+\t * Copying a (8|16)-byte iovec for a single byte is gross, but my\n+\t * attempt to stuff output_delim into the trailing NUL byte of\n+\t * iov[1].iov_base (and restoring it after writev(2) for the\n+\t * OI_DBCACHED case) to drop iovcnt from 3->2 wasn't faster.\n+\t */\n+\tdata->iov[2].iov_base = &opt->output_delim;\n+\tdata->iov[2].iov_len = 1;\n+\n+\tif (opt->buffer_output)\n+\t\tfwritev_or_die(stdout, data->iov, 3);\n+\telse\n+\t\twritev_or_die(1, data->iov, 3);\n+\n+\t/* writev_or_die may move iov[1].iov_base, so it's invalid */\n+\tdata->iov[1].iov_base = NULL;\n+}\n+\n+static void print_object_or_die(struct batch_options *opt,\n+\t\t\t\tstruct expand_data *data, struct strbuf *hdr)\n {\n \tconst struct object_id *oid = &data->oid;\n \n \tassert(data->info.typep);\n \n-\tif (data->content) {\n-\t\tvoid *content = data->content;\n+\tif (data->iov[1].iov_base) {\n+\t\tvoid *content = data->iov[1].iov_base;\n \t\tunsigned long size = data->size;\n \n-\t\tdata->content = NULL;\n \t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n \t\t\t\t\tdata->type == OBJ_TAG)) {\n \t\t\tsize_t s = size;\n@@ -401,10 +426,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t}\n \n \t\t\tcontent = replace_idents_using_mailmap(content, &s);\n+\t\t\tdata->iov[1].iov_base = content;\n \t\t\tsize = cast_size_t_to_ulong(s);\n \t\t}\n-\n-\t\tbatch_write(opt, content, size);\n+\t\tbatch_writev(opt, data, hdr, size);\n \t\tswitch (data->info.whence) {\n \t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n \t\tcase OI_LOOSE:\n@@ -419,8 +444,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t}\n \t} else {\n \t\tassert(data->type == OBJ_BLOB);\n-\t\tif (opt->buffer_output)\n-\t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n \t\t\tchar *contents;\n \t\t\tunsigned long size;\n@@ -447,10 +470,15 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t\t\t oid_to_hex(oid), data->rest);\n \t\t\t} else\n \t\t\t\tBUG(\"invalid transform_mode: %c\", opt->transform_mode);\n-\t\t\tbatch_write(opt, contents, size);\n+\t\t\tdata->iov[1].iov_base = contents;\n+\t\t\tbatch_writev(opt, data, hdr, size);\n \t\t\tfree(contents);\n \t\t} else {\n+\t\t\tbatch_write(opt, hdr->buf, hdr->len);\n+\t\t\tif (opt->buffer_output)\n+\t\t\t\tfflush(stdout);\n \t\t\tstream_blob(oid);\n+\t\t\tbatch_write(opt, &opt->output_delim, 1);\n \t\t}\n \t}\n }\n@@ -519,12 +547,10 @@ static void batch_object_write(const char *obj_name,\n \t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n \n-\tbatch_write(opt, scratch->buf, scratch->len);\n-\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n-\t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, &opt->output_delim, 1);\n-\t}\n+\tif (opt->batch_mode == BATCH_MODE_CONTENTS)\n+\t\tprint_object_or_die(opt, data, scratch);\n+\telse\n+\t\tbatch_write(opt, scratch->buf, scratch->len);\n }\n \n static void batch_one_object(const char *obj_name,\n@@ -666,7 +692,7 @@ static void parse_cmd_contents(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_CONTENTS;\n-\tdata->info.contentp = &data->content;\n+\tdata->info.contentp = &data->iov[1].iov_base;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -823,7 +849,7 @@ static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \t\tif (!opt->transform_mode) {\n \t\t\tdata.info.sizep = &data.size;\n-\t\t\tdata.info.contentp = &data.content;\n+\t\t\tdata.info.contentp = &data.iov[1].iov_base;\n \t\t\tdata.info.content_limit = big_file_threshold;\n \t\t\tdata.info.direct_cache = USE_DIRECT_CACHE;\n \t\t}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 85d63821ec..8ce8776657 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -69,6 +69,7 @@ ifeq ($(uname_S),Linux)\n \t\tBASIC_CFLAGS += -std=c99\n endif\n \tLINK_FUZZ_PROGRAMS = YesPlease\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n@@ -77,6 +78,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)\n \tDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n \tLIBC_CONTAINS_LIBINTL = YesPlease\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),UnixWare)\n \tCC = cc\n@@ -292,6 +294,7 @@ ifeq ($(uname_S),FreeBSD)\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -307,6 +310,7 @@ ifeq ($(uname_S),OpenBSD)\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),MirBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -329,6 +333,7 @@ ifeq ($(uname_S),NetBSD)\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n \tCSPRNG_METHOD = arc4random\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/exe\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),AIX)\n \tDEFAULT_PAGER = more\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ca7678a379..afde8abc99 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -388,6 +388,16 @@ static inline int git_setitimer(int which UNUSED,\n #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)\n #endif\n \n+#ifdef HAVE_WRITEV\n+#include \n+#define git_iovec iovec\n+#else /* !HAVE_WRITEV */\n+struct git_iovec {\n+\tvoid *iov_base;\n+\tsize_t iov_len;\n+};\n+#endif /* !HAVE_WRITEV */\n+\n #ifndef NO_LIBGEN_H\n #include \n #else\ndiff --git a/wrapper.c b/wrapper.c\nindex f87d90bf57..066c772145 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -262,6 +262,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+#ifdef HAVE_WRITEV\n+ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt)\n+{\n+\twhile (1) {\n+\t\tssize_t nr = writev(fd, iov, iovcnt);\n+\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (handle_nonblock(fd, POLLOUT, errno))\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\treturn nr;\n+\t}\n+}\n+#endif /* !HAVE_WRITEV */\n+\n /*\n * xpread() is the same as pread(), but it automatically restarts pread()\n * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\ndiff --git a/wrapper.h b/wrapper.h\nindex 1b2b047ea0..3d33c63d4f 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_\n int xopen(const char *path, int flags, ...);\n ssize_t xread(int fd, void *buf, size_t len);\n ssize_t xwrite(int fd, const void *buf, size_t len);\n+ssize_t xwritev(int fd, const struct git_iovec *, int iovcnt);\n ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n int xdup(int fd);\n FILE *xfopen(const char *path, const char *mode);\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 01a9a51fa2..227b051165 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -107,3 +107,69 @@ void fflush_or_die(FILE *f)\n \tif (fflush(f))\n \t\tdie_errno(\"fflush error\");\n }\n+\n+void fwritev_or_die(FILE *fp, const struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < iovcnt; i++) {\n+\t\tsize_t n = iov[i].iov_len;\n+\n+\t\tif (fwrite(iov[i].iov_base, 1, n, fp) != n)\n+\t\t\tdie_errno(\"unable to write to FD=%d\", fileno(fp));\n+\t}\n+}\n+\n+/*\n+ * note: we don't care about atomicity from writev(2) right now.\n+ * The goal is to avoid allocations+copies in the writer and\n+ * reduce wakeups+syscalls in the reader.\n+ * n.b. @iov is not const since we modify it to avoid allocating\n+ * on partial write.\n+ */\n+#ifdef HAVE_WRITEV\n+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\twhile (iovcnt > 0) {\n+\t\tssize_t n = xwritev(fd, iov, iovcnt);\n+\n+\t\t/* EINVAL happens when sum of iov_len exceeds SSIZE_MAX */\n+\t\tif (n < 0 && errno == EINVAL)\n+\t\t\tn = xwrite(fd, iov[0].iov_base, iov[0].iov_len);\n+\t\tif (n < 0) {\n+\t\t\tcheck_pipe(errno);\n+\t\t\tdie_errno(\"writev error\");\n+\t\t} else if (!n) {\n+\t\t\terrno = ENOSPC;\n+\t\t\tdie_errno(\"writev_error\");\n+\t\t}\n+\t\t/* skip fully written iovs, retry from the first partial iov */\n+\t\tfor (i = 0; i < iovcnt; i++) {\n+\t\t\tif (n >= iov[i].iov_len) {\n+\t\t\t\tn -= iov[i].iov_len;\n+\t\t\t} else {\n+\t\t\t\tiov[i].iov_len -= n;\n+\t\t\t\tiov[i].iov_base = (char *)iov[i].iov_base + n;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tiovcnt -= i;\n+\t\tiov += i;\n+\t}\n+}\n+#else /* !HAVE_WRITEV */\n+\n+/*\n+ * n.b. don't use stdio fwrite here even if it's faster, @fd may be\n+ * non-blocking and stdio isn't equipped for EAGAIN\n+ */\n+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < iovcnt; i++)\n+\t\twrite_or_die(fd, iov[i].iov_base, iov[i].iov_len);\n+}\n+#endif /* !HAVE_WRITEV */\ndiff --git a/write-or-die.h b/write-or-die.h\nindex 65a5c42a47..20abec211c 100644\n--- a/write-or-die.h\n+++ b/write-or-die.h\n@@ -7,6 +7,8 @@ void fprintf_or_die(FILE *, const char *fmt, ...);\n void fwrite_or_die(FILE *f, const void *buf, size_t count);\n void fflush_or_die(FILE *f);\n void write_or_die(int fd, const void *buf, size_t count);\n+void writev_or_die(int fd, struct git_iovec *, int iovcnt);\n+void fwritev_or_die(FILE *, const struct git_iovec *, int iovcnt);\n \n /*\n * These values are used to help identify parts of a repository to fsync.\n"},{"id":"498706","messageId":"20240715003519.2671385-1-e@80x24.org","threadId":"61778","inReplyTo":null,"subject":"[PATCH v1 00/10] cat-file speedups","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-15T00:35:09Z","receivedAt":"2024-07-15T00:43:57Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This continues the work of Jeff King and my initial work to\nspeed up cat-file --batch(-contents)? users in\nhttps://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/\n\nThere's more speedups I'm working on, but this series touches\non the work Jeff and I have already published.\n\nI've started putting some Perl5 + Inline::C benchmarks with\nseveral knobs up at: git clone https://80x24.org/misc-git-benchmarks.git\n\nI've found it necessary to use schedtool(1) on Linux to pin all\nprocesses to a single CPU on multicore systems.\n\nSome patches make more sense for largish objects, some for\nsmaller objects. Small objects (several KB) were my main focus,\nbut I figure 5/10 could help with some pathological big cases\nand also open the door to expanding the use of caching down the\nline.\n\n10/10 actually ended up being more significant than I originally\nanticipated for repeat lookups of the same objects (common for\nweb frontends getting hammered).\n\nJeff: I started writing commit messages for your patches (1 and\n2), but there's probably better explanations you could do :>\n\nEric Wong (8):\n packfile: fix off-by-one in content_limit comparison\n packfile: inline cache_or_unpack_entry\n cat-file: use delta_base_cache entries directly\n packfile: packed_object_info avoids packed_to_object_type\n object_info: content_limit only applies to blobs\n cat-file: batch-command uses content_limit\n cat-file: batch_write: use size_t for length\n cat-file: use writev(2) if available\n\nJeff King (2):\n packfile: move sizep computation\n packfile: allow content-limit for cat-file\n\n Makefile | 3 ++\n builtin/cat-file.c | 124 +++++++++++++++++++++++++++++++-------------\n config.mak.uname | 5 ++\n git-compat-util.h | 10 ++++\n object-file.c | 12 +++++\n object-store-ll.h | 8 +++\n packfile.c | 120 ++++++++++++++++++++++++++----------------\n packfile.h | 4 ++\n t/t1006-cat-file.sh | 19 +++++--\n wrapper.c | 18 +++++++\n wrapper.h | 1 +\n write-or-die.c | 66 +++++++++++++++++++++++\n write-or-die.h | 2 +\n 13 files changed, 308 insertions(+), 84 deletions(-)\n"},{"id":"499230","messageId":"ZqC82sDnj7Se_aVB@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"Re: [PATCH v1 00/10] cat-file speedups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:35:38Z","receivedAt":"2024-07-24T08:35:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:09AM +0000, Eric Wong wrote:\n> This continues the work of Jeff King and my initial work to\n> speed up cat-file --batch(-contents)? users in\n> https://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/\n> \n> There's more speedups I'm working on, but this series touches\n> on the work Jeff and I have already published.\n> \n> I've started putting some Perl5 + Inline::C benchmarks with\n> several knobs up at: git clone https://80x24.org/misc-git-benchmarks.git\n> \n> I've found it necessary to use schedtool(1) on Linux to pin all\n> processes to a single CPU on multicore systems.\n> \n> Some patches make more sense for largish objects, some for\n> smaller objects. Small objects (several KB) were my main focus,\n> but I figure 5/10 could help with some pathological big cases\n> and also open the door to expanding the use of caching down the\n> line.\n> \n> 10/10 actually ended up being more significant than I originally\n> anticipated for repeat lookups of the same objects (common for\n> web frontends getting hammered).\n> \n> Jeff: I started writing commit messages for your patches (1 and\n> 2), but there's probably better explanations you could do :>\n\nI definitely think that most of the commit messages could use some\ndeeper explanations. I had quite a hard time to figure out the idea\nbehind the commits because the messages only really talk about what they\nare doing, but don't mention why they are doing it or why the\ntransformations are safe.\n\nIt might also help with attracting more folks to review this patch\nseries if things have better explanations :)\n\nPatrick\n"},{"id":"499231","messageId":"ZqC835glYpBFAqu8@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-2-e@80x24.org","subject":"Re: [PATCH v1 01/10] packfile: move sizep computation","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:35:43Z","receivedAt":"2024-07-24T08:35:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:10AM +0000, Eric Wong wrote:\n> From: Jeff King \n> \n> This makes the next commit to avoid redundant object info\n\nStarting with \"this\" without mentioning what \"this\" is in the commit\nsubject reads a bit weird. I know you mention what you do in the commit\ntitle, but we usually use fully self-contained commit messages here.\n\n> lookups easier to understand.\n> \n> [ew: commit message]\n> \n> Signed-off-by: Jeff King \n> Signed-off-by: Eric Wong \n> ---\n> packfile.c | 35 ++++++++++++++++++-----------------\n> 1 file changed, 18 insertions(+), 17 deletions(-)\n> \n> diff --git a/packfile.c b/packfile.c\n> index 813584646f..e547522e3d 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1527,7 +1527,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \n> \t/*\n> \t * We always get the representation type, but only convert it to\n> -\t * a \"real\" type later if the caller is interested.\n> +\t * a \"real\" type later if the caller is interested. Likewise...\n> +\t * tbd.\n\nThis comment gets addressed in the next commit, so this should likely\nnot be changed here?\n\nPatrick\n"},{"id":"499232","messageId":"ZqC85Z5QzfdvpOpX@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-3-e@80x24.org","subject":"Re: [PATCH v1 02/10] packfile: allow content-limit for cat-file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:35:49Z","receivedAt":"2024-07-24T08:35:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:11AM +0000, Eric Wong wrote:\n> From: Jeff King \n> \n> This avoids unnecessary round trips to the object store to speed\n\nSame comment regarding \"this\". Despite not being self-contained, I also\nthink that the commit message could do a better job of explaining what\nthe problem is that you're fixing in the first place. Right now, I'm\nleft second-guessing what the idea is that this patch has to make\ngit-cat-file(1) faster.\n\n> up cat-file contents retrievals. The majority of packed objects\n> don't benefit from the streaming interface at all and we end up\n> having to load them in core anyways to satisfy our streaming\n> API.\n> \n> This drops the runtime of\n> `git cat-file --batch-all-objects --unordered --batch' from\n> ~7.1s to ~6.1s on Jeff's machine.\n\nIt would be nice to get some more context here for the benchmark. Most\nimportantly, what kind of repository did this run in? Otherwise it is\ngoing to be next to impossible to get remotely comparable results.\n\nPatrick\n"},{"id":"499233","messageId":"ZqC86t7YpVhdmLh_@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-4-e@80x24.org","subject":"Re: [PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:35:54Z","receivedAt":"2024-07-24T08:35:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:12AM +0000, Eric Wong wrote:\n> object-file.c::loose_object_info() accepts objects matching\n> content_limit exactly, so it follows packfile handling allows\n> slurping objects which match loose object handling and slurp\n> objects with size matching the content_limit exactly.\n> \n> Signed-off-by: Eric Wong \n> ---\n> packfile.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/packfile.c b/packfile.c\n> index 54b9d46928..371da96cdb 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1558,7 +1558,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t}\n> \n> \t\tif (oi->contentp) {\n> -\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n> +\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n> \t\t\t\t\t\t\t\t oi->sizep, &type);\n> \t\t\t\tif (!*oi->contentp)\n\nIn practice this doesn't really fix a user-visible bug, right? The only\ndifference before and after is that we now start to stream contents\nearlier? And that's why we cannot have a test for this.\n\nIf so, I'd recommend to explain this in the commit message.\n\nPatrick\n"},{"id":"499234","messageId":"ZqC872ExETzRH60Z@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-6-e@80x24.org","subject":"Re: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:35:59Z","receivedAt":"2024-07-24T08:36:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:14AM +0000, Eric Wong wrote:\n> For objects already in the delta_base_cache, we can safely use\n> them directly to avoid the malloc+memcpy+free overhead.\n\nSame here, I feel like you need to explain a bit more in depth what the\nactual idea behind your patch is to help reviewers.\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index bc4bb89610..769c8b48d2 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -24,6 +24,7 @@\n> #include \"promisor-remote.h\"\n> #include \"mailmap.h\"\n> #include \"write-or-die.h\"\n> +#define USE_DIRECT_CACHE 1\n\nI'm confused by this. Why do we introduce a macro that is always defined\nto a trueish value? Why don't we just remove the code guarded by this?\n\n> enum batch_mode {\n> \tBATCH_MODE_CONTENTS,\n> @@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \n> \tif (data->content) {\n> \t\tbatch_write(opt, data->content, data->size);\n> -\t\tFREE_AND_NULL(data->content);\n> +\t\tswitch (data->info.whence) {\n> +\t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n\nIs this something that will get addressed in a subsequent patch? If so,\nthe commit message and the message here should likely mention this. If\nnot, we should have a comment here saying why this is fine to be kept.\n\n> +\t\tcase OI_LOOSE:\n> +\t\tcase OI_PACKED:\n> +\t\t\tFREE_AND_NULL(data->content);\n> +\t\t\tbreak;\n> +\t\tcase OI_DBCACHED:\n> +\t\t\tif (USE_DIRECT_CACHE)\n> +\t\t\t\tunlock_delta_base_cache();\n> +\t\t\telse\n> +\t\t\t\tFREE_AND_NULL(data->content);\n> +\t\t}\n> \t} else if (data->type == OBJ_BLOB) {\n> \t\tif (opt->buffer_output)\n> \t\t\tfflush(stdout);\n> @@ -815,6 +827,7 @@ static int batch_objects(struct batch_options *opt)\n> \t\t\tdata.info.sizep = &data.size;\n> \t\t\tdata.info.contentp = &data.content;\n> \t\t\tdata.info.content_limit = big_file_threshold;\n> +\t\t\tdata.info.direct_cache = USE_DIRECT_CACHE;\n> \t\t}\n> \t}\n> \n> diff --git a/object-file.c b/object-file.c\n> index 1cc29c3c58..19100e823d 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n> \t\tif (oi->type_name)\n> \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n> +\t\t/*\n> +\t\t * Currently `blame' is the only command which creates\n> +\t\t * OI_CACHED, and direct_cache is only used by `cat-file'.\n> +\t\t */\n> +\t\tassert(!oi->direct_cache);\n\nWe shouldn't use asserts, but rather use `BUG()` statements in our\ncodebase. `assert()`s don't help users that run production builds.\n\n> \t\tif (oi->contentp)\n> \t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n> \t\toi->whence = OI_CACHED;\n> diff --git a/object-store-ll.h b/object-store-ll.h\n> index b71a15f590..50c5219308 100644\n> --- a/object-store-ll.h\n> +++ b/object-store-ll.h\n> @@ -298,6 +298,13 @@ struct object_info {\n> \t\tOI_PACKED,\n> \t\tOI_DBCACHED\n> \t} whence;\n> +\n> +\t/*\n> +\t * set if caller is able to use OI_DBCACHED entries without copying\n> +\t * TODO OI_CACHED if its use goes beyond blame\n> +\t */\n> +\tunsigned direct_cache:1;\n> +\n\nThis comment looks unfinished to me.\n\n> \tunion {\n> \t\t/*\n> \t\t * struct {\n> diff --git a/packfile.c b/packfile.c\n> index 1a409ec142..b2660e14f9 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,\n> static struct hashmap delta_base_cache;\n> static size_t delta_base_cached;\n> \n> +/* ensures oi->direct_cache is used properly */\n> +static int delta_base_cache_lock;\n> +\n\nHow exactly does it ensure it? What is the intent of this variable and\nhow would it be used correctly?\n\n> static LIST_HEAD(delta_base_cache_lru);\n> \n> struct delta_base_cache_key {\n> @@ -1444,6 +1447,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)\n> \tfree(ent);\n> }\n> \n> +static void lock_delta_base_cache(void)\n> +{\n> +\tdelta_base_cache_lock++;\n> +\tassert(delta_base_cache_lock == 1);\n> +}\n> +\n> +void unlock_delta_base_cache(void)\n> +{\n> +\tdelta_base_cache_lock--;\n> +\tassert(delta_base_cache_lock == 0);\n> +}\n\nHum. So this looks like a pseudo-mutex to me? Are there any code paths\nwhere this may be used in a threaded context? I assume not in the\ncurrent state of affairs as we only use it in git-cat-file(1).\n\n> static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n> {\n> \tfree(ent->data);\n> @@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n> void clear_delta_base_cache(void)\n> {\n> \tstruct list_head *lru, *tmp;\n> +\tassert(!delta_base_cache_lock);\n> \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n> \t\tstruct delta_base_cache_entry *entry =\n> \t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n> @@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n> \tstruct delta_base_cache_entry *ent;\n> \tstruct list_head *lru, *tmp;\n> \n> +\tassert(!delta_base_cache_lock);\n> \t/*\n> \t * Check required to avoid redundant entries when more than one thread\n> \t * is unpacking the same object, in unpack_entry() (since its phases I\n> @@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\tif (oi->sizep)\n> \t\t\t*oi->sizep = ent->size;\n> \t\tif (oi->contentp) {\n> -\t\t\tif (!oi->content_limit ||\n> -\t\t\t\t\tent->size <= oi->content_limit)\n> +\t\t\t/* ignore content_limit if avoiding copy from cache */\n> +\t\t\tif (oi->direct_cache) {\n> +\t\t\t\tlock_delta_base_cache();\n> +\t\t\t\t*oi->contentp = ent->data;\n> +\t\t\t} else if (!oi->content_limit ||\n> +\t\t\t\t\tent->size <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n> -\t\t\telse\n> +\t\t\t} else {\n> \t\t\t\t*oi->contentp = NULL; /* caller must stream */\n> +\t\t\t}\n> \t\t}\n> \t} else if (oi->contentp && !oi->content_limit) {\n> \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n\nOkay, this hunk is the gist of this patch. Instead of copying over the\ndelta base, we simply take its data pointer as the content pointer. All\nthe other infra that you're adding is mostly only added as a safeguard\nto make sure that we don't discard the delta base while the object is\ngetting accessed.\n\nPatrick\n"},{"id":"499235","messageId":"ZqC89ArZWgaZWY7a@tanuki","threadId":"61778","inReplyTo":"20240715003519.2671385-7-e@80x24.org","subject":"Re: [PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T08:36:04Z","receivedAt":"2024-07-24T08:36:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 15, 2024 at 12:35:15AM +0000, Eric Wong wrote:\n> For calls the delta base cache, packed_to_object_type calls\n> can be omitted. This prepares us to bypass content_limit for\n> non-blob types in the following commit.\n> \n> Signed-off-by: Eric Wong \n> ---\n> packfile.c | 18 ++++++++++--------\n> 1 file changed, 10 insertions(+), 8 deletions(-)\n> \n> diff --git a/packfile.c b/packfile.c\n> index b2660e14f9..c2ba6ab203 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> {\n> \tstruct pack_window *w_curs = NULL;\n> \toff_t curpos = obj_offset;\n> -\tenum object_type type;\n> +\tenum object_type type, final_type = OBJ_BAD;\n> \tstruct delta_base_cache_entry *ent;\n\nI think it might help this patch to move `type` to the scopes where it's\nused to demonstrate that all code paths set `final_type` as expected.\n\n> \t/*\n> @@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \tent = get_delta_base_cache_entry(p, obj_offset);\n> \tif (ent) {\n> \t\toi->whence = OI_DBCACHED;\n> -\t\ttype = ent->type;\n> +\t\tfinal_type = type = ent->type;\n> \t\tif (oi->sizep)\n> \t\t\t*oi->sizep = ent->size;\n> \t\tif (oi->contentp) {\n> @@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t} else if (oi->contentp && !oi->content_limit) {\n> \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n> \t\t\t\t\t\toi->sizep);\n> +\t\tfinal_type = type;\n> \t\tif (!*oi->contentp)\n> \t\t\ttype = OBJ_BAD;\n> \t} else {\n> @@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n> \t\t\t\t\t\t\t&type, oi->sizep);\n> +\t\t\t\tfinal_type = type;\n> \t\t\t\tif (!*oi->contentp)\n> \t\t\t\t\ttype = OBJ_BAD;\n> \t\t\t} else {\n> @@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t}\n> \n> \tif (oi->typep || oi->type_name) {\n> -\t\tenum object_type ptot;\n> -\t\tptot = packed_to_object_type(r, p, obj_offset,\n> -\t\t\t\t\t type, &w_curs, curpos);\n> +\t\tif (final_type < 0)\n> +\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n> +\t\t\t\t\t\t type, &w_curs, curpos);\n\nSo this is the actual change we're interested in, right? Instead of\nunconditionally calling `packed_to_object_type()`, we skip that call in\ncase we know that we have already figured out the correct object type.\n\nWouldn't it be easier to manage this with a single `type` variable,\nonly, and then conditionally call `packed_to_object_type()` only in the\ncases where `type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA`? Not sure\nwhether that would be all that useful though given that the function\nalready knows to exit without doing anything in case the type is already\nproperly resolved. So maybe the next patch will enlighten me.\n\nPatrick\n\n> \t\tif (oi->typep)\n> -\t\t\t*oi->typep = ptot;\n> +\t\t\t*oi->typep = final_type;\n> \t\tif (oi->type_name) {\n> -\t\t\tconst char *tn = type_name(ptot);\n> +\t\t\tconst char *tn = type_name(final_type);\n> \t\t\tif (tn)\n> \t\t\t\tstrbuf_addstr(oi->type_name, tn);\n> \t\t}\n> -\t\tif (ptot < 0) {\n> +\t\tif (final_type < 0) {\n> \t\t\ttype = OBJ_BAD;\n> \t\t\tgoto out;\n> \t\t}\n> \n"},{"id":"499397","messageId":"20240726073013.M358835@dcvr","threadId":"61778","inReplyTo":"ZqC85Z5QzfdvpOpX@tanuki","subject":"Re: [PATCH v1 02/10] packfile: allow content-limit for cat-file","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-26T07:30:13Z","receivedAt":"2024-07-26T07:39:00Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Steinhardt wrote:\n> On Mon, Jul 15, 2024 at 12:35:11AM +0000, Eric Wong wrote:\n> > From: Jeff King \n> > \n> > This avoids unnecessary round trips to the object store to speed\n> \n> Same comment regarding \"this\". Despite not being self-contained, I also\n> think that the commit message could do a better job of explaining what\n> the problem is that you're fixing in the first place. Right now, I'm\n> left second-guessing what the idea is that this patch has to make\n> git-cat-file(1) faster.\n\nI was hoping Jeff would flesh out the commit messages for the\nchanges he authored. I'll take a closer look and update the\nmessages if he's too busy.\n\n> > up cat-file contents retrievals. The majority of packed objects\n> > don't benefit from the streaming interface at all and we end up\n> > having to load them in core anyways to satisfy our streaming\n> > API.\n> > \n> > This drops the runtime of\n> > `git cat-file --batch-all-objects --unordered --batch' from\n> > ~7.1s to ~6.1s on Jeff's machine.\n> \n> It would be nice to get some more context here for the benchmark. Most\n> importantly, what kind of repository did this run in? Otherwise it is\n> going to be next to impossible to get remotely comparable results.\n\nOops, that was for git.git\n\n"},{"id":"499398","messageId":"20240726074201.M876490@dcvr","threadId":"61778","inReplyTo":"ZqC872ExETzRH60Z@tanuki","subject":"Re: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-26T07:42:01Z","receivedAt":"2024-07-26T07:42:02Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Steinhardt wrote:\n> On Mon, Jul 15, 2024 at 12:35:14AM +0000, Eric Wong wrote:\n> > For objects already in the delta_base_cache, we can safely use\n> > them directly to avoid the malloc+memcpy+free overhead.\n> \n> Same here, I feel like you need to explain a bit more in depth what the\n> actual idea behind your patch is to help reviewers.\n\nI elaborated more on the speedup gained in the second paragraph\nof the commit message:\n\n\t... this avoids up to 96MB of duplicated memory in the worst\n\tcase with the default git config. For a more reasonable 1MB\n\tdelta base object, this eliminates the speed penalty of\n\tduplicating large objects into memory and speeds up those 1MB\n\tdelta base cached content retrievals by roughly 30%.\n\n> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> > index bc4bb89610..769c8b48d2 100644\n> > --- a/builtin/cat-file.c\n> > +++ b/builtin/cat-file.c\n> > @@ -24,6 +24,7 @@\n> > #include \"promisor-remote.h\"\n> > #include \"mailmap.h\"\n> > #include \"write-or-die.h\"\n> > +#define USE_DIRECT_CACHE 1\n> \n> I'm confused by this. Why do we introduce a macro that is always defined\n> to a trueish value? Why don't we just remove the code guarded by this?\n\nI wanted to be able to toggle the feature for comparison during\ndevelopment. I can eliminate it for v2.\n\n> > enum batch_mode {\n> > \tBATCH_MODE_CONTENTS,\n> > @@ -386,7 +387,18 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> > \n> > \tif (data->content) {\n> > \t\tbatch_write(opt, data->content, data->size);\n> > -\t\tFREE_AND_NULL(data->content);\n> > +\t\tswitch (data->info.whence) {\n> > +\t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n> \n> Is this something that will get addressed in a subsequent patch? If so,\n> the commit message and the message here should likely mention this. If\n> not, we should have a comment here saying why this is fine to be kept.\n\nNot in this series. I'm not sure if we'll ever need OI_CACHED\nsupport, here. However, I've been considering an new cache\nthat can be shared across multiple cat-file processes, but\nthat'll be a separate series.\n\n> > diff --git a/object-file.c b/object-file.c\n> > index 1cc29c3c58..19100e823d 100644\n> > --- a/object-file.c\n> > +++ b/object-file.c\n> > @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,\n> > \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n> > \t\tif (oi->type_name)\n> > \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n> > +\t\t/*\n> > +\t\t * Currently `blame' is the only command which creates\n> > +\t\t * OI_CACHED, and direct_cache is only used by `cat-file'.\n> > +\t\t */\n> > +\t\tassert(!oi->direct_cache);\n> \n> We shouldn't use asserts, but rather use `BUG()` statements in our\n> codebase. `assert()`s don't help users that run production builds.\n\nOK.\n\n> > \t\tif (oi->contentp)\n> > \t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n> > \t\toi->whence = OI_CACHED;\n> > diff --git a/object-store-ll.h b/object-store-ll.h\n> > index b71a15f590..50c5219308 100644\n> > --- a/object-store-ll.h\n> > +++ b/object-store-ll.h\n> > @@ -298,6 +298,13 @@ struct object_info {\n> > \t\tOI_PACKED,\n> > \t\tOI_DBCACHED\n> > \t} whence;\n> > +\n> > +\t/*\n> > +\t * set if caller is able to use OI_DBCACHED entries without copying\n> > +\t * TODO OI_CACHED if its use goes beyond blame\n> > +\t */\n> > +\tunsigned direct_cache:1;\n> > +\n> \n> This comment looks unfinished to me.\n\nYeah. I'll elaborate on it's only intended for cat-file atm and\nwould break if blame (or other callers) used it.\n\n> > \tunion {\n> > \t\t/*\n> > \t\t * struct {\n> > diff --git a/packfile.c b/packfile.c\n> > index 1a409ec142..b2660e14f9 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -1362,6 +1362,9 @@ static enum object_type packed_to_object_type(struct repository *r,\n> > static struct hashmap delta_base_cache;\n> > static size_t delta_base_cached;\n> > \n> > +/* ensures oi->direct_cache is used properly */\n> > +static int delta_base_cache_lock;\n> > +\n> \n> How exactly does it ensure it? What is the intent of this variable and\n> how would it be used correctly?\n\nIt prevents multiple cache entries from being acquired at once.\n\n> > +static void lock_delta_base_cache(void)\n> > +{\n> > +\tdelta_base_cache_lock++;\n> > +\tassert(delta_base_cache_lock == 1);\n> > +}\n> > +\n> > +void unlock_delta_base_cache(void)\n> > +{\n> > +\tdelta_base_cache_lock--;\n> > +\tassert(delta_base_cache_lock == 0);\n> > +}\n> \n> Hum. So this looks like a pseudo-mutex to me? Are there any code paths\n> where this may be used in a threaded context? I assume not in the\n> current state of affairs as we only use it in git-cat-file(1).\n\nNo parallelism or threads at all. It's to ensure callers can't\nload multiple entries at the same time since retrieving a delta\nbase cache entry could invalidate an entry that's already\nacquired for use.\n\n> > static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n> > {\n> > \tfree(ent->data);\n> > @@ -1453,6 +1468,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n> > void clear_delta_base_cache(void)\n> > {\n> > \tstruct list_head *lru, *tmp;\n> > +\tassert(!delta_base_cache_lock);\n> > \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n> > \t\tstruct delta_base_cache_entry *entry =\n> > \t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n> > @@ -1466,6 +1482,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n> > \tstruct delta_base_cache_entry *ent;\n> > \tstruct list_head *lru, *tmp;\n> > \n> > +\tassert(!delta_base_cache_lock);\n> > \t/*\n> > \t * Check required to avoid redundant entries when more than one thread\n> > \t * is unpacking the same object, in unpack_entry() (since its phases I\n> > @@ -1521,11 +1538,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t\tif (oi->sizep)\n> > \t\t\t*oi->sizep = ent->size;\n> > \t\tif (oi->contentp) {\n> > -\t\t\tif (!oi->content_limit ||\n> > -\t\t\t\t\tent->size <= oi->content_limit)\n> > +\t\t\t/* ignore content_limit if avoiding copy from cache */\n> > +\t\t\tif (oi->direct_cache) {\n> > +\t\t\t\tlock_delta_base_cache();\n> > +\t\t\t\t*oi->contentp = ent->data;\n> > +\t\t\t} else if (!oi->content_limit ||\n> > +\t\t\t\t\tent->size <= oi->content_limit) {\n> > \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n> > -\t\t\telse\n> > +\t\t\t} else {\n> > \t\t\t\t*oi->contentp = NULL; /* caller must stream */\n> > +\t\t\t}\n> > \t\t}\n> > \t} else if (oi->contentp && !oi->content_limit) {\n> > \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n> \n> Okay, this hunk is the gist of this patch. Instead of copying over the\n> delta base, we simply take its data pointer as the content pointer. All\n> the other infra that you're adding is mostly only added as a safeguard\n> to make sure that we don't discard the delta base while the object is\n> getting accessed.\n\nRight. I'll switch the asserts to BUG calls for v2.\n"},{"id":"499399","messageId":"20240726074355.M208461@dcvr","threadId":"61778","inReplyTo":"ZqC86t7YpVhdmLh_@tanuki","subject":"Re: [PATCH v1 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-26T07:43:55Z","receivedAt":"2024-07-26T07:43:55Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Steinhardt wrote:\n> In practice this doesn't really fix a user-visible bug, right? The only\n> difference before and after is that we now start to stream contents\n> earlier? And that's why we cannot have a test for this.\n> \n> If so, I'd recommend to explain this in the commit message.\n\nRight, it's just for consistency with the rest of the code base.\nWill update the message for v2.\n"},{"id":"499400","messageId":"20240726080159.M14165@dcvr","threadId":"61778","inReplyTo":"ZqC89ArZWgaZWY7a@tanuki","subject":"Re: [PATCH v1 06/10] packfile: packed_object_info avoids packed_to_object_type","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-26T08:01:58Z","receivedAt":"2024-07-26T08:01:59Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Patrick Steinhardt wrote:\n> On Mon, Jul 15, 2024 at 12:35:15AM +0000, Eric Wong wrote:\n> > For calls the delta base cache, packed_to_object_type calls\n> > can be omitted. This prepares us to bypass content_limit for\n> > non-blob types in the following commit.\n> > \n> > Signed-off-by: Eric Wong \n> > ---\n> > packfile.c | 18 ++++++++++--------\n> > 1 file changed, 10 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/packfile.c b/packfile.c\n> > index b2660e14f9..c2ba6ab203 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -1522,7 +1522,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > {\n> > \tstruct pack_window *w_curs = NULL;\n> > \toff_t curpos = obj_offset;\n> > -\tenum object_type type;\n> > +\tenum object_type type, final_type = OBJ_BAD;\n> > \tstruct delta_base_cache_entry *ent;\n> \n> I think it might help this patch to move `type` to the scopes where it's\n> used to demonstrate that all code paths set `final_type` as expected.\n\nThe condition at the end of packed_object_info() requires the original\n`type' to keep its top-level scope:\n\n if (oi->delta_base_oid) {\n if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n\nBut yeah, the whole function is huge and remains a bit convoluted.\nInlining cache_or_unpack_entry in 4/10 helped some, I think.\n\n> > \t/*\n> > @@ -1534,7 +1534,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \tent = get_delta_base_cache_entry(p, obj_offset);\n> > \tif (ent) {\n> > \t\toi->whence = OI_DBCACHED;\n> > -\t\ttype = ent->type;\n> > +\t\tfinal_type = type = ent->type;\n> > \t\tif (oi->sizep)\n> > \t\t\t*oi->sizep = ent->size;\n> > \t\tif (oi->contentp) {\n> > @@ -1552,6 +1552,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t} else if (oi->contentp && !oi->content_limit) {\n> > \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n> > \t\t\t\t\t\toi->sizep);\n> > +\t\tfinal_type = type;\n> > \t\tif (!*oi->contentp)\n> > \t\t\ttype = OBJ_BAD;\n> > \t} else {\n> > @@ -1581,6 +1582,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n> > \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n> > \t\t\t\t\t\t\t&type, oi->sizep);\n> > +\t\t\t\tfinal_type = type;\n> > \t\t\t\tif (!*oi->contentp)\n> > \t\t\t\t\ttype = OBJ_BAD;\n> > \t\t\t} else {\n> > @@ -1602,17 +1604,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t}\n> > \n> > \tif (oi->typep || oi->type_name) {\n> > -\t\tenum object_type ptot;\n> > -\t\tptot = packed_to_object_type(r, p, obj_offset,\n> > -\t\t\t\t\t type, &w_curs, curpos);\n> > +\t\tif (final_type < 0)\n> > +\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n> > +\t\t\t\t\t\t type, &w_curs, curpos);\n> \n> So this is the actual change we're interested in, right? Instead of\n> unconditionally calling `packed_to_object_type()`, we skip that call in\n> case we know that we have already figured out the correct object type.\n> \n> Wouldn't it be easier to manage this with a single `type` variable,\n> only, and then conditionally call `packed_to_object_type()` only in the\n> cases where `type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA`? Not sure\n> whether that would be all that useful though given that the function\n> already knows to exit without doing anything in case the type is already\n> properly resolved. So maybe the next patch will enlighten me.\n\nAs I mentioned above, I think the `type' var remains necessary.\n"},{"id":"501241","messageId":"20240818173637.M96307@dcvr","threadId":"61778","inReplyTo":"20240726074201.M876490@dcvr","subject":"assert vs BUG [was: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly]","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-18T17:36:37Z","receivedAt":"2024-08-18T17:44:04Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong wrote:\n> Patrick Steinhardt wrote:\n> > We shouldn't use asserts, but rather use `BUG()` statements in our\n> > codebase. `assert()`s don't help users that run production builds.\n> \n> OK.\n\nThinking about this more, I still favor assert() in common code\npaths since it's only meant to be used during development and\nlater removed or neutralized (via -DNDEBUG).\n\nIOW, I treat assert() as scaffolding that can/should later be\nremoved once the code is proven to work well. We also have\nplenty of existing asserts in our codebase.\n\nFurthermore, assert() is also a well known API which reduces the\nlearning curve for drive-by hackers (I still consider myself\na drive-by since my I do minimal C).\n"},{"id":"501284","messageId":"xmqqr0akfr5a.fsf@gitster.g","threadId":"61778","inReplyTo":"20240818173637.M96307@dcvr","subject":"Re: assert vs BUG [was: [PATCH v1 05/10] cat-file: use delta_base_cache entries directly]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-19T15:50:41Z","receivedAt":"2024-08-19T15:50:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Eric Wong wrote:\n>> Patrick Steinhardt wrote:\n>> > We shouldn't use asserts, but rather use `BUG()` statements in our\n>> > codebase. `assert()`s don't help users that run production builds.\n>> \n>> OK.\n>\n> Thinking about this more, I still favor assert() in common code\n> paths since it's only meant to be used during development and\n> later removed or neutralized (via -DNDEBUG).\n\nI have a mixed feeling.\n\nI agree that assert() is only meant to be used during development,\nbut we only want code that is polished enough to be added to the\nsystem, so there is no place for assert() in the code in 'master'.\n\nThe point of BUG() is that it is not easily \"neutralized\", so it can\nhelp safeguarding the production code from harming the end-user data\ndue to doing nonsense things without noticing that the precondition\nis not satisfied for it to perform correctly. It makes sense to\nhave it in both during development and after deployment.\n\n"},{"id":"501613","messageId":"20240823224630.1180772-1-e@80x24.org","threadId":"61778","inReplyTo":"20240715003519.2671385-1-e@80x24.org","subject":"[PATCH v2 00/10] cat-file speedups","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:20Z","receivedAt":"2024-08-23T22:46:37Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This continues the work of Jeff King and my initial work to\nspeed up cat-file --batch(-contents)? users in\nhttps://lore.kernel.org/git/20240621062915.GA2105230@coredump.intra.peff.net/T/\n\nv1 is here:\nhttps://lore.kernel.org/git/20240715003519.2671385-1-e@80x24.org/T/\n\nv2 changes:\n\n- attempts to improve various commit messages\n (the human language part of my brain has been pretty broken\n for a few years, now :<)\n- expand comments around delta_base_cache_lock\n- remove DIRECT_CACHE knob since it's always on\n- move `else' arm removal in print_object_or_die from 7/10 to\n 8/10 to fix t1006 under bisect\n\nI've kept the assert() calls rather than using BUG() since they're\nin easily tested code paths and the tests they perform aren't\nuseful in release builds. The assertions should remain useful for\nfuture development if we introduce more caching.\n\nThanks to Patrick for reviewing v1 and Jeff for the\ncontent_limit work.\n\nEric Wong (8):\n packfile: fix off-by-one in content_limit comparison\n packfile: inline cache_or_unpack_entry\n cat-file: use delta_base_cache entries directly\n packfile: packed_object_info avoids packed_to_object_type\n object_info: content_limit only applies to blobs\n cat-file: batch-command uses content_limit\n cat-file: batch_write: use size_t for length\n cat-file: use writev(2) if available\n\nJeff King (2):\n packfile: move sizep computation\n packfile: allow content-limit for cat-file\n\n Makefile | 3 ++\n builtin/cat-file.c | 124 +++++++++++++++++++++++++++++++-------------\n config.mak.uname | 5 ++\n git-compat-util.h | 10 ++++\n object-file.c | 12 +++++\n object-store-ll.h | 9 ++++\n packfile.c | 122 ++++++++++++++++++++++++++++---------------\n packfile.h | 4 ++\n t/t1006-cat-file.sh | 19 +++++--\n wrapper.c | 18 +++++++\n wrapper.h | 1 +\n write-or-die.c | 66 +++++++++++++++++++++++\n write-or-die.h | 2 +\n 13 files changed, 312 insertions(+), 83 deletions(-)\n\nRange-diff against v1:\n 1: 36b799ab67 ! 1: b4025cee1f packfile: move sizep computation\n @@ Metadata\n ## Commit message ##\n packfile: move sizep computation\n \n - This makes the next commit to avoid redundant object info\n - lookups easier to understand.\n + Moving the sizep computation now makes the next commit to avoid\n + redundant object info lookups easier to understand. There is\n + no user-visible change, here.\n \n [ew: commit message]\n \n @@ Commit message\n \n ## packfile.c ##\n @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,\n - \n - \t/*\n - \t * We always get the representation type, but only convert it to\n --\t * a \"real\" type later if the caller is interested.\n -+\t * a \"real\" type later if the caller is interested. Likewise...\n -+\t * tbd.\n - \t */\n - \tif (oi->contentp) {\n - \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n -@@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\ttype = OBJ_BAD;\n \t} else {\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n 2: 50f576ab16 ! 2: bdf6f57fae packfile: allow content-limit for cat-file\n @@ Metadata\n ## Commit message ##\n packfile: allow content-limit for cat-file\n \n - This avoids unnecessary round trips to the object store to speed\n + Avoid unnecessary round trips to the object store to speed\n up cat-file contents retrievals. The majority of packed objects\n don't benefit from the streaming interface at all and we end up\n having to load them in core anyways to satisfy our streaming\n API.\n \n This drops the runtime of\n - `git cat-file --batch-all-objects --unordered --batch' from\n - ~7.1s to ~6.1s on Jeff's machine.\n + `git cat-file --batch-all-objects --unordered --batch' on\n + git.git from ~7.1s to ~6.1s on Jeff's machine.\n \n [ew: commit message]\n \n @@ object-store-ll.h: struct object_info {\n \n ## packfile.c ##\n @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,\n - \t * a \"real\" type later if the caller is interested. Likewise...\n - \t * tbd.\n + \t * We always get the representation type, but only convert it to\n + \t * a \"real\" type later if the caller is interested.\n \t */\n -\tif (oi->contentp) {\n +\tif (oi->contentp && !oi->content_limit) {\n 3: 6eb732401a ! 3: 7e762e3481 packfile: fix off-by-one in content_limit comparison\n @@ Commit message\n slurping objects which match loose object handling and slurp\n objects with size matching the content_limit exactly.\n \n + This change is merely for consistency with the majority of\n + existing code and there is no user visible change in nearly all\n + cases. The only exception being the corner case when the object\n + size matches content_limit exactly where users will see a\n + speedup from avoiding an extra lookup.\n +\n Signed-off-by: Eric Wong \n \n ## packfile.c ##\n 4: 9476824ac7 ! 4: a558101b85 packfile: inline cache_or_unpack_entry\n @@ Commit message\n packfile: inline cache_or_unpack_entry\n \n We need to check delta_base_cache anyways to fill in the\n - `whence' field in `struct object_info'. Inlining\n - cache_or_unpack_entry() makes it easier to only do the hashmap\n - lookup once and avoid a redundant lookup later on.\n + `whence' field in `struct object_info'. Inlining (and getting\n + rid of) cache_or_unpack_entry() makes it easier to only do the\n + hashmap lookup once and avoid a redundant lookup later on.\n \n This code reorganization will also make an optimization to\n - use the cache entry directly easier to implement.\n + use the cache entry directly easier to implement in the next\n + commit.\n \n Signed-off-by: Eric Wong \n \n @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,\n \n \t/*\n \t * We always get the representation type, but only convert it to\n - \t * a \"real\" type later if the caller is interested. Likewise...\n - \t * tbd.\n + \t * a \"real\" type later if the caller is interested.\n \t */\n -\tif (oi->contentp && !oi->content_limit) {\n -\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n 5: c99dfb84d4 ! 5: 74d21ac89d cat-file: use delta_base_cache entries directly\n @@ Commit message\n cat-file: use delta_base_cache entries directly\n \n For objects already in the delta_base_cache, we can safely use\n - them directly to avoid the malloc+memcpy+free overhead.\n + one entry at-a-time directly to avoid the malloc+memcpy+free\n + overhead. For a 1MB delta base object, this eliminates the\n + speed penalty of duplicating large objects into memory and\n + speeds up those 1MB delta base cached content retrievals by\n + roughly 30%.\n \n While only 2-7% of objects are delta bases in repos I've looked\n at, this avoids up to 96MB of duplicated memory in the worst\n - case with the default git config. For a more reasonable 1MB\n - delta base object, this eliminates the speed penalty of\n - duplicating large objects into memory and speeds up those 1MB\n - delta base cached content retrievals by roughly 30%.\n + case with the default git config.\n \n The new delta_base_cache_lock is a simple single-threaded\n - assertion to ensure cat-file is the exclusive user of the\n - delta_base_cache.\n + assertion to ensure cat-file (and similar) is the exclusive user\n + of the delta_base_cache. In other words, we cannot have diff\n + or similar commands using two or more entries directly from the\n + delta base cache. The new lock has nothing to do with parallel\n + access via multiple threads at the moment.\n \n Signed-off-by: Eric Wong \n \n ## builtin/cat-file.c ##\n -@@\n - #include \"promisor-remote.h\"\n - #include \"mailmap.h\"\n - #include \"write-or-die.h\"\n -+#define USE_DIRECT_CACHE 1\n - \n - enum batch_mode {\n - \tBATCH_MODE_CONTENTS,\n @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \tif (data->content) {\n \t\tbatch_write(opt, data->content, data->size);\n -\t\tFREE_AND_NULL(data->content);\n +\t\tswitch (data->info.whence) {\n -+\t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n ++\t\tcase OI_CACHED:\n ++\t\t\t/*\n ++\t\t\t * only blame uses OI_CACHED atm, so it's unlikely\n ++\t\t\t * we'll ever hit this path\n ++\t\t\t */\n ++\t\t\tBUG(\"TODO OI_CACHED support not done\");\n +\t\tcase OI_LOOSE:\n +\t\tcase OI_PACKED:\n +\t\t\tFREE_AND_NULL(data->content);\n +\t\t\tbreak;\n +\t\tcase OI_DBCACHED:\n -+\t\t\tif (USE_DIRECT_CACHE)\n -+\t\t\t\tunlock_delta_base_cache();\n -+\t\t\telse\n -+\t\t\t\tFREE_AND_NULL(data->content);\n ++\t\t\tunlock_delta_base_cache();\n +\t\t}\n \t} else if (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)\n \t\t\tdata.info.sizep = &data.size;\n \t\t\tdata.info.contentp = &data.content;\n \t\t\tdata.info.content_limit = big_file_threshold;\n -+\t\t\tdata.info.direct_cache = USE_DIRECT_CACHE;\n ++\t\t\tdata.info.direct_cache = 1;\n \t\t}\n \t}\n \n @@ object-store-ll.h: struct object_info {\n \t} whence;\n +\n +\t/*\n -+\t * set if caller is able to use OI_DBCACHED entries without copying\n -+\t * TODO OI_CACHED if its use goes beyond blame\n ++\t * Set if caller is able to use OI_DBCACHED entries without copying.\n ++\t * This only applies to OI_DBCACHED entries at the moment,\n ++\t * not OI_CACHED or any other type of entry.\n +\t */\n +\tunsigned direct_cache:1;\n +\n @@ packfile.c: static enum object_type packed_to_object_type(struct repository *r,\n static struct hashmap delta_base_cache;\n static size_t delta_base_cached;\n \n -+/* ensures oi->direct_cache is used properly */\n ++/*\n ++ * Ensures only a single object is used at-a-time via oi->direct_cache.\n ++ * Using two objects directly at once (e.g. diff) would cause corruption\n ++ * since populating the cache may invalidate existing entries.\n ++ * This lock has nothing to do with parallelism at the moment.\n ++ */\n +static int delta_base_cache_lock;\n +\n static LIST_HEAD(delta_base_cache_lru);\n 6: 79a84221b2 ! 6: 83b6367950 packfile: packed_object_info avoids packed_to_object_type\n @@ Metadata\n ## Commit message ##\n packfile: packed_object_info avoids packed_to_object_type\n \n - For calls the delta base cache, packed_to_object_type calls\n + For entries in the delta base cache, packed_to_object_type calls\n can be omitted. This prepares us to bypass content_limit for\n non-blob types in the following commit.\n \n 7: 63b36d759d ! 7: 7e0f8c0cf6 object_info: content_limit only applies to blobs\n @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s\n +\t\t\t\t\tdata->type == OBJ_TAG)) {\n +\t\t\tsize_t s = size;\n +\n -+\t\t\tif (USE_DIRECT_CACHE &&\n -+\t\t\t\t\tdata->info.whence == OI_DBCACHED) {\n ++\t\t\tif (data->info.whence == OI_DBCACHED) {\n +\t\t\t\tcontent = xmemdupz(content, s);\n +\t\t\t\tdata->info.whence = OI_PACKED;\n +\t\t\t}\n @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s\n +\n +\t\tbatch_write(opt, content, size);\n \t\tswitch (data->info.whence) {\n - \t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n + \t\tcase OI_CACHED:\n + \t\t\t/*\n +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n + \t\t\tBUG(\"TODO OI_CACHED support not done\");\n \t\tcase OI_LOOSE:\n \t\tcase OI_PACKED:\n -\t\t\tFREE_AND_NULL(data->content);\n +\t\t\tfree(content);\n \t\t\tbreak;\n \t\tcase OI_DBCACHED:\n - \t\t\tif (USE_DIRECT_CACHE)\n - \t\t\t\tunlock_delta_base_cache();\n - \t\t\telse\n --\t\t\t\tFREE_AND_NULL(data->content);\n -+\t\t\t\tfree(content);\n - \t\t}\n --\t} else if (data->type == OBJ_BLOB) {\n -+\t} else {\n -+\t\tassert(data->type == OBJ_BLOB);\n - \t\tif (opt->buffer_output)\n - \t\t\tfflush(stdout);\n - \t\tif (opt->transform_mode) {\n -@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n - \t\t\tstream_blob(oid);\n - \t\t}\n - \t}\n --\telse {\n --\t\tenum object_type type;\n --\t\tunsigned long size;\n --\t\tvoid *contents;\n --\n --\t\tcontents = repo_read_object_file(the_repository, oid, &type,\n --\t\t\t\t\t\t &size);\n --\t\tif (!contents)\n --\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n --\n --\t\tif (use_mailmap) {\n --\t\t\tsize_t s = size;\n --\t\t\tcontents = replace_idents_using_mailmap(contents, &s);\n --\t\t\tsize = cast_size_t_to_ulong(s);\n --\t\t}\n --\n --\t\tif (type != data->type)\n --\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n --\t\tif (data->info.sizep && size != data->size && !use_mailmap)\n --\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n --\n --\t\tbatch_write(opt, contents, size);\n --\t\tfree(contents);\n --\t}\n - }\n - \n - static void print_default_format(struct strbuf *scratch, struct expand_data *data,\n + \t\t\tunlock_delta_base_cache();\n \n ## object-file.c ##\n @@ object-file.c: static int loose_object_info(struct repository *r,\n @@ packfile.c: int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n +\n + ## t/t1006-cat-file.sh ##\n +@@ t/t1006-cat-file.sh: test_expect_success 'confirm that neither loose blob is a delta' '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success 'setup delta base tests' '\n ++\tfoo=\"$(git rev-parse HEAD:foo)\" &&\n ++\tfoo_plus=\"$(git rev-parse HEAD:foo-plus)\" &&\n ++\tgit repack -ad\n ++'\n ++\n + # To avoid relying too much on the current delta heuristics,\n + # we will check only that one of the two objects is a delta\n + # against the other, but not the order. We can do so by just\n + # asking for the base of both, and checking whether either\n + # oid appears in the output.\n + test_expect_success '%(deltabase) reports packed delta bases' '\n +-\tgit repack -ad &&\n + \tgit cat-file --batch-check=\"%(deltabase)\" actual &&\n + \t{\n +-\t\tgrep \"$(git rev-parse HEAD:foo)\" actual ||\n +-\t\tgrep \"$(git rev-parse HEAD:foo-plus)\" actual\n ++\t\tgrep \"$foo\" actual || grep \"$foo_plus\" actual\n + \t}\n + '\n + \n ++test_expect_success 'delta base direct cache use succeeds w/o asserting' '\n ++\tcommands=\"info $foo\n ++info $foo_plus\n ++contents $foo_plus\n ++contents $foo\" &&\n ++\techo \"$commands\" >in &&\n ++\tgit cat-file --batch-command out\n ++'\n ++\n + test_expect_success 'setup bogus data' '\n + \tbogus_short_type=\"bogus\" &&\n + \tbogus_short_content=\"bogus\" &&\n 8: 271f6241bd ! 8: ef83e8b426 cat-file: batch-command uses content_limit\n @@ Commit message\n Signed-off-by: Eric Wong \n \n ## builtin/cat-file.c ##\n +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n + \t\tcase OI_DBCACHED:\n + \t\t\tunlock_delta_base_cache();\n + \t\t}\n +-\t} else if (data->type == OBJ_BLOB) {\n ++\t} else {\n ++\t\tassert(data->type == OBJ_BLOB);\n + \t\tif (opt->buffer_output)\n + \t\t\tfflush(stdout);\n + \t\tif (opt->transform_mode) {\n +@@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n + \t\t\tstream_blob(oid);\n + \t\t}\n + \t}\n +-\telse {\n +-\t\tenum object_type type;\n +-\t\tunsigned long size;\n +-\t\tvoid *contents;\n +-\n +-\t\tcontents = repo_read_object_file(the_repository, oid, &type,\n +-\t\t\t\t\t\t &size);\n +-\t\tif (!contents)\n +-\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n +-\n +-\t\tif (use_mailmap) {\n +-\t\t\tsize_t s = size;\n +-\t\t\tcontents = replace_idents_using_mailmap(contents, &s);\n +-\t\t\tsize = cast_size_t_to_ulong(s);\n +-\t\t}\n +-\n +-\t\tif (type != data->type)\n +-\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n +-\t\tif (data->info.sizep && size != data->size && !use_mailmap)\n +-\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n +-\n +-\t\tbatch_write(opt, contents, size);\n +-\t\tfree(contents);\n +-\t}\n + }\n + \n + static void print_default_format(struct strbuf *scratch, struct expand_data *data,\n @@ builtin/cat-file.c: static void parse_cmd_contents(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \t\tif (!opt->transform_mode) {\n \t\t\tdata.info.sizep = &data.size;\n -\n - ## t/t1006-cat-file.sh ##\n -@@ t/t1006-cat-file.sh: test_expect_success 'confirm that neither loose blob is a delta' '\n - \ttest_cmp expect actual\n - '\n - \n -+test_expect_success 'setup delta base tests' '\n -+\tfoo=\"$(git rev-parse HEAD:foo)\" &&\n -+\tfoo_plus=\"$(git rev-parse HEAD:foo-plus)\" &&\n -+\tgit repack -ad\n -+'\n -+\n - # To avoid relying too much on the current delta heuristics,\n - # we will check only that one of the two objects is a delta\n - # against the other, but not the order. We can do so by just\n - # asking for the base of both, and checking whether either\n - # oid appears in the output.\n - test_expect_success '%(deltabase) reports packed delta bases' '\n --\tgit repack -ad &&\n - \tgit cat-file --batch-check=\"%(deltabase)\" actual &&\n - \t{\n --\t\tgrep \"$(git rev-parse HEAD:foo)\" actual ||\n --\t\tgrep \"$(git rev-parse HEAD:foo-plus)\" actual\n -+\t\tgrep \"$foo\" actual || grep \"$foo_plus\" actual\n - \t}\n - '\n - \n -+test_expect_success 'delta base direct cache use succeeds w/o asserting' '\n -+\tcommands=\"info $foo\n -+info $foo_plus\n -+contents $foo_plus\n -+contents $foo\" &&\n -+\techo \"$commands\" >in &&\n -+\tgit cat-file --batch-command out\n -+'\n -+\n - test_expect_success 'setup bogus data' '\n - \tbogus_short_type=\"bogus\" &&\n - \tbogus_short_content=\"bogus\" &&\n 9: d91030b69c = 9: 6a94452e54 cat-file: batch_write: use size_t for length\n10: c356b9e1ce ! 10: 1442e43ec7 cat-file: use writev(2) if available\n @@ Metadata\n ## Commit message ##\n cat-file: use writev(2) if available\n \n - Using writev here is can be 20-40% faster than three write\n - syscalls in succession for smaller (1-10k) objects in the delta\n - base cache. This advantage decreases as object sizes approach\n - pipe size (64k on Linux). This reduces wakeups and syscalls on\n - the read side, as well, especially if the reader is relying on\n - non-blocking I/O.\n + Using writev here is 20-40% faster than three write syscalls in\n + succession for smaller (1-10k) objects in the delta base cache.\n + This advantage decreases as object sizes approach pipe size (64k\n + on Linux).\n +\n + writev reduces wakeups and syscalls on the read side as well:\n + each write(2) syscall may trigger one or more corresponding\n + read(2) syscalls in the reader. Attempting atomicity in the\n + writer via writev also reduces the likelyhood of non-blocking\n + readers failing with EAGAIN and having to call poll||select\n + before attempting to read again.\n \n Unfortunately, this turns into a small (1-3%) slowdown for\n gigantic objects of a megabyte or more even with after\n @@ Commit message\n defaults to being compatible with non-blocking stdout and able\n to poll(2) after hitting EAGAIN on write(2). Using stdio on\n files with the O_NONBLOCK flag is (AFAIK) unspecified and likely\n - subject to portability problems.\n + subject to portability problems and thus avoided.\n \n Signed-off-by: Eric Wong \n \n @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, s\n -\t\tbatch_write(opt, content, size);\n +\t\tbatch_writev(opt, data, hdr, size);\n \t\tswitch (data->info.whence) {\n - \t\tcase OI_CACHED: BUG(\"FIXME OI_CACHED support not done\");\n - \t\tcase OI_LOOSE:\n + \t\tcase OI_CACHED:\n + \t\t\t/*\n @@ builtin/cat-file.c: static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t}\n \t} else {\n @@ builtin/cat-file.c: static int batch_objects(struct batch_options *opt)\n -\t\t\tdata.info.contentp = &data.content;\n +\t\t\tdata.info.contentp = &data.iov[1].iov_base;\n \t\t\tdata.info.content_limit = big_file_threshold;\n - \t\t\tdata.info.direct_cache = USE_DIRECT_CACHE;\n + \t\t\tdata.info.direct_cache = 1;\n \t\t}\n \n ## config.mak.uname ##\n\nbase-commit: a7dae3bdc8b516d36f630b12bb01e853a667e0d9\n"},{"id":"501614","messageId":"20240823224630.1180772-2-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 01/10] packfile: move sizep computation","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:21Z","receivedAt":"2024-08-23T22:46:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Jeff King \n\nMoving the sizep computation now makes the next commit to avoid\nredundant object info lookups easier to understand. There is\nno user-visible change, here.\n\n[ew: commit message]\n\nSigned-off-by: Jeff King \nSigned-off-by: Eric Wong \n---\n packfile.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 813584646f..4028763947 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1536,24 +1536,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\ttype = OBJ_BAD;\n \t} else {\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n-\t}\n \n-\tif (!oi->contentp && oi->sizep) {\n-\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n-\t\t\toff_t tmp_pos = curpos;\n-\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n-\t\t\t\t\t\t\t type, obj_offset);\n-\t\t\tif (!base_offset) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n+\t\tif (oi->sizep) {\n+\t\t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n+\t\t\t\toff_t tmp_pos = curpos;\n+\t\t\t\toff_t base_offset = get_delta_base(p, &w_curs, &tmp_pos,\n+\t\t\t\t\t\t\t\t type, obj_offset);\n+\t\t\t\tif (!base_offset) {\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t\t*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n+\t\t\t\tif (*oi->sizep == 0) {\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\t*oi->sizep = size;\n \t\t\t}\n-\t\t\t*oi->sizep = get_size_from_delta(p, &w_curs, tmp_pos);\n-\t\t\tif (*oi->sizep == 0) {\n-\t\t\t\ttype = OBJ_BAD;\n-\t\t\t\tgoto out;\n-\t\t\t}\n-\t\t} else {\n-\t\t\t*oi->sizep = size;\n \t\t}\n \t}\n \n"},{"id":"501615","messageId":"20240823224630.1180772-3-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 02/10] packfile: allow content-limit for cat-file","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:22Z","receivedAt":"2024-08-23T22:46:52Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"From: Jeff King \n\nAvoid unnecessary round trips to the object store to speed\nup cat-file contents retrievals. The majority of packed objects\ndon't benefit from the streaming interface at all and we end up\nhaving to load them in core anyways to satisfy our streaming\nAPI.\n\nThis drops the runtime of\n`git cat-file --batch-all-objects --unordered --batch' on\ngit.git from ~7.1s to ~6.1s on Jeff's machine.\n\n[ew: commit message]\n\nSigned-off-by: Jeff King \nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 17 +++++++++++++++--\n object-file.c | 6 ++++++\n object-store-ll.h | 1 +\n packfile.c | 13 ++++++++++++-\n 4 files changed, 34 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8..bc4bb89610 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -280,6 +280,7 @@ struct expand_data {\n \toff_t disk_size;\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n+\tvoid *content;\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -383,7 +384,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \tassert(data->info.typep);\n \n-\tif (data->type == OBJ_BLOB) {\n+\tif (data->content) {\n+\t\tbatch_write(opt, data->content, data->size);\n+\t\tFREE_AND_NULL(data->content);\n+\t} else if (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n@@ -801,9 +805,18 @@ static int batch_objects(struct batch_options *opt)\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\n+\t *\n+\t * Likewise, grab the content in the initial request if it's small\n+\t * and we're not planning to filter it.\n \t */\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS)\n+\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n \t\tdata.info.typep = &data.type;\n+\t\tif (!opt->transform_mode) {\n+\t\t\tdata.info.sizep = &data.size;\n+\t\t\tdata.info.contentp = &data.content;\n+\t\t\tdata.info.content_limit = big_file_threshold;\n+\t\t}\n+\t}\n \n \tif (opt->all_objects) {\n \t\tstruct object_cb_data cb;\ndiff --git a/object-file.c b/object-file.c\nindex 065103be3e..1cc29c3c58 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,\n \n \t\tif (!oi->contentp)\n \t\t\tbreak;\n+\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n+\t\t\tgit_inflate_end(&stream);\n+\t\t\toi->contentp = NULL;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\n \t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n \t\tif (*oi->contentp)\n \t\t\tgoto cleanup;\ndiff --git a/object-store-ll.h b/object-store-ll.h\nindex c5f2bb2fc2..b71a15f590 100644\n--- a/object-store-ll.h\n+++ b/object-store-ll.h\n@@ -289,6 +289,7 @@ struct object_info {\n \tstruct object_id *delta_base_oid;\n \tstruct strbuf *type_name;\n \tvoid **contentp;\n+\tsize_t content_limit;\n \n \t/* Response */\n \tenum {\ndiff --git a/packfile.c b/packfile.c\nindex 4028763947..c12a0515b3 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t * We always get the representation type, but only convert it to\n \t * a \"real\" type later if the caller is interested.\n \t */\n-\tif (oi->contentp) {\n+\tif (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n \t\t\t\t\t\t &type);\n \t\tif (!*oi->contentp)\n@@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\t\t*oi->sizep = size;\n \t\t\t}\n \t\t}\n+\n+\t\tif (oi->contentp) {\n+\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n+\t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n+\t\t\t\t\t\t\t\t oi->sizep, &type);\n+\t\t\t\tif (!*oi->contentp)\n+\t\t\t\t\ttype = OBJ_BAD;\n+\t\t\t} else {\n+\t\t\t\t*oi->contentp = NULL;\n+\t\t\t}\n+\t\t}\n \t}\n \n \tif (oi->disk_sizep) {\n"},{"id":"501616","messageId":"20240823224630.1180772-4-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:23Z","receivedAt":"2024-08-23T22:46:59Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"object-file.c::loose_object_info() accepts objects matching\ncontent_limit exactly, so it follows packfile handling allows\nslurping objects which match loose object handling and slurp\nobjects with size matching the content_limit exactly.\n\nThis change is merely for consistency with the majority of\nexisting code and there is no user visible change in nearly all\ncases. The only exception being the corner case when the object\nsize matches content_limit exactly where users will see a\nspeedup from avoiding an extra lookup.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex c12a0515b3..8ec86d2d69 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1557,7 +1557,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t}\n \n \t\tif (oi->contentp) {\n-\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n+\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n \t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t\t oi->sizep, &type);\n \t\t\t\tif (!*oi->contentp)\n"},{"id":"501617","messageId":"20240823224630.1180772-5-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 04/10] packfile: inline cache_or_unpack_entry","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:24Z","receivedAt":"2024-08-23T22:47:06Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"We need to check delta_base_cache anyways to fill in the\n`whence' field in `struct object_info'. Inlining (and getting\nrid of) cache_or_unpack_entry() makes it easier to only do the\nhashmap lookup once and avoid a redundant lookup later on.\n\nThis code reorganization will also make an optimization to\nuse the cache entry directly easier to implement in the next\ncommit.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 48 +++++++++++++++++++++---------------------------\n 1 file changed, 21 insertions(+), 27 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 8ec86d2d69..0a90a5ed67 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1444,23 +1444,6 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)\n \tfree(ent);\n }\n \n-static void *cache_or_unpack_entry(struct repository *r, struct packed_git *p,\n-\t\t\t\t off_t base_offset, unsigned long *base_size,\n-\t\t\t\t enum object_type *type)\n-{\n-\tstruct delta_base_cache_entry *ent;\n-\n-\tent = get_delta_base_cache_entry(p, base_offset);\n-\tif (!ent)\n-\t\treturn unpack_entry(r, p, base_offset, type, base_size);\n-\n-\tif (type)\n-\t\t*type = ent->type;\n-\tif (base_size)\n-\t\t*base_size = ent->size;\n-\treturn xmemdupz(ent->data, ent->size);\n-}\n-\n static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n {\n \tfree(ent->data);\n@@ -1521,20 +1504,35 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t off_t obj_offset, struct object_info *oi)\n {\n \tstruct pack_window *w_curs = NULL;\n-\tunsigned long size;\n \toff_t curpos = obj_offset;\n \tenum object_type type;\n+\tstruct delta_base_cache_entry *ent;\n \n \t/*\n \t * We always get the representation type, but only convert it to\n \t * a \"real\" type later if the caller is interested.\n \t */\n-\tif (oi->contentp && !oi->content_limit) {\n-\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n-\t\t\t\t\t\t &type);\n+\toi->whence = OI_PACKED;\n+\tent = get_delta_base_cache_entry(p, obj_offset);\n+\tif (ent) {\n+\t\toi->whence = OI_DBCACHED;\n+\t\ttype = ent->type;\n+\t\tif (oi->sizep)\n+\t\t\t*oi->sizep = ent->size;\n+\t\tif (oi->contentp) {\n+\t\t\tif (!oi->content_limit ||\n+\t\t\t\t\tent->size <= oi->content_limit)\n+\t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n+\t\t\telse\n+\t\t\t\t*oi->contentp = NULL; /* caller must stream */\n+\t\t}\n+\t} else if (oi->contentp && !oi->content_limit) {\n+\t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n+\t\t\t\t\t\toi->sizep);\n \t\tif (!*oi->contentp)\n \t\t\ttype = OBJ_BAD;\n \t} else {\n+\t\tunsigned long size;\n \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n \n \t\tif (oi->sizep) {\n@@ -1558,8 +1556,8 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \n \t\tif (oi->contentp) {\n \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n-\t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n-\t\t\t\t\t\t\t\t oi->sizep, &type);\n+\t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n+\t\t\t\t\t\t\t&type, oi->sizep);\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n@@ -1608,10 +1606,6 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t} else\n \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n \t}\n-\n-\toi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :\n-\t\t\t\t\t\t\t OI_PACKED;\n-\n out:\n \tunuse_pack(&w_curs);\n \treturn type;\n"},{"id":"501618","messageId":"20240823224630.1180772-6-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 05/10] cat-file: use delta_base_cache entries directly","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:25Z","receivedAt":"2024-08-23T22:47:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"For objects already in the delta_base_cache, we can safely use\none entry at-a-time directly to avoid the malloc+memcpy+free\noverhead. For a 1MB delta base object, this eliminates the\nspeed penalty of duplicating large objects into memory and\nspeeds up those 1MB delta base cached content retrievals by\nroughly 30%.\n\nWhile only 2-7% of objects are delta bases in repos I've looked\nat, this avoids up to 96MB of duplicated memory in the worst\ncase with the default git config.\n\nThe new delta_base_cache_lock is a simple single-threaded\nassertion to ensure cat-file (and similar) is the exclusive user\nof the delta_base_cache. In other words, we cannot have diff\nor similar commands using two or more entries directly from the\ndelta base cache. The new lock has nothing to do with parallel\naccess via multiple threads at the moment.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 16 +++++++++++++++-\n object-file.c | 5 +++++\n object-store-ll.h | 8 ++++++++\n packfile.c | 33 ++++++++++++++++++++++++++++++---\n packfile.h | 4 ++++\n 5 files changed, 62 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex bc4bb89610..8debcdca3e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -386,7 +386,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \tif (data->content) {\n \t\tbatch_write(opt, data->content, data->size);\n-\t\tFREE_AND_NULL(data->content);\n+\t\tswitch (data->info.whence) {\n+\t\tcase OI_CACHED:\n+\t\t\t/*\n+\t\t\t * only blame uses OI_CACHED atm, so it's unlikely\n+\t\t\t * we'll ever hit this path\n+\t\t\t */\n+\t\t\tBUG(\"TODO OI_CACHED support not done\");\n+\t\tcase OI_LOOSE:\n+\t\tcase OI_PACKED:\n+\t\t\tFREE_AND_NULL(data->content);\n+\t\t\tbreak;\n+\t\tcase OI_DBCACHED:\n+\t\t\tunlock_delta_base_cache();\n+\t\t}\n \t} else if (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n@@ -815,6 +828,7 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.info.sizep = &data.size;\n \t\t\tdata.info.contentp = &data.content;\n \t\t\tdata.info.content_limit = big_file_threshold;\n+\t\t\tdata.info.direct_cache = 1;\n \t\t}\n \t}\n \ndiff --git a/object-file.c b/object-file.c\nindex 1cc29c3c58..19100e823d 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n \t\tif (oi->type_name)\n \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n+\t\t/*\n+\t\t * Currently `blame' is the only command which creates\n+\t\t * OI_CACHED, and direct_cache is only used by `cat-file'.\n+\t\t */\n+\t\tassert(!oi->direct_cache);\n \t\tif (oi->contentp)\n \t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n \t\toi->whence = OI_CACHED;\ndiff --git a/object-store-ll.h b/object-store-ll.h\nindex b71a15f590..669bb93784 100644\n--- a/object-store-ll.h\n+++ b/object-store-ll.h\n@@ -298,6 +298,14 @@ struct object_info {\n \t\tOI_PACKED,\n \t\tOI_DBCACHED\n \t} whence;\n+\n+\t/*\n+\t * Set if caller is able to use OI_DBCACHED entries without copying.\n+\t * This only applies to OI_DBCACHED entries at the moment,\n+\t * not OI_CACHED or any other type of entry.\n+\t */\n+\tunsigned direct_cache:1;\n+\n \tunion {\n \t\t/*\n \t\t * struct {\ndiff --git a/packfile.c b/packfile.c\nindex 0a90a5ed67..40c6c2e387 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1362,6 +1362,14 @@ static enum object_type packed_to_object_type(struct repository *r,\n static struct hashmap delta_base_cache;\n static size_t delta_base_cached;\n \n+/*\n+ * Ensures only a single object is used at-a-time via oi->direct_cache.\n+ * Using two objects directly at once (e.g. diff) would cause corruption\n+ * since populating the cache may invalidate existing entries.\n+ * This lock has nothing to do with parallelism at the moment.\n+ */\n+static int delta_base_cache_lock;\n+\n static LIST_HEAD(delta_base_cache_lru);\n \n struct delta_base_cache_key {\n@@ -1444,6 +1452,18 @@ static void detach_delta_base_cache_entry(struct delta_base_cache_entry *ent)\n \tfree(ent);\n }\n \n+static void lock_delta_base_cache(void)\n+{\n+\tdelta_base_cache_lock++;\n+\tassert(delta_base_cache_lock == 1);\n+}\n+\n+void unlock_delta_base_cache(void)\n+{\n+\tdelta_base_cache_lock--;\n+\tassert(delta_base_cache_lock == 0);\n+}\n+\n static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n {\n \tfree(ent->data);\n@@ -1453,6 +1473,7 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n void clear_delta_base_cache(void)\n {\n \tstruct list_head *lru, *tmp;\n+\tassert(!delta_base_cache_lock);\n \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n \t\tstruct delta_base_cache_entry *entry =\n \t\t\tlist_entry(lru, struct delta_base_cache_entry, lru);\n@@ -1466,6 +1487,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tstruct delta_base_cache_entry *ent;\n \tstruct list_head *lru, *tmp;\n \n+\tassert(!delta_base_cache_lock);\n \t/*\n \t * Check required to avoid redundant entries when more than one thread\n \t * is unpacking the same object, in unpack_entry() (since its phases I\n@@ -1520,11 +1542,16 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\tif (oi->sizep)\n \t\t\t*oi->sizep = ent->size;\n \t\tif (oi->contentp) {\n-\t\t\tif (!oi->content_limit ||\n-\t\t\t\t\tent->size <= oi->content_limit)\n+\t\t\t/* ignore content_limit if avoiding copy from cache */\n+\t\t\tif (oi->direct_cache) {\n+\t\t\t\tlock_delta_base_cache();\n+\t\t\t\t*oi->contentp = ent->data;\n+\t\t\t} else if (!oi->content_limit ||\n+\t\t\t\t\tent->size <= oi->content_limit) {\n \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n-\t\t\telse\n+\t\t\t} else {\n \t\t\t\t*oi->contentp = NULL; /* caller must stream */\n+\t\t\t}\n \t\t}\n \t} else if (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\ndiff --git a/packfile.h b/packfile.h\nindex eb18ec15db..94941bbe80 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -210,4 +210,8 @@ int is_promisor_object(const struct object_id *oid);\n int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t size_t idx_size, struct packed_git *p);\n \n+/*\n+ * release lock acquired via oi->direct_cache\n+ */\n+void unlock_delta_base_cache(void);\n #endif\n"},{"id":"501619","messageId":"20240823224630.1180772-7-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:26Z","receivedAt":"2024-08-23T22:47:20Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"For entries in the delta base cache, packed_to_object_type calls\ncan be omitted. This prepares us to bypass content_limit for\nnon-blob types in the following commit.\n\nSigned-off-by: Eric Wong \n---\n packfile.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 40c6c2e387..94d20034e4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1527,7 +1527,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n {\n \tstruct pack_window *w_curs = NULL;\n \toff_t curpos = obj_offset;\n-\tenum object_type type;\n+\tenum object_type type, final_type = OBJ_BAD;\n \tstruct delta_base_cache_entry *ent;\n \n \t/*\n@@ -1538,7 +1538,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \tent = get_delta_base_cache_entry(p, obj_offset);\n \tif (ent) {\n \t\toi->whence = OI_DBCACHED;\n-\t\ttype = ent->type;\n+\t\tfinal_type = type = ent->type;\n \t\tif (oi->sizep)\n \t\t\t*oi->sizep = ent->size;\n \t\tif (oi->contentp) {\n@@ -1556,6 +1556,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t} else if (oi->contentp && !oi->content_limit) {\n \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n \t\t\t\t\t\toi->sizep);\n+\t\tfinal_type = type;\n \t\tif (!*oi->contentp)\n \t\t\ttype = OBJ_BAD;\n \t} else {\n@@ -1585,6 +1586,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t&type, oi->sizep);\n+\t\t\t\tfinal_type = type;\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\n@@ -1606,17 +1608,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t}\n \n \tif (oi->typep || oi->type_name) {\n-\t\tenum object_type ptot;\n-\t\tptot = packed_to_object_type(r, p, obj_offset,\n-\t\t\t\t\t type, &w_curs, curpos);\n+\t\tif (final_type < 0)\n+\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n+\t\t\t\t\t\t type, &w_curs, curpos);\n \t\tif (oi->typep)\n-\t\t\t*oi->typep = ptot;\n+\t\t\t*oi->typep = final_type;\n \t\tif (oi->type_name) {\n-\t\t\tconst char *tn = type_name(ptot);\n+\t\t\tconst char *tn = type_name(final_type);\n \t\t\tif (tn)\n \t\t\t\tstrbuf_addstr(oi->type_name, tn);\n \t\t}\n-\t\tif (ptot < 0) {\n+\t\tif (final_type < 0) {\n \t\t\ttype = OBJ_BAD;\n \t\t\tgoto out;\n \t\t}\n"},{"id":"501620","messageId":"20240823224630.1180772-8-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 07/10] object_info: content_limit only applies to blobs","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:27Z","receivedAt":"2024-08-23T22:47:28Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Streaming is only supported for blobs, so we'd end up having to\nslurp all the other object types into memory regardless. So\nslurp all the non-blob types up front when requesting content\nsince we always handle them in-core, anyways.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 21 +++++++++++++++++++--\n object-file.c | 3 ++-\n packfile.c | 8 +++++---\n t/t1006-cat-file.sh | 19 ++++++++++++++++---\n 4 files changed, 42 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 8debcdca3e..2aedd62324 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -385,7 +385,24 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \tassert(data->info.typep);\n \n \tif (data->content) {\n-\t\tbatch_write(opt, data->content, data->size);\n+\t\tvoid *content = data->content;\n+\t\tunsigned long size = data->size;\n+\n+\t\tdata->content = NULL;\n+\t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n+\t\t\t\t\tdata->type == OBJ_TAG)) {\n+\t\t\tsize_t s = size;\n+\n+\t\t\tif (data->info.whence == OI_DBCACHED) {\n+\t\t\t\tcontent = xmemdupz(content, s);\n+\t\t\t\tdata->info.whence = OI_PACKED;\n+\t\t\t}\n+\n+\t\t\tcontent = replace_idents_using_mailmap(content, &s);\n+\t\t\tsize = cast_size_t_to_ulong(s);\n+\t\t}\n+\n+\t\tbatch_write(opt, content, size);\n \t\tswitch (data->info.whence) {\n \t\tcase OI_CACHED:\n \t\t\t/*\n@@ -395,7 +412,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tBUG(\"TODO OI_CACHED support not done\");\n \t\tcase OI_LOOSE:\n \t\tcase OI_PACKED:\n-\t\t\tFREE_AND_NULL(data->content);\n+\t\t\tfree(content);\n \t\t\tbreak;\n \t\tcase OI_DBCACHED:\n \t\t\tunlock_delta_base_cache();\ndiff --git a/object-file.c b/object-file.c\nindex 19100e823d..59842cfe1b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,\n \n \t\tif (!oi->contentp)\n \t\t\tbreak;\n-\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n+\t\tif (oi->content_limit && *oi->typep == OBJ_BLOB &&\n+\t\t\t\t*oi->sizep > oi->content_limit) {\n \t\t\tgit_inflate_end(&stream);\n \t\t\toi->contentp = NULL;\n \t\t\tgoto cleanup;\ndiff --git a/packfile.c b/packfile.c\nindex 94d20034e4..a592e0b32c 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1546,7 +1546,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\tif (oi->direct_cache) {\n \t\t\t\tlock_delta_base_cache();\n \t\t\t\t*oi->contentp = ent->data;\n-\t\t\t} else if (!oi->content_limit ||\n+\t\t\t} else if (type != OBJ_BLOB || !oi->content_limit ||\n \t\t\t\t\tent->size <= oi->content_limit) {\n \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n \t\t\t} else {\n@@ -1583,10 +1583,12 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t}\n \n \t\tif (oi->contentp) {\n-\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n+\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n+\t\t\t\t\t\t type, &w_curs, curpos);\n+\t\t\tif (final_type != OBJ_BLOB || (oi->sizep &&\n+\t\t\t\t\t*oi->sizep <= oi->content_limit)) {\n \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n \t\t\t\t\t\t\t&type, oi->sizep);\n-\t\t\t\tfinal_type = type;\n \t\t\t\tif (!*oi->contentp)\n \t\t\t\t\ttype = OBJ_BAD;\n \t\t\t} else {\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ff9bf213aa..841e8567e9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -622,20 +622,33 @@ test_expect_success 'confirm that neither loose blob is a delta' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup delta base tests' '\n+\tfoo=\"$(git rev-parse HEAD:foo)\" &&\n+\tfoo_plus=\"$(git rev-parse HEAD:foo-plus)\" &&\n+\tgit repack -ad\n+'\n+\n # To avoid relying too much on the current delta heuristics,\n # we will check only that one of the two objects is a delta\n # against the other, but not the order. We can do so by just\n # asking for the base of both, and checking whether either\n # oid appears in the output.\n test_expect_success '%(deltabase) reports packed delta bases' '\n-\tgit repack -ad &&\n \tgit cat-file --batch-check=\"%(deltabase)\" actual &&\n \t{\n-\t\tgrep \"$(git rev-parse HEAD:foo)\" actual ||\n-\t\tgrep \"$(git rev-parse HEAD:foo-plus)\" actual\n+\t\tgrep \"$foo\" actual || grep \"$foo_plus\" actual\n \t}\n '\n \n+test_expect_success 'delta base direct cache use succeeds w/o asserting' '\n+\tcommands=\"info $foo\n+info $foo_plus\n+contents $foo_plus\n+contents $foo\" &&\n+\techo \"$commands\" >in &&\n+\tgit cat-file --batch-command out\n+'\n+\n test_expect_success 'setup bogus data' '\n \tbogus_short_type=\"bogus\" &&\n \tbogus_short_content=\"bogus\" &&\n"},{"id":"501621","messageId":"20240823224630.1180772-9-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 08/10] cat-file: batch-command uses content_limit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:28Z","receivedAt":"2024-08-23T22:47:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"As with the normal `--batch' mode, we can use the content_limit\nround trip optimization to avoid a redundant lookup. The only\ntricky thing here is we need to enable/disable setting the\nobject_info.contentp field depending on whether we hit an `info'\nor `contents' command.\n\nt1006 is updated to ensure we can switch back and forth between\n`info' and `contents' commands without problems.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 32 ++++++--------------------------\n 1 file changed, 6 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 2aedd62324..067cdbdbf9 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -417,7 +417,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\tcase OI_DBCACHED:\n \t\t\tunlock_delta_base_cache();\n \t\t}\n-\t} else if (data->type == OBJ_BLOB) {\n+\t} else {\n+\t\tassert(data->type == OBJ_BLOB);\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n@@ -452,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tstream_blob(oid);\n \t\t}\n \t}\n-\telse {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\t\tvoid *contents;\n-\n-\t\tcontents = repo_read_object_file(the_repository, oid, &type,\n-\t\t\t\t\t\t &size);\n-\t\tif (!contents)\n-\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n-\n-\t\tif (use_mailmap) {\n-\t\t\tsize_t s = size;\n-\t\t\tcontents = replace_idents_using_mailmap(contents, &s);\n-\t\t\tsize = cast_size_t_to_ulong(s);\n-\t\t}\n-\n-\t\tif (type != data->type)\n-\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n-\t\tif (data->info.sizep && size != data->size && !use_mailmap)\n-\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n-\n-\t\tbatch_write(opt, contents, size);\n-\t\tfree(contents);\n-\t}\n }\n \n static void print_default_format(struct strbuf *scratch, struct expand_data *data,\n@@ -689,6 +666,7 @@ static void parse_cmd_contents(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_CONTENTS;\n+\tdata->info.contentp = &data->content;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -698,6 +676,7 @@ static void parse_cmd_info(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_INFO;\n+\tdata->info.contentp = NULL;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -839,7 +818,8 @@ static int batch_objects(struct batch_options *opt)\n \t * Likewise, grab the content in the initial request if it's small\n \t * and we're not planning to filter it.\n \t */\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n+\tif ((opt->batch_mode == BATCH_MODE_CONTENTS) ||\n+\t\t\t(opt->batch_mode == BATCH_MODE_QUEUE_AND_DISPATCH)) {\n \t\tdata.info.typep = &data.type;\n \t\tif (!opt->transform_mode) {\n \t\t\tdata.info.sizep = &data.size;\n"},{"id":"501622","messageId":"20240823224630.1180772-10-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 09/10] cat-file: batch_write: use size_t for length","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:29Z","receivedAt":"2024-08-23T22:47:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"fwrite(3) and write(2), and all of our wrappers for them use\nsize_t while object size is `unsigned long', so there's no\nexcuse to use a potentially smaller representation.\n\nSigned-off-by: Eric Wong \n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 067cdbdbf9..bf81054662 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -369,7 +369,7 @@ static void expand_format(struct strbuf *sb, const char *start,\n \t}\n }\n \n-static void batch_write(struct batch_options *opt, const void *data, int len)\n+static void batch_write(struct batch_options *opt, const void *data, size_t len)\n {\n \tif (opt->buffer_output) {\n \t\tif (fwrite(data, 1, len, stdout) != len)\n"},{"id":"501623","messageId":"20240823224630.1180772-11-e@80x24.org","threadId":"61778","inReplyTo":"20240823224630.1180772-1-e@80x24.org","subject":"[PATCH v2 10/10] cat-file: use writev(2) if available","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-23T22:46:30Z","receivedAt":"2024-08-23T22:47:49Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Using writev here is 20-40% faster than three write syscalls in\nsuccession for smaller (1-10k) objects in the delta base cache.\nThis advantage decreases as object sizes approach pipe size (64k\non Linux).\n\nwritev reduces wakeups and syscalls on the read side as well:\neach write(2) syscall may trigger one or more corresponding\nread(2) syscalls in the reader. Attempting atomicity in the\nwriter via writev also reduces the likelyhood of non-blocking\nreaders failing with EAGAIN and having to call poll||select\nbefore attempting to read again.\n\nUnfortunately, this turns into a small (1-3%) slowdown for\ngigantic objects of a megabyte or more even with after\nincreasing pipe size to 1MB via the F_SETPIPE_SZ fcntl(2) op.\nThis slowdown is acceptable to me since the vast majority of\nobjects are 64K or less for projects I've looked at.\n\nRelying on stdio buffering and fflush(3) after each response was\nconsidered for users without --buffer, but historically cat-file\ndefaults to being compatible with non-blocking stdout and able\nto poll(2) after hitting EAGAIN on write(2). Using stdio on\nfiles with the O_NONBLOCK flag is (AFAIK) unspecified and likely\nsubject to portability problems and thus avoided.\n\nSigned-off-by: Eric Wong \n---\n Makefile | 3 +++\n builtin/cat-file.c | 62 ++++++++++++++++++++++++++++++-------------\n config.mak.uname | 5 ++++\n git-compat-util.h | 10 +++++++\n wrapper.c | 18 +++++++++++++\n wrapper.h | 1 +\n write-or-die.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n write-or-die.h | 2 ++\n 8 files changed, 149 insertions(+), 18 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3eab701b10..c7a062de00 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1844,6 +1844,9 @@ ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n endif\n+ifdef HAVE_WRITEV\n+\tCOMPAT_CFLAGS += -DHAVE_WRITEV\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex bf81054662..016b7d26a7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -280,7 +280,7 @@ struct expand_data {\n \toff_t disk_size;\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n-\tvoid *content;\n+\tstruct git_iovec iov[3];\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -378,17 +378,42 @@ static void batch_write(struct batch_options *opt, const void *data, size_t len)\n \t\twrite_or_die(1, data, len);\n }\n \n-static void print_object_or_die(struct batch_options *opt, struct expand_data *data)\n+static void batch_writev(struct batch_options *opt, struct expand_data *data,\n+\t\t\tconst struct strbuf *hdr, size_t size)\n+{\n+\tdata->iov[0].iov_base = hdr->buf;\n+\tdata->iov[0].iov_len = hdr->len;\n+\tdata->iov[1].iov_len = size;\n+\n+\t/*\n+\t * Copying a (8|16)-byte iovec for a single byte is gross, but my\n+\t * attempt to stuff output_delim into the trailing NUL byte of\n+\t * iov[1].iov_base (and restoring it after writev(2) for the\n+\t * OI_DBCACHED case) to drop iovcnt from 3->2 wasn't faster.\n+\t */\n+\tdata->iov[2].iov_base = &opt->output_delim;\n+\tdata->iov[2].iov_len = 1;\n+\n+\tif (opt->buffer_output)\n+\t\tfwritev_or_die(stdout, data->iov, 3);\n+\telse\n+\t\twritev_or_die(1, data->iov, 3);\n+\n+\t/* writev_or_die may move iov[1].iov_base, so it's invalid */\n+\tdata->iov[1].iov_base = NULL;\n+}\n+\n+static void print_object_or_die(struct batch_options *opt,\n+\t\t\t\tstruct expand_data *data, struct strbuf *hdr)\n {\n \tconst struct object_id *oid = &data->oid;\n \n \tassert(data->info.typep);\n \n-\tif (data->content) {\n-\t\tvoid *content = data->content;\n+\tif (data->iov[1].iov_base) {\n+\t\tvoid *content = data->iov[1].iov_base;\n \t\tunsigned long size = data->size;\n \n-\t\tdata->content = NULL;\n \t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n \t\t\t\t\tdata->type == OBJ_TAG)) {\n \t\t\tsize_t s = size;\n@@ -399,10 +424,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t}\n \n \t\t\tcontent = replace_idents_using_mailmap(content, &s);\n+\t\t\tdata->iov[1].iov_base = content;\n \t\t\tsize = cast_size_t_to_ulong(s);\n \t\t}\n-\n-\t\tbatch_write(opt, content, size);\n+\t\tbatch_writev(opt, data, hdr, size);\n \t\tswitch (data->info.whence) {\n \t\tcase OI_CACHED:\n \t\t\t/*\n@@ -419,8 +444,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t}\n \t} else {\n \t\tassert(data->type == OBJ_BLOB);\n-\t\tif (opt->buffer_output)\n-\t\t\tfflush(stdout);\n \t\tif (opt->transform_mode) {\n \t\t\tchar *contents;\n \t\t\tunsigned long size;\n@@ -447,10 +470,15 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t\t\t oid_to_hex(oid), data->rest);\n \t\t\t} else\n \t\t\t\tBUG(\"invalid transform_mode: %c\", opt->transform_mode);\n-\t\t\tbatch_write(opt, contents, size);\n+\t\t\tdata->iov[1].iov_base = contents;\n+\t\t\tbatch_writev(opt, data, hdr, size);\n \t\t\tfree(contents);\n \t\t} else {\n+\t\t\tbatch_write(opt, hdr->buf, hdr->len);\n+\t\t\tif (opt->buffer_output)\n+\t\t\t\tfflush(stdout);\n \t\t\tstream_blob(oid);\n+\t\t\tbatch_write(opt, &opt->output_delim, 1);\n \t\t}\n \t}\n }\n@@ -519,12 +547,10 @@ static void batch_object_write(const char *obj_name,\n \t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n \n-\tbatch_write(opt, scratch->buf, scratch->len);\n-\n-\tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n-\t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, &opt->output_delim, 1);\n-\t}\n+\tif (opt->batch_mode == BATCH_MODE_CONTENTS)\n+\t\tprint_object_or_die(opt, data, scratch);\n+\telse\n+\t\tbatch_write(opt, scratch->buf, scratch->len);\n }\n \n static void batch_one_object(const char *obj_name,\n@@ -666,7 +692,7 @@ static void parse_cmd_contents(struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \topt->batch_mode = BATCH_MODE_CONTENTS;\n-\tdata->info.contentp = &data->content;\n+\tdata->info.contentp = &data->iov[1].iov_base;\n \tbatch_one_object(line, output, opt, data);\n }\n \n@@ -823,7 +849,7 @@ static int batch_objects(struct batch_options *opt)\n \t\tdata.info.typep = &data.type;\n \t\tif (!opt->transform_mode) {\n \t\t\tdata.info.sizep = &data.size;\n-\t\t\tdata.info.contentp = &data.content;\n+\t\t\tdata.info.contentp = &data.iov[1].iov_base;\n \t\t\tdata.info.content_limit = big_file_threshold;\n \t\t\tdata.info.direct_cache = 1;\n \t\t}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 85d63821ec..8ce8776657 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -69,6 +69,7 @@ ifeq ($(uname_S),Linux)\n \t\tBASIC_CFLAGS += -std=c99\n endif\n \tLINK_FUZZ_PROGRAMS = YesPlease\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n@@ -77,6 +78,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)\n \tDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n \tLIBC_CONTAINS_LIBINTL = YesPlease\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),UnixWare)\n \tCC = cc\n@@ -292,6 +294,7 @@ ifeq ($(uname_S),FreeBSD)\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -307,6 +310,7 @@ ifeq ($(uname_S),OpenBSD)\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n \tFILENO_IS_A_MACRO = UnfortunatelyYes\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),MirBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -329,6 +333,7 @@ ifeq ($(uname_S),NetBSD)\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n \tCSPRNG_METHOD = arc4random\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/exe\n+\tHAVE_WRITEV = YesPlease\n endif\n ifeq ($(uname_S),AIX)\n \tDEFAULT_PAGER = more\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ca7678a379..afde8abc99 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -388,6 +388,16 @@ static inline int git_setitimer(int which UNUSED,\n #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)\n #endif\n \n+#ifdef HAVE_WRITEV\n+#include \n+#define git_iovec iovec\n+#else /* !HAVE_WRITEV */\n+struct git_iovec {\n+\tvoid *iov_base;\n+\tsize_t iov_len;\n+};\n+#endif /* !HAVE_WRITEV */\n+\n #ifndef NO_LIBGEN_H\n #include \n #else\ndiff --git a/wrapper.c b/wrapper.c\nindex f87d90bf57..066c772145 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -262,6 +262,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+#ifdef HAVE_WRITEV\n+ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt)\n+{\n+\twhile (1) {\n+\t\tssize_t nr = writev(fd, iov, iovcnt);\n+\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (handle_nonblock(fd, POLLOUT, errno))\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\treturn nr;\n+\t}\n+}\n+#endif /* !HAVE_WRITEV */\n+\n /*\n * xpread() is the same as pread(), but it automatically restarts pread()\n * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\ndiff --git a/wrapper.h b/wrapper.h\nindex 1b2b047ea0..3d33c63d4f 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_\n int xopen(const char *path, int flags, ...);\n ssize_t xread(int fd, void *buf, size_t len);\n ssize_t xwrite(int fd, const void *buf, size_t len);\n+ssize_t xwritev(int fd, const struct git_iovec *, int iovcnt);\n ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n int xdup(int fd);\n FILE *xfopen(const char *path, const char *mode);\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 01a9a51fa2..227b051165 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -107,3 +107,69 @@ void fflush_or_die(FILE *f)\n \tif (fflush(f))\n \t\tdie_errno(\"fflush error\");\n }\n+\n+void fwritev_or_die(FILE *fp, const struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < iovcnt; i++) {\n+\t\tsize_t n = iov[i].iov_len;\n+\n+\t\tif (fwrite(iov[i].iov_base, 1, n, fp) != n)\n+\t\t\tdie_errno(\"unable to write to FD=%d\", fileno(fp));\n+\t}\n+}\n+\n+/*\n+ * note: we don't care about atomicity from writev(2) right now.\n+ * The goal is to avoid allocations+copies in the writer and\n+ * reduce wakeups+syscalls in the reader.\n+ * n.b. @iov is not const since we modify it to avoid allocating\n+ * on partial write.\n+ */\n+#ifdef HAVE_WRITEV\n+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\twhile (iovcnt > 0) {\n+\t\tssize_t n = xwritev(fd, iov, iovcnt);\n+\n+\t\t/* EINVAL happens when sum of iov_len exceeds SSIZE_MAX */\n+\t\tif (n < 0 && errno == EINVAL)\n+\t\t\tn = xwrite(fd, iov[0].iov_base, iov[0].iov_len);\n+\t\tif (n < 0) {\n+\t\t\tcheck_pipe(errno);\n+\t\t\tdie_errno(\"writev error\");\n+\t\t} else if (!n) {\n+\t\t\terrno = ENOSPC;\n+\t\t\tdie_errno(\"writev_error\");\n+\t\t}\n+\t\t/* skip fully written iovs, retry from the first partial iov */\n+\t\tfor (i = 0; i < iovcnt; i++) {\n+\t\t\tif (n >= iov[i].iov_len) {\n+\t\t\t\tn -= iov[i].iov_len;\n+\t\t\t} else {\n+\t\t\t\tiov[i].iov_len -= n;\n+\t\t\t\tiov[i].iov_base = (char *)iov[i].iov_base + n;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tiovcnt -= i;\n+\t\tiov += i;\n+\t}\n+}\n+#else /* !HAVE_WRITEV */\n+\n+/*\n+ * n.b. don't use stdio fwrite here even if it's faster, @fd may be\n+ * non-blocking and stdio isn't equipped for EAGAIN\n+ */\n+void writev_or_die(int fd, struct git_iovec *iov, int iovcnt)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < iovcnt; i++)\n+\t\twrite_or_die(fd, iov[i].iov_base, iov[i].iov_len);\n+}\n+#endif /* !HAVE_WRITEV */\ndiff --git a/write-or-die.h b/write-or-die.h\nindex 65a5c42a47..20abec211c 100644\n--- a/write-or-die.h\n+++ b/write-or-die.h\n@@ -7,6 +7,8 @@ void fprintf_or_die(FILE *, const char *fmt, ...);\n void fwrite_or_die(FILE *f, const void *buf, size_t count);\n void fflush_or_die(FILE *f);\n void write_or_die(int fd, const void *buf, size_t count);\n+void writev_or_die(int fd, struct git_iovec *, int iovcnt);\n+void fwritev_or_die(FILE *, const struct git_iovec *, int iovcnt);\n \n /*\n * These values are used to help identify parts of a repository to fsync.\n"},{"id":"501676","messageId":"xmqq1q2bmdfy.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-4-e@80x24.org","subject":"Re: [PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T16:55:13Z","receivedAt":"2024-08-26T16:55:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> object-file.c::loose_object_info() accepts objects matching\n> content_limit exactly, so it follows packfile handling allows\n> slurping objects which match loose object handling and slurp\n> objects with size matching the content_limit exactly.\n>\n> This change is merely for consistency with the majority of\n> existing code and there is no user visible change in nearly all\n> cases. The only exception being the corner case when the object\n> size matches content_limit exactly where users will see a\n> speedup from avoiding an extra lookup.\n>\n> Signed-off-by: Eric Wong \n> ---\n\nI would have preferred to see this (and also \"is oi->content_limit\nzero?\" check I mentioned earlier) as part of the previous step,\nwhich added this comparison that is not consistent with the majority\nof existing code. It's not like importing from an external project\nwe communicate with only occasionally, in which case we may want to\nimport \"pristine\" source and fix it up separetly in order to make it\neasier to re-import updated material.\n\n> packfile.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/packfile.c b/packfile.c\n> index c12a0515b3..8ec86d2d69 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1557,7 +1557,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t}\n> \n> \t\tif (oi->contentp) {\n> -\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n> +\t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n> \t\t\t\t\t\t\t\t oi->sizep, &type);\n> \t\t\t\tif (!*oi->contentp)\n"},{"id":"501677","messageId":"xmqqjzg3ky77.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-5-e@80x24.org","subject":"Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T17:09:48Z","receivedAt":"2024-08-26T17:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> We need to check delta_base_cache anyways to fill in the\n> `whence' field in `struct object_info'. Inlining (and getting\n> rid of) cache_or_unpack_entry() makes it easier to only do the\n> hashmap lookup once and avoid a redundant lookup later on.\n>\n> This code reorganization will also make an optimization to\n> use the cache entry directly easier to implement in the next\n> commit.\n\n\"cache entry\" -> \"cached entry\"; we tend to use \"cache entry\"\nexclusively to mean an entry in the in-core index structure,\nand not the cached objects held in the object layer.\n\n> {\n> \tstruct pack_window *w_curs = NULL;\n> -\tunsigned long size;\n> \toff_t curpos = obj_offset;\n> \tenum object_type type;\n> +\tstruct delta_base_cache_entry *ent;\n> \n> \t/*\n> \t * We always get the representation type, but only convert it to\n> \t * a \"real\" type later if the caller is interested.\n> \t */\n> -\tif (oi->contentp && !oi->content_limit) {\n> -\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n> -\t\t\t\t\t\t &type);\n> +\toi->whence = OI_PACKED;\n> +\tent = get_delta_base_cache_entry(p, obj_offset);\n> +\tif (ent) {\n> +\t\toi->whence = OI_DBCACHED;\n\nOK. This is very straight-forward. It is packed but if we grabbed\nit from the delta-base-cache, that is the only case we know it is\ndbcached.\n\n> +\t\ttype = ent->type;\n> +\t\tif (oi->sizep)\n> +\t\t\t*oi->sizep = ent->size;\n> +\t\tif (oi->contentp) {\n> +\t\t\tif (!oi->content_limit ||\n> +\t\t\t\t\tent->size <= oi->content_limit)\n> +\t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n> +\t\t\telse\n> +\t\t\t\t*oi->contentp = NULL; /* caller must stream */\n\nThis assignment of NULL is more explicit than the original; is it\nbecause the original assumed that *(oi->contentp) is initialized to\nNULL if oi->contentp asks us to give the contents?\n\n> +\t} else if (oi->contentp && !oi->content_limit) {\n> +\t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n> +\t\t\t\t\t\toi->sizep);\n> \t\tif (!*oi->contentp)\n> \t\t\ttype = OBJ_BAD;\n\nNice. The code structure is still easy to follow, even though the\nif/else cascade here are organized differently with more cases (used\nto be \"are we peeking the contents, or not?\"---now it is \"do this if\nwe can grab from the delta base cache, do one of these other things\nif we have go to the packfile\").\n\n"},{"id":"501678","messageId":"xmqqcylvky69.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-3-e@80x24.org","subject":"Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T17:10:22Z","receivedAt":"2024-08-26T17:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> From: Jeff King \n>\n> Avoid unnecessary round trips to the object store to speed\n> up cat-file contents retrievals. The majority of packed objects\n> don't benefit from the streaming interface at all and we end up\n> having to load them in core anyways to satisfy our streaming\n> API.\n\nWhat I found missing from the description is something like ...\n\n The new trick used is to teach oid_object_info_extended() that a\n non-NULL oi->contentp that means \"grab the contents of the objects\n here\" can be told to refrain from grabbing an object that is too\n large.\n\n> diff --git a/object-file.c b/object-file.c\n> index 065103be3e..1cc29c3c58 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,\n> \n> \t\tif (!oi->contentp)\n> \t\t\tbreak;\n> +\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n\nI cannot convince myself enough to say \"content limit\" is a great\nname. It invites \"limited by what? text files are allowed but\nimages are not?\".\n\n> diff --git a/object-store-ll.h b/object-store-ll.h\n> index c5f2bb2fc2..b71a15f590 100644\n> --- a/object-store-ll.h\n> +++ b/object-store-ll.h\n> @@ -289,6 +289,7 @@ struct object_info {\n> \tstruct object_id *delta_base_oid;\n> \tstruct strbuf *type_name;\n> \tvoid **contentp;\n> +\tsize_t content_limit;\n> \n> \t/* Response */\n> \tenum {\n> diff --git a/packfile.c b/packfile.c\n> index 4028763947..c12a0515b3 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t * We always get the representation type, but only convert it to\n> \t * a \"real\" type later if the caller is interested.\n> \t */\n> -\tif (oi->contentp) {\n> +\tif (oi->contentp && !oi->content_limit) {\n> \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n> \t\t\t\t\t\t &type);\n> \t\tif (!*oi->contentp)\n> @@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t\t\t*oi->sizep = size;\n> \t\t\t}\n> \t\t}\n> +\n> +\t\tif (oi->contentp) {\n> +\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n\nIt happens that with the current code structure, at this point,\noi->content_limit is _always_ non-zero. But it felt somewhat\nfragile to rely on it, and I would have appreciated if this was\nwritten with an explicit check for oi->content_limit, just like how\nit is done in loose_object_info() function.\n\nOther than that, looking very good.\n\nThanks.\n\n\n> +\t\t\t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset,\n> +\t\t\t\t\t\t\t\t oi->sizep, &type);\n> +\t\t\t\tif (!*oi->contentp)\n> +\t\t\t\t\ttype = OBJ_BAD;\n> +\t\t\t} else {\n> +\t\t\t\t*oi->contentp = NULL;\n> +\t\t\t}\n> +\t\t}\n> \t}\n> \n> \tif (oi->disk_sizep) {\n"},{"id":"501700","messageId":"xmqqjzg3geea.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-6-e@80x24.org","subject":"Re: [PATCH v2 05/10] cat-file: use delta_base_cache entries directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T21:31:09Z","receivedAt":"2024-08-26T21:31:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> For objects already in the delta_base_cache, we can safely use\n> one entry at-a-time directly to avoid the malloc+memcpy+free\n> overhead. For a 1MB delta base object, this eliminates the\n> speed penalty of duplicating large objects into memory and\n> speeds up those 1MB delta base cached content retrievals by\n> roughly 30%.\n>\n> While only 2-7% of objects are delta bases in repos I've looked\n> at, this avoids up to 96MB of duplicated memory in the worst\n> case with the default git config.\n>\n> The new delta_base_cache_lock is a simple single-threaded\n> assertion to ensure cat-file (and similar) is the exclusive user\n> of the delta_base_cache. In other words, we cannot have diff\n> or similar commands using two or more entries directly from the\n> delta base cache. The new lock has nothing to do with parallel\n> access via multiple threads at the moment.\n>\n> Signed-off-by: Eric Wong \n> ---\n> builtin/cat-file.c | 16 +++++++++++++++-\n> object-file.c | 5 +++++\n> object-store-ll.h | 8 ++++++++\n> packfile.c | 33 ++++++++++++++++++++++++++++++---\n> packfile.h | 4 ++++\n> 5 files changed, 62 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index bc4bb89610..8debcdca3e 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -386,7 +386,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \n> \tif (data->content) {\n> \t\tbatch_write(opt, data->content, data->size);\n> -\t\tFREE_AND_NULL(data->content);\n> +\t\tswitch (data->info.whence) {\n> +\t\tcase OI_CACHED:\n> +\t\t\t/*\n> +\t\t\t * only blame uses OI_CACHED atm, so it's unlikely\n> +\t\t\t * we'll ever hit this path\n> +\t\t\t */\n> +\t\t\tBUG(\"TODO OI_CACHED support not done\");\n> +\t\tcase OI_LOOSE:\n> +\t\tcase OI_PACKED:\n> +\t\t\tFREE_AND_NULL(data->content);\n> +\t\t\tbreak;\n> +\t\tcase OI_DBCACHED:\n> +\t\t\tunlock_delta_base_cache();\n> +\t\t}\n> \t} else if (data->type == OBJ_BLOB) {\n> \t\tif (opt->buffer_output)\n> \t\t\tfflush(stdout);\n> @@ -815,6 +828,7 @@ static int batch_objects(struct batch_options *opt)\n> \t\t\tdata.info.sizep = &data.size;\n> \t\t\tdata.info.contentp = &data.content;\n> \t\t\tdata.info.content_limit = big_file_threshold;\n> +\t\t\tdata.info.direct_cache = 1;\n> \t\t}\n> \t}\n> \n> diff --git a/object-file.c b/object-file.c\n> index 1cc29c3c58..19100e823d 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1586,6 +1586,11 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\t\toidclr(oi->delta_base_oid, the_repository->hash_algo);\n> \t\tif (oi->type_name)\n> \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n> +\t\t/*\n> +\t\t * Currently `blame' is the only command which creates\n> +\t\t * OI_CACHED, and direct_cache is only used by `cat-file'.\n> +\t\t */\n> +\t\tassert(!oi->direct_cache);\n\nIf \"git cat-file\" ever gets enhanced to also use\npretend_object_file(), this assumption gets violated. \n\nWhat happens then? Would we return inconsistent answer to the\ncaller that may result in garbage output, breaking the downstream\nprocess somehow? If so, I'd prefer to see this guarded more\nexplicitly with \"if (...) BUG()\" than assert() here.\n\n> \t\tif (oi->contentp)\n> \t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n\nRegardless of the answer of the question above about assert(),\nshouldn't the step [02/10] of this series also have been made to pay\nattention to oi->content_limit mechanism? It looks inconsistent.\n\n> diff --git a/object-store-ll.h b/object-store-ll.h\n> index b71a15f590..669bb93784 100644\n> --- a/object-store-ll.h\n> +++ b/object-store-ll.h\n> @@ -298,6 +298,14 @@ struct object_info {\n> \t\tOI_PACKED,\n> \t\tOI_DBCACHED\n> \t} whence;\n> +\n> +\t/*\n> +\t * Set if caller is able to use OI_DBCACHED entries without copying.\n> +\t * This only applies to OI_DBCACHED entries at the moment,\n> +\t * not OI_CACHED or any other type of entry.\n> +\t */\n> +\tunsigned direct_cache:1;\n\nThis is a \"response\" from the API to the caller, but \"Set if ...\"\nsounds as if you are telling the caller what to do. Would the\ncaller set this bit when making a request to say \"I am cat-file and\nI use entry before making any other requests to the object layer so\nI am sure object layer will not evict the entry I am using\"? No,\nthat is not a \"response\".\n\nYou seem to set this bit in batch_objects(), so it does sound like\nthat the bit is expected to be set by the caller to tell the API\nsomething. If that is the case, then (1) move it to \"request\" part\nof the object_info structure, and (2) define what it means to be\n\"able to use ... without copying\". Mechanically, it may mean\n\"contentp directly points into the delta base cache\", but what\nimplication does it have to callers? If the caller obtains such a\npointer in .contentp, what is required for its access pattern to\nmake accessing the pointer safe? The caller cannot use the pointed\nmemory after it accesses another object? What is the definition of\n\"access\" in the context of this discussion? Does \"checking for\nexistence of an object\" count as an access?\n\n> +static void lock_delta_base_cache(void)\n> +{\n> +\tdelta_base_cache_lock++;\n> +\tassert(delta_base_cache_lock == 1);\n> +}\n> +\n> +void unlock_delta_base_cache(void)\n> +{\n> +\tdelta_base_cache_lock--;\n> +\tassert(delta_base_cache_lock == 0);\n> +}\n\nI view assert() validating precondition for the control flow to pass\nthe point in the code path, so the above looks somewhat surprising.\n\nShouldn't we instead checking the current value first and then only\nif the caller is _allowed_ to lock/unlock from that state (i.e. the\nlock is not held/the lock is held), increment/decrement the variable?\n\n> +\t\t\t/* ignore content_limit if avoiding copy from cache */\n> +\t\t\tif (oi->direct_cache) {\n> +\t\t\t\tlock_delta_base_cache();\n> +\t\t\t\t*oi->contentp = ent->data;\n> +\t\t\t} else if (!oi->content_limit ||\n> +\t\t\t\t\tent->size <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n\nThis somehow looks making the memory ownership rules confusing.\n\nHow does the caller know when it can free(oi->contentp) after a call\nreturns? If it sets direct_cache and got a non-NULL *oi->contentp\nback, is it always pointing at a borrowed piece memory, so that it\ndoes not have to worry about freeing, but if it didn't set\ndirect_cache, it can be holding an allocated piece of memory and you\nare responsible for freeing it?\n\nThe following is a tangent that is not necessary to be addressed in\nthis series, but since I've spent time to think about it already,\nlet me record it as #leftoverbits here.\n\nWhatever the answers to the earlier questions on the comment on the\ndirect_cache member are, I wonder if the access pattern of the\ncaller that satisfies such requirement allows a lot simpler\nsolution. Would it give us similar performance boost by (morally)\nreverting 85fe35ab (cache_or_unpack_entry: drop keep_cache\nparameter, 2016-08-22) to resurrect the \"pass ownership of a delta\nbase cache entry to the caller\" feature? We give the content to the\ncaller of object_info, and evict the cached delta base. Ideally, we\nshouldn't have to ask the caller to do anything special, other than\njust setting contentp to non-NULL (and optionally setting\ncontent_limit), and the choice of copying or passing the ownership\nshould be made in the object access layer, which knows how big the\nobject is (e.g., small things may be easier to recreate), how deep\nthe delta chain the entry in the delta base cache is (e.g., a delta\nbase object that is simply deflated can be recreated cheaply, while\none based on top of 100-delta deep chain is very expensive to\nrecreate), etc.\n\n"},{"id":"501701","messageId":"xmqq7cc3gdhc.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-7-e@80x24.org","subject":"Re: [PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T21:50:55Z","receivedAt":"2024-08-26T21:50:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Subject: Re: [PATCH v2 06/10] packfile: packed_object_info avoids packed_to_object_type\n\nI was confused if \"avoids\" is talking about the status quo, avoiding\nis the bad thing, and this patch is about fixing that so that it\nwould not avoid. That is not what this does.\n\n Subject: packfile: skip packed_to_object_type call in packed_object_info\n\nor something, perhaps?\n\n> For entries in the delta base cache, packed_to_object_type calls\n> can be omitted.\n\n\"... because an entry in delta base cache knows what the final type\nof the object is\"?\n\nI had \"what the code should do in the error cases\" question in a few\nplaces. Please describe (and justify if needed) the choice of\nbehaviour in the proposed log message.\n\n> @@ -1538,7 +1538,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \tent = get_delta_base_cache_entry(p, obj_offset);\n> \tif (ent) {\n> \t\toi->whence = OI_DBCACHED;\n> -\t\ttype = ent->type;\n> +\t\tfinal_type = type = ent->type;\n> \t\tif (oi->sizep)\n> \t\t\t*oi->sizep = ent->size;\n> \t\tif (oi->contentp) {\n\nOK.\n\n> @@ -1556,6 +1556,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t} else if (oi->contentp && !oi->content_limit) {\n> \t\t*oi->contentp = unpack_entry(r, p, obj_offset, &type,\n> \t\t\t\t\t\toi->sizep);\n> +\t\tfinal_type = type;\n> \t\tif (!*oi->contentp)\n> \t\t\ttype = OBJ_BAD;\n\nDo we want to still yield the \"type\" not \"OBJ_BAD\" in final_type in\nthis case?\n\n> @@ -1585,6 +1586,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t\tif (oi->sizep && *oi->sizep <= oi->content_limit) {\n> \t\t\t\t*oi->contentp = unpack_entry(r, p, obj_offset,\n> \t\t\t\t\t\t\t&type, oi->sizep);\n> +\t\t\t\tfinal_type = type;\n> \t\t\t\tif (!*oi->contentp)\n> \t\t\t\t\ttype = OBJ_BAD;\n\nDitto.\n\n> @@ -1606,17 +1608,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t}\n> \n> \tif (oi->typep || oi->type_name) {\n> -\t\tenum object_type ptot;\n> -\t\tptot = packed_to_object_type(r, p, obj_offset,\n> -\t\t\t\t\t type, &w_curs, curpos);\n> +\t\tif (final_type < 0)\n> +\t\t\tfinal_type = packed_to_object_type(r, p, obj_offset,\n> +\t\t\t\t\t\t type, &w_curs, curpos);\n> \t\tif (oi->typep)\n> -\t\t\t*oi->typep = ptot;\n> +\t\t\t*oi->typep = final_type;\n> \t\tif (oi->type_name) {\n> -\t\t\tconst char *tn = type_name(ptot);\n> +\t\t\tconst char *tn = type_name(final_type);\n> \t\t\tif (tn)\n> \t\t\t\tstrbuf_addstr(oi->type_name, tn);\n> \t\t}\n> -\t\tif (ptot < 0) {\n> +\t\tif (final_type < 0) {\n> \t\t\ttype = OBJ_BAD;\n> \t\t\tgoto out;\n> \t\t}\n"},{"id":"501702","messageId":"xmqqwmk3eydh.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-8-e@80x24.org","subject":"Re: [PATCH v2 07/10] object_info: content_limit only applies to blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T22:02:34Z","receivedAt":"2024-08-26T22:02:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Streaming is only supported for blobs, so we'd end up having to\n> slurp all the other object types into memory regardless. So\n> slurp all the non-blob types up front when requesting content\n> since we always handle them in-core, anyways.\n>\n> Signed-off-by: Eric Wong \n> ---\n> builtin/cat-file.c | 21 +++++++++++++++++++--\n> object-file.c | 3 ++-\n> packfile.c | 8 +++++---\n> t/t1006-cat-file.sh | 19 ++++++++++++++++---\n> 4 files changed, 42 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 8debcdca3e..2aedd62324 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -385,7 +385,24 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \tassert(data->info.typep);\n> \n> \tif (data->content) {\n> -\t\tbatch_write(opt, data->content, data->size);\n> +\t\tvoid *content = data->content;\n> +\t\tunsigned long size = data->size;\n> +\n> +\t\tdata->content = NULL;\n> +\t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n> +\t\t\t\t\tdata->type == OBJ_TAG)) {\n\nLine-wrap at higher level in the parse tree, i.e.\n\n\t\tif (use_mailmap &&\n\t\t (data->type == OBJ_COMMIT || data->type == OBJ_TAG)) {\n\n> +\t\t\tsize_t s = size;\n> +\n> +\t\t\tif (data->info.whence == OI_DBCACHED) {\n> +\t\t\t\tcontent = xmemdupz(content, s);\n> +\t\t\t\tdata->info.whence = OI_PACKED;\n> +\t\t\t}\n> +\n> +\t\t\tcontent = replace_idents_using_mailmap(content, &s);\n> +\t\t\tsize = cast_size_t_to_ulong(s);\n\nThis piece of code does look good, but it makes me wonder where the\nneed to duplicate the code comes from. Before this patch, if\nsomebody asked use_mailmap, we must have been making the\nreplace_idents_using_mailmap() call and showing the result elsewhere\nin the existing code, and it is curious why that code path is not\nremoved, or if we can share more common code with that code paths\nhere.\n\n> +\t\t}\n> +\n> +\t\tbatch_write(opt, content, size);\n> \t\tswitch (data->info.whence) {\n> \t\tcase OI_CACHED:\n> \t\t\t/*\n> @@ -395,7 +412,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\t\tBUG(\"TODO OI_CACHED support not done\");\n> \t\tcase OI_LOOSE:\n> \t\tcase OI_PACKED:\n> -\t\t\tFREE_AND_NULL(data->content);\n> +\t\t\tfree(content);\n\ndata->content has been nuked earlier, and content that holds the new\ncopy with replaced one is freed (and the original data->content was\nfreed by replace_idents_using_mailmap(), so there is no leak or\ndouble-free here. Good.\n\n> diff --git a/object-file.c b/object-file.c\n> index 19100e823d..59842cfe1b 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1492,7 +1492,8 @@ static int loose_object_info(struct repository *r,\n> \n> \t\tif (!oi->contentp)\n> \t\t\tbreak;\n> -\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n> +\t\tif (oi->content_limit && *oi->typep == OBJ_BLOB &&\n> +\t\t\t\t*oi->sizep > oi->content_limit) {\n> \t\t\tgit_inflate_end(&stream);\n> \t\t\toi->contentp = NULL;\n> \t\t\tgoto cleanup;\n\nOK, so non-BLOB objects we would fall through this if/else cascade\nand inflate them fully.\n\n"},{"id":"501703","messageId":"xmqqo75fexvl.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-9-e@80x24.org","subject":"Re: [PATCH v2 08/10] cat-file: batch-command uses content_limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T22:13:18Z","receivedAt":"2024-08-26T22:13:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 2aedd62324..067cdbdbf9 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -417,7 +417,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\tcase OI_DBCACHED:\n> \t\t\tunlock_delta_base_cache();\n> \t\t}\n> -\t} else if (data->type == OBJ_BLOB) {\n> +\t} else {\n> +\t\tassert(data->type == OBJ_BLOB);\n> \t\tif (opt->buffer_output)\n> \t\t\tfflush(stdout);\n> \t\tif (opt->transform_mode) {\n\nHmph, because until this step, the control could reach this function\nwith data->content NULL for a non-BLOB type, even though we\neliminated that with the previous step for \"--batch\" command user.\nThis step covers \"--batch-command\" user to also force the\ndata->content to be populated, so we no longer will see anything but\nBLOB when data->content is NULL.\n\nThat sounds correct with the current code, but it somehow feels a\nbit too subtle to my taste. In addition to an assert(), future\nreaders of this code would deserve a comment describing why we can\nsafely assume that blobs are the only types we will see here. That\nway, they can tell when they add another command that ends up calling\nthis function that they too will need to do the contentp thing.\n\n> @@ -452,30 +453,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\t\tstream_blob(oid);\n> \t\t}\n> \t}\n> -\telse {\n> -\t\tenum object_type type;\n> -\t\tunsigned long size;\n> -\t\tvoid *contents;\n> -\n> -\t\tcontents = repo_read_object_file(the_repository, oid, &type,\n> -\t\t\t\t\t\t &size);\n> -\t\tif (!contents)\n> -\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n> -\n> -\t\tif (use_mailmap) {\n> -\t\t\tsize_t s = size;\n> -\t\t\tcontents = replace_idents_using_mailmap(contents, &s);\n> -\t\t\tsize = cast_size_t_to_ulong(s);\n> -\t\t}\n> -\n> -\t\tif (type != data->type)\n> -\t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n> -\t\tif (data->info.sizep && size != data->size && !use_mailmap)\n> -\t\t\tdie(\"object %s changed size!?\", oid_to_hex(oid));\n> -\n> -\t\tbatch_write(opt, contents, size);\n> -\t\tfree(contents);\n> -\t}\n\nAnd it certainly is nice that we can get rid of this fallback code.\n\nBy the way, the previous step sshould rename the .content_limit\nmember to make it clear that (1) it is about the size limit, and (2)\nit only applies to blobs. Ideally (1) should be done much earlier\nin the series, and (2) must be done when the code starts to ignore\nthe member for non-blob types.\n\nThanks.\n"},{"id":"501705","messageId":"xmqqcyluga1e.fsf@gitster.g","threadId":"61778","inReplyTo":"xmqqjzg3geea.fsf@gitster.g","subject":"Re: [PATCH v2 05/10] cat-file: use delta_base_cache entries directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T23:05:17Z","receivedAt":"2024-08-26T23:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\t/*\n>> +\t * Set if caller is able to use OI_DBCACHED entries without copying.\n>> +\t * This only applies to OI_DBCACHED entries at the moment,\n>> +\t * not OI_CACHED or any other type of entry.\n>> +\t */\n>> +\tunsigned direct_cache:1;\n> ...\n> You seem to set this bit in batch_objects(), so it does sound like\n> that the bit is expected to be set by the caller to tell the API\n> something. If that is the case, then (1) move it to \"request\" part\n> of the object_info structure, and (2) define what it means to be\n> \"able to use ... without copying\". Mechanically, it may mean\n> \"contentp directly points into the delta base cache\", but what\n> implication does it have to callers? If the caller obtains such a\n> pointer in .contentp, what is required for its access pattern to\n> make accessing the pointer safe? The caller cannot use the pointed\n> memory after it accesses another object? What is the definition of\n> \"access\" in the context of this discussion? Does \"checking for\n> existence of an object\" count as an access?\n\nAnother thing that makes me worry about this approach (as opposed to\na much simpler to reason about alternative like \"transfer ownership\nto the caller\") is that it is very hard to guarantee that other\nobject access that is not under caller's control will never happen,\nand it is even harder to make sure that the code will keep giving\nsuch a guarantee.\n\nIn other words, the arrangement smells a bit too brittle.\n\nFor example, would it be possible for a lazy fetching of (unrelated)\nobjects triggers after the caller asks about an object and borrows a\npointer into delta base cache, and would it scramble the entries of\ndelta base cache, silently invalidating the borrowed piece of\nmemory? Would it be possible for the textconv and other filtering\nmechanism driven by the attribute system trigger access to\nconfigured attribute blob, which has to be lazily fetched from other\nplace?\n\nThanks.\n"},{"id":"501712","messageId":"xmqqr0aad078.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-10-e@80x24.org","subject":"Re: [PATCH v2 09/10] cat-file: batch_write: use size_t for length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T05:06:03Z","receivedAt":"2024-08-27T05:06:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> fwrite(3) and write(2), and all of our wrappers for them use\n> size_t while object size is `unsigned long', so there's no\n> excuse to use a potentially smaller representation.\n>\n> Signed-off-by: Eric Wong \n> ---\n> builtin/cat-file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nObviously correct. Thanks.\n\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 067cdbdbf9..bf81054662 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -369,7 +369,7 @@ static void expand_format(struct strbuf *sb, const char *start,\n> \t}\n> }\n> \n> -static void batch_write(struct batch_options *opt, const void *data, int len)\n> +static void batch_write(struct batch_options *opt, const void *data, size_t len)\n> {\n> \tif (opt->buffer_output) {\n> \t\tif (fwrite(data, 1, len, stdout) != len)\n"},{"id":"501713","messageId":"xmqq1q2acyjo.fsf@gitster.g","threadId":"61778","inReplyTo":"20240823224630.1180772-11-e@80x24.org","subject":"Re: [PATCH v2 10/10] cat-file: use writev(2) if available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T05:41:47Z","receivedAt":"2024-08-27T05:41:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index bf81054662..016b7d26a7 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -280,7 +280,7 @@ struct expand_data {\n> \toff_t disk_size;\n> \tconst char *rest;\n> \tstruct object_id delta_base_oid;\n> -\tvoid *content;\n> +\tstruct git_iovec iov[3];\n\n\nThe earlier content pointer hinted that the caller that obtained\ndata into this structure from the object layer can use it for any\npurpose that suits it, but using git_iovec structure screams that\n\"we are going to write this thing out!\". As \"expand_data\" is about\nwhat we are going to write out from cat-file anyway, is probably OK.\n\nHaving said that ...\n\n> -static void print_object_or_die(struct batch_options *opt, struct expand_data *data)\n> +static void batch_writev(struct batch_options *opt, struct expand_data *data,\n> +\t\t\tconst struct strbuf *hdr, size_t size)\n> +{\n> +\tdata->iov[0].iov_base = hdr->buf;\n> +\tdata->iov[0].iov_len = hdr->len;\n> +\tdata->iov[1].iov_len = size;\n> +\n> +\t/*\n> +\t * Copying a (8|16)-byte iovec for a single byte is gross, but my\n> +\t * attempt to stuff output_delim into the trailing NUL byte of\n> +\t * iov[1].iov_base (and restoring it after writev(2) for the\n> +\t * OI_DBCACHED case) to drop iovcnt from 3->2 wasn't faster.\n> +\t */\n> +\tdata->iov[2].iov_base = &opt->output_delim;\n> +\tdata->iov[2].iov_len = 1;\n> +\tif (opt->buffer_output)\n> +\t\tfwritev_or_die(stdout, data->iov, 3);\n> +\telse\n> +\t\twritev_or_die(1, data->iov, 3);\n> +\n> +\t/* writev_or_die may move iov[1].iov_base, so it's invalid */\n> +\tdata->iov[1].iov_base = NULL;\n> +}\n\n... the above made me read it twice, wondering \"where does\niov[1].iov_base comes from???\" The location of the git_iovec\nstructure in the expand_data forces this rather unnatural calling\nconvention where the iovec is passed by address (as part of the\nexpand_data structure), with only one of six slots filled, and the\nother five slots are filled by this function from the parameters\npassed to it.\n\nI wonder if we can rework the data structure to\n\n - Not embed git_iovec iov[] in expand_data;\n\n - Keep \"void *content\" instead there;\n\n - Define an on-stack \"struct git_iovec iov[3]\" local to this function;\n\n - Pass \"void *content\" from the caller to this function;\n\n - Populate iov[] fully from hdr->{buf,len}, content, size, and\n opt->output_delim and consume it in this function by either\n calling fwritev_or_die() or writev_or_die().\n\nThat way, the caller does not have to use data->iov[1].iov_base in\nplace of data->content, which is the source of \"Huh? Why is the 2nd\nelement of the 3-element array so special?\" puzzlement readers would\nfeel while reading the caller---after all, the fact that we are\nusing writev with three chunks is an implementation detail that the\ncaller does not have to know to correctly use this helper function.\n\nOr am I missing something?\n\n> +static void print_object_or_die(struct batch_options *opt,\n> +\t\t\t\tstruct expand_data *data, struct strbuf *hdr)\n> {\n> \tconst struct object_id *oid = &data->oid;\n> \n> \tassert(data->info.typep);\n> \n> -\tif (data->content) {\n> -\t\tvoid *content = data->content;\n> +\tif (data->iov[1].iov_base) {\n> +\t\tvoid *content = data->iov[1].iov_base;\n> \t\tunsigned long size = data->size;\n> \n> -\t\tdata->content = NULL;\n> \t\tif (use_mailmap && (data->type == OBJ_COMMIT ||\n> \t\t\t\t\tdata->type == OBJ_TAG)) {\n> \t\t\tsize_t s = size;\n> @@ -399,10 +424,10 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\t\t}\n> \n> \t\t\tcontent = replace_idents_using_mailmap(content, &s);\n> +\t\t\tdata->iov[1].iov_base = content;\n> \t\t\tsize = cast_size_t_to_ulong(s);\n> \t\t}\n> -\n> -\t\tbatch_write(opt, content, size);\n> +\t\tbatch_writev(opt, data, hdr, size);\n> \t\tswitch (data->info.whence) {\n> \t\tcase OI_CACHED:\n> \t\t\t/*\n\nAnd with the \"let's make iov[3] a local implementation detail of\nbatch_writev()\" approach, the above two hunks would shrink and\nessentialy we'd replace batch_write() with batch_writev() (with\nadjusted parameters).\n\n> @@ -419,8 +444,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\t}\n> \t} else {\n> \t\tassert(data->type == OBJ_BLOB);\n> -\t\tif (opt->buffer_output)\n> -\t\t\tfflush(stdout);\n\nWe used to fflush whatever we have written before entering this\n\"else\" clause. We no longer do so\n\n> \t\tif (opt->transform_mode) {\n> \t\t\tchar *contents;\n> \t\t\tunsigned long size;\n> @@ -447,10 +470,15 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n> \t\t\t\t\t oid_to_hex(oid), data->rest);\n> \t\t\t} else\n> \t\t\t\tBUG(\"invalid transform_mode: %c\", opt->transform_mode);\n> -\t\t\tbatch_write(opt, contents, size);\n> +\t\t\tdata->iov[1].iov_base = contents;\n> +\t\t\tbatch_writev(opt, data, hdr, size);\n\nAnd in the buffer_output mode, batch_writev() ends up calling\nfwritev_or_die(), which is merely a series of fwrite() calls. And\nthe removed fflush() earlier is perfectly fine, as it was solely\nbecause we wanted to make sure fflush() before going down to direct\nfile descriptor access with write(2) and we are now still going\nthrough stdio layer.\n\n> \t\t\tfree(contents);\n> \t\t} else {\n> +\t\t\tbatch_write(opt, hdr->buf, hdr->len);\n> +\t\t\tif (opt->buffer_output)\n> +\t\t\t\tfflush(stdout);\n\nThe bigger else clause is entered with potentially unflushed bytes\nin the stdio buffer, as that was why we first fflush(). Then we do\nbatch_write() here, which uses fwrite() in the buffer_output mode,\nwithout having to fflush(). But before doing stream_blob() below,\nwe do need to fflush(). Makes sense.\n\n> static void batch_one_object(const char *obj_name,\n> @@ -666,7 +692,7 @@ static void parse_cmd_contents(struct batch_options *opt,\n> \t\t\t struct expand_data *data)\n> {\n> \topt->batch_mode = BATCH_MODE_CONTENTS;\n> -\tdata->info.contentp = &data->content;\n> +\tdata->info.contentp = &data->iov[1].iov_base;\n> \tbatch_one_object(line, output, opt, data);\n> }\n> \n> @@ -823,7 +849,7 @@ static int batch_objects(struct batch_options *opt)\n> \t\tdata.info.typep = &data.type;\n> \t\tif (!opt->transform_mode) {\n> \t\t\tdata.info.sizep = &data.size;\n> -\t\t\tdata.info.contentp = &data.content;\n> +\t\t\tdata.info.contentp = &data.iov[1].iov_base;\n> \t\t\tdata.info.content_limit = big_file_threshold;\n> \t\t\tdata.info.direct_cache = 1;\n> \t\t}\n\nIf we do the \"let's not leak the iov[3] implementation detail from\nbatch_writev()\" update, the above two hunks can be eliminated.\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index ca7678a379..afde8abc99 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -388,6 +388,16 @@ static inline int git_setitimer(int which UNUSED,\n> #define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)\n> #endif\n> \n> +#ifdef HAVE_WRITEV\n> +#include \n> +#define git_iovec iovec\n> +#else /* !HAVE_WRITEV */\n> +struct git_iovec {\n> +\tvoid *iov_base;\n> +\tsize_t iov_len;\n> +};\n> +#endif /* !HAVE_WRITEV */\n\nOK.\n\n> diff --git a/write-or-die.c b/write-or-die.c\n> index 01a9a51fa2..227b051165 100644\n> --- a/write-or-die.c\n> +++ b/write-or-die.c\n> @@ -107,3 +107,69 @@ void fflush_or_die(FILE *f)\n> \tif (fflush(f))\n> \t\tdie_errno(\"fflush error\");\n> }\n> +\n> +void fwritev_or_die(FILE *fp, const struct git_iovec *iov, int iovcnt)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < iovcnt; i++) {\n> +\t\tsize_t n = iov[i].iov_len;\n> +\n> +\t\tif (fwrite(iov[i].iov_base, 1, n, fp) != n)\n> +\t\t\tdie_errno(\"unable to write to FD=%d\", fileno(fp));\n> +\t}\n> +}\n\nOK.\n\n"},{"id":"501718","messageId":"xmqqwmk2as3w.fsf@gitster.g","threadId":"61778","inReplyTo":"xmqq1q2acyjo.fsf@gitster.g","subject":"Re: [PATCH v2 10/10] cat-file: use writev(2) if available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T15:43:47Z","receivedAt":"2024-08-27T15:43:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +static void batch_writev(struct batch_options *opt, struct expand_data *data,\n>> +\t\t\tconst struct strbuf *hdr, size_t size)\n>> +{\n>> ...\n>> +}\n>\n> ... the above made me read it twice, wondering \"where does\n> iov[1].iov_base comes from???\" The location of the git_iovec\n> structure in the expand_data forces this rather unnatural calling\n> convention where the iovec is passed by address (as part of the\n> expand_data structure), with only one of six slots filled, and the\n> other five slots are filled by this function from the parameters\n> passed to it.\n>\n> I wonder if we can rework the data structure to\n>\n> - Not embed git_iovec iov[] in expand_data;\n>\n> - Keep \"void *content\" instead there;\n>\n> - Define an on-stack \"struct git_iovec iov[3]\" local to this function;\n>\n> - Pass \"void *content\" from the caller to this function;\n>\n> - Populate iov[] fully from hdr->{buf,len}, content, size, and\n> opt->output_delim and consume it in this function by either\n> calling fwritev_or_die() or writev_or_die().\n>\n> That way, the caller does not have to use data->iov[1].iov_base in\n> place of data->content, which is the source of \"Huh? Why is the 2nd\n> element of the 3-element array so special?\" puzzlement readers would\n> feel while reading the caller---after all, the fact that we are\n> using writev with three chunks is an implementation detail that the\n> caller does not have to know to correctly use this helper function.\n>\n> Or am I missing something?\n\nAdditional thought. Perhaps we can introduce\n\n static void batch_write(struct batch_options *opt,\n\t\t\t const void *data, ...);\n\nthat is a vararg function that takes pairs repeated at\nthe end, with data==NULL as sentinel. It may technically need to be\ncalled batch_writel(), but that is a backward compatible interface\nfor existing batch_write() callers.\n\nThen the use of writev() can be encapsulated inside the updated\nbatch_write() function. If you get only a single pair,\nyou would do a single write_or_die() or fwrite_or_die(). Otherwise\nyou'd do the writev() thing, and the function can stay oblivious to\nthe meaning of what it is writing out. There is no need for the\nfunction to know that the payload is \"header followed by body\nfollowed by delimiter byte\", as that is what the callers express at\nthe call sites of the function.\n\nHmm?\n\n\t\t\t \n\n"},{"id":"501729","messageId":"20240827202359.M464972@dcvr","threadId":"61778","inReplyTo":"xmqqcylvky69.fsf@gitster.g","subject":"Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-08-27T20:23:59Z","receivedAt":"2024-08-27T20:24:05Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> > From: Jeff King \n> >\n> > Avoid unnecessary round trips to the object store to speed\n> > up cat-file contents retrievals. The majority of packed objects\n> > don't benefit from the streaming interface at all and we end up\n> > having to load them in core anyways to satisfy our streaming\n> > API.\n> \n> What I found missing from the description is something like ...\n> \n> The new trick used is to teach oid_object_info_extended() that a\n> non-NULL oi->contentp that means \"grab the contents of the objects\n> here\" can be told to refrain from grabbing an object that is too\n> large.\n\nOK.\n\n> > diff --git a/object-file.c b/object-file.c\n> > index 065103be3e..1cc29c3c58 100644\n> > --- a/object-file.c\n> > +++ b/object-file.c\n> > @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,\n> > \n> > \t\tif (!oi->contentp)\n> > \t\t\tbreak;\n> > +\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n> \n> I cannot convince myself enough to say \"content limit\" is a great\n> name. It invites \"limited by what? text files are allowed but\n> images are not?\".\n\nHmm... naming is a most difficult problem :<\n\n->slurp_max? It could be ->content_slurp_max, but I think\nthat's too long...\n\nWould welcome other suggestions...\n\n> > diff --git a/object-store-ll.h b/object-store-ll.h\n> > index c5f2bb2fc2..b71a15f590 100644\n> > --- a/object-store-ll.h\n> > +++ b/object-store-ll.h\n> > @@ -289,6 +289,7 @@ struct object_info {\n> > \tstruct object_id *delta_base_oid;\n> > \tstruct strbuf *type_name;\n> > \tvoid **contentp;\n> > +\tsize_t content_limit;\n> > \n> > \t/* Response */\n> > \tenum {\n> > diff --git a/packfile.c b/packfile.c\n> > index 4028763947..c12a0515b3 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -1529,7 +1529,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t * We always get the representation type, but only convert it to\n> > \t * a \"real\" type later if the caller is interested.\n> > \t */\n> > -\tif (oi->contentp) {\n> > +\tif (oi->contentp && !oi->content_limit) {\n> > \t\t*oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,\n> > \t\t\t\t\t\t &type);\n> > \t\tif (!*oi->contentp)\n> > @@ -1555,6 +1555,17 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> > \t\t\t\t*oi->sizep = size;\n> > \t\t\t}\n> > \t\t}\n> > +\n> > +\t\tif (oi->contentp) {\n> > +\t\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n> \n> It happens that with the current code structure, at this point,\n> oi->content_limit is _always_ non-zero. But it felt somewhat\n> fragile to rely on it, and I would have appreciated if this was\n> written with an explicit check for oi->content_limit, just like how\n> it is done in loose_object_info() function.\n\nRight. I actually think something like:\n\n\t\tassert(oi->content_limit); /* see `if' above */\n\t\tif (oi->sizep && *oi->sizep < oi->content_limit) {\n\nis good for documentation purposes since this is in the `else'\nbranch of the `if (oi->contentp && !oi->content_limit) {' condition.\n"},{"id":"502937","messageId":"ZulUw6nPfjE/aC+f@nand.local","threadId":"61778","inReplyTo":"20240823224630.1180772-2-e@80x24.org","subject":"Re: [PATCH v2 01/10] packfile: move sizep computation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T10:06:59Z","receivedAt":"2024-09-17T10:07:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Aug 23, 2024 at 10:46:21PM +0000, Eric Wong wrote:\n> From: Jeff King \n>\n> Moving the sizep computation now makes the next commit to avoid\n> redundant object info lookups easier to understand. There is\n> no user-visible change, here.\n>\n> [ew: commit message]\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Eric Wong \n> ---\n> packfile.c | 32 ++++++++++++++++----------------\n> 1 file changed, 16 insertions(+), 16 deletions(-)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 813584646f..4028763947 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1536,24 +1536,24 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n> \t\t\ttype = OBJ_BAD;\n> \t} else {\n> \t\ttype = unpack_object_header(p, &w_curs, &curpos, &size);\n> -\t}\n\nOmitted from the context here is that the \"if\" statement which this\n\"else\" belongs to is \"if (oi->contentp)\", so placing the \"if\n(oi->sizep)\" conditional within the else block is equivalent to the\npre-image as you suggest.\n\nWith that additional detail, this patch looks obviously correct to me.\n\nThanks,\nTaylor\n"},{"id":"502946","messageId":"ZulVmP3pBEEajjr5@nand.local","threadId":"61778","inReplyTo":"20240827202359.M464972@dcvr","subject":"Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T10:10:32Z","receivedAt":"2024-09-17T10:10:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 27, 2024 at 08:23:59PM +0000, Eric Wong wrote:\n> > > diff --git a/object-file.c b/object-file.c\n> > > index 065103be3e..1cc29c3c58 100644\n> > > --- a/object-file.c\n> > > +++ b/object-file.c\n> > > @@ -1492,6 +1492,12 @@ static int loose_object_info(struct repository *r,\n> > >\n> > > \t\tif (!oi->contentp)\n> > > \t\t\tbreak;\n> > > +\t\tif (oi->content_limit && *oi->sizep > oi->content_limit) {\n> >\n> > I cannot convince myself enough to say \"content limit\" is a great\n> > name. It invites \"limited by what? text files are allowed but\n> > images are not?\".\n>\n> Hmm... naming is a most difficult problem :<\n>\n> ->slurp_max? It could be ->content_slurp_max, but I think\n> that's too long...\n>\n> Would welcome other suggestions...\n\nI don't have a huge problem with \"content_limit\" as a name, but perhaps\n\"content_size_limit\", \"streaming_limit\", or \"streaming_threshold\" (with\na vague preference towards the latter) might be more descriptive? I\ndunno.\n\nThanks,\nTaylor\n"},{"id":"502947","messageId":"ZulV1nSKdvf5MtpA@nand.local","threadId":"61778","inReplyTo":"xmqq1q2bmdfy.fsf@gitster.g","subject":"Re: [PATCH v2 03/10] packfile: fix off-by-one in content_limit comparison","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T10:11:34Z","receivedAt":"2024-09-17T10:11:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 26, 2024 at 09:55:13AM -0700, Junio C Hamano wrote:\n> Eric Wong writes:\n>\n> > object-file.c::loose_object_info() accepts objects matching\n> > content_limit exactly, so it follows packfile handling allows\n> > slurping objects which match loose object handling and slurp\n> > objects with size matching the content_limit exactly.\n> >\n> > This change is merely for consistency with the majority of\n> > existing code and there is no user visible change in nearly all\n> > cases. The only exception being the corner case when the object\n> > size matches content_limit exactly where users will see a\n> > speedup from avoiding an extra lookup.\n> >\n> > Signed-off-by: Eric Wong \n> > ---\n>\n> I would have preferred to see this (and also \"is oi->content_limit\n> zero?\" check I mentioned earlier) as part of the previous step,\n> which added this comparison that is not consistent with the majority\n> of existing code. It's not like importing from an external project\n> we communicate with only occasionally, in which case we may want to\n> import \"pristine\" source and fix it up separetly in order to make it\n> easier to re-import updated material.\n\nSame here. I don't think there is any reason to split this change out\ninto a separate patch, but I do not feel strongly about it either way.\n\nThanks,\nTaylor\n"},{"id":"502970","messageId":"xmqq5xquj82g.fsf@gitster.g","threadId":"61778","inReplyTo":"ZulVmP3pBEEajjr5@nand.local","subject":"Re: [PATCH v2 02/10] packfile: allow content-limit for cat-file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-17T21:15:19Z","receivedAt":"2024-09-17T21:15:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I don't have a huge problem with \"content_limit\" as a name, but perhaps\n> \"content_size_limit\", \"streaming_limit\", or \"streaming_threshold\" (with\n> a vague preference towards the latter) might be more descriptive? I\n> dunno.\n\nYour statement does highlight the second point that should have been\npointed out, but I failed to. The \"limit\" is about \"maximum content\nsize\" (as opposed to content type or color or smell), but also it is\nimportant to avoid sounding like we are forbidding a blob to exist\nif it is larger than that limit. The limit is only about whether\nthe contents are prefetched through the contentp pointer (and\nanything larger than the limit must be obtained via a separate API\ncall to obtain the contents).\n\nWe can flip the polarity to say \"minimum content size to stream\"\n(i.e., anything larger than this threshold will be streamed out\ninstead of held in core at once), and it may certainly be a better\nway to explain what is going on than \"maximum content size to be\nheld in-core via contentp\".\n\nThanks.\n"},{"id":"504201","messageId":"20241006174033.M85696@dcvr","threadId":"61778","inReplyTo":"xmqqjzg3ky77.fsf@gitster.g","subject":"Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-10-06T17:40:33Z","receivedAt":"2024-10-06T17:40:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> \n> > We need to check delta_base_cache anyways to fill in the\n> > `whence' field in `struct object_info'. Inlining (and getting\n> > rid of) cache_or_unpack_entry() makes it easier to only do the\n> > hashmap lookup once and avoid a redundant lookup later on.\n> >\n> > This code reorganization will also make an optimization to\n> > use the cache entry directly easier to implement in the next\n> > commit.\n> \n> \"cache entry\" -> \"cached entry\"; we tend to use \"cache entry\"\n> exclusively to mean an entry in the in-core index structure,\n> and not the cached objects held in the object layer.\n\nOK, will do for v3 (apologies for the delay, Real Life is awful :<).\n\n> > +\t\ttype = ent->type;\n> > +\t\tif (oi->sizep)\n> > +\t\t\t*oi->sizep = ent->size;\n> > +\t\tif (oi->contentp) {\n> > +\t\t\tif (!oi->content_limit ||\n> > +\t\t\t\t\tent->size <= oi->content_limit)\n> > +\t\t\t\t*oi->contentp = xmemdupz(ent->data, ent->size);\n> > +\t\t\telse\n> > +\t\t\t\t*oi->contentp = NULL; /* caller must stream */\n> \n> This assignment of NULL is more explicit than the original; is it\n> because the original assumed that *(oi->contentp) is initialized to\n> NULL if oi->contentp asks us to give the contents?\n\nThe original code unconditionally assigned cache_or_unpack_entry() result\nto *oi->contentp, and there's a bunch of non-cat-file callers which pass\na non-NULL *oi->contentp and expect packed_object_info() to NULL it.\n"}]}