{"thread":{"id":"58693","subject":"[PATCH] index: add trace2 region for clear skip worktree","startedAt":"2022-10-26T00:05:30Z","lastAt":"2022-11-07T20:50:49Z","messageCount":18,"participants":["Anh Le via GitGitGadget","Junio C Hamano","Jeff Hostetler","Anh Le","Derrick Stolee","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465747","messageId":"pull.1368.git.git.1666742722502.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":null,"subject":"[PATCH] index: add trace2 region for clear skip worktree","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-26T00:05:22Z","receivedAt":"2022-10-26T00:05:30Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"From: Anh Le \n\nIn a large repository using sparse checkout, checking\nwhether a file marked with skip worktree is present\non disk and its skip worktree bit should be cleared\ncan take a considerable amount of time. Add a trace2\nregion to surface this information.\n\nSigned-off-by: Anh Le \n---\n index: add trace2 region for clear skip worktree\n \n In large repository using sparse checkout, checking whether a file\n marked with skip worktree is present on disk and its skip worktree bit\n should be cleared can take a considerable amount of time. Add a trace2\n region to surface this information.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1368%2FHaizzz%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1368/Haizzz/master-v1\nPull-Request: https://github.com/git/git/pull/1368\n\n sparse-index.c | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e4a54ce1943..d11049c8aeb 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -493,24 +493,33 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \tint dir_found = 1;\n \n \tint i;\n+\tintmax_t path_count = 0;\n+\tintmax_t restart_count = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n \n-\t\tif (ce_skip_worktree(ce) &&\n-\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n-\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n-\t\t\t\tensure_full_index(istate);\n-\t\t\t\tgoto restart;\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpath_count++;\n+\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n+\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tensure_full_index(istate);\n+\t\t\t\t\trestart_count++;\n+\t\t\t\t\tgoto restart;\n+\t\t\t\t}\n+\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t\t}\n-\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t}\n \t}\n+\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_count);\n+\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/restart_count\", restart_count);\n+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n }\n \n /*\n\nbase-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886\n-- \ngitgitgadget\n"},{"id":"465749","messageId":"xmqq35bbmfz6.fsf@gitster.g","threadId":"58693","inReplyTo":"pull.1368.git.git.1666742722502.gitgitgadget@gmail.com","subject":"Re: [PATCH] index: add trace2 region for clear skip worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T03:16:45Z","receivedAt":"2022-10-26T03:16:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Anh Le via GitGitGadget\" writes:\n\n> From: Anh Le \n>\n> In a large repository using sparse checkout, checking\n> whether a file marked with skip worktree is present\n> on disk and its skip worktree bit should be cleared\n> can take a considerable amount of time. Add a trace2\n> region to surface this information.\n>\n> Signed-off-by: Anh Le \n> ---\n> index: add trace2 region for clear skip worktree\n> \n> In large repository using sparse checkout, checking whether a file\n> marked with skip worktree is present on disk and its skip worktree bit\n> should be cleared can take a considerable amount of time. Add a trace2\n> region to surface this information.\n\nIt is easy to see that the change is no-op from functionality's\nstandpoint. The condition under which ce->ce_flags loses the\nCE_SKIP_WORKTREE bit is the same as before, and the only change is\nthat in the iteration a couple of variables are incremented, which\nmay (or may not) have performance impact, but shouldn't break\ncorrectness.\n\nI am not sure about the value of these counters, honestly.\n\nIf we hit a sparse dir just once, we fully flatten the index and go\nback to the restart state, and it would be a bug if we still see a\nsparsified directory in the in-core index after that point, so\nrestart_count would be at most one, wouldn't it? Why do we even\nneed to count with intmax_t?\n\nSimilarly, path_count is bounded by istate->cache_nr. As you know\nthe approximate size of your project, I am not sure what extra\ninformation you want to get by counting the paths with the skip bit\nset before the first sparsified directory in the in-core index\ntwice, and the other paths with the skip bit set just once, and\nadding these numbers together. Also again, I am not quite sure what\nthe point is to count the paths in intmax_t.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1368%2FHaizzz%2Fmaster-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1368/Haizzz/master-v1\n> Pull-Request: https://github.com/git/git/pull/1368\n>\n> sparse-index.c | 21 +++++++++++++++------\n> 1 file changed, 15 insertions(+), 6 deletions(-)\n>\n> diff --git a/sparse-index.c b/sparse-index.c\n> index e4a54ce1943..d11049c8aeb 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -493,24 +493,33 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n> \tint dir_found = 1;\n> \n> \tint i;\n> +\tintmax_t path_count = 0;\n> +\tintmax_t restart_count = 0;\n> \n> \tif (!core_apply_sparse_checkout ||\n> \t sparse_expect_files_outside_of_patterns)\n> \t\treturn;\n> \n> +\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> restart:\n> \tfor (i = 0; i < istate->cache_nr; i++) {\n> \t\tstruct cache_entry *ce = istate->cache[i];\n> \n> -\t\tif (ce_skip_worktree(ce) &&\n> -\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> -\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> -\t\t\t\tensure_full_index(istate);\n> -\t\t\t\tgoto restart;\n> +\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\tpath_count++;\n> +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> +\t\t\t\t\tensure_full_index(istate);\n> +\t\t\t\t\trestart_count++;\n> +\t\t\t\t\tgoto restart;\n> +\t\t\t\t}\n> +\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t\t}\n> -\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t}\n> \t}\n> +\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_count);\n> +\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/restart_count\", restart_count);\n> +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> }\n> \n> /*\n>\n> base-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886\n"},{"id":"465764","messageId":"d4103788-5153-11f2-487f-5cc795d261a8@jeffhostetler.com","threadId":"58693","inReplyTo":"xmqq35bbmfz6.fsf@gitster.g","subject":"Re: [PATCH] index: add trace2 region for clear skip worktree","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-10-26T14:13:18Z","receivedAt":"2022-10-26T14:13:23Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 10/25/22 11:16 PM, Junio C Hamano wrote:\n> \"Anh Le via GitGitGadget\" writes:\n> \n>> From: Anh Le \n>>\n>> In a large repository using sparse checkout, checking\n>> whether a file marked with skip worktree is present\n>> on disk and its skip worktree bit should be cleared\n>> can take a considerable amount of time. Add a trace2\n>> region to surface this information.\n>>\n>> Signed-off-by: Anh Le \n>> ---\n>> index: add trace2 region for clear skip worktree\n>> \n>> In large repository using sparse checkout, checking whether a file\n>> marked with skip worktree is present on disk and its skip worktree bit\n>> should be cleared can take a considerable amount of time. Add a trace2\n>> region to surface this information.\n> \n> It is easy to see that the change is no-op from functionality's\n> standpoint. The condition under which ce->ce_flags loses the\n> CE_SKIP_WORKTREE bit is the same as before, and the only change is\n> that in the iteration a couple of variables are incremented, which\n> may (or may not) have performance impact, but shouldn't break\n> correctness.\n> \n> I am not sure about the value of these counters, honestly.\n\nI suggested adding the counters while the series was still over\non GitGitGadget.\n\nThe original goal was to measure the time spent in that loop\nwith the region_enter/region_leave events. They will tell you\nwhether the loop was slow or not and you can compare that with\nother (peer or containing) regions to see if it significant.\n\nHowever, it doesn't tell us anything about WHY it was slow or\nwas slower than another instance. The path_count gives us a\nsimple estimate on the number of lstat() calls -- which is\none of frequent sources of slowness. Yes, it is bounded by\n`istate->cache_nr`, but if I have a repo with 1M cache-entries\nand 95% are sparse, this loop is going to be much slower than\nif only 5% are sparse.\n\nThe restart_count (which we only expect to be 0 or 1), will\ntell us whether or not at some point mid-loop we had to fully\ninflate the index -- which is another source of slowness.\nGranted, `ensure_full_index()` probably contains a region of\nits own, so the restart_count here may not absolutely necessary,\nbut in the context of this loop -- this counter may point to the\nother smoking gun.\n\nIn the worst case, we walk the entire index and lstat() for a\nsignificant number of skipped-and-not-present files, then near\nthe end of the loop, we find a skipped-but-present directory\nand have to restart the loop. The second pass will still run\nthe full loop again. Will the second pass actually see any\nskipped cache-entries? Will it re-lstat() them? Could the\n`goto restart` just be a `break` or `return`?\n\nI haven't had time to look under the hood here, but I was\nhoping that these two counters would help the series author\ncollect telemetry over many runs and gain more insight into\nthe perf problem.\n\nContinuing the example from above, if we've already paid the\ncosts to lstat() the 95% sparse files AND THEN near the bottom\nof the loop we have to do a restart, then we should expect\nthis loop to be doubly slow. It was my hope that this combination\nof counters would help us understand the variations in perf.\n\nWRT the `intmax_t` vs just `int`: either is fine. I suggested\nthe former because that is what `trace2_data_intmax()` takes.\nI really don't expect any usage to overflow a regular 32bit int.\n\n[...]\n>> +\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_count);\n>> +\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/restart_count\", restart_count);\n\nOne thing I forgot to mention in my GGG suggestion was to only\nemit these data events if the counter is > 0. We've been trying\nto avoid logging zeroes.\n\n>> +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n[...]\n\nJeff\n"},{"id":"465805","messageId":"xmqq8rl2lgl3.fsf@gitster.g","threadId":"58693","inReplyTo":"d4103788-5153-11f2-487f-5cc795d261a8@jeffhostetler.com","subject":"Re: [PATCH] index: add trace2 region for clear skip worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T16:01:12Z","receivedAt":"2022-10-26T16:02:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> In the worst case, we walk the entire index and lstat() for a\n> significant number of skipped-and-not-present files, then near\n> the end of the loop, we find a skipped-but-present directory\n> and have to restart the loop. The second pass will still run\n> the full loop again. Will the second pass actually see any\n> skipped cache-entries? Will it re-lstat() them? Could the\n> `goto restart` just be a `break` or `return`?\n>\n> I haven't had time to look under the hood here, but I was\n> hoping that these two counters would help the series author\n> collect telemetry over many runs and gain more insight into\n> the perf problem.\n\nWithout being able to answer these questions, would we be able to\ninterpret the numbers reported from these counters?\n\n> Continuing the example from above, if we've already paid the\n> costs to lstat() the 95% sparse files AND THEN near the bottom\n> of the loop we have to do a restart, then we should expect\n> this loop to be doubly slow. It was my hope that this combination\n> of counters would help us understand the variations in perf.\n\nYes, I understand that double-counting may give some clue to detect\nthat, but it just looked roundabout way to do that. Perhaps\ncounting the path inspected during the first iteration and the path\ninspected during the second iteration, separately, without the \"how\nmany times did we repeat?\", would give you a better picture? \"After\ninspecting 2400 paths, we need to go back and then ended up scanning\n3000 paths in the flattened index in the second round\" would be\neasier to interpret than \"We needed flattening, and scanned 5400\npaths in total in the two iterations\".\n\n> WRT the `intmax_t` vs just `int`: either is fine.\n\nI thought \"int\" was supposed to be natural machine word, while\nincrementing \"intmax_t\" is allowed to be much slower than \"int\".\nDo we expect more than 2 billion paths?\n"},{"id":"465810","messageId":"0bb8dc64-aad1-e3c1-66ef-c2e8d6600189@jeffhostetler.com","threadId":"58693","inReplyTo":"xmqq8rl2lgl3.fsf@gitster.g","subject":"Re: [PATCH] index: add trace2 region for clear skip worktree","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-10-26T18:29:09Z","receivedAt":"2022-10-26T18:29:57Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 10/26/22 12:01 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> In the worst case, we walk the entire index and lstat() for a\n>> significant number of skipped-and-not-present files, then near\n>> the end of the loop, we find a skipped-but-present directory\n>> and have to restart the loop. The second pass will still run\n>> the full loop again. Will the second pass actually see any\n>> skipped cache-entries? Will it re-lstat() them? Could the\n>> `goto restart` just be a `break` or `return`?\n>>\n>> I haven't had time to look under the hood here, but I was\n>> hoping that these two counters would help the series author\n>> collect telemetry over many runs and gain more insight into\n>> the perf problem.\n> \n> Without being able to answer these questions, would we be able to\n> interpret the numbers reported from these counters?\n> \n>> Continuing the example from above, if we've already paid the\n>> costs to lstat() the 95% sparse files AND THEN near the bottom\n>> of the loop we have to do a restart, then we should expect\n>> this loop to be doubly slow. It was my hope that this combination\n>> of counters would help us understand the variations in perf.\n> \n> Yes, I understand that double-counting may give some clue to detect\n> that, but it just looked roundabout way to do that. Perhaps\n> counting the path inspected during the first iteration and the path\n> inspected during the second iteration, separately, without the \"how\n> many times did we repeat?\", would give you a better picture? \"After\n> inspecting 2400 paths, we need to go back and then ended up scanning\n> 3000 paths in the flattened index in the second round\" would be\n> easier to interpret than \"We needed flattening, and scanned 5400\n> paths in total in the two iterations\".\n\nGood point and I was wondering about whether we might see \"2 x 95%\"\nvalues for path_count in really slow cases. And yes, it would be\nbetter to have `int path_count[2]` and tally each loop pass\nindependently.\n\nI guess I was looking for a first-order \"where is the pain?\" knowing\nthat we could always dig deeper later. That is, is the lstat's or\nis it the ensure-full and index rewrite? Or both?\n\nWe have other places that do lstat() over the cache-entries and have\nadded code to spread the loop across n threads. I doubt that is needed\nhere and I didn't want to lead with it.\n\n\n> \n>> WRT the `intmax_t` vs just `int`: either is fine.\n> \n> I thought \"int\" was supposed to be natural machine word, while\n> incrementing \"intmax_t\" is allowed to be much slower than \"int\".\n> Do we expect more than 2 billion paths?\n> \n\nYou're right. An `int` would be fine here.\n\nThanks,\nJeff\n"},{"id":"465850","messageId":"CAOZNrpHkh+FWTLAJhCZVPdh2fo1sq1YfgsG5Ou3vO2013njCaA@mail.gmail.com","threadId":"58693","inReplyTo":"0bb8dc64-aad1-e3c1-66ef-c2e8d6600189@jeffhostetler.com","subject":"Re: [PATCH] index: add trace2 region for clear skip worktree","fromName":"Anh Le","fromEmail":"anh@canva.com","sentAt":"2022-10-27T00:04:01Z","receivedAt":"2022-10-27T00:04:44Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"Thank you Junio and Jeff for the feedback! I hadn't considered that\n`ensure_full_index()` would mean the loop can only at most restart\nonce. I'll action the feedbacks:\n- count both loop iterations separately with int instead of intmax_t\n- remove the restart counter\n- don't log metrics if it's 0\n\nRegards,\nAnh\n\n\nRegards,\nAnh\n\n\nOn Thu, Oct 27, 2022 at 5:29 AM Jeff Hostetler wrote:\n>\n>\n>\n> On 10/26/22 12:01 PM, Junio C Hamano wrote:\n> > Jeff Hostetler writes:\n> >\n> >> In the worst case, we walk the entire index and lstat() for a\n> >> significant number of skipped-and-not-present files, then near\n> >> the end of the loop, we find a skipped-but-present directory\n> >> and have to restart the loop. The second pass will still run\n> >> the full loop again. Will the second pass actually see any\n> >> skipped cache-entries? Will it re-lstat() them? Could the\n> >> `goto restart` just be a `break` or `return`?\n> >>\n> >> I haven't had time to look under the hood here, but I was\n> >> hoping that these two counters would help the series author\n> >> collect telemetry over many runs and gain more insight into\n> >> the perf problem.\n> >\n> > Without being able to answer these questions, would we be able to\n> > interpret the numbers reported from these counters?\n> >\n> >> Continuing the example from above, if we've already paid the\n> >> costs to lstat() the 95% sparse files AND THEN near the bottom\n> >> of the loop we have to do a restart, then we should expect\n> >> this loop to be doubly slow. It was my hope that this combination\n> >> of counters would help us understand the variations in perf.\n> >\n> > Yes, I understand that double-counting may give some clue to detect\n> > that, but it just looked roundabout way to do that. Perhaps\n> > counting the path inspected during the first iteration and the path\n> > inspected during the second iteration, separately, without the \"how\n> > many times did we repeat?\", would give you a better picture? \"After\n> > inspecting 2400 paths, we need to go back and then ended up scanning\n> > 3000 paths in the flattened index in the second round\" would be\n> > easier to interpret than \"We needed flattening, and scanned 5400\n> > paths in total in the two iterations\".\n>\n> Good point and I was wondering about whether we might see \"2 x 95%\"\n> values for path_count in really slow cases. And yes, it would be\n> better to have `int path_count[2]` and tally each loop pass\n> independently.\n>\n> I guess I was looking for a first-order \"where is the pain?\" knowing\n> that we could always dig deeper later. That is, is the lstat's or\n> is it the ensure-full and index rewrite? Or both?\n>\n> We have other places that do lstat() over the cache-entries and have\n> added code to spread the loop across n threads. I doubt that is needed\n> here and I didn't want to lead with it.\n>\n>\n> >\n> >> WRT the `intmax_t` vs just `int`: either is fine.\n> >\n> > I thought \"int\" was supposed to be natural machine word, while\n> > incrementing \"intmax_t\" is allowed to be much slower than \"int\".\n> > Do we expect more than 2 billion paths?\n> >\n>\n> You're right. An `int` would be fine here.\n>\n> Thanks,\n> Jeff\n\n-- \n**\n** Empowering the world to design\nWe're hiring, \napply here ! Check out the latest news and \nlearnings from our team on the Canva Newsroom \n.\n \n \n    \n \n \n\n\n\n"},{"id":"465922","messageId":"pull.1368.v2.git.git.1666917961644.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":"pull.1368.git.git.1666742722502.gitgitgadget@gmail.com","subject":"[PATCH v2] index: add trace2 region for clear skip worktree","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-28T00:46:01Z","receivedAt":"2022-10-28T00:46:07Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"From: Anh Le \n\nIn a large repository using sparse checkout, checking\nwhether a file marked with skip worktree is present\non disk and its skip worktree bit should be cleared\ncan take a considerable amount of time. Add a trace2\nregion to surface this information, keeping a count of how many paths\nhave been checked and separately\nkeep counts for after a full index is\nmaterialised.\n\nSigned-off-by: Anh Le \n---\n index: add trace2 region for clear skip worktree\n \n In large repository using sparse checkout, checking whether a file\n marked with skip worktree is present on disk and its skip worktree bit\n should be cleared can take a considerable amount of time. Add a trace2\n region to surface this information.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1368%2FHaizzz%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1368/Haizzz/master-v2\nPull-Request: https://github.com/git/git/pull/1368\n\nRange-diff vs v1:\n\n 1: 7d1bb3cba2a ! 1: effe6b5b912 index: add trace2 region for clear skip worktree\n @@ Commit message\n whether a file marked with skip worktree is present\n on disk and its skip worktree bit should be cleared\n can take a considerable amount of time. Add a trace2\n - region to surface this information.\n + region to surface this information, keeping a count of how many paths\n + have been checked and separately\n + keep counts for after a full index is\n + materialised.\n \n Signed-off-by: Anh Le \n \n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n \tint dir_found = 1;\n \n \tint i;\n -+\tintmax_t path_count = 0;\n -+\tintmax_t restart_count = 0;\n ++\tint path_counts[2] = {0, 0};\n ++\tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n -\t\t\t\tensure_full_index(istate);\n -\t\t\t\tgoto restart;\n +\t\tif (ce_skip_worktree(ce)) {\n -+\t\t\tpath_count++;\n ++\t\t\tpath_counts[restarted]++;\n +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n +\t\t\t\t\tensure_full_index(istate);\n -+\t\t\t\t\trestart_count++;\n ++\t\t\t\t\trestarted = 1;\n +\t\t\t\t\tgoto restart;\n +\t\t\t\t}\n +\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n -\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t}\n \t}\n -+\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_count);\n -+\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/restart_count\", restart_count);\n ++\n ++\tif (path_counts[0] > 0) {\n ++\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n ++\t}\n ++\tif (restarted) {\n ++\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n ++\t}\n +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n }\n \n\n\n sparse-index.c | 26 ++++++++++++++++++++------\n 1 file changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e4a54ce1943..dbf647949c1 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -493,24 +493,38 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \tint dir_found = 1;\n \n \tint i;\n+\tint path_counts[2] = {0, 0};\n+\tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n \n-\t\tif (ce_skip_worktree(ce) &&\n-\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n-\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n-\t\t\t\tensure_full_index(istate);\n-\t\t\t\tgoto restart;\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpath_counts[restarted]++;\n+\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n+\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tensure_full_index(istate);\n+\t\t\t\t\trestarted = 1;\n+\t\t\t\t\tgoto restart;\n+\t\t\t\t}\n+\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t\t}\n-\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t}\n \t}\n+\n+\tif (path_counts[0] > 0) {\n+\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n+\t}\n+\tif (restarted) {\n+\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n+\t}\n+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n }\n \n /*\n\nbase-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886\n-- \ngitgitgadget\n"},{"id":"465954","messageId":"313a6619-273c-066e-c3da-a519b38c03af@github.com","threadId":"58693","inReplyTo":"pull.1368.v2.git.git.1666917961644.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] index: add trace2 region for clear skip worktree","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-10-28T15:49:20Z","receivedAt":"2022-10-28T15:49:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/27/2022 8:46 PM, Anh Le via GitGitGadget wrote:\n> From: Anh Le \n> \n> In a large repository using sparse checkout, checking\n> whether a file marked with skip worktree is present\n> on disk and its skip worktree bit should be cleared\n> can take a considerable amount of time. Add a trace2\n> region to surface this information, keeping a count of how many paths\n> have been checked and separately\n> keep counts for after a full index is\n> materialised.\nYou have some strange line wrapping here. You can wrap them better\nusing several editors, but here is brief technique if your core.editor\nis vim (do within 'git commit --amend'):\n\n1. Use \"SHIFT+V\" to get into visual mode.\n2. Use up and down arrows to select the entire paragraph.\n3. Type \"G\" then \"Q\" to word wrap the selection.\n4. Save and close the editor.\n> \tint i;\n> +\tint path_counts[2] = {0, 0};\n> +\tint restarted = 0;\n\nThis count mechanism is a good one. Nice and simple.\n\n> \n> \tif (!core_apply_sparse_checkout ||\n> \t sparse_expect_files_outside_of_patterns)\n> \t\treturn;\n> \n> +\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> restart:\n> \tfor (i = 0; i < istate->cache_nr; i++) {\n> \t\tstruct cache_entry *ce = istate->cache[i];\n> \n> -\t\tif (ce_skip_worktree(ce) &&\n> -\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> -\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> -\t\t\t\tensure_full_index(istate);\n> -\t\t\t\tgoto restart;\n> +\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\tpath_counts[restarted]++;\n> +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> +\t\t\t\t\tensure_full_index(istate);\n> +\t\t\t\t\trestarted = 1;\n> +\t\t\t\t\tgoto restart;\n> +\t\t\t\t}\n> +\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t\t}\n> -\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t}\n> \t}\n> +\n> +\tif (path_counts[0] > 0) {\n> +\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n> +\t}\n> +\tif (restarted) {\n> +\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n> +\t}\n\nA few style things:\n\n1. Use \"if (path_counts[0])\" to detect a non-zero value.\n2. Don't use { } around a single-line block.\n3. Your lines are quite long, due a lot from your long keys.\n Shorten the keys (maybe \"sparse_path_count\" and \"restarted_count\"\n is enough context) and consider using a line break in\n the middle of the parameters.\n\n> +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n\nThanks,\n-Stolee\n"},{"id":"465959","messageId":"ff5d79c9-35f8-8881-8349-63977732c422@jeffhostetler.com","threadId":"58693","inReplyTo":"pull.1368.v2.git.git.1666917961644.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] index: add trace2 region for clear skip worktree","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-10-28T16:50:02Z","receivedAt":"2022-10-28T16:50:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 10/27/22 8:46 PM, Anh Le via GitGitGadget wrote:\n[...]\n> diff --git a/sparse-index.c b/sparse-index.c\n> index e4a54ce1943..dbf647949c1 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -493,24 +493,38 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n> \tint dir_found = 1;\n> \n> \tint i;\n> +\tint path_counts[2] = {0, 0};\n> +\tint restarted = 0;\n> \n> \tif (!core_apply_sparse_checkout ||\n> \t sparse_expect_files_outside_of_patterns)\n> \t\treturn;\n> \n> +\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> restart:\n> \tfor (i = 0; i < istate->cache_nr; i++) {\n> \t\tstruct cache_entry *ce = istate->cache[i];\n> \n> -\t\tif (ce_skip_worktree(ce) &&\n> -\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> -\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> -\t\t\t\tensure_full_index(istate);\n> -\t\t\t\tgoto restart;\n> +\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\tpath_counts[restarted]++;\n> +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> +\t\t\t\t\tensure_full_index(istate);\n> +\t\t\t\t\trestarted = 1;\n> +\t\t\t\t\tgoto restart;\n> +\t\t\t\t}\n> +\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t\t}\n> -\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t}\n> \t}\n> +\n> +\tif (path_counts[0] > 0) {\n> +\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n> +\t}\n> +\tif (restarted) {\n> +\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n> +\t}\n> +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> }\n\nThis looks good. And I think it sets us up to later\nanswer some of our earlier perf questions after you\nget some data.\n\nFor example, if the path_count[0] phase is expensive,\ndoes the restart always need to restart the loop at i=0?\nGranted, the `ensure_full_index()` may create a new cache-entry\narray, so the value of `i` prior to the call may not correspond\nto the same cell after the call at the `goto`, but we could\ncache the strdup the pathname of cache[i] before the call and\ndo a find on the new cache[] afterwards to get the corresponding\ni-prime value and let the restart start the loop on i-prime.\nThat's just an idea, but let's wait for the data to see if that\nwould be helpful.\n\n\nThanks!\nJeff\n\n"},{"id":"465961","messageId":"xmqqy1szj29n.fsf@gitster.g","threadId":"58693","inReplyTo":"313a6619-273c-066e-c3da-a519b38c03af@github.com","subject":"Re: [PATCH v2] index: add trace2 region for clear skip worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-28T17:17:56Z","receivedAt":"2022-10-28T17:18:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> A few style things:\n>\n> 1. Use \"if (path_counts[0])\" to detect a non-zero value.\n> 2. Don't use { } around a single-line block.\n> 3. Your lines are quite long, due a lot from your long keys.\n> Shorten the keys (maybe \"sparse_path_count\" and \"restarted_count\"\n> is enough context) and consider using a line break in\n> the middle of the parameters.\n\nAll good suggestions. Let me add another one.\n\n 4. Call an array you primarily access its individual elements in\n singular. That way, you can say path_count[0] (i.e. the count\n for the zeroth round). An array that exists mostly to be\n passed around as a whole to represent a \"set of things\", on the\n other hand, can be called in plural.\n\nTaking them all together, perhaps something like this is what you\nmeant?\n\n sparse-index.c | 24 ++++++++++++++----------\n 1 file changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git c/sparse-index.c w/sparse-index.c\nindex dbf647949c..8c269dab80 100644\n--- c/sparse-index.c\n+++ w/sparse-index.c\n@@ -493,22 +493,25 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \tint dir_found = 1;\n \n \tint i;\n-\tint path_counts[2] = {0, 0};\n+\tint path_count[2] = {0, 0};\n \tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n-\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n \n \t\tif (ce_skip_worktree(ce)) {\n-\t\t\tpath_counts[restarted]++;\n+\t\t\tpath_count[restarted]++;\n \t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n \t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tif (restarted)\n+\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n \t\t\t\t\tensure_full_index(istate);\n \t\t\t\t\trestarted = 1;\n \t\t\t\t\tgoto restart;\n@@ -518,13 +521,14 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \t\t}\n \t}\n \n-\tif (path_counts[0] > 0) {\n-\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n-\t}\n-\tif (restarted) {\n-\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n-\t}\n-\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n+\tif (path_count[0])\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t \"sparse_path_count\", path_count[0]);\n+\tif (restarted)\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t istate->repo);\n }\n \n /*\n\n"},{"id":"466074","messageId":"CAOZNrpH7DSF9m+M7r=Za--dVWDiHyL4hdAUWHfW=DDy10n5eeQ@mail.gmail.com","threadId":"58693","inReplyTo":"xmqqy1szj29n.fsf@gitster.g","subject":"Re: [PATCH v2] index: add trace2 region for clear skip worktree","fromName":"Anh Le","fromEmail":"anh@canva.com","sentAt":"2022-10-30T23:28:18Z","receivedAt":"2022-10-30T23:28:59Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"Thank you everyone, lots of things for me to take notes of. I'll send\nin another patch.\n\nRegards,\nAnh\n\nOn Sat, Oct 29, 2022 at 4:17 AM Junio C Hamano wrote:\n>\n> Derrick Stolee writes:\n>\n> > A few style things:\n> >\n> > 1. Use \"if (path_counts[0])\" to detect a non-zero value.\n> > 2. Don't use { } around a single-line block.\n> > 3. Your lines are quite long, due a lot from your long keys.\n> > Shorten the keys (maybe \"sparse_path_count\" and \"restarted_count\"\n> > is enough context) and consider using a line break in\n> > the middle of the parameters.\n>\n> All good suggestions. Let me add another one.\n>\n> 4. Call an array you primarily access its individual elements in\n> singular. That way, you can say path_count[0] (i.e. the count\n> for the zeroth round). An array that exists mostly to be\n> passed around as a whole to represent a \"set of things\", on the\n> other hand, can be called in plural.\n>\n> Taking them all together, perhaps something like this is what you\n> meant?\n>\n> sparse-index.c | 24 ++++++++++++++----------\n> 1 file changed, 14 insertions(+), 10 deletions(-)\n>\n> diff --git c/sparse-index.c w/sparse-index.c\n> index dbf647949c..8c269dab80 100644\n> --- c/sparse-index.c\n> +++ w/sparse-index.c\n> @@ -493,22 +493,25 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n> int dir_found = 1;\n>\n> int i;\n> - int path_counts[2] = {0, 0};\n> + int path_count[2] = {0, 0};\n> int restarted = 0;\n>\n> if (!core_apply_sparse_checkout ||\n> sparse_expect_files_outside_of_patterns)\n> return;\n>\n> - trace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> + trace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n> + istate->repo);\n> restart:\n> for (i = 0; i < istate->cache_nr; i++) {\n> struct cache_entry *ce = istate->cache[i];\n>\n> if (ce_skip_worktree(ce)) {\n> - path_counts[restarted]++;\n> + path_count[restarted]++;\n> if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> if (S_ISSPARSEDIR(ce->ce_mode)) {\n> + if (restarted)\n> + BUG(\"ensure-full-index did not fully flatten?\");\n> ensure_full_index(istate);\n> restarted = 1;\n> goto restart;\n> @@ -518,13 +521,14 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n> }\n> }\n>\n> - if (path_counts[0] > 0) {\n> - trace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n> - }\n> - if (restarted) {\n> - trace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n> - }\n> - trace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n> + if (path_count[0])\n> + trace2_data_intmax(\"index\", istate->repo,\n> + \"sparse_path_count\", path_count[0]);\n> + if (restarted)\n> + trace2_data_intmax(\"index\", istate->repo,\n> + \"sparse_path_count_full\", path_count[1]);\n> + trace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n> + istate->repo);\n> }\n>\n> /*\n>\n\n-- \n**\n** Empowering the world to design\nWe're hiring, \napply here ! Check out the latest news and \nlearnings from our team on the Canva Newsroom \n.\n \n \n    \n \n \n\n\n\n"},{"id":"466076","messageId":"pull.1368.v3.git.git.1667177791591.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":"pull.1368.v2.git.git.1666917961644.gitgitgadget@gmail.com","subject":"[PATCH v3] index: add trace2 region for clear skip worktree","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-31T00:56:31Z","receivedAt":"2022-10-31T00:56:38Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"From: Anh Le \n\nIn a large repository using sparse checkout, checking whether a file marked\nwith skip worktree is present on disk and its skip worktree bit should be\ncleared can take a considerable amount of time. Add a trace2 region to\nsurface this information, keeping a count of how many paths have been\nchecked and separately keep counts for after a full index is materialised.\n\nSigned-off-by: Anh Le \n---\n index: add trace2 region for clear skip worktree\n \n In large repository using sparse checkout, checking whether a file\n marked with skip worktree is present on disk and its skip worktree bit\n should be cleared can take a considerable amount of time. Add a trace2\n region to surface this information.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1368%2FHaizzz%2Fmaster-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1368/Haizzz/master-v3\nPull-Request: https://github.com/git/git/pull/1368\n\nRange-diff vs v2:\n\n 1: effe6b5b912 ! 1: d0d9f258b08 index: add trace2 region for clear skip worktree\n @@ Metadata\n ## Commit message ##\n index: add trace2 region for clear skip worktree\n \n - In a large repository using sparse checkout, checking\n - whether a file marked with skip worktree is present\n - on disk and its skip worktree bit should be cleared\n - can take a considerable amount of time. Add a trace2\n - region to surface this information, keeping a count of how many paths\n - have been checked and separately\n - keep counts for after a full index is\n - materialised.\n + In a large repository using sparse checkout, checking whether a file marked\n + with skip worktree is present on disk and its skip worktree bit should be\n + cleared can take a considerable amount of time. Add a trace2 region to\n + surface this information, keeping a count of how many paths have been\n + checked and separately keep counts for after a full index is materialised.\n \n Signed-off-by: Anh Le \n \n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n \tint dir_found = 1;\n \n \tint i;\n -+\tint path_counts[2] = {0, 0};\n ++\tint path_count[2] = {0, 0};\n +\tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n -+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n ++\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n ++\t\t\t\t\t\t\t\t\t\t\tistate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n -\t\t\t\tensure_full_index(istate);\n -\t\t\t\tgoto restart;\n +\t\tif (ce_skip_worktree(ce)) {\n -+\t\t\tpath_counts[restarted]++;\n ++\t\t\tpath_count[restarted]++;\n +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n ++\t\t\t\t\tif (restarted)\n ++\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n +\t\t\t\t\tensure_full_index(istate);\n +\t\t\t\t\trestarted = 1;\n +\t\t\t\t\tgoto restart;\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n \t\t}\n \t}\n +\n -+\tif (path_counts[0] > 0) {\n -+\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/path_count\", path_counts[0]);\n -+\t}\n -+\tif (restarted) {\n -+\t\ttrace2_data_intmax(\"index\", istate->repo, \"clear_skip_worktree_from_present_files/full_index/path_count\", path_counts[1]);\n -+\t}\n -+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\", istate->repo);\n ++\tif (path_count[0])\n ++\t\ttrace2_data_intmax(\"index\", istate->repo,\n ++\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count\", path_count[0]);\n ++\tif (restarted)\n ++\t\ttrace2_data_intmax(\"index\", istate->repo,\n ++\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n ++\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n ++\t\t\t\t\t\t\t\t\t\t istate->repo);\n }\n \n /*\n\n\n sparse-index.c | 30 ++++++++++++++++++++++++------\n 1 file changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e4a54ce1943..8301129bf8f 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -493,24 +493,42 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \tint dir_found = 1;\n \n \tint i;\n+\tint path_count[2] = {0, 0};\n+\tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t\t\t\t\t\t\t\t\tistate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n \n-\t\tif (ce_skip_worktree(ce) &&\n-\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n-\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n-\t\t\t\tensure_full_index(istate);\n-\t\t\t\tgoto restart;\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpath_count[restarted]++;\n+\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n+\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tif (restarted)\n+\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n+\t\t\t\t\tensure_full_index(istate);\n+\t\t\t\t\trestarted = 1;\n+\t\t\t\t\tgoto restart;\n+\t\t\t\t}\n+\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t\t}\n-\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t}\n \t}\n+\n+\tif (path_count[0])\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count\", path_count[0]);\n+\tif (restarted)\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t\t\t\t\t\t\t\t istate->repo);\n }\n \n /*\n\nbase-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886\n-- \ngitgitgadget\n"},{"id":"466130","messageId":"Y2BNdvgCw/mxgV1M@nand.local","threadId":"58693","inReplyTo":"pull.1368.v3.git.git.1667177791591.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] index: add trace2 region for clear skip worktree","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-31T22:34:30Z","receivedAt":"2022-10-31T22:34:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 12:56:31AM +0000, Anh Le via GitGitGadget wrote:\n> From: Anh Le \n>\n> In a large repository using sparse checkout, checking whether a file marked\n> with skip worktree is present on disk and its skip worktree bit should be\n> cleared can take a considerable amount of time. Add a trace2 region to\n> surface this information, keeping a count of how many paths have been\n> checked and separately keep counts for after a full index is materialised.\n\nI think the text you wrote here is fine, but it did take me a couple of\npasses to make sure I understood it correctly.\n\nPerhaps the following might be clearer:\n\n When using sparse checkout, clear_skip_worktree_from_present_files()\n must enumerate index entries to find ones with the SKIP_WORKTREE bit\n to determine whether those index entries exist on disk (in which\n case their SKIP_WORKTREE bit should be removed).\n\n In a large repository, this may take considerable time depending on\n the size of the index.\n\n Add a trace2 region to surface this information, keeping a count o\n fhow many paths have been checked. Separately, keep counts after a\n full index is materialized.\n\nIs that accurate?\n\n> diff --git a/sparse-index.c b/sparse-index.c\n> index e4a54ce1943..8301129bf8f 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -493,24 +493,42 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n> \tint dir_found = 1;\n>\n> \tint i;\n> +\tint path_count[2] = {0, 0};\n> +\tint restarted = 0;\n\nLooks good.\n\n> \tif (!core_apply_sparse_checkout ||\n> \t sparse_expect_files_outside_of_patterns)\n> \t\treturn;\n>\n> +\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n> +\t\t\t\t\t\t\t\t\t\t\tistate->repo);\n\nHere and below, there is some funky indentation. When I apply the patch\nlocally, I find that I need to squash this in:\n\n--- >8 ---\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 8301129bf8..23f997e103 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -501,7 +501,7 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \t\treturn;\n\n \ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n-\t\t\t\t\t\t\t\t\t\t\tistate->repo);\n+\t\t\t istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n--- 8< ---\n\n> restart:\n> \tfor (i = 0; i < istate->cache_nr; i++) {\n> \t\tstruct cache_entry *ce = istate->cache[i];\n>\n> -\t\tif (ce_skip_worktree(ce) &&\n> -\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> -\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> -\t\t\t\tensure_full_index(istate);\n> -\t\t\t\tgoto restart;\n> +\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\tpath_count[restarted]++;\n> +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n> +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n> +\t\t\t\t\tif (restarted)\n> +\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n> +\t\t\t\t\tensure_full_index(istate);\n> +\t\t\t\t\trestarted = 1;\n> +\t\t\t\t\tgoto restart;\n> +\t\t\t\t}\n> +\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n> \t\t\t}\n> -\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n\nLooks fine, though the \"if (restarted) BUG(...)\" is new. It's not wrong,\nper-se, but seeing it in a separate commit either before or after the\nmain change would be appreciated.\n\n> \t\t}\n> \t}\n> +\n> +\tif (path_count[0])\n> +\t\ttrace2_data_intmax(\"index\", istate->repo,\n> +\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count\", path_count[0]);\n> +\tif (restarted)\n> +\t\ttrace2_data_intmax(\"index\", istate->repo,\n> +\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n> +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n> +\t\t\t\t\t\t\t\t\t\t istate->repo);\n\nSame note about indentation here.\n\nThanks,\nTaylor\n"},{"id":"466458","messageId":"pull.1368.v4.git.git.1667516701.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":"pull.1368.v3.git.git.1667177791591.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] index: add trace2 region for clear skip worktree","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-03T23:04:59Z","receivedAt":"2022-11-03T23:05:18Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"In large repository using sparse checkout, checking whether a file marked\nwith skip worktree is present on disk and its skip worktree bit should be\ncleared can take a considerable amount of time. Add a trace2 region to\nsurface this information.\n\nAnh Le (2):\n index: add trace2 region for clear skip worktree\n index: raise a bug if the index is materialised more than once\n\n sparse-index.c | 30 ++++++++++++++++++++++++------\n 1 file changed, 24 insertions(+), 6 deletions(-)\n\n\nbase-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1368%2FHaizzz%2Fmaster-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1368/Haizzz/master-v4\nPull-Request: https://github.com/git/git/pull/1368\n\nRange-diff vs v3:\n\n 1: d0d9f258b08 ! 1: 33e9b2afd91 index: add trace2 region for clear skip worktree\n @@ Metadata\n ## Commit message ##\n index: add trace2 region for clear skip worktree\n \n - In a large repository using sparse checkout, checking whether a file marked\n - with skip worktree is present on disk and its skip worktree bit should be\n - cleared can take a considerable amount of time. Add a trace2 region to\n - surface this information, keeping a count of how many paths have been\n - checked and separately keep counts for after a full index is materialised.\n + When using sparse checkout, clear_skip_worktree_from_present_files() must\n + enumerate index entries to find ones with the SKIP_WORKTREE bit to\n + determine whether those index entries exist on disk (in which case their\n + SKIP_WORKTREE bit should be removed).\n +\n + In a large repository, this may take considerable time depending on the\n + size of the index.\n +\n + Add a trace2 region to surface this information, keeping a count of how\n + many paths have been checked. Separately, keep counts after a full index is\n + materialized.\n \n Signed-off-by: Anh Le \n \n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n \t\treturn;\n \n +\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n -+\t\t\t\t\t\t\t\t\t\t\tistate->repo);\n ++\t\t\t istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n +\t\t\tpath_count[restarted]++;\n +\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n +\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n -+\t\t\t\t\tif (restarted)\n -+\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n +\t\t\t\t\tensure_full_index(istate);\n +\t\t\t\t\trestarted = 1;\n +\t\t\t\t\tgoto restart;\n @@ sparse-index.c: void clear_skip_worktree_from_present_files(struct index_state *\n +\n +\tif (path_count[0])\n +\t\ttrace2_data_intmax(\"index\", istate->repo,\n -+\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count\", path_count[0]);\n ++\t\t\t\t \"sparse_path_count\", path_count[0]);\n +\tif (restarted)\n +\t\ttrace2_data_intmax(\"index\", istate->repo,\n -+\t\t\t\t\t\t\t\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n ++\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n +\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n -+\t\t\t\t\t\t\t\t\t\t istate->repo);\n ++\t\t\t istate->repo);\n }\n \n /*\n -: ----------- > 2: 91ad7973307 index: raise a bug if the index is materialised more than once\n\n-- \ngitgitgadget\n"},{"id":"466459","messageId":"91ad797330731e48907767d13213c8d8f899d996.1667516701.git.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":"pull.1368.v4.git.git.1667516701.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] index: raise a bug if the index is materialised more than once","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-03T23:05:01Z","receivedAt":"2022-11-03T23:05:24Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"From: Anh Le \n\nIf clear_skip_worktree_from_present_files() encounter a sparse directory,\nit fully materialise the index which should expand any sparse directories\nand start going through each entries again. If this happens more than once,\nraise it with a BUG.\n\nSigned-off-by: Anh Le \n---\n sparse-index.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 8713a15611d..8c269dab803 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -510,6 +510,8 @@ restart:\n \t\t\tpath_count[restarted]++;\n \t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n \t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tif (restarted)\n+\t\t\t\t\t\tBUG(\"ensure-full-index did not fully flatten?\");\n \t\t\t\t\tensure_full_index(istate);\n \t\t\t\t\trestarted = 1;\n \t\t\t\t\tgoto restart;\n-- \ngitgitgadget\n"},{"id":"466460","messageId":"33e9b2afd91f4376ef9a64fd267fa61f1a8de07f.1667516701.git.gitgitgadget@gmail.com","threadId":"58693","inReplyTo":"pull.1368.v4.git.git.1667516701.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] index: add trace2 region for clear skip worktree","fromName":"Anh Le via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-03T23:05:00Z","receivedAt":"2022-11-03T23:05:27Z","isPatch":true,"sender":{"key":"anh@canva.com","avatar":"https://avatars.githubusercontent.com/u/5275680?v=4"},"body":"From: Anh Le \n\nWhen using sparse checkout, clear_skip_worktree_from_present_files() must\nenumerate index entries to find ones with the SKIP_WORKTREE bit to\ndetermine whether those index entries exist on disk (in which case their\nSKIP_WORKTREE bit should be removed).\n\nIn a large repository, this may take considerable time depending on the\nsize of the index.\n\nAdd a trace2 region to surface this information, keeping a count of how\nmany paths have been checked. Separately, keep counts after a full index is\nmaterialized.\n\nSigned-off-by: Anh Le \n---\n sparse-index.c | 28 ++++++++++++++++++++++------\n 1 file changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e4a54ce1943..8713a15611d 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -493,24 +493,40 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)\n \tint dir_found = 1;\n \n \tint i;\n+\tint path_count[2] = {0, 0};\n+\tint restarted = 0;\n \n \tif (!core_apply_sparse_checkout ||\n \t sparse_expect_files_outside_of_patterns)\n \t\treturn;\n \n+\ttrace2_region_enter(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t istate->repo);\n restart:\n \tfor (i = 0; i < istate->cache_nr; i++) {\n \t\tstruct cache_entry *ce = istate->cache[i];\n \n-\t\tif (ce_skip_worktree(ce) &&\n-\t\t path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n-\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n-\t\t\t\tensure_full_index(istate);\n-\t\t\t\tgoto restart;\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpath_count[restarted]++;\n+\t\t\tif (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) {\n+\t\t\t\tif (S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\t\tensure_full_index(istate);\n+\t\t\t\t\trestarted = 1;\n+\t\t\t\t\tgoto restart;\n+\t\t\t\t}\n+\t\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t\t}\n-\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\t}\n \t}\n+\n+\tif (path_count[0])\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t \"sparse_path_count\", path_count[0]);\n+\tif (restarted)\n+\t\ttrace2_data_intmax(\"index\", istate->repo,\n+\t\t\t\t \"sparse_path_count_full\", path_count[1]);\n+\ttrace2_region_leave(\"index\", \"clear_skip_worktree_from_present_files\",\n+\t\t\t istate->repo);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"466569","messageId":"Y2WuZfur3iorHxdt@nand.local","threadId":"58693","inReplyTo":"pull.1368.v4.git.git.1667516701.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] index: add trace2 region for clear skip worktree","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-05T00:29:25Z","receivedAt":"2022-11-05T00:29:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 03, 2022 at 11:04:59PM +0000, Anh Le via GitGitGadget wrote:\n> In large repository using sparse checkout, checking whether a file marked\n> with skip worktree is present on disk and its skip worktree bit should be\n> cleared can take a considerable amount of time. Add a trace2 region to\n> surface this information.\n>\n> Anh Le (2):\n> index: add trace2 region for clear skip worktree\n> index: raise a bug if the index is materialised more than once\n>\n> sparse-index.c | 30 ++++++++++++++++++++++++------\n> 1 file changed, 24 insertions(+), 6 deletions(-)\n\nThis version is looking good. Thanks, will queue.\n\n\nThanks,\nTaylor\n"},{"id":"466738","messageId":"b0483b03-518a-ee97-c561-cc941521451e@github.com","threadId":"58693","inReplyTo":"Y2WuZfur3iorHxdt@nand.local","subject":"Re: [PATCH v4 0/2] index: add trace2 region for clear skip worktree","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T20:50:43Z","receivedAt":"2022-11-07T20:50:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/4/22 8:29 PM, Taylor Blau wrote:\n> On Thu, Nov 03, 2022 at 11:04:59PM +0000, Anh Le via GitGitGadget wrote:\n>> In large repository using sparse checkout, checking whether a file marked\n>> with skip worktree is present on disk and its skip worktree bit should be\n>> cleared can take a considerable amount of time. Add a trace2 region to\n>> surface this information.\n>>\n>> Anh Le (2):\n>> index: add trace2 region for clear skip worktree\n>> index: raise a bug if the index is materialised more than once\n>>\n>> sparse-index.c | 30 ++++++++++++++++++++++++------\n>> 1 file changed, 24 insertions(+), 6 deletions(-)\n> \n> This version is looking good. Thanks, will queue.\n\nI also took a full re-read of this version and agree\nthat it is ready to merge.\n\nThanks,\n-Stolee\n"}]}