{"thread":{"id":"58618","subject":"[PATCH 0/4] midx: trace2 regions and grab-bag patches","startedAt":"2022-10-12T22:05:09Z","lastAt":"2022-10-13T20:35:32Z","messageCount":7,"participants":["Taylor Blau","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"464782","messageId":"cover.1665612094.git.me@ttaylorr.com","threadId":"58618","inReplyTo":null,"subject":"[PATCH 0/4] midx: trace2 regions and grab-bag patches","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T22:01:44Z","receivedAt":"2022-10-12T22:05:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small handful of MIDX and MIDX bitmap-related patches that\nI've been carrying in GitHub's fork for a while now and forgot to send\nupstream.\n\nThe first is a small typofix, and the second is a legitimate bug fix\nwhich allows us to consider annotated tags as bitmap candidates during\ncommit selection. The final two are trace2 regions and instrumentation\nthat I've found helpful when rolling out MIDX bitmaps in a production\nsetting.\n\nSorry that these are so disjointed in nature ;-). I figured that it was\nbetter to send a grab-bag series like this than to hold onto these\npatches forever!\n\nThanks in advance for your review.\n\nTaylor Blau (4):\n  midx.c: fix whitespace typo\n  midx.c: consider annotated tags during bitmap selection\n  midx.c: instrument MIDX and bitmap generation with trace2 regions\n  pack-bitmap-write.c: instrument number of reused bitmaps\n\n midx.c                        | 34 +++++++++++++++++++++++++++++++++-\n pack-bitmap-write.c           |  8 +++++++-\n t/t5326-multi-pack-bitmaps.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 64 insertions(+), 2 deletions(-)\n\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"464783","messageId":"6e92b7707dbe6ec993cf4a36180f5c118e6011d7.1665612094.git.me@ttaylorr.com","threadId":"58618","inReplyTo":"cover.1665612094.git.me@ttaylorr.com","subject":"[PATCH 1/4] midx.c: fix whitespace typo","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T22:01:48Z","receivedAt":"2022-10-12T22:05:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This was unintentionally introduced via 893b563505 (midx: inline\nnth_midxed_pack_entry(), 2021-09-11) where \"struct repository *r\"\nbecame \"struct repository * r\".\n\nThe latter does not adhere to our usual style conventions, so fix that\nup to look more like our usual declarations.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3a8dcfe98e..a9df753220 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -278,7 +278,7 @@ 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-int fill_midx_entry(struct repository * r,\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-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"464784","messageId":"b646c9d841aee066c57562a439ac59da47220a93.1665612094.git.me@ttaylorr.com","threadId":"58618","inReplyTo":"cover.1665612094.git.me@ttaylorr.com","subject":"[PATCH 2/4] midx.c: consider annotated tags during bitmap selection","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T22:01:52Z","receivedAt":"2022-10-12T22:05:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When generating a multi-pack bitmap without a `--refs-snapshot` (e.g.,\nby running `git multi-pack-index write --bitmap` directly), we determine\nthe set of bitmap-able commits by enumerating each reference, and adding\nthe referrent as the tip of a reachability traversal when it appears\nsomewhere in the MIDX. (Any commit we encounter during the reachability\ntraversal then becomes a candidate for bitmap selection).\n\nBut we incorrectly avoid peeling the object at the tip of each\nreference. So if we see some reference that points at an annotated tag\n(which in turn points through zero or more additional annotated tags at\na commit), that we will not add it as a tip for the reachability\ntraversal. This means that if some commit C is only referenced through\none or more annotated tag(s), then C won't become a bitmap candidate.\n\nCorrect this by peeling the reference tips as we enumerate them to\nensure that we consider commits which are the targets of annotated tags,\nin addition to commits which are referenced directly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c                        |  4 ++++\n t/t5326-multi-pack-bitmaps.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex a9df753220..d41dfd8174 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -980,6 +980,7 @@ static int add_ref_to_pending(const char *refname,\n \t\t\t      int flag, void *cb_data)\n {\n \tstruct rev_info *revs = (struct rev_info*)cb_data;\n+\tstruct object_id peeled;\n \tstruct object *object;\n \n \tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n@@ -987,6 +988,9 @@ static int add_ref_to_pending(const char *refname,\n \t\treturn 0;\n \t}\n \n+\tif (!peel_iterated_oid(oid, &peeled))\n+\t\toid = &peeled;\n+\n \tobject = parse_object_or_die(oid, refname);\n \tif (object->type != OBJ_COMMIT)\n \t\treturn 0;\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex ad6eea5fa0..0882cbb6e4 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -410,4 +410,28 @@ test_expect_success 'preferred pack change with existing MIDX bitmap' '\n \t)\n '\n \n+test_expect_success 'tagged commits are selected for bitmapping' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit --annotate base &&\n+\t\tgit repack -d &&\n+\n+\t\t# Remove refs/heads/main which points at the commit directly,\n+\t\t# leaving only a reference to the annotated tag.\n+\t\tgit branch -M main &&\n+\t\tgit checkout base &&\n+\t\tgit branch -d main &&\n+\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\tgit rev-parse HEAD >want &&\n+\t\ttest-tool bitmap list-commits >actual &&\n+\t\tgrep $(cat want) actual\n+\t)\n+'\n+\n test_done\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"464785","messageId":"dc50527d99680fb0ff1f3240531105badaa221dc.1665612094.git.me@ttaylorr.com","threadId":"58618","inReplyTo":"cover.1665612094.git.me@ttaylorr.com","subject":"[PATCH 3/4] midx.c: instrument MIDX and bitmap generation with trace2 regions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T22:01:55Z","receivedAt":"2022-10-12T22:05:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When debugging MIDX and MIDX-bitmap related issues, it is useful to\nfigure out where Git is spending its time.\n\nGitHub has been using the below trace2 regions to instrument various\ncomponents of generating a MIDX itself, as well time spent preparing to\nbuild a MIDX bitmap.\n\nThese are limited to instrumenting the following functions:\n\n  - midx.c::find_commits_for_midx_bitmap()\n  - midx.c::midx_pack_order()\n  - midx.c::prepare_midx_packing_data()\n  - midx.c::write_midx_bitmap()\n  - midx.c::write_midx_internal()\n  - midx.c::write_midx_reverse_index()\n\nto start and end with a trace2_region_enter() and trace2_region_leave(),\nrespectively.\n\nThe category for all of these is \"midx\", which matches the existing\nconvention. The region description matches the name of the function.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex d41dfd8174..7cfad04a24 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -913,6 +913,8 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)\n \tuint32_t *pack_order;\n \tuint32_t i;\n \n+\ttrace2_region_enter(\"midx\", \"midx_pack_order\", the_repository);\n+\n \tALLOC_ARRAY(data, ctx->entries_nr);\n \tfor (i = 0; i < ctx->entries_nr; i++) {\n \t\tstruct pack_midx_entry *e = &ctx->entries[i];\n@@ -930,6 +932,8 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)\n \t\tpack_order[i] = data[i].nr;\n \tfree(data);\n \n+\ttrace2_region_leave(\"midx\", \"midx_pack_order\", the_repository);\n+\n \treturn pack_order;\n }\n \n@@ -939,6 +943,8 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *tmp_file;\n \n+\ttrace2_region_enter(\"midx\", \"write_midx_reverse_index\", the_repository);\n+\n \tstrbuf_addf(&buf, \"%s-%s.rev\", midx_name, hash_to_hex(midx_hash));\n \n \ttmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,\n@@ -948,6 +954,8 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \t\tdie(_(\"cannot store reverse index file\"));\n \n \tstrbuf_release(&buf);\n+\n+\ttrace2_region_leave(\"midx\", \"write_midx_reverse_index\", the_repository);\n }\n \n static void clear_midx_files_ext(const char *object_dir, const char *ext,\n@@ -963,6 +971,8 @@ static void prepare_midx_packing_data(struct packing_data *pdata,\n {\n \tuint32_t i;\n \n+\ttrace2_region_enter(\"midx\", \"prepare_midx_packing_data\", the_repository);\n+\n \tmemset(pdata, 0, sizeof(struct packing_data));\n \tprepare_packing_data(the_repository, pdata);\n \n@@ -973,6 +983,8 @@ static void prepare_midx_packing_data(struct packing_data *pdata,\n \t\toe_set_in_pack(pdata, to,\n \t\t\t       ctx->info[ctx->pack_perm[from->pack_int_id]].p);\n \t}\n+\n+\ttrace2_region_leave(\"midx\", \"prepare_midx_packing_data\", the_repository);\n }\n \n static int add_ref_to_pending(const char *refname,\n@@ -1070,6 +1082,9 @@ static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr\n \tstruct rev_info revs;\n \tstruct bitmap_commit_cb cb = {0};\n \n+\ttrace2_region_enter(\"midx\", \"find_commits_for_midx_bitmap\",\n+\t\t\t    the_repository);\n+\n \tcb.ctx = ctx;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n@@ -1103,6 +1118,10 @@ static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr\n \t\t*indexed_commits_nr_p = cb.commits_nr;\n \n \trelease_revisions(&revs);\n+\n+\ttrace2_region_leave(\"midx\", \"find_commits_for_midx_bitmap\",\n+\t\t\t    the_repository);\n+\n \treturn cb.commits;\n }\n \n@@ -1120,6 +1139,8 @@ static int write_midx_bitmap(const char *midx_name,\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name,\n \t\t\t\t\thash_to_hex(midx_hash));\n \n+\ttrace2_region_enter(\"midx\", \"write_midx_bitmap\", the_repository);\n+\n \tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n \t\toptions |= BITMAP_OPT_HASH_CACHE;\n \n@@ -1165,6 +1186,9 @@ static int write_midx_bitmap(const char *midx_name,\n cleanup:\n \tfree(index);\n \tfree(bitmap_name);\n+\n+\ttrace2_region_leave(\"midx\", \"write_midx_bitmap\", the_repository);\n+\n \treturn ret;\n }\n \n@@ -1211,6 +1235,8 @@ static int write_midx_internal(const char *object_dir,\n \tint result = 0;\n \tstruct chunkfile *cf;\n \n+\ttrace2_region_enter(\"midx\", \"write_midx_internal\", the_repository);\n+\n \tget_midx_filename(&midx_name, object_dir);\n \tif (safe_create_leading_directories(midx_name.buf))\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n@@ -1552,6 +1578,8 @@ static int write_midx_internal(const char *object_dir,\n \tfree(ctx.pack_order);\n \tstrbuf_release(&midx_name);\n \n+\ttrace2_region_leave(\"midx\", \"write_midx_internal\", the_repository);\n+\n \treturn result;\n }\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"464786","messageId":"dd5ceb1ec3c01e8a2af55130718fff0b5eaf2de0.1665612094.git.me@ttaylorr.com","threadId":"58618","inReplyTo":"cover.1665612094.git.me@ttaylorr.com","subject":"[PATCH 4/4] pack-bitmap-write.c: instrument number of reused bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-12T22:01:57Z","receivedAt":"2022-10-12T22:05:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When debugging bitmap generation performance, it is useful to know how\nmany bitmaps were generated from scratch, and how many were the result\nof permuting the bit-order of an existing bitmap.\n\nKeep track of the latter, and emit the count as a trace2_data line to\naid in debugging.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap-write.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex a213f5eddc..cfa67a510f 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -384,6 +384,8 @@ static int fill_bitmap_tree(struct bitmap *bitmap,\n \treturn 0;\n }\n \n+static int reused_bitmaps_nr;\n+\n static int fill_bitmap_commit(struct bb_commit *ent,\n \t\t\t      struct commit *commit,\n \t\t\t      struct prio_queue *queue,\n@@ -409,8 +411,10 @@ static int fill_bitmap_commit(struct bb_commit *ent,\n \t\t\t * bitmap and add its bits to this one. No need to walk\n \t\t\t * parents or the tree for this commit.\n \t\t\t */\n-\t\t\tif (old && !rebuild_bitmap(mapping, old, ent->bitmap))\n+\t\t\tif (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {\n+\t\t\t\treused_bitmaps_nr++;\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\t/*\n@@ -526,6 +530,8 @@ int bitmap_writer_build(struct packing_data *to_pack)\n \n \ttrace2_region_leave(\"pack-bitmap-write\", \"building_bitmaps_total\",\n \t\t\t    the_repository);\n+\ttrace2_data_intmax(\"pack-bitmap-write\", the_repository,\n+\t\t\t   \"building_bitmaps_reused\", reused_bitmaps_nr);\n \n \tstop_progress(&writer.progress);\n \n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"464826","messageId":"fa47a48b-6da7-a456-05d6-cd5c17b159c3@github.com","threadId":"58618","inReplyTo":"cover.1665612094.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] midx: trace2 regions and grab-bag patches","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-10-13T13:10:16Z","receivedAt":"2022-10-13T13:10:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/12/2022 6:01 PM, Taylor Blau wrote:\n> Here is a small handful of MIDX and MIDX bitmap-related patches that\n> I've been carrying in GitHub's fork for a while now and forgot to send\n> upstream.\n> \n> The first is a small typofix, and the second is a legitimate bug fix\n> which allows us to consider annotated tags as bitmap candidates during\n> commit selection. The final two are trace2 regions and instrumentation\n> that I've found helpful when rolling out MIDX bitmaps in a production\n> setting.\n> \n> Sorry that these are so disjointed in nature ;-). I figured that it was\n> better to send a grab-bag series like this than to hold onto these\n> patches forever!\n\nAs advertised, this set of patches are all nice and small. I've found\nthe additional tracing useful during performance investigations and\nunobtrusive otherwise.\n\nThough they looked familiar, I gave them a careful read and have no\ncomments. LGTM.\n\nThanks,\n-Stolee\n"},{"id":"464890","messageId":"xmqq1qrbfoms.fsf@gitster.g","threadId":"58618","inReplyTo":"fa47a48b-6da7-a456-05d6-cd5c17b159c3@github.com","subject":"Re: [PATCH 0/4] midx: trace2 regions and grab-bag patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-13T20:35:23Z","receivedAt":"2022-10-13T20:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> On 10/12/2022 6:01 PM, Taylor Blau wrote:\n>> Here is a small handful of MIDX and MIDX bitmap-related patches that\n>> I've been carrying in GitHub's fork for a while now and forgot to send\n>> upstream.\n>> \n>> The first is a small typofix, and the second is a legitimate bug fix\n>> which allows us to consider annotated tags as bitmap candidates during\n>> commit selection. The final two are trace2 regions and instrumentation\n>> that I've found helpful when rolling out MIDX bitmaps in a production\n>> setting.\n>> \n>> Sorry that these are so disjointed in nature ;-). I figured that it was\n>> better to send a grab-bag series like this than to hold onto these\n>> patches forever!\n>\n> As advertised, this set of patches are all nice and small. I've found\n> the additional tracing useful during performance investigations and\n> unobtrusive otherwise.\n>\n> Though they looked familiar, I gave them a careful read and have no\n> comments. LGTM.\n\nYeah, they look good to me, too.\n\nThanks, both.\n"}]}