{"thread":{"id":"56484","subject":"[PATCH 0/3] packfile: use oidset for bad objects","startedAt":"2021-09-11T07:56:01Z","lastAt":"2021-09-12T04:01:45Z","messageCount":28,"participants":["René Scharfe","Jeff King","Derrick Stolee","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"435523","messageId":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","threadId":"56484","inReplyTo":null,"subject":"[PATCH 0/3] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T07:50:36Z","receivedAt":"2021-09-11T07:56:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Replace the custom hash array for remembering corrupt pack entries with\nan oidset.  This shortens and simplifies the code.\n\n  packfile: convert mark_bad_packed_object() to object_id\n  packfile: convert has_packed_and_bad() to object_id\n  packfile: use oidset for bad objects\n\n midx.c         | 13 ++++---------\n object-file.c  |  4 ++--\n object-store.h |  4 ++--\n packfile.c     | 37 ++++++++++---------------------------\n packfile.h     |  4 ++--\n 5 files changed, 20 insertions(+), 42 deletions(-)\n\n--\n2.33.0\n"},{"id":"435524","messageId":"a5918f78-48c2-e68c-af28-06d1e30375a0@web.de","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"[PATCH 1/3] packfile: convert mark_bad_packed_object() to object_id","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T07:59:16Z","receivedAt":"2021-09-11T07:59:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"All callers have full object IDs, so pass them on instead of just their\nhash member.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n object-file.c |  2 +-\n packfile.c    | 12 ++++++------\n packfile.h    |  2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex a8be899481..fb5a385a06 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1616,7 +1616,7 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\treturn 0;\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n-\t\tmark_bad_packed_object(e.p, real->hash);\n+\t\tmark_bad_packed_object(e.p, real);\n \t\treturn do_oid_object_info_extended(r, real, oi, 0);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\ndiff --git a/packfile.c b/packfile.c\nindex 4d0d625238..fb15fc5b49 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1161,17 +1161,17 @@ int unpack_object_header(struct packed_git *p,\n \treturn type;\n }\n\n-void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1)\n+void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n {\n \tunsigned i;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\tif (hasheq(sha1, p->bad_object_sha1 + hashsz * i))\n+\t\tif (hasheq(oid->hash, p->bad_object_sha1 + hashsz * i))\n \t\t\treturn;\n \tp->bad_object_sha1 = xrealloc(p->bad_object_sha1,\n \t\t\t\t      st_mult(GIT_MAX_RAWSZ,\n \t\t\t\t\t      st_add(p->num_bad_objects, 1)));\n-\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, sha1);\n+\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, oid->hash);\n \tp->num_bad_objects++;\n }\n\n@@ -1272,7 +1272,7 @@ static int retry_bad_packed_offset(struct repository *r,\n \tif (offset_to_pack_pos(p, obj_offset, &pos) < 0)\n \t\treturn OBJ_BAD;\n \tnth_packed_object_id(&oid, p, pack_pos_to_index(p, pos));\n-\tmark_bad_packed_object(p, oid.hash);\n+\tmark_bad_packed_object(p, &oid);\n \ttype = oid_object_info(r, &oid, NULL);\n \tif (type <= OBJ_NONE)\n \t\treturn OBJ_BAD;\n@@ -1722,7 +1722,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\t\tnth_packed_object_id(&oid, p, index_pos);\n \t\t\t\terror(\"bad packed object CRC for %s\",\n \t\t\t\t      oid_to_hex(&oid));\n-\t\t\t\tmark_bad_packed_object(p, oid.hash);\n+\t\t\t\tmark_bad_packed_object(p, &oid);\n \t\t\t\tdata = NULL;\n \t\t\t\tgoto out;\n \t\t\t}\n@@ -1811,7 +1811,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\t\t      \" at offset %\"PRIuMAX\" from %s\",\n \t\t\t\t      oid_to_hex(&base_oid), (uintmax_t)obj_offset,\n \t\t\t\t      p->pack_name);\n-\t\t\t\tmark_bad_packed_object(p, base_oid.hash);\n+\t\t\t\tmark_bad_packed_object(p, &base_oid);\n \t\t\t\tbase = read_object(r, &base_oid, &type, &base_size);\n \t\t\t\texternal_base = base;\n \t\t\t}\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8ae..a982ed9994 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -159,7 +159,7 @@ int packed_object_info(struct repository *r,\n \t\t       struct packed_git *pack,\n \t\t       off_t offset, struct object_info *);\n\n-void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n+void mark_bad_packed_object(struct packed_git *, const struct object_id *);\n const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n\n #define ON_DISK_KEEP_PACKS 1\n--\n2.33.0\n"},{"id":"435525","messageId":"8d7328ee-57b5-247c-7b28-8d28ece8a4f2@web.de","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"[PATCH 2/3] packfile: convert has_packed_and_bad() to object_id","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T08:00:37Z","receivedAt":"2021-09-11T08:00:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The single caller has a full object ID, so pass it on instead of just\nits hash member.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n object-file.c | 2 +-\n packfile.c    | 4 ++--\n packfile.h    | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex fb5a385a06..01e7058b4e 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1725,7 +1725,7 @@ void *read_object_file_extended(struct repository *r,\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(repl), path);\n\n-\tif ((p = has_packed_and_bad(r, repl->hash)) != NULL)\n+\tif ((p = has_packed_and_bad(r, repl)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(repl), p->pack_name);\n \tobj_read_unlock();\ndiff --git a/packfile.c b/packfile.c\nindex fb15fc5b49..04080a558b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1176,14 +1176,14 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n }\n\n const struct packed_git *has_packed_and_bad(struct repository *r,\n-\t\t\t\t\t    const unsigned char *sha1)\n+\t\t\t\t\t    const struct object_id *oid)\n {\n \tstruct packed_git *p;\n \tunsigned i;\n\n \tfor (p = r->objects->packed_git; p; p = p->next)\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(sha1,\n+\t\t\tif (hasheq(oid->hash,\n \t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n \t\t\t\treturn p;\n \treturn NULL;\ndiff --git a/packfile.h b/packfile.h\nindex a982ed9994..186146779d 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -160,7 +160,7 @@ int packed_object_info(struct repository *r,\n \t\t       off_t offset, struct object_info *);\n\n void mark_bad_packed_object(struct packed_git *, const struct object_id *);\n-const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n+const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *);\n\n #define ON_DISK_KEEP_PACKS 1\n #define IN_CORE_KEEP_PACKS 2\n--\n2.33.0\n"},{"id":"435526","messageId":"14d48124-d8bb-aa34-aad0-4203d699e17e@web.de","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"[PATCH 3/3] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T08:01:40Z","receivedAt":"2021-09-11T08:01:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Store the object ID of broken pack entries in an oidset instead of\nkeeping only their hashes in an unsorted array.  The resulting code is\nshorter and easier to read.  It also handles the (hopefully) very rare\ncase of having a high number of bad objects better.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n midx.c         | 13 ++++---------\n object-store.h |  4 ++--\n packfile.c     | 27 +++++----------------------\n 3 files changed, 11 insertions(+), 33 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 321c6fdd2f..01623fb339 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -283,6 +283,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n {\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n+\tstruct object_id oid;\n\n \tif (pos >= m->num_objects)\n \t\treturn 0;\n@@ -303,15 +304,9 @@ static int nth_midxed_pack_entry(struct repository *r,\n \tif (!is_pack_valid(p))\n \t\treturn 0;\n\n-\tif (p->num_bad_objects) {\n-\t\tuint32_t i;\n-\t\tstruct object_id oid;\n-\t\tnth_midxed_object_oid(&oid, m, pos);\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid.hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn 0;\n-\t}\n+\tnth_midxed_object_oid(&oid, m, pos);\n+\tif (oidset_contains(&p->bad_objects, &oid))\n+\t\treturn 0;\n\n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\ndiff --git a/object-store.h b/object-store.h\nindex b4dc6668aa..c7bead66f6 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -10,6 +10,7 @@\n #include \"khash.h\"\n #include \"dir.h\"\n #include \"oidtree.h\"\n+#include \"oidset.h\"\n\n struct object_directory {\n \tstruct object_directory *next;\n@@ -75,9 +76,8 @@ struct packed_git {\n \tconst void *index_data;\n \tsize_t index_size;\n \tuint32_t num_objects;\n-\tuint32_t num_bad_objects;\n \tuint32_t crc_offset;\n-\tunsigned char *bad_object_sha1;\n+\tstruct oidset bad_objects;\n \tint index_version;\n \ttime_t mtime;\n \tint pack_fd;\ndiff --git a/packfile.c b/packfile.c\nindex 04080a558b..8f6d1d6328 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1163,29 +1163,17 @@ int unpack_object_header(struct packed_git *p,\n\n void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n {\n-\tunsigned i;\n-\tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\tif (hasheq(oid->hash, p->bad_object_sha1 + hashsz * i))\n-\t\t\treturn;\n-\tp->bad_object_sha1 = xrealloc(p->bad_object_sha1,\n-\t\t\t\t      st_mult(GIT_MAX_RAWSZ,\n-\t\t\t\t\t      st_add(p->num_bad_objects, 1)));\n-\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, oid->hash);\n-\tp->num_bad_objects++;\n+\toidset_insert(&p->bad_objects, oid);\n }\n\n const struct packed_git *has_packed_and_bad(struct repository *r,\n \t\t\t\t\t    const struct object_id *oid)\n {\n \tstruct packed_git *p;\n-\tunsigned i;\n\n \tfor (p = r->objects->packed_git; p; p = p->next)\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid->hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn p;\n+\t\tif (oidset_contains(&p->bad_objects, oid))\n+\t\t\treturn p;\n \treturn NULL;\n }\n\n@@ -2016,13 +2004,8 @@ static int fill_pack_entry(const struct object_id *oid,\n {\n \toff_t offset;\n\n-\tif (p->num_bad_objects) {\n-\t\tunsigned i;\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid->hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn 0;\n-\t}\n+\tif (oidset_contains(&p->bad_objects, oid))\n+\t\treturn 0;\n\n \toffset = find_pack_entry_one(oid->hash, p);\n \tif (!offset)\n--\n2.33.0\n"},{"id":"435551","messageId":"YTy8o3qXvyVv8fNr@coredump.intra.peff.net","threadId":"56484","inReplyTo":"14d48124-d8bb-aa34-aad0-4203d699e17e@web.de","subject":"Re: [PATCH 3/3] packfile: use oidset for bad objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T14:26:43Z","receivedAt":"2021-09-11T14:26:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 10:01:40AM +0200, René Scharfe wrote:\n\n> Store the object ID of broken pack entries in an oidset instead of\n> keeping only their hashes in an unsorted array.  The resulting code is\n> shorter and easier to read.  It also handles the (hopefully) very rare\n> case of having a high number of bad objects better.\n\nYay, I'm very happy to see this kind of cleanup replacing ad hoc data\nstructures with well-tested ones.\n\n> @@ -303,15 +304,9 @@ static int nth_midxed_pack_entry(struct repository *r,\n>  \tif (!is_pack_valid(p))\n>  \t\treturn 0;\n> \n> -\tif (p->num_bad_objects) {\n> -\t\tuint32_t i;\n> -\t\tstruct object_id oid;\n> -\t\tnth_midxed_object_oid(&oid, m, pos);\n> -\t\tfor (i = 0; i < p->num_bad_objects; i++)\n> -\t\t\tif (hasheq(oid.hash,\n> -\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n> -\t\t\t\treturn 0;\n> -\t}\n> +\tnth_midxed_object_oid(&oid, m, pos);\n> +\tif (oidset_contains(&p->bad_objects, &oid))\n> +\t\treturn 0;\n\nCalling nth_midxed_object_oid() implies a memcpy() under the hood. In\nthe old code, we'd skip that in the common case that we had no corrupt\nobjects, but now we'll pay the cost regardless. memcpy() isn't _that_\nexpensive, but I'd expect this to be a relatively hot code path.\n\nIs it worth sticking all of this inside:\n\n  if (oidset_size(&p->bad_objects))\n\n?\n\n> diff --git a/packfile.c b/packfile.c\n> index 04080a558b..8f6d1d6328 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1163,29 +1163,17 @@ int unpack_object_header(struct packed_git *p,\n> \n>  void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n>  {\n> -\tunsigned i;\n> -\tconst unsigned hashsz = the_hash_algo->rawsz;\n> -\tfor (i = 0; i < p->num_bad_objects; i++)\n> -\t\tif (hasheq(oid->hash, p->bad_object_sha1 + hashsz * i))\n> -\t\t\treturn;\n\nI cringed at the hasheq() and hashcpy() calls in the earlier patches.\nHappy to see them go away now. :)\n\n> -\tp->bad_object_sha1 = xrealloc(p->bad_object_sha1,\n> -\t\t\t\t      st_mult(GIT_MAX_RAWSZ,\n> -\t\t\t\t\t      st_add(p->num_bad_objects, 1)));\n> -\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, oid->hash);\n> -\tp->num_bad_objects++;\n> +\toidset_insert(&p->bad_objects, oid);\n>  }\n\nSo now marking a bad object  is a one-liner. We _could_ just inline it\nat the callers, but I like keeping the implementation abstract.\n\n>  const struct packed_git *has_packed_and_bad(struct repository *r,\n>  \t\t\t\t\t    const struct object_id *oid)\n>  {\n>  \tstruct packed_git *p;\n> -\tunsigned i;\n> \n>  \tfor (p = r->objects->packed_git; p; p = p->next)\n> -\t\tfor (i = 0; i < p->num_bad_objects; i++)\n> -\t\t\tif (hasheq(oid->hash,\n> -\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n> -\t\t\t\treturn p;\n> +\t\tif (oidset_contains(&p->bad_objects, oid))\n> +\t\t\treturn p;\n>  \treturn NULL;\n>  }\n\nNot related to your patch, but I noticed how terribly inefficient this\nfunction could be in a repo with a lot of packs. But we only call it\nonce in the error case right before we die(), so a linear scan is no\nproblem.\n\n> @@ -2016,13 +2004,8 @@ static int fill_pack_entry(const struct object_id *oid,\n>  {\n>  \toff_t offset;\n> \n> -\tif (p->num_bad_objects) {\n> -\t\tunsigned i;\n> -\t\tfor (i = 0; i < p->num_bad_objects; i++)\n> -\t\t\tif (hasheq(oid->hash,\n> -\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n> -\t\t\t\treturn 0;\n> -\t}\n> +\tif (oidset_contains(&p->bad_objects, oid))\n> +\t\treturn 0;\n\nAnd this one (and the previous) have the oid already, so they don't have\nto worry about optimizing the is-it-empty check first.\n\n-Peff\n"},{"id":"435552","messageId":"YTy81+Xq5G1fRxU8@coredump.intra.peff.net","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"Re: [PATCH 0/3] packfile: use oidset for bad objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T14:27:35Z","receivedAt":"2021-09-11T14:27:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 09:50:36AM +0200, René Scharfe wrote:\n\n> Replace the custom hash array for remembering corrupt pack entries with\n> an oidset.  This shortens and simplifies the code.\n\nThanks, these were a pleasure to read.\n\nI noticed one possible small performance change in the third patch. It\nshould be easy to remedy if we want (but I could also believe that it\ndoesn't matter either way, if you care to argue that :) ).\n\n-Peff\n"},{"id":"435558","messageId":"3d637be1-f89f-414c-19ef-65c3943457e9@web.de","threadId":"56484","inReplyTo":"YTy8o3qXvyVv8fNr@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T16:08:38Z","receivedAt":"2021-09-11T16:08:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 16:26 schrieb Jeff King:\n> On Sat, Sep 11, 2021 at 10:01:40AM +0200, René Scharfe wrote:\n>\n>> Store the object ID of broken pack entries in an oidset instead of\n>> keeping only their hashes in an unsorted array.  The resulting code is\n>> shorter and easier to read.  It also handles the (hopefully) very rare\n>> case of having a high number of bad objects better.\n>\n> Yay, I'm very happy to see this kind of cleanup replacing ad hoc data\n> structures with well-tested ones.\n>\n>> @@ -303,15 +304,9 @@ static int nth_midxed_pack_entry(struct repository *r,\n>>  \tif (!is_pack_valid(p))\n>>  \t\treturn 0;\n>>\n>> -\tif (p->num_bad_objects) {\n>> -\t\tuint32_t i;\n>> -\t\tstruct object_id oid;\n>> -\t\tnth_midxed_object_oid(&oid, m, pos);\n>> -\t\tfor (i = 0; i < p->num_bad_objects; i++)\n>> -\t\t\tif (hasheq(oid.hash,\n>> -\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n>> -\t\t\t\treturn 0;\n>> -\t}\n>> +\tnth_midxed_object_oid(&oid, m, pos);\n>> +\tif (oidset_contains(&p->bad_objects, &oid))\n>> +\t\treturn 0;\n>\n> Calling nth_midxed_object_oid() implies a memcpy() under the hood. In\n> the old code, we'd skip that in the common case that we had no corrupt\n> objects, but now we'll pay the cost regardless. memcpy() isn't _that_\n> expensive, but I'd expect this to be a relatively hot code path.\n>\n> Is it worth sticking all of this inside:\n>\n>   if (oidset_size(&p->bad_objects))\n>\n> ?\n\nHard to say.  It would certainly match the old code more closely.  Is a\nfunction call cheaper than copying 32 bytes?  Depends on the CPU and\nwhether the hash is cached, I guess.  And cached it probably is, because\nthe caller did a binary search for it..\n\nWe can pass on the original oid to avoid the nth_midxed_object_oid()\ncall, but inlining the whole thing might even be nicer.\n\nRené\n"},{"id":"435559","messageId":"7d9e67bf-e057-694c-c976-ba19e9521882@web.de","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"[PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T16:08:42Z","receivedAt":"2021-09-11T16:08:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fill_midx_entry() finds the position of an object ID and passes it to\nnth_midxed_pack_entry(), which uses the position to look up the object\nID for its own purposes.  Inline the latter into the former to avoid\nthat lookup.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n midx.c | 29 +++++++++--------------------\n 1 file changed, 9 insertions(+), 20 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 01623fb339..59517938a8 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -276,14 +276,17 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \t\t\t(off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);\n }\n\n-static int nth_midxed_pack_entry(struct repository *r,\n-\t\t\t\t struct multi_pack_index *m,\n-\t\t\t\t struct pack_entry *e,\n-\t\t\t\t uint32_t pos)\n+int fill_midx_entry(struct repository * r,\n+\t\t    const struct object_id *oid,\n+\t\t    struct pack_entry *e,\n+\t\t    struct multi_pack_index *m)\n {\n+\tuint32_t pos;\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n-\tstruct object_id oid;\n+\n+\tif (!bsearch_midx(oid, m, &pos))\n+\t\treturn 0;\n\n \tif (pos >= m->num_objects)\n \t\treturn 0;\n@@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n \tif (!is_pack_valid(p))\n \t\treturn 0;\n\n-\tnth_midxed_object_oid(&oid, m, pos);\n-\tif (oidset_contains(&p->bad_objects, &oid))\n+\tif (oidset_contains(&p->bad_objects, oid))\n \t\treturn 0;\n\n \te->offset = nth_midxed_offset(m, pos);\n@@ -314,19 +316,6 @@ static int nth_midxed_pack_entry(struct repository *r,\n \treturn 1;\n }\n\n-int fill_midx_entry(struct repository * r,\n-\t\t    const struct object_id *oid,\n-\t\t    struct pack_entry *e,\n-\t\t    struct multi_pack_index *m)\n-{\n-\tuint32_t pos;\n-\n-\tif (!bsearch_midx(oid, m, &pos))\n-\t\treturn 0;\n-\n-\treturn nth_midxed_pack_entry(r, m, e, pos);\n-}\n-\n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n static int cmp_idx_or_pack_name(const char *idx_or_pack_name,\n \t\t\t\tconst char *idx_name)\n--\n2.33.0\n"},{"id":"435569","messageId":"YTzhVfmObOf7DHqm@coredump.intra.peff.net","threadId":"56484","inReplyTo":"3d637be1-f89f-414c-19ef-65c3943457e9@web.de","subject":"Re: [PATCH 3/3] packfile: use oidset for bad objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T17:03:17Z","receivedAt":"2021-09-11T17:03:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 06:08:38PM +0200, René Scharfe wrote:\n\n> >> +\tnth_midxed_object_oid(&oid, m, pos);\n> >> +\tif (oidset_contains(&p->bad_objects, &oid))\n> >> +\t\treturn 0;\n> >\n> > Calling nth_midxed_object_oid() implies a memcpy() under the hood. In\n> > the old code, we'd skip that in the common case that we had no corrupt\n> > objects, but now we'll pay the cost regardless. memcpy() isn't _that_\n> > expensive, but I'd expect this to be a relatively hot code path.\n> >\n> > Is it worth sticking all of this inside:\n> >\n> >   if (oidset_size(&p->bad_objects))\n> >\n> > ?\n> \n> Hard to say.  It would certainly match the old code more closely.  Is a\n> function call cheaper than copying 32 bytes?  Depends on the CPU and\n> whether the hash is cached, I guess.  And cached it probably is, because\n> the caller did a binary search for it..\n\nYou already have a function call for nth_midxed_object_oid(), so\nchecking oidset_size() would be a strict improvement.\n\n> We can pass on the original oid to avoid the nth_midxed_object_oid()\n> call, but inlining the whole thing might even be nicer.\n\nYeah, it occurs to me that oidset_size() would be a good candidate for\ninlining, if that's what you mean.\n\n-Peff\n"},{"id":"435570","messageId":"165ffe5c-f8b0-a4e3-cb01-71ad74778f09@web.de","threadId":"56484","inReplyTo":"7d9e67bf-e057-694c-c976-ba19e9521882@web.de","subject":"Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T17:03:21Z","receivedAt":"2021-09-11T17:03:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 18:08 schrieb René Scharfe:\n> fill_midx_entry() finds the position of an object ID and passes it to\n> nth_midxed_pack_entry(), which uses the position to look up the object\n> ID for its own purposes.  Inline the latter into the former to avoid\n> that lookup.\n>\n\nForgot:\n\nReported-by: Jeff King <peff@peff.net>\n\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n\nRené\n"},{"id":"435574","messageId":"YTziaNywmCMn07IS@coredump.intra.peff.net","threadId":"56484","inReplyTo":"7d9e67bf-e057-694c-c976-ba19e9521882@web.de","subject":"Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T17:07:52Z","receivedAt":"2021-09-11T17:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:\n\n> fill_midx_entry() finds the position of an object ID and passes it to\n> nth_midxed_pack_entry(), which uses the position to look up the object\n> ID for its own purposes.  Inline the latter into the former to avoid\n> that lookup.\n\nAh, I see what you mean now by \"inline\" in the other part of the thread.\n\nYes, I think this makes sense since there is no other reasonable caller\nof the nth_midxed_pack_entry() helper (and its one caller is itself\ntrivial).\n\n> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n>  \tif (!is_pack_valid(p))\n>  \t\treturn 0;\n> \n> -\tnth_midxed_object_oid(&oid, m, pos);\n> -\tif (oidset_contains(&p->bad_objects, &oid))\n> +\tif (oidset_contains(&p->bad_objects, oid))\n>  \t\treturn 0;\n\nSo we get to avoid the nth_midxed_object_oid() copy entirely. Very nice.\n\nCompared to the code before your series, we still have an extra function\ncall to oidset_contains(), which will (in the common case) notice we\nhave no entries and immediately return. But I think that's getting into\npointless micro-optimization.\n\n-Peff\n"},{"id":"435576","messageId":"480dba11-e321-68f8-3983-1d918c24fbbc@web.de","threadId":"56484","inReplyTo":"YTzhVfmObOf7DHqm@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T17:16:51Z","receivedAt":"2021-09-11T17:17:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 19:03 schrieb Jeff King:\n> On Sat, Sep 11, 2021 at 06:08:38PM +0200, René Scharfe wrote:\n>\n>>>> +\tnth_midxed_object_oid(&oid, m, pos);\n>>>> +\tif (oidset_contains(&p->bad_objects, &oid))\n>>>> +\t\treturn 0;\n>>>\n>>> Calling nth_midxed_object_oid() implies a memcpy() under the hood. In\n>>> the old code, we'd skip that in the common case that we had no corrupt\n>>> objects, but now we'll pay the cost regardless. memcpy() isn't _that_\n>>> expensive, but I'd expect this to be a relatively hot code path.\n>>>\n>>> Is it worth sticking all of this inside:\n>>>\n>>>   if (oidset_size(&p->bad_objects))\n>>>\n>>> ?\n>>\n>> Hard to say.  It would certainly match the old code more closely.  Is a\n>> function call cheaper than copying 32 bytes?  Depends on the CPU and\n>> whether the hash is cached, I guess.  And cached it probably is, because\n>> the caller did a binary search for it..\n>\n> You already have a function call for nth_midxed_object_oid(), so\n> checking oidset_size() would be a strict improvement.\n\nIf I read the assembly correctly nth_midxed_object_oid() is inlined by the\ncompiler in my build, as is nth_midxed_pack_entry().  Both are defined in\nthe same file, so other compilers may easily do the same.\n\n>> We can pass on the original oid to avoid the nth_midxed_object_oid()\n>> call, but inlining the whole thing might even be nicer.\n>\n> Yeah, it occurs to me that oidset_size() would be a good candidate for\n> inlining, if that's what you mean.\n\nTrue, but I meant something else (see patch 4/3). :)\n\nRené\n"},{"id":"435590","messageId":"120ffa79-d3cb-2e81-1da6-358e7407ec69@web.de","threadId":"56484","inReplyTo":"YTziaNywmCMn07IS@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:31:34Z","receivedAt":"2021-09-11T20:32:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 19:07 schrieb Jeff King:\n> On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:\n>\n>> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n>>  \tif (!is_pack_valid(p))\n>>  \t\treturn 0;\n>>\n>> -\tnth_midxed_object_oid(&oid, m, pos);\n>> -\tif (oidset_contains(&p->bad_objects, &oid))\n>> +\tif (oidset_contains(&p->bad_objects, oid))\n>>  \t\treturn 0;\n>\n> So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice.\n>\n> Compared to the code before your series, we still have an extra function\n> call to oidset_contains(), which will (in the common case) notice we\n> have no entries and immediately return. But I think that's getting into\n> pointless micro-optimization.\n\nRight.  I measure a 0.5% slowdown for git multi-pack-index verify.  An\ninline oidset_size call avoids it.  That's easy enough to add, so let's\nhave it!\n\nRené\n"},{"id":"435591","messageId":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","threadId":"56484","inReplyTo":"4a702bfe-afd0-669a-c893-0262289c24b8@web.de","subject":"[PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:31:52Z","receivedAt":"2021-09-11T20:32:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Replace the custom hash array for remembering corrupt pack entries with\nan oidset.  This shortens and simplifies the code.\n\nChanges since v1:\n- inline oidset_size()\n- inline nth_midxed_pack_entry() early\n- use oidset_size() to avoid a function call if no bad objects exist\n\n  oidset: make oidset_size() an inline function\n  midx: inline nth_midxed_pack_entry()\n  packfile: convert mark_bad_packed_object() to object_id\n  packfile: convert has_packed_and_bad() to object_id\n  packfile: use oidset for bad objects\n\n midx.c         | 37 +++++++++++--------------------------\n object-file.c  |  4 ++--\n object-store.h |  4 ++--\n oidset.c       |  5 -----\n oidset.h       |  5 ++++-\n packfile.c     | 38 +++++++++++---------------------------\n packfile.h     |  4 ++--\n 7 files changed, 32 insertions(+), 65 deletions(-)\n\n--\n2.33.0\n"},{"id":"435592","messageId":"3ec3b543-d5a7-5dce-ea5c-243ae8c78fbf@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"[PATCH 1/5] oidset: make oidset_size() an inline function","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:36:40Z","receivedAt":"2021-09-11T20:36:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"oidset_size() just reads a single word from memory and returns it.\nAvoid the function call overhead for this trivial operation by turning\nit into an inline function.\n\nWhile we're at it, declare its parameter const to allow it to be used\non read-only oidsets.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n oidset.c | 5 -----\n oidset.h | 5 ++++-\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/oidset.c b/oidset.c\nindex 5aac633c1f..b36a2bae86 100644\n--- a/oidset.c\n+++ b/oidset.c\n@@ -36,11 +36,6 @@ void oidset_clear(struct oidset *set)\n \toidset_init(set, 0);\n }\n\n-int oidset_size(struct oidset *set)\n-{\n-\treturn kh_size(&set->set);\n-}\n-\n void oidset_parse_file(struct oidset *set, const char *path)\n {\n \toidset_parse_file_carefully(set, path, NULL, NULL);\ndiff --git a/oidset.h b/oidset.h\nindex 01f6560283..ba4a5a2cd3 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -57,7 +57,10 @@ int oidset_remove(struct oidset *set, const struct object_id *oid);\n /**\n  * Returns the number of oids in the set.\n  */\n-int oidset_size(struct oidset *set);\n+static inline int oidset_size(const struct oidset *set)\n+{\n+\treturn kh_size(&set->set);\n+}\n\n /**\n  * Remove all entries from the oidset, freeing any resources associated with\n--\n2.33.0\n"},{"id":"435593","messageId":"06f0f546-31c6-9cf5-5e11-999cb354c91a@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"[PATCH 2/5] midx: inline nth_midxed_pack_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:39:31Z","receivedAt":"2021-09-11T20:39:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fill_midx_entry() finds the position of an object ID and passes it to\nnth_midxed_pack_entry(), which uses the position to look up the object\nID for its own purposes.  Inline the latter into the former to avoid\nthat lookup.\n\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n midx.c | 29 +++++++++--------------------\n 1 file changed, 9 insertions(+), 20 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 321c6fdd2f..8cb063023c 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -276,14 +276,18 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \t\t\t(off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);\n }\n\n-static int nth_midxed_pack_entry(struct repository *r,\n-\t\t\t\t struct multi_pack_index *m,\n-\t\t\t\t struct pack_entry *e,\n-\t\t\t\t uint32_t pos)\n+int fill_midx_entry(struct repository * r,\n+\t\t    const struct object_id *oid,\n+\t\t    struct pack_entry *e,\n+\t\t    struct multi_pack_index *m)\n {\n+\tuint32_t pos;\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n\n+\tif (!bsearch_midx(oid, m, &pos))\n+\t\treturn 0;\n+\n \tif (pos >= m->num_objects)\n \t\treturn 0;\n\n@@ -305,10 +309,8 @@ static int nth_midxed_pack_entry(struct repository *r,\n\n \tif (p->num_bad_objects) {\n \t\tuint32_t i;\n-\t\tstruct object_id oid;\n-\t\tnth_midxed_object_oid(&oid, m, pos);\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid.hash,\n+\t\t\tif (hasheq(oid->hash,\n \t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n \t\t\t\treturn 0;\n \t}\n@@ -319,19 +321,6 @@ static int nth_midxed_pack_entry(struct repository *r,\n \treturn 1;\n }\n\n-int fill_midx_entry(struct repository * r,\n-\t\t    const struct object_id *oid,\n-\t\t    struct pack_entry *e,\n-\t\t    struct multi_pack_index *m)\n-{\n-\tuint32_t pos;\n-\n-\tif (!bsearch_midx(oid, m, &pos))\n-\t\treturn 0;\n-\n-\treturn nth_midxed_pack_entry(r, m, e, pos);\n-}\n-\n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n static int cmp_idx_or_pack_name(const char *idx_or_pack_name,\n \t\t\t\tconst char *idx_name)\n--\n2.33.0\n"},{"id":"435594","messageId":"a6d7114f-08c1-567a-d8d4-40becd2384f6@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"[PATCH 3/5] packfile: convert mark_bad_packed_object() to object_id","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:40:33Z","receivedAt":"2021-09-11T20:40:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"All callers have full object IDs, so pass them on instead of just their\nhash member.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n object-file.c |  2 +-\n packfile.c    | 12 ++++++------\n packfile.h    |  2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex a8be899481..fb5a385a06 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1616,7 +1616,7 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\treturn 0;\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n-\t\tmark_bad_packed_object(e.p, real->hash);\n+\t\tmark_bad_packed_object(e.p, real);\n \t\treturn do_oid_object_info_extended(r, real, oi, 0);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\ndiff --git a/packfile.c b/packfile.c\nindex 4d0d625238..fb15fc5b49 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1161,17 +1161,17 @@ int unpack_object_header(struct packed_git *p,\n \treturn type;\n }\n\n-void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1)\n+void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n {\n \tunsigned i;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\tif (hasheq(sha1, p->bad_object_sha1 + hashsz * i))\n+\t\tif (hasheq(oid->hash, p->bad_object_sha1 + hashsz * i))\n \t\t\treturn;\n \tp->bad_object_sha1 = xrealloc(p->bad_object_sha1,\n \t\t\t\t      st_mult(GIT_MAX_RAWSZ,\n \t\t\t\t\t      st_add(p->num_bad_objects, 1)));\n-\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, sha1);\n+\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, oid->hash);\n \tp->num_bad_objects++;\n }\n\n@@ -1272,7 +1272,7 @@ static int retry_bad_packed_offset(struct repository *r,\n \tif (offset_to_pack_pos(p, obj_offset, &pos) < 0)\n \t\treturn OBJ_BAD;\n \tnth_packed_object_id(&oid, p, pack_pos_to_index(p, pos));\n-\tmark_bad_packed_object(p, oid.hash);\n+\tmark_bad_packed_object(p, &oid);\n \ttype = oid_object_info(r, &oid, NULL);\n \tif (type <= OBJ_NONE)\n \t\treturn OBJ_BAD;\n@@ -1722,7 +1722,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\t\tnth_packed_object_id(&oid, p, index_pos);\n \t\t\t\terror(\"bad packed object CRC for %s\",\n \t\t\t\t      oid_to_hex(&oid));\n-\t\t\t\tmark_bad_packed_object(p, oid.hash);\n+\t\t\t\tmark_bad_packed_object(p, &oid);\n \t\t\t\tdata = NULL;\n \t\t\t\tgoto out;\n \t\t\t}\n@@ -1811,7 +1811,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\t\t      \" at offset %\"PRIuMAX\" from %s\",\n \t\t\t\t      oid_to_hex(&base_oid), (uintmax_t)obj_offset,\n \t\t\t\t      p->pack_name);\n-\t\t\t\tmark_bad_packed_object(p, base_oid.hash);\n+\t\t\t\tmark_bad_packed_object(p, &base_oid);\n \t\t\t\tbase = read_object(r, &base_oid, &type, &base_size);\n \t\t\t\texternal_base = base;\n \t\t\t}\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8ae..a982ed9994 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -159,7 +159,7 @@ int packed_object_info(struct repository *r,\n \t\t       struct packed_git *pack,\n \t\t       off_t offset, struct object_info *);\n\n-void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n+void mark_bad_packed_object(struct packed_git *, const struct object_id *);\n const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n\n #define ON_DISK_KEEP_PACKS 1\n--\n2.33.0\n"},{"id":"435595","messageId":"dcf75a98-8659-2df0-9d6d-4015aec81c6c@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"[PATCH v2 4/5] packfile: convert has_packed_and_bad() to object_id","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:42:20Z","receivedAt":"2021-09-11T20:42:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The single caller has a full object ID, so pass it on instead of just\nits hash member.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n object-file.c | 2 +-\n packfile.c    | 4 ++--\n packfile.h    | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex fb5a385a06..01e7058b4e 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1725,7 +1725,7 @@ void *read_object_file_extended(struct repository *r,\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(repl), path);\n\n-\tif ((p = has_packed_and_bad(r, repl->hash)) != NULL)\n+\tif ((p = has_packed_and_bad(r, repl)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(repl), p->pack_name);\n \tobj_read_unlock();\ndiff --git a/packfile.c b/packfile.c\nindex fb15fc5b49..04080a558b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1176,14 +1176,14 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n }\n\n const struct packed_git *has_packed_and_bad(struct repository *r,\n-\t\t\t\t\t    const unsigned char *sha1)\n+\t\t\t\t\t    const struct object_id *oid)\n {\n \tstruct packed_git *p;\n \tunsigned i;\n\n \tfor (p = r->objects->packed_git; p; p = p->next)\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(sha1,\n+\t\t\tif (hasheq(oid->hash,\n \t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n \t\t\t\treturn p;\n \treturn NULL;\ndiff --git a/packfile.h b/packfile.h\nindex a982ed9994..186146779d 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -160,7 +160,7 @@ int packed_object_info(struct repository *r,\n \t\t       off_t offset, struct object_info *);\n\n void mark_bad_packed_object(struct packed_git *, const struct object_id *);\n-const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n+const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *);\n\n #define ON_DISK_KEEP_PACKS 1\n #define IN_CORE_KEEP_PACKS 2\n--\n2.33.0\n"},{"id":"435596","messageId":"fb64d00f-b44a-6a23-3ddd-027f1df83a22@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"[PATCH v2 5/5] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:43:26Z","receivedAt":"2021-09-11T20:43:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Store the object ID of broken pack entries in an oidset instead of\nkeeping only their hashes in an unsorted array.  The resulting code is\nshorter and easier to read.  It also handles the (hopefully) very rare\ncase of having a high number of bad objects better.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n midx.c         | 10 +++-------\n object-store.h |  4 ++--\n packfile.c     | 28 ++++++----------------------\n 3 files changed, 11 insertions(+), 31 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8cb063023c..76322b713c 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -307,13 +307,9 @@ int fill_midx_entry(struct repository * r,\n \tif (!is_pack_valid(p))\n \t\treturn 0;\n\n-\tif (p->num_bad_objects) {\n-\t\tuint32_t i;\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid->hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn 0;\n-\t}\n+\tif (oidset_size(&p->bad_objects) &&\n+\t    oidset_contains(&p->bad_objects, oid))\n+\t\treturn 0;\n\n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\ndiff --git a/object-store.h b/object-store.h\nindex b4dc6668aa..c7bead66f6 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -10,6 +10,7 @@\n #include \"khash.h\"\n #include \"dir.h\"\n #include \"oidtree.h\"\n+#include \"oidset.h\"\n\n struct object_directory {\n \tstruct object_directory *next;\n@@ -75,9 +76,8 @@ struct packed_git {\n \tconst void *index_data;\n \tsize_t index_size;\n \tuint32_t num_objects;\n-\tuint32_t num_bad_objects;\n \tuint32_t crc_offset;\n-\tunsigned char *bad_object_sha1;\n+\tstruct oidset bad_objects;\n \tint index_version;\n \ttime_t mtime;\n \tint pack_fd;\ndiff --git a/packfile.c b/packfile.c\nindex 04080a558b..caba29c624 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1163,29 +1163,17 @@ int unpack_object_header(struct packed_git *p,\n\n void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n {\n-\tunsigned i;\n-\tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\tif (hasheq(oid->hash, p->bad_object_sha1 + hashsz * i))\n-\t\t\treturn;\n-\tp->bad_object_sha1 = xrealloc(p->bad_object_sha1,\n-\t\t\t\t      st_mult(GIT_MAX_RAWSZ,\n-\t\t\t\t\t      st_add(p->num_bad_objects, 1)));\n-\thashcpy(p->bad_object_sha1 + hashsz * p->num_bad_objects, oid->hash);\n-\tp->num_bad_objects++;\n+\toidset_insert(&p->bad_objects, oid);\n }\n\n const struct packed_git *has_packed_and_bad(struct repository *r,\n \t\t\t\t\t    const struct object_id *oid)\n {\n \tstruct packed_git *p;\n-\tunsigned i;\n\n \tfor (p = r->objects->packed_git; p; p = p->next)\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid->hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn p;\n+\t\tif (oidset_contains(&p->bad_objects, oid))\n+\t\t\treturn p;\n \treturn NULL;\n }\n\n@@ -2016,13 +2004,9 @@ static int fill_pack_entry(const struct object_id *oid,\n {\n \toff_t offset;\n\n-\tif (p->num_bad_objects) {\n-\t\tunsigned i;\n-\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\tif (hasheq(oid->hash,\n-\t\t\t\t   p->bad_object_sha1 + the_hash_algo->rawsz * i))\n-\t\t\t\treturn 0;\n-\t}\n+\tif (oidset_size(&p->bad_objects) &&\n+\t    oidset_contains(&p->bad_objects, oid))\n+\t\treturn 0;\n\n \toffset = find_pack_entry_one(oid->hash, p);\n \tif (!offset)\n--\n2.33.0\n"},{"id":"435597","messageId":"14d4f3d0-b94f-974e-0c65-bddc54d89cca@web.de","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T20:45:15Z","receivedAt":"2021-09-11T20:45:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 22:31 schrieb René Scharfe:\n> Replace the custom hash array for remembering corrupt pack entries with\n> an oidset.  This shortens and simplifies the code.\n>\n> Changes since v1:\n> - inline oidset_size()\n> - inline nth_midxed_pack_entry() early\n> - use oidset_size() to avoid a function call if no bad objects exist\n\n- forgot to add \"v2\" to the subject of patches 1, 2, 3 :-/\n\nRené\n"},{"id":"435598","messageId":"YT0dgTgLVrtS9md6@coredump.intra.peff.net","threadId":"56484","inReplyTo":"120ffa79-d3cb-2e81-1da6-358e7407ec69@web.de","subject":"Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T21:20:01Z","receivedAt":"2021-09-11T21:20:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 10:31:34PM +0200, René Scharfe wrote:\n\n> Am 11.09.21 um 19:07 schrieb Jeff King:\n> > On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:\n> >\n> >> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n> >>  \tif (!is_pack_valid(p))\n> >>  \t\treturn 0;\n> >>\n> >> -\tnth_midxed_object_oid(&oid, m, pos);\n> >> -\tif (oidset_contains(&p->bad_objects, &oid))\n> >> +\tif (oidset_contains(&p->bad_objects, oid))\n> >>  \t\treturn 0;\n> >\n> > So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice.\n> >\n> > Compared to the code before your series, we still have an extra function\n> > call to oidset_contains(), which will (in the common case) notice we\n> > have no entries and immediately return. But I think that's getting into\n> > pointless micro-optimization.\n> \n> Right.  I measure a 0.5% slowdown for git multi-pack-index verify.  An\n> inline oidset_size call avoids it.  That's easy enough to add, so let's\n> have it!\n\nI don't mind that, but I wonder if we can have our cake and eat it, too.\n\noidset_contains() is short, too, and could be inlined. Or if we're\nworried about the size of the embedded kh_get_oid_set() getting inlined,\nwe could do something like:\n\n  static inline int oidset_contains(const struct oidset *set, const\n\t\t\t\t    struct object_id *oid)\n  {\n\tif (!oidset_size(set))\n\t\treturn 0;\n\treturn oidset_contains_func(set, oid);\n  }\n\nThat saves callers from having to deal with it, at the expense of a\nslightly complicated oidset implementation.\n\nI guess it's an extra integer comparison for callers that _do_ expect to\nhave a non-empty set. So maybe it is better left to the caller to\ndecide whether to optimize in this way.\n\n(A totally inline oidset_contains() avoids the extra check, but possibly\nat the cost of larger code size).\n\n-Peff\n"},{"id":"435599","messageId":"YT0eBC16DLkd80Ak@coredump.intra.peff.net","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-11T21:22:12Z","receivedAt":"2021-09-11T21:22:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 11, 2021 at 10:31:52PM +0200, René Scharfe wrote:\n\n> Replace the custom hash array for remembering corrupt pack entries with\n> an oidset.  This shortens and simplifies the code.\n> \n> Changes since v1:\n> - inline oidset_size()\n> - inline nth_midxed_pack_entry() early\n> - use oidset_size() to avoid a function call if no bad objects exist\n\nThanks, these all look fine to me. I raised a question elsewhere in the\nthread about inlining oidset_contains(), but I think that can be\nconsidered separately (and is less clear-cut; we might win by saving a\nfunction call, but we might lose due to larger code size).\n\n-Peff\n"},{"id":"435604","messageId":"a96f8f25-46c6-5d61-6b3d-4b2da2e44566@web.de","threadId":"56484","inReplyTo":"YT0dgTgLVrtS9md6@coredump.intra.peff.net","subject":"Re: [PATCH 4/3] midx: inline nth_midxed_pack_entry()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T23:39:52Z","receivedAt":"2021-09-11T23:40:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.21 um 23:20 schrieb Jeff King:\n> On Sat, Sep 11, 2021 at 10:31:34PM +0200, René Scharfe wrote:\n>\n>> Am 11.09.21 um 19:07 schrieb Jeff King:\n>>> On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote:\n>>>\n>>>> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n>>>>  \tif (!is_pack_valid(p))\n>>>>  \t\treturn 0;\n>>>>\n>>>> -\tnth_midxed_object_oid(&oid, m, pos);\n>>>> -\tif (oidset_contains(&p->bad_objects, &oid))\n>>>> +\tif (oidset_contains(&p->bad_objects, oid))\n>>>>  \t\treturn 0;\n>>>\n>>> So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice.\n>>>\n>>> Compared to the code before your series, we still have an extra function\n>>> call to oidset_contains(), which will (in the common case) notice we\n>>> have no entries and immediately return. But I think that's getting into\n>>> pointless micro-optimization.\n>>\n>> Right.  I measure a 0.5% slowdown for git multi-pack-index verify.  An\n>> inline oidset_size call avoids it.  That's easy enough to add, so let's\n>> have it!\n>\n> I don't mind that, but I wonder if we can have our cake and eat it, too.\n>\n> oidset_contains() is short, too, and could be inlined. Or if we're\n> worried about the size of the embedded kh_get_oid_set() getting inlined,\n> we could do something like:\n>\n>   static inline int oidset_contains(const struct oidset *set, const\n> \t\t\t\t    struct object_id *oid)\n>   {\n> \tif (!oidset_size(set))\n> \t\treturn 0;\n> \treturn oidset_contains_func(set, oid);\n>   }\n>\n> That saves callers from having to deal with it, at the expense of a\n> slightly complicated oidset implementation.\n>\n> I guess it's an extra integer comparison for callers that _do_ expect to\n> have a non-empty set. So maybe it is better left to the caller to\n> decide whether to optimize in this way.\n>\n> (A totally inline oidset_contains() avoids the extra check, but possibly\n> at the cost of larger code size).\n\nI wondered the same.\n\nInlining oidset_contains() would follow the spirit of khash.  It adds\n16KB to my build (ca. 688 bytes per caller).  Hmm.\n\nI expected the hybrid approach with an inlined emptiness check and a\nshared actual contains function to be as fast as the original code, due\nto caching.  I actually saw the 0.1% slowdown of git multi-pack-index\nverify when I added a fake bad object at the end of prepare_midx_pack()\nto simulate a non-empty oidset.  Hmm!\n\nBoth are probably defensible, but for this series I took the more\ntargeted approach to limit the impact.\n\nRené\n"},{"id":"435605","messageId":"9cdf54db-132e-d771-e177-6673f7ad02bf@gmail.com","threadId":"56484","inReplyTo":"e50c1465-59de-7fe1-de01-800404c7640e@web.de","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-11T23:59:40Z","receivedAt":"2021-09-11T23:59:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/21 4:31 PM, René Scharfe wrote:\n> Replace the custom hash array for remembering corrupt pack entries with\n> an oidset.  This shortens and simplifies the code.\n> \n> Changes since v1:\n> - inline oidset_size()\n> - inline nth_midxed_pack_entry() early\n> - use oidset_size() to avoid a function call if no bad objects exist\n> \n>   oidset: make oidset_size() an inline function\n>   midx: inline nth_midxed_pack_entry()\n>   packfile: convert mark_bad_packed_object() to object_id\n>   packfile: convert has_packed_and_bad() to object_id\n>   packfile: use oidset for bad objects\n\nThese were easy reads, and I understand the value of them.\n\nI initially hesitated to support the drop of\nnth_midxed_pack_entry(), since it was designed with things\nlike midx bitmaps in mind (specifically, to also support\nlex-order-to-stable-order conversions). However, it seems\nthat the midx bitmap series by Taylor is succeeding without\nneeding such a translation.\n\nThanks,\n-Stolee\n"},{"id":"435616","messageId":"YT1dCAuCE7xIIIk/@nand.local","threadId":"56484","inReplyTo":"9cdf54db-132e-d771-e177-6673f7ad02bf@gmail.com","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-12T01:51:04Z","receivedAt":"2021-09-12T01:51:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 11, 2021 at 07:59:40PM -0400, Derrick Stolee wrote:\n> I initially hesitated to support the drop of\n> nth_midxed_pack_entry(), since it was designed with things\n> like midx bitmaps in mind (specifically, to also support\n> lex-order-to-stable-order conversions).\n\nI didn't know that nth_midxed_pack_entry was designed with either\npurpose in mind, since it predates midx bitmaps by quite a bit.\n\n> However, it seems that the midx bitmap series by Taylor is succeeding\n> without needing such a translation.\n\nRight, it looks like that function is only called by fill_midx_entry()\nwhere it was inlined.\n\nThanks,\nTaylor\n"},{"id":"435620","messageId":"YT1l75uQ50e2IaXM@nand.local","threadId":"56484","inReplyTo":"YT1dCAuCE7xIIIk/@nand.local","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-12T02:29:03Z","receivedAt":"2021-09-12T02:29:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 11, 2021 at 09:51:04PM -0400, Taylor Blau wrote:\n> On Sat, Sep 11, 2021 at 07:59:40PM -0400, Derrick Stolee wrote:\n> > I initially hesitated to support the drop of\n> > nth_midxed_pack_entry(), since it was designed with things\n> > like midx bitmaps in mind (specifically, to also support\n> > lex-order-to-stable-order conversions).\n>\n> I didn't know that nth_midxed_pack_entry was designed with either\n> purpose in mind, since it predates midx bitmaps by quite a bit.\n\nThinking on it more, I can imagine that you wrote this function\naspirationally envisioning something like MIDX bitmaps. And since you\nand I discussed the design together quite a bit, I imagine that that's\nthe case ;-).\n\nBut I agree that after reading this series again, that the inline-ing\nsuggested makes sense (and doesn't conflict with any series I have in\nflight which don't add any new callers).\n\nThanks,\nTaylor\n"},{"id":"435623","messageId":"4d76db59-e049-32c3-c4dd-ab7f70cbc20b@gmail.com","threadId":"56484","inReplyTo":"YT1l75uQ50e2IaXM@nand.local","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-12T03:51:36Z","receivedAt":"2021-09-12T03:51:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/21 10:29 PM, Taylor Blau wrote:\n> On Sat, Sep 11, 2021 at 09:51:04PM -0400, Taylor Blau wrote:\n>> On Sat, Sep 11, 2021 at 07:59:40PM -0400, Derrick Stolee wrote:\n>>> I initially hesitated to support the drop of\n>>> nth_midxed_pack_entry(), since it was designed with things\n>>> like midx bitmaps in mind (specifically, to also support\n>>> lex-order-to-stable-order conversions).\n>>\n>> I didn't know that nth_midxed_pack_entry was designed with either\n>> purpose in mind, since it predates midx bitmaps by quite a bit.\n> \n> Thinking on it more, I can imagine that you wrote this function\n> aspirationally envisioning something like MIDX bitmaps. And since you\n> and I discussed the design together quite a bit, I imagine that that's\n> the case ;-).\n> \n> But I agree that after reading this series again, that the inline-ing\n> suggested makes sense (and doesn't conflict with any series I have in\n> flight which don't add any new callers).\n\nI'm thinking more to my original design of the multi-pack-index.\nAt that time, I was thinking about the possible integration\nwith bitmaps based on my experience in other systems which used\na stable object order to allow writing bitmaps asynchronously\nwith respect to the multi-pack-index write and object packing.\n\nOne thing that you did when first considering bitmaps over the\nmulti-pack-index was to demonstrate that a stable object order\nis not required, which surprised and delighted me. It greatly\nreduced the complexity of the problem, and being able to inline\nthis method is only one small fallout from that simplicity.\n\nThanks,\n-Stolee\n"},{"id":"435624","messageId":"YT17pcvyq2nyyc3o@nand.local","threadId":"56484","inReplyTo":"4d76db59-e049-32c3-c4dd-ab7f70cbc20b@gmail.com","subject":"Re: [PATCH v2 0/5] packfile: use oidset for bad objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-12T04:01:41Z","receivedAt":"2021-09-12T04:01:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 11, 2021 at 11:51:36PM -0400, Derrick Stolee wrote:\n> On 9/11/21 10:29 PM, Taylor Blau wrote:\n> > On Sat, Sep 11, 2021 at 09:51:04PM -0400, Taylor Blau wrote:\n> >> On Sat, Sep 11, 2021 at 07:59:40PM -0400, Derrick Stolee wrote:\n> >>> I initially hesitated to support the drop of\n> >>> nth_midxed_pack_entry(), since it was designed with things\n> >>> like midx bitmaps in mind (specifically, to also support\n> >>> lex-order-to-stable-order conversions).\n> >>\n> >> I didn't know that nth_midxed_pack_entry was designed with either\n> >> purpose in mind, since it predates midx bitmaps by quite a bit.\n> >\n> > Thinking on it more, I can imagine that you wrote this function\n> > aspirationally envisioning something like MIDX bitmaps. And since you\n> > and I discussed the design together quite a bit, I imagine that that's\n> > the case ;-).\n> >\n> > But I agree that after reading this series again, that the inline-ing\n> > suggested makes sense (and doesn't conflict with any series I have in\n> > flight which don't add any new callers).\n>\n> I'm thinking more to my original design of the multi-pack-index.\n> At that time, I was thinking about the possible integration\n> with bitmaps based on my experience in other systems which used\n> a stable object order to allow writing bitmaps asynchronously\n> with respect to the multi-pack-index write and object packing.\n\nMakes sense, and thank you for clarifying. After re-reading my first\nemail, I figured that this is what you must have been talking about,\nwhich is why I felt like I should rephrase (hence the follow-up email).\n\n> One thing that you did when first considering bitmaps over the\n> multi-pack-index was to demonstrate that a stable object order\n> is not required, which surprised and delighted me. It greatly\n> reduced the complexity of the problem, and being able to inline\n> this method is only one small fallout from that simplicity.\n\nThis was definitely a consequence of what I had observed from seeing\nwhat was \"slow\" when running bitmaps in production at GitHub. There,\nrepacking a repository's objects all into one pack each time we ran our\nautomated background jobs far outpaced the amount of time we spent\ngenerating bitmaps.\n\nAnd with your and Peff's work on improving bitmap generation itself,\nthings are in a pretty good place. I do have some potential ideas for\nfuture improvement, like a mode where bitmaps are only \"fast forwarded\",\nmeaning that new bitmaps are only added between the commits selected for\nbitmapping in the previous round and the current reference tips.\n\nI think things like that end up getting you pretty far, but it may be\ninteresting to come back eventually and revisit adding a stable object\norder. In the meantime, though... ;)\n\nThanks,\nTaylor\n"}]}