{"thread":{"id":"52120","subject":"[PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor","startedAt":"2019-10-25T15:23:33Z","lastAt":"2019-11-21T04:15:56Z","messageCount":26,"participants":["Utsav Shah via GitGitGadget","Junio C Hamano","Utsav Shah","Kevin Willford"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"384870","messageId":"pull.424.git.1572017008.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":null,"subject":"[PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-25T15:23:27Z","receivedAt":"2019-10-25T15:23:33Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"git stash runs git reset --hard as its final step, which can be fairly slow\non a large repository. This change lets us skip that if fsmonitor thinks\nthose files aren't modified.\n\ngit stash goes from ~8s -> 2s on my repository after this change.\n\nUtsav Shah (1):\n unpack-trees: skip lstat based on fsmonitor\n\n unpack-trees.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/424\n-- \ngitgitgadget\n"},{"id":"384871","messageId":"609c7c5047719a619ba22425dafc6ecd105e2cda.1572017008.git.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.git.1572017008.gitgitgadget@gmail.com","subject":"[PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-25T15:23:28Z","receivedAt":"2019-10-25T15:23:33Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah \n\ngit stash runs git reset --hard as its final step, which can be fairly slow on a large repository.\nThis change lets us skip that if fsmonitor thinks those files aren't modified.\n\ngit stash goes from ~8s -> 2s on my repository after this change.\n\nSigned-off-by: Utsav Shah \n---\n unpack-trees.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 33ea7810d8..5d07a2d0ea 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2384,7 +2384,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n+\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n+\t\t !(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n-- \ngitgitgadget\n"},{"id":"384965","messageId":"xmqq8sp5a6cd.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"609c7c5047719a619ba22425dafc6ecd105e2cda.1572017008.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-28T03:37:06Z","receivedAt":"2019-10-28T03:37:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Utsav Shah via GitGitGadget\" writes:\n\n> From: Utsav Shah \n>\n> git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.\n> This change lets us skip that if fsmonitor thinks those files aren't modified.\n>\n> git stash goes from ~8s -> 2s on my repository after this change.\n\nPlease line-wrap overlong lines.\n\nMore importantly, \"stash\" may be a mere symptom that does not\ndeserve this much emphasis. What you improved directly is \"git\nreset --hard\" isn't it?\n\n The fsmonitor may know that a path hasn't been modified but \n \"git reset --hard\" did not pay attention to it and performed\n its own check based on ie_match_stat(), which was inefficient.\n\nor something like that?\n\n> \tif (old && same(old, a)) {\n> \t\tint update = 0;\n> -\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> +\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> +\t\t !(old->ce_flags & CE_FSMONITOR_VALID)) {\n\nI wonder if !ce_uptodate(old) should say \"this one is up to date and\nnot modified\" when CE_FSMONITOR_VALID bit is set. Are there other\ncodepaths that use ce_uptodate(ce) to decide to do X without paying\nattention to CE_FSMONITOR_VALID bit? If there are, are they buggy\nin the same way as you found this instance, or do they have legitimate\nreason why they only check ce_uptodate(ce) and ignore fsmonitor?\n\nIf there isn't, would it make sense to get rid of CE_FSMONITOR_VALID\nbit and have fsmonitor directly set CE_UPTODATE bit instead? That\nwould make this fix unnecessary and fix other codepaths that check\nonly ce_uptodate() without checking fsmonitor.\n\n"},{"id":"384969","messageId":"CAPYzU3N9mDfHVogfq=mhJFj6VOjS2z4ui4msnDdK6pOtVBa_QA@mail.gmail.com","threadId":"52120","inReplyTo":"xmqq8sp5a6cd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-10-28T06:39:42Z","receivedAt":"2019-10-28T06:39:57Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"Thanks for the review.\n\nOn Sun, Oct 27, 2019 at 8:37 PM Junio C Hamano wrote:\n>\n> \"Utsav Shah via GitGitGadget\" writes:\n>\n> > From: Utsav Shah \n> >\n> > git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.\n> > This change lets us skip that if fsmonitor thinks those files aren't modified.\n> >\n> > git stash goes from ~8s -> 2s on my repository after this change.\n>\n> Please line-wrap overlong lines.\n>\n> More importantly, \"stash\" may be a mere symptom that does not\n> deserve this much emphasis. What you improved directly is \"git\n> reset --hard\" isn't it?\n>\n> The fsmonitor may know that a path hasn't been modified but\n> \"git reset --hard\" did not pay attention to it and performed\n> its own check based on ie_match_stat(), which was inefficient.\n>\n> or something like that?\n>\n> > if (old && same(old, a)) {\n> > int update = 0;\n> > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> > + !(old->ce_flags & CE_FSMONITOR_VALID)) {\n>\n> I wonder if !ce_uptodate(old) should say \"this one is up to date and\n> not modified\" when CE_FSMONITOR_VALID bit is set. Are there other\n> codepaths that use ce_uptodate(ce) to decide to do X without paying\n> attention to CE_FSMONITOR_VALID bit? If there are, are they buggy\n> in the same way as you found this instance, or do they have legitimate\n> reason why they only check ce_uptodate(ce) and ignore fsmonitor?\n>\n\nYes, there are other code paths as well. After reading the code some\nmore, it seems like there's no legitimate need to ignore fsmonitor.\n\n> If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID\n> bit and have fsmonitor directly set CE_UPTODATE bit instead? That\n> would make this fix unnecessary and fix other codepaths that check\n> only ce_uptodate() without checking fsmonitor.\n>\n\nThere's a few issues with replacing it entirely that I've found.\n\nOne is the \"CE_MATCH_IGNORE_FSMONITOR\" flag. This flag can be set to\nlet ie_match_stat skip calling refresh_fsmonitor repeatedly. This is\nset only in one place right now in preload-index, and it's unclear how\nnecessary this optimization even is, given that refresh_fsmonitor has\na check whether it's been called already, and returns if true.\n\nThe second is that git ls-files has an \"f\" option that makes it \"use\nlowercase letters for 'fsmonitor clean' files\". I think this can\nsimply be replaced by checking if a file is up to date instead of\nspecifically via fsmonitor.\n\nIf we do go ahead with the replace, we will have to be diligent about\ncalling refresh_fsmonitor everywhere, or we will have correctness\nissues. I patched git locally to do this, and immediately saw a bug in\ngit stash where the underlying git reset --hard skipped modifying a\nfile it should have. In my opinion refresh_fsmonitor should be called\nsomewhere top level, like an initialization, but I'm not sure if that\nmakes sense for all git subcommands.\n\nDo you think it's worth cleaning up and sending this patch instead? It\nwill reduce the surface area of bugs and remove a bunch of functions\nlike mark_fsmonitor_valid/mark_fsmonitor_invalid\n"},{"id":"385012","messageId":"BN6PR21MB078676C4FA54391B1954D5F791660@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52120","inReplyTo":"CAPYzU3N9mDfHVogfq=mhJFj6VOjS2z4ui4msnDdK6pOtVBa_QA@mail.gmail.com","subject":"RE: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-10-28T19:23:04Z","receivedAt":"2019-10-28T19:23:08Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"On Monday, October 28, 2019 12:40 AM Utsav Shah \nwrote:\n\n> > I wonder if !ce_uptodate(old) should say \"this one is up to date and\n> > not modified\" when CE_FSMONITOR_VALID bit is set. Are there other\n> > codepaths that use ce_uptodate(ce) to decide to do X without paying\n> > attention to CE_FSMONITOR_VALID bit? If there are, are they buggy in\n> > the same way as you found this instance, or do they have legitimate\n> > reason why they only check ce_uptodate(ce) and ignore fsmonitor?\n> >\n> \n> Yes, there are other code paths as well. After reading the code some more, it\n> seems like there's no legitimate need to ignore fsmonitor.\n> \n> > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID\n> > bit and have fsmonitor directly set CE_UPTODATE bit instead? That\n> > would make this fix unnecessary and fix other codepaths that check\n> > only ce_uptodate() without checking fsmonitor.\n> >\n> \n\nI would need to go back and see if there was some reasoning why the\nnew flag was added but using CE_UPTODATE makes sense especially when\nmost calls to ce_mark_uptodate is followed directly by a call to\nmark_fsmonitor_valid. There is a little more going on in the\nmark_fsmonitor_X than just setting the bit though and the invalid\ncalls are not matched with code to clear the CE_UPTODATE flag.\n\nThe change to use CE_UPTODATE would have\nmore extensive effects and like you said we would need to make sure it\nwould not cause correctness issues in some corner case.\n\nDid you run all the git tests with GIT_TEST_FSMONITOR set to\nt/t7519/fsmonitor-all? This will run the tests with fsmonitor on. I was\ngetting multiple failures with this change and fsmonitor on.\n\nI added the refresh_fsmonitor call to the tweak_fsmonitor after\nusing the bitmap to set the dirty entries. This fixed most of the test \nfailures but there are still some failures that I haven't tracked down the\nreason for.\n\nI will do some more digging and testing to see what other pitfalls there\nmight be with this change.\n\nThanks,\nKevin\n"},{"id":"385094","messageId":"CAPYzU3NZZ-H_PWcRH_ooXzrYnRNjvF9ayX4_xeMJeMiHB=8fLg@mail.gmail.com","threadId":"52120","inReplyTo":"BN6PR21MB078676C4FA54391B1954D5F791660@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-10-29T19:06:33Z","receivedAt":"2019-10-29T19:06:55Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"On Mon, Oct 28, 2019 at 12:23 PM Kevin Willford\n wrote:\n>\n> On Monday, October 28, 2019 12:40 AM Utsav Shah \n> wrote:\n>\n> > > I wonder if !ce_uptodate(old) should say \"this one is up to date and\n> > > not modified\" when CE_FSMONITOR_VALID bit is set. Are there other\n> > > codepaths that use ce_uptodate(ce) to decide to do X without paying\n> > > attention to CE_FSMONITOR_VALID bit? If there are, are they buggy in\n> > > the same way as you found this instance, or do they have legitimate\n> > > reason why they only check ce_uptodate(ce) and ignore fsmonitor?\n> > >\n> >\n> > Yes, there are other code paths as well. After reading the code some more, it\n> > seems like there's no legitimate need to ignore fsmonitor.\n> >\n> > > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID\n> > > bit and have fsmonitor directly set CE_UPTODATE bit instead? That\n> > > would make this fix unnecessary and fix other codepaths that check\n> > > only ce_uptodate() without checking fsmonitor.\n> > >\n> >\n>\n> I would need to go back and see if there was some reasoning why the\n> new flag was added but using CE_UPTODATE makes sense especially when\n> most calls to ce_mark_uptodate is followed directly by a call to\n> mark_fsmonitor_valid.\n\nI've been playing around with the patch and trying to get the tests pass.\n\nI've found that we set CE_UPTODATE when we try to skip worktrees to\nstat in the case of sparse checkouts, and there are cases where we\nmark cache entries up to date without consulting fsmonitor or stating\nthem.\n\nIt seems like making fsmonitor only modify CE_UPTODATE makes it hard\nto verify and test correct fsmonitor behavior and debugging fsmonitor\nwith git ls-files -f. I think the patch also makes things overall\nslightly more complicated.\n\nThere is a little more going on in the\n> mark_fsmonitor_X than just setting the bit though and the invalid\n> calls are not matched with code to clear the CE_UPTODATE flag.\n\nYeah. The patch to replace CE_FSMONITOR_VALID doesn't remove the need\nfor calling mark_fsmonitor_valid/mark_fsmonitor_invalid, since there's\nspecial behavior like modifying the untracked cache which doesn't make\nsense in a more general mark_ce_not_uptodate function.\n\n>\n> The change to use CE_UPTODATE would have\n> more extensive effects and like you said we would need to make sure it\n> would not cause correctness issues in some corner case.\n>\n> Did you run all the git tests with GIT_TEST_FSMONITOR set to\n> t/t7519/fsmonitor-all? This will run the tests with fsmonitor on. I was\n> getting multiple failures with this change and fsmonitor on.\n>\n> I added the refresh_fsmonitor call to the tweak_fsmonitor after\n> using the bitmap to set the dirty entries. This fixed most of the test\n> failures but there are still some failures that I haven't tracked down the\n> reason for.\n\nI'm getting the same test failures with or without\nGIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor\nin tweak_fsmonitor. Could you share your patch? I'm probably messing\nsomething up, and I can try taking a look at fixing test cases as\nwell.\n\n>\n> I will do some more digging and testing to see what other pitfalls there\n> might be with this change.\n>\n> Thanks,\n> Kevin\n"},{"id":"385103","messageId":"BN6PR21MB078689FFE9ED95A31998F41A91610@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52120","inReplyTo":"CAPYzU3NZZ-H_PWcRH_ooXzrYnRNjvF9ayX4_xeMJeMiHB=8fLg@mail.gmail.com","subject":"RE: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-10-29T20:12:00Z","receivedAt":"2019-10-29T20:12:05Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"> On Tuesday, October 29, 2019 1:07 PM Utsav Shah \n> wrote:\n> \n> I'm getting the same test failures with or without\n> GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor\n> in tweak_fsmonitor. Could you share your patch? I'm probably messing\n> something up, and I can try taking a look at fixing test cases as well.\n\nI have the tests passing with the following commit.\n\nhttps://github.com/kewillford/git/commit/3b1fdf5a4b1cd1d654b1733ce058faa4f087f75f\n\nThings to note:\n1. Not sure if fsmonitor was tested with split index so for now I removed that from the\ncheck of entries in fsmonitor bitmap vs the number of cache entries\n2. With these changes update-index was triggering the post-index-change hook with the\nupdated_skipworktree flag set which it wasn't before.\n3. Copied the fsmonitor_last_update to the result index so the fsmonitor data will be \ncarried over to the new index in unpack_trees. This is to make sure that the next call\nto git will have the fsmonitor data to use. We found that running `git status` after any\ncommand that ran unpack_trees (checkout, reset --hard, etc.) was very slow the first\ncall but and subsequent calls were fast.\n\nI'm still testing and reviewing these changes to make sure there isn't something I\nhave missed and that I made the right changes to the tests that were failing.\n\nKevin\n"},{"id":"385119","messageId":"CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com","threadId":"52120","inReplyTo":"BN6PR21MB078689FFE9ED95A31998F41A91610@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-10-29T23:50:54Z","receivedAt":"2019-10-29T23:51:09Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"Thanks for testing it out. The unpack_trees bugfix is especially useful.\n\nThere's tons of places where we're using ce_uptodate(ce) that could be\noptimized by checking CE_FSMONITOR_VALID. One example is in\nrun_diff_files in diff-lib.c\n\nShould we add a check for CE_FSMONITOR_VALID in all of them? Should we\ndo that in this patch? Or should we take the time to refactor and\nflesh out bugs in unifying it with CE_UPTODATE? It would be nice to\nget more opinions. I've taken a look and believe that it will make\nthings a little more complicated to merge it with CE_UPTODATE,\nespecially since it's used in a few places for other reasons like\nsparse checkouts. On the other hand, I'm a first time contributor, so\nmy perspective towards a large refactor like might be too\nconservative.\n\n\nOn Tue, Oct 29, 2019 at 1:12 PM Kevin Willford\n wrote:\n>\n> > On Tuesday, October 29, 2019 1:07 PM Utsav Shah \n> > wrote:\n> >\n> > I'm getting the same test failures with or without\n> > GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor\n> > in tweak_fsmonitor. Could you share your patch? I'm probably messing\n> > something up, and I can try taking a look at fixing test cases as well.\n>\n> I have the tests passing with the following commit.\n>\n> https://github.com/kewillford/git/commit/3b1fdf5a4b1cd1d654b1733ce058faa4f087f75f\n>\n> Things to note:\n> 1. Not sure if fsmonitor was tested with split index so for now I removed that from the\n> check of entries in fsmonitor bitmap vs the number of cache entries\n> 2. With these changes update-index was triggering the post-index-change hook with the\n> updated_skipworktree flag set which it wasn't before.\n> 3. Copied the fsmonitor_last_update to the result index so the fsmonitor data will be\n> carried over to the new index in unpack_trees. This is to make sure that the next call\n> to git will have the fsmonitor data to use. We found that running `git status` after any\n> command that ran unpack_trees (checkout, reset --hard, etc.) was very slow the first\n> call but and subsequent calls were fast.\n>\n> I'm still testing and reviewing these changes to make sure there isn't something I\n> have missed and that I made the right changes to the tests that were failing.\n>\n> Kevin\n"},{"id":"385122","messageId":"xmqqh83r5bi0.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-30T00:21:27Z","receivedAt":"2019-10-30T00:21:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Utsav Shah writes:\n\n> Thanks for testing it out. The unpack_trees bugfix is especially useful.\n>\n> There's tons of places where we're using ce_uptodate(ce) that could be\n> optimized by checking CE_FSMONITOR_VALID. One example is in\n> run_diff_files in diff-lib.c\n>\n> Should we add a check for CE_FSMONITOR_VALID in all of them? Should we\n> do that in this patch? Or should we take the time to refactor and\n> flesh out bugs in unifying it with CE_UPTODATE?\n\nIf we rephrase the first question slightly, i.e. \"should these\nplaces all be avoiding lstat() based check when fsmonitor says the\npath is up to date?\", I would imagine the answer is absolutely yes.\n\nI would further imagine that the implementation of the interface to\nexternal fsmonitor itself may have to distinguish \"we know/have known\nthis path is clean\" vs \"we just got told by fsmonitor that this path\nis clean\", so losing FSMONITOR_VALID bit might not be an easy or\nclean conversion, in which case my earlier \"can we perhaps lose it\nand have fsmonitor interfacing code to directly set UPTODATE bit?\"\nwould lead us in a wrong direction.\n\nBut ce_uptodate(ce) being the primary way for the callers that care\nabout \"is the path known to be up to date?\", it is unsatisfying that\nall of them have to ask\n\n\tif (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID))\n\t\t... process ce that is not up-to-date ...\n\nSo I would say that the longer term goal should be to let them ask\nce_uptodate(ce) and have that macro automatically take FSMONITOR bit\ninto account (in other words, those who want to know if ce is fresh\nshould not have to even know about what fsmonitor is).\n\nPerhaps we can take a polished version of this \"'reset --hard' can\nand should notice paths known-to-be-uptodate via fsmonitor\" as an\nindependent patch (to reduce the number of things we have to worry\nby one) for now? Taking this patch means we would now have one more\nplace that checks both ce_uptodate() and FSMONITOR_VALID bit, but if\nwe would be auditing all such places before we can decide what the\nbest way to reach the goal of allowing them to just say ce_uptodate()\nwithout having to spell FSMONITOR_VALID, that probably is a cost\nworth paying.\n\nThanks for working on this topic.\n\n\n\n"},{"id":"385162","messageId":"CAPYzU3MuacmCJoC0pioL4ymyRA7tku8BbNmSTrtZtJWMHpvJNg@mail.gmail.com","threadId":"52120","inReplyTo":"xmqqh83r5bi0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-10-30T16:41:37Z","receivedAt":"2019-10-30T16:41:53Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"Thanks, that makes a lot of sense. ce_uptodate doesn't have too many\ncallers either, so modifying it and checking CE_FSMONITOR_VALID there\nshould not be hard to audit.\n\n\n\nOn Tue, Oct 29, 2019 at 5:21 PM Junio C Hamano wrote:\n>\n> Utsav Shah writes:\n>\n> > Thanks for testing it out. The unpack_trees bugfix is especially useful.\n> >\n> > There's tons of places where we're using ce_uptodate(ce) that could be\n> > optimized by checking CE_FSMONITOR_VALID. One example is in\n> > run_diff_files in diff-lib.c\n> >\n> > Should we add a check for CE_FSMONITOR_VALID in all of them? Should we\n> > do that in this patch? Or should we take the time to refactor and\n> > flesh out bugs in unifying it with CE_UPTODATE?\n>\n> If we rephrase the first question slightly, i.e. \"should these\n> places all be avoiding lstat() based check when fsmonitor says the\n> path is up to date?\", I would imagine the answer is absolutely yes.\n>\n> I would further imagine that the implementation of the interface to\n> external fsmonitor itself may have to distinguish \"we know/have known\n> this path is clean\" vs \"we just got told by fsmonitor that this path\n> is clean\", so losing FSMONITOR_VALID bit might not be an easy or\n> clean conversion, in which case my earlier \"can we perhaps lose it\n> and have fsmonitor interfacing code to directly set UPTODATE bit?\"\n> would lead us in a wrong direction.\n>\n> But ce_uptodate(ce) being the primary way for the callers that care\n> about \"is the path known to be up to date?\", it is unsatisfying that\n> all of them have to ask\n>\n> if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID))\n> ... process ce that is not up-to-date ...\n>\n> So I would say that the longer term goal should be to let them ask\n> ce_uptodate(ce) and have that macro automatically take FSMONITOR bit\n> into account (in other words, those who want to know if ce is fresh\n> should not have to even know about what fsmonitor is).\n>\n> Perhaps we can take a polished version of this \"'reset --hard' can\n> and should notice paths known-to-be-uptodate via fsmonitor\" as an\n> independent patch (to reduce the number of things we have to worry\n> by one) for now? Taking this patch means we would now have one more\n> place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if\n> we would be auditing all such places before we can decide what the\n> best way to reach the goal of allowing them to just say ce_uptodate()\n> without having to spell FSMONITOR_VALID, that probably is a cost\n> worth paying.\n>\n> Thanks for working on this topic.\n>\n>\n>\n"},{"id":"385366","messageId":"xmqqsgn4xjpr.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"CAPYzU3MuacmCJoC0pioL4ymyRA7tku8BbNmSTrtZtJWMHpvJNg@mail.gmail.com","subject":"Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-04T06:02:24Z","receivedAt":"2019-11-04T06:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Utsav Shah writes:\n\n[jc: we avoid top-posting for readability, so swapped paragraphs in\nthis quote]\n\n>> Perhaps we can take a polished version of this \"'reset --hard' can\n>> and should notice paths known-to-be-uptodate via fsmonitor\" as an\n>> independent patch (to reduce the number of things we have to worry\n>> by one) for now? Taking this patch means we would now have one more\n>> place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if\n>> we would be auditing all such places before we can decide what the\n>> best way to reach the goal of allowing them to just say ce_uptodate()\n>> without having to spell FSMONITOR_VALID, that probably is a cost\n>> worth paying.\n>>\n>> Thanks for working on this topic.\n>\n> Thanks, that makes a lot of sense. ce_uptodate doesn't have too many\n> callers either, so modifying it and checking CE_FSMONITOR_VALID there\n> should not be hard to audit.\n\nOK, so let's see an updated and hopefully final version of [*1*],\nperhaps with Kevin's help you mentioned in [*2*] for now?\n\n\n[References]\n\n*1* \n*2* \n"},{"id":"385452","messageId":"pull.424.v2.git.1572967644.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.git.1572017008.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-05T15:27:22Z","receivedAt":"2019-11-05T15:27:29Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"The index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat which\ncan be inefficient on certain filesystems. This significantly slows down\ncommands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered unchanged\nthrough fsmonitor and skips ie_match_stat on it. unpack-trees also now\ncorrectly copies over fsmonitor validity state from the source index.\nFinally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard internally) go\nfrom 8s or more to ~2s on a 250k file repository on a mac.\n\nSigned-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]\n\nUtsav Shah (1):\n unpack-trees: skip stat on fsmonitor-valid files\n\n fsmonitor.c | 20 +++++++++++---------\n t/t7113-post-index-change-hook.sh | 3 ---\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 4 files changed, 23 insertions(+), 15 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/424\n\nRange-diff vs v1:\n\n 1: 609c7c5047 ! 1: f76ba554ed unpack-trees: skip lstat based on fsmonitor\n @@ -1,24 +1,147 @@\n Author: Utsav Shah \n \n - unpack-trees: skip lstat based on fsmonitor\n + unpack-trees: skip stat on fsmonitor-valid files\n \n - git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.\n - This change lets us skip that if fsmonitor thinks those files aren't modified.\n + The index might be aware that a file hasn't modified via fsmonitor, but\n + unpack-trees did not pay attention to it and checked via ie_match_stat\n + which can be inefficient on certain filesystems. This significantly slows\n + down commands that run oneway_merge, like checkout and reset --hard.\n \n - git stash goes from ~8s -> 2s on my repository after this change.\n + This patch makes oneway_merge check whether a file is considered\n + unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n + also now correctly copies over fsmonitor validity state from the source\n + index. Finally, for correctness, we force a refresh of fsmonitor state in\n + tweak_fsmonitor.\n +\n + After this change, commands like stash (that use reset --hard\n + internally) go from 8s or more to ~2s on a 250k file repository on a\n + mac.\n \n Signed-off-by: Utsav Shah \n \n + diff --git a/fsmonitor.c b/fsmonitor.c\n + --- a/fsmonitor.c\n + +++ b/fsmonitor.c\n +@@\n + \t}\n + \tistate->fsmonitor_dirty = fsmonitor_dirty;\n + \n +-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n +-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n +-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n ++\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n ++\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n ++\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n ++\n + \n + \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n + \treturn 0;\n +@@\n + \tuint32_t ewah_size = 0;\n + \tint fixup = 0;\n + \n +-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n +-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n +-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n ++\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n ++\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n ++\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n + \n + \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n + \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n +@@\n + \t\t}\n + \t\tif (bol < query_result.len)\n + \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n ++\n ++\t\tif (istate->untracked)\n ++\t\t\tistate->untracked->use_fsmonitor = 1;\n + \t} else {\n + \t\t/* Mark all entries invalid */\n + \t\tfor (i = 0; i < istate->cache_nr; i++)\n +@@\n + \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n + \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n + \n +-\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n +-\t\t\tif (istate->untracked)\n +-\t\t\t\tistate->untracked->use_fsmonitor = 1;\n ++\t\t\trefresh_fsmonitor(istate);\n + \t\t}\n + \n + \t\tewah_free(istate->fsmonitor_dirty);\n +\n + diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh\n + --- a/t/t7113-post-index-change-hook.sh\n + +++ b/t/t7113-post-index-change-hook.sh\n +@@\n + \tgit checkout -- dir1/file1.txt &&\n + \ttest_path_is_file testsuccess && rm -f testsuccess &&\n + \ttest_path_is_missing testfailure &&\n +-\tgit update-index &&\n +-\ttest_path_is_missing testsuccess &&\n +-\ttest_path_is_missing testfailure &&\n + \tgit reset --soft &&\n + \ttest_path_is_missing testsuccess &&\n + \ttest_path_is_missing testfailure\n +\n + diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n + --- a/t/t7519-status-fsmonitor.sh\n + +++ b/t/t7519-status-fsmonitor.sh\n +@@\n + \n + # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n + test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n ++\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n ++\tEOF\n + \tgit update-index --fsmonitor &&\n + \tgit update-index --fsmonitor-valid dir1/modified &&\n + \tgit update-index --fsmonitor-valid dir2/modified &&\n +@@\n + \n + # test that newly added files are marked valid\n + test_expect_success 'newly added files are marked valid' '\n ++\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n ++\tEOF\n + \tgit add new &&\n + \tgit add dir1/new &&\n + \tgit add dir2/new &&\n +@@\n + # Ensure commands that call refresh_index() to move the index back in time\n + # properly invalidate the fsmonitor cache\n + test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n +-\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n +-\tEOF\n + \tclean_repo &&\n ++\twrite_integration_script &&\n + \tdirty_repo &&\n + \tgit add . &&\n ++\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n ++\tEOF\n + \tgit commit -m \"to reset\" &&\n + \tgit reset HEAD~1 &&\n + \tgit status >actual &&\n +\n diff --git a/unpack-trees.c b/unpack-trees.c\n --- a/unpack-trees.c\n +++ b/unpack-trees.c\n @@\n + \to->merge_size = len;\n + \tmark_all_ce_unused(o->src_index);\n + \n ++\tif (o->src_index->fsmonitor_last_update)\n ++\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n ++\n + \t/*\n + \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n + \t */\n +@@\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n -\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n +\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n -+\t\t !(old->ce_flags & CE_FSMONITOR_VALID)) {\n ++\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n\n-- \ngitgitgadget\n"},{"id":"385453","messageId":"f76ba554ed25fb9877a223ef6481834f1831c8ca.1572967644.git.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.v2.git.1572967644.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-05T15:27:23Z","receivedAt":"2019-11-05T15:27:58Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah \n\nThe index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat\nwhich can be inefficient on certain filesystems. This significantly slows\ndown commands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered\nunchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\nalso now correctly copies over fsmonitor validity state from the source\nindex. Finally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard\ninternally) go from 8s or more to ~2s on a 250k file repository on a\nmac.\n\nSigned-off-by: Utsav Shah \n---\n fsmonitor.c | 20 +++++++++++---------\n t/t7113-post-index-change-hook.sh | 3 ---\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 4 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..4362bc6ee9 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n+\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -189,6 +190,9 @@ void refresh_fsmonitor(struct index_state *istate)\n \t\t}\n \t\tif (bol < query_result.len)\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n+\n+\t\tif (istate->untracked)\n+\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n \t\t/* Mark all entries invalid */\n \t\tfor (i = 0; i < istate->cache_nr; i++)\n@@ -257,9 +261,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n-\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n-\t\t\tif (istate->untracked)\n-\t\t\t\tistate->untracked->use_fsmonitor = 1;\n+\t\t\trefresh_fsmonitor(istate);\n \t\t}\n \n \t\tewah_free(istate->fsmonitor_dirty);\ndiff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh\nindex f011ad7eec..5ca2279d0d 100755\n--- a/t/t7113-post-index-change-hook.sh\n+++ b/t/t7113-post-index-change-hook.sh\n@@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags\n \tgit checkout -- dir1/file1.txt &&\n \ttest_path_is_file testsuccess && rm -f testsuccess &&\n \ttest_path_is_missing testfailure &&\n-\tgit update-index &&\n-\ttest_path_is_missing testsuccess &&\n-\ttest_path_is_missing testfailure &&\n \tgit reset --soft &&\n \ttest_path_is_missing testsuccess &&\n \ttest_path_is_missing testfailure\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex d8df990972..9cac3d3d8e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -106,6 +106,8 @@ EOF\n \n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n \tgit update-index --fsmonitor-valid dir2/modified &&\n@@ -164,6 +166,8 @@ EOF\n \n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n \tgit add dir2/new &&\n@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged\n # Ensure commands that call refresh_index() to move the index back in time\n # properly invalidate the fsmonitor cache\n test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n-\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n-\tEOF\n \tclean_repo &&\n+\twrite_integration_script &&\n \tdirty_repo &&\n \tgit add . &&\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit commit -m \"to reset\" &&\n \tgit reset HEAD~1 &&\n \tgit status >actual &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 33ea7810d8..fc5ceb932c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \tmark_all_ce_unused(o->src_index);\n \n+\tif (o->src_index->fsmonitor_last_update)\n+\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n+\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n+\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n-- \ngitgitgadget\n"},{"id":"385488","messageId":"BN6PR21MB07869D880E1D0111F1A80E42917E0@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52120","inReplyTo":"f76ba554ed25fb9877a223ef6481834f1831c8ca.1572967644.git.gitgitgadget@gmail.com","subject":"RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-11-05T21:40:29Z","receivedAt":"2019-11-05T21:40:34Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"> Sent: Tuesday, November 5, 2019 8:27 AM\n> From: Utsav Shah \n> \n> diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-\n> hook.sh\n> index f011ad7eec..5ca2279d0d 100755\n> --- a/t/t7113-post-index-change-hook.sh\n> +++ b/t/t7113-post-index-change-hook.sh\n> @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others\n> trigger hook without flags\n> \tgit checkout -- dir1/file1.txt &&\n> \ttest_path_is_file testsuccess && rm -f testsuccess &&\n> \ttest_path_is_missing testfailure &&\n> -\tgit update-index &&\n> -\ttest_path_is_missing testsuccess &&\n> -\ttest_path_is_missing testfailure &&\n> \tgit reset --soft &&\n> \ttest_path_is_missing testsuccess &&\n> \ttest_path_is_missing testfailure\n\nLooking into this change and I wonder if instead we should be updating\nrefresh_fsmonitor to only update istate->cache_changed if there was an\nentry where CE_FSMONITOR_VALID was turned off.\n\nThe reason I bring this up is because with this change, the post-index-change\nhook will behave differently depending on fsmonitor. It will pass if\nGIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set\nto fsmonitor-all it will fail because it is going down the code path that\ninvalidates all the entries and sets istate->cache_changed.\n\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index\n> d8df990972..9cac3d3d8e 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -106,6 +106,8 @@ EOF\n> \n> # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n> test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor\n> valid bit' '\n> +\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\tEOF\n> \tgit update-index --fsmonitor &&\n> \tgit update-index --fsmonitor-valid dir1/modified &&\n> \tgit update-index --fsmonitor-valid dir2/modified && @@ -164,6\n> +166,8 @@ EOF\n> \n> # test that newly added files are marked valid test_expect_success 'newly\n> added files are marked valid' '\n> +\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\tEOF\n> \tgit add new &&\n> \tgit add dir1/new &&\n> \tgit add dir2/new &&\n> @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the\n> integration script get flagged # Ensure commands that call refresh_index() to\n> move the index back in time # properly invalidate the fsmonitor cache\n> test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n> -\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> -\tEOF\n> \tclean_repo &&\n> +\twrite_integration_script &&\n> \tdirty_repo &&\n> \tgit add . &&\n> +\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\tEOF\n> \tgit commit -m \"to reset\" &&\n> \tgit reset HEAD~1 &&\n> \tgit status >actual &&\n\nWe need to take a close look at all the tests in\nt7519-status-fsmonitor.sh and see if we are doing the right thing with\nthese changes because before most commands that read the\nindex would only apply the bits from the fsmonitor bitmap to\nthe cache entries. Whereas now, it does that but also applies what the\nfsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is\nnow affecting tests and commands where it was not before.\n\nSo if .git/hooks/fsmonitor-test has paths even git ls-files gets those\npaths marked dirty and that command is being used to validate the state of\nthe CE_FSMONITOR_VALID. So I think in most cases for these tests we\nwant the .git/hooks/fsmonitor-test to be empty before calling git ls-files\nso that doesn't change the index state.\n\n> \n> \tif (old && same(old, a)) {\n> \t\tint update = 0;\n> -\t\tif (o->reset && o->update && !ce_uptodate(old) &&\n> !ce_skip_worktree(old)) {\n> +\t\tif (o->reset && o->update && !ce_uptodate(old) &&\n> !ce_skip_worktree(old) &&\n> +\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n> \t\t\tstruct stat st;\n> \t\t\tif (lstat(old->name, &st) ||\n> \t\t\t ie_match_stat(o->src_index, old, &st,\n> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n\nFYI I have been testing with the ce_uptodate macro checking the\nCE_FSMONITOR_VALID flag instead and only have failures when using\nthe fsmonitor-watchman script which I'm not sure if all the tests were\never passing using it.\n\n"},{"id":"385531","messageId":"CAPYzU3PLVhGWcautao1oenbKbgcOB_99PoyVEcK1hnUk5XErKg@mail.gmail.com","threadId":"52120","inReplyTo":"BN6PR21MB07869D880E1D0111F1A80E42917E0@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-11-06T04:36:21Z","receivedAt":"2019-11-06T04:36:37Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"On Tue, Nov 5, 2019 at 1:40 PM Kevin Willford\n wrote:\n>\n> > Sent: Tuesday, November 5, 2019 8:27 AM\n> > From: Utsav Shah \n> >\n> > diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-\n> > hook.sh\n> > index f011ad7eec..5ca2279d0d 100755\n> > --- a/t/t7113-post-index-change-hook.sh\n> > +++ b/t/t7113-post-index-change-hook.sh\n> > @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others\n> > trigger hook without flags\n> > git checkout -- dir1/file1.txt &&\n> > test_path_is_file testsuccess && rm -f testsuccess &&\n> > test_path_is_missing testfailure &&\n> > - git update-index &&\n> > - test_path_is_missing testsuccess &&\n> > - test_path_is_missing testfailure &&\n> > git reset --soft &&\n> > test_path_is_missing testsuccess &&\n> > test_path_is_missing testfailure\n>\n> Looking into this change and I wonder if instead we should be updating\n> refresh_fsmonitor to only update istate->cache_changed if there was an\n> entry where CE_FSMONITOR_VALID was turned off.\n>\n> The reason I bring this up is because with this change, the post-index-change\n> hook will behave differently depending on fsmonitor. It will pass if\n> GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set\n> to fsmonitor-all it will fail because it is going down the code path that\n> invalidates all the entries and sets istate->cache_changed.\n\nThanks, this observation was correct. v3 of this patch will check if\nthe index actually needs to mark its cache as changed, and this test\npasses without modification.\n\n>\n> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index\n> > d8df990972..9cac3d3d8e 100755\n> > --- a/t/t7519-status-fsmonitor.sh\n> > +++ b/t/t7519-status-fsmonitor.sh\n> > @@ -106,6 +106,8 @@ EOF\n> >\n> > # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n> > test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor\n> > valid bit' '\n> > + write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> > + EOF\n> > git update-index --fsmonitor &&\n> > git update-index --fsmonitor-valid dir1/modified &&\n> > git update-index --fsmonitor-valid dir2/modified && @@ -164,6\n> > +166,8 @@ EOF\n> >\n> > # test that newly added files are marked valid test_expect_success 'newly\n> > added files are marked valid' '\n> > + write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> > + EOF\n> > git add new &&\n> > git add dir1/new &&\n> > git add dir2/new &&\n> > @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the\n> > integration script get flagged # Ensure commands that call refresh_index() to\n> > move the index back in time # properly invalidate the fsmonitor cache\n> > test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n> > - write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> > - EOF\n> > clean_repo &&\n> > + write_integration_script &&\n> > dirty_repo &&\n> > git add . &&\n> > + write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> > + EOF\n> > git commit -m \"to reset\" &&\n> > git reset HEAD~1 &&\n> > git status >actual &&\n>\n> We need to take a close look at all the tests in\n> t7519-status-fsmonitor.sh and see if we are doing the right thing with\n> these changes because before most commands that read the\n> index would only apply the bits from the fsmonitor bitmap to\n> the cache entries. Whereas now, it does that but also applies what the\n> fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is\n> now affecting tests and commands where it was not before.\n>\n> So if .git/hooks/fsmonitor-test has paths even git ls-files gets those\n> paths marked dirty and that command is being used to validate the state of\n> the CE_FSMONITOR_VALID. So I think in most cases for these tests we\n> want the .git/hooks/fsmonitor-test to be empty before calling git ls-files\n> so that doesn't change the index state.\n\nI audited these tests very closely, and to the best of my knowledge,\nthe modifications made are valid.\n\nFor test failures of\n\ntest_expect_success 'update-index --fsmonitor-valid sets the fsmonitor\nvalid bit'\ntest_expect_success 'newly added files are marked valid'\n\nIt's relatively straightforward that our patch now runs the fsmonitor\nhook so we need to make sure the hook doesn't return anything.\n\nThe trickiest case was \"refresh_index()\" test, and I've made a slight\nchange to make it clearer why that test was failing.\n\n@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by\nthe integration script get flagged\n # Ensure commands that call refresh_index() to move the index back in time\n # properly invalidate the fsmonitor cache\n test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n- write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n- EOF\n clean_repo &&\n dirty_repo &&\n+ write_integration_script &&\n git add . &&\n+ write_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+ EOF\n git commit -m \"to reset\" &&\n git reset HEAD~1 &&\n git status >actual &&\n\nWith patch v2, git add was failing to add all files, since it now\nconsults the fsmonitor hook which wrongly implied that no files were\nmodified. This was rectified by the write_integration_script. After\nthat, we immediately ensure that the test fsmonitor doesn't return any\nfiles, and the test passes.\n\n\n>\n> >\n> > if (old && same(old, a)) {\n> > int update = 0;\n> > - if (o->reset && o->update && !ce_uptodate(old) &&\n> > !ce_skip_worktree(old)) {\n> > + if (o->reset && o->update && !ce_uptodate(old) &&\n> > !ce_skip_worktree(old) &&\n> > + !(old->ce_flags & CE_FSMONITOR_VALID)) {\n> > struct stat st;\n> > if (lstat(old->name, &st) ||\n> > ie_match_stat(o->src_index, old, &st,\n> > CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n>\n> FYI I have been testing with the ce_uptodate macro checking the\n> CE_FSMONITOR_VALID flag instead and only have failures when using\n> the fsmonitor-watchman script which I'm not sure if all the tests were\n> ever passing using it.\n>\n\nYeah, I see the same results.\n\nThe one part that I don't fully understand if safe is copying over the\no->src_index->fsmonitor_last_update to the result index in\nunpack-trees. I don't understand the implications of that, and if\nthat's the only field worth copying over, or if we should be copying\nover other fields like the bitmap as well.\n"},{"id":"385536","messageId":"pull.424.v3.git.1573016055.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.v2.git.1572967644.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T04:54:14Z","receivedAt":"2019-11-06T04:54:20Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"The index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat which\ncan be inefficient on certain filesystems. This significantly slows down\ncommands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered unchanged\nthrough fsmonitor and skips ie_match_stat on it. unpack-trees also now\ncorrectly copies over fsmonitor validity state from the source index.\nFinally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard internally) go\nfrom 8s or more to ~2s on a 250k file repository on a mac.\n\nSigned-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]\n\nUtsav Shah (1):\n unpack-trees: skip stat on fsmonitor-valid files\n\n fsmonitor.c | 39 ++++++++++++++++++++++++-------------\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 3 files changed, 37 insertions(+), 17 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/424\n\nRange-diff vs v2:\n\n 1: f76ba554ed ! 1: 4bea7075cf unpack-trees: skip stat on fsmonitor-valid files\n @@ -23,6 +23,15 @@\n --- a/fsmonitor.c\n +++ b/fsmonitor.c\n @@\n + \n + \tif (pos >= istate->cache_nr)\n + \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n +-\t\t (uintmax_t)pos, istate->cache_nr);\n ++\t\t\t(uintmax_t)pos, istate->cache_nr);\n + \n + \tce = istate->cache[pos];\n + \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n +@@\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n @@ -31,7 +40,7 @@\n -\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n +\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n -+\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n ++\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n +\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n @@ -45,7 +54,7 @@\n -\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n +\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n -+\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n ++\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n @@ -57,10 +66,33 @@\n +\t\tif (istate->untracked)\n +\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n ++\n ++\t\t/* We only want to run the post index changed hook if we've actually changed entries, so keep track\n ++\t\t * if we actually changed entries or not */\n ++\t\tint is_cache_changed = 0;\n \t\t/* Mark all entries invalid */\n - \t\tfor (i = 0; i < istate->cache_nr; i++)\n +-\t\tfor (i = 0; i < istate->cache_nr; i++)\n +-\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n ++\t\tfor (i = 0; i < istate->cache_nr; i++) {\n ++\t\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n ++\t\t\t\tis_cache_changed = 1;\n ++\t\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n ++\t\t\t}\n ++\t\t}\n + \n + \t\t/* If we're going to check every file, ensure we save the results */\n +-\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n ++\t\tif (is_cache_changed)\n ++\t\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n + \n + \t\tif (istate->untracked)\n + \t\t\tistate->untracked->use_fsmonitor = 0;\n @@\n - \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n + \t\t\t/* Mark all previously saved entries as dirty */\n + \t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n + \t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n +-\t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n ++\t\t\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n -\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n @@ -71,20 +103,6 @@\n \n \t\tewah_free(istate->fsmonitor_dirty);\n \n - diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh\n - --- a/t/t7113-post-index-change-hook.sh\n - +++ b/t/t7113-post-index-change-hook.sh\n -@@\n - \tgit checkout -- dir1/file1.txt &&\n - \ttest_path_is_file testsuccess && rm -f testsuccess &&\n - \ttest_path_is_missing testfailure &&\n --\tgit update-index &&\n --\ttest_path_is_missing testsuccess &&\n --\ttest_path_is_missing testfailure &&\n - \tgit reset --soft &&\n - \ttest_path_is_missing testsuccess &&\n - \ttest_path_is_missing testfailure\n -\n diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n --- a/t/t7519-status-fsmonitor.sh\n +++ b/t/t7519-status-fsmonitor.sh\n @@ -113,8 +131,8 @@\n -\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n -\tEOF\n \tclean_repo &&\n -+\twrite_integration_script &&\n \tdirty_repo &&\n ++\twrite_integration_script &&\n \tgit add . &&\n +\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n +\tEOF\n\n-- \ngitgitgadget\n"},{"id":"385537","messageId":"4bea7075cfcac013f5947cd3e9254d38e86e675c.1573016055.git.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.v3.git.1573016055.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T04:54:15Z","receivedAt":"2019-11-06T04:54:21Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah \n\nThe index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat\nwhich can be inefficient on certain filesystems. This significantly slows\ndown commands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered\nunchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\nalso now correctly copies over fsmonitor validity state from the source\nindex. Finally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard\ninternally) go from 8s or more to ~2s on a 250k file repository on a\nmac.\n\nSigned-off-by: Utsav Shah \n---\n fsmonitor.c | 39 ++++++++++++++++++++++++-------------\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 3 files changed, 37 insertions(+), 17 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..04d6232531 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n \n \tif (pos >= istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n-\t\t (uintmax_t)pos, istate->cache_nr);\n+\t\t\t(uintmax_t)pos, istate->cache_nr);\n \n \tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n@@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n+\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)\n \t\t}\n \t\tif (bol < query_result.len)\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n+\n+\t\tif (istate->untracked)\n+\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n+\n+\t\t/* We only want to run the post index changed hook if we've actually changed entries, so keep track\n+\t\t * if we actually changed entries or not */\n+\t\tint is_cache_changed = 0;\n \t\t/* Mark all entries invalid */\n-\t\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n+\t\t\t\tis_cache_changed = 1;\n+\t\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\t\t}\n+\t\t}\n \n \t\t/* If we're going to check every file, ensure we save the results */\n-\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n+\t\tif (is_cache_changed)\n+\t\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n \n \t\tif (istate->untracked)\n \t\t\tistate->untracked->use_fsmonitor = 0;\n@@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t/* Mark all previously saved entries as dirty */\n \t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\t\t\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n-\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n-\t\t\tif (istate->untracked)\n-\t\t\t\tistate->untracked->use_fsmonitor = 1;\n+\t\t\trefresh_fsmonitor(istate);\n \t\t}\n \n \t\tewah_free(istate->fsmonitor_dirty);\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex d8df990972..69908b6a9b 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -106,6 +106,8 @@ EOF\n \n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n \tgit update-index --fsmonitor-valid dir2/modified &&\n@@ -164,6 +166,8 @@ EOF\n \n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n \tgit add dir2/new &&\n@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged\n # Ensure commands that call refresh_index() to move the index back in time\n # properly invalidate the fsmonitor cache\n test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n-\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n-\tEOF\n \tclean_repo &&\n \tdirty_repo &&\n+\twrite_integration_script &&\n \tgit add . &&\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit commit -m \"to reset\" &&\n \tgit reset HEAD~1 &&\n \tgit status >actual &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 33ea7810d8..fc5ceb932c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \tmark_all_ce_unused(o->src_index);\n \n+\tif (o->src_index->fsmonitor_last_update)\n+\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n+\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n+\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n-- \ngitgitgadget\n"},{"id":"385578","messageId":"xmqqk18dtimh.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"pull.424.v3.git.1573016055.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T10:16:22Z","receivedAt":"2019-11-06T10:16:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Utsav Shah via GitGitGadget\" writes:\n\n> The index might be aware that a file hasn't modified via fsmonitor, but\n> unpack-trees did not pay attention to it and checked via ie_match_stat which\n> can be inefficient on certain filesystems. This significantly slows down\n> commands that run oneway_merge, like checkout and reset --hard.\n>\n> This patch makes oneway_merge check whether a file is considered unchanged\n> through fsmonitor and skips ie_match_stat on it. unpack-trees also now\n> correctly copies over fsmonitor validity state from the source index.\n> Finally, for correctness, we force a refresh of fsmonitor state in\n> tweak_fsmonitor.\n>\n> After this change, commands like stash (that use reset --hard internally) go\n> from 8s or more to ~2s on a 250k file repository on a mac.\n\nIf the above is verbatim copy of what you wrote in 1/1, that is very\nmuch unappreciated X-<. A cover letter for a topic serves primarily\njust one purpose:\n\n It is a good place to present a birds-eye-view of a multi-patch\n topic; a high level description of the problem (e.g. how the\n issue manifests to the end users), an explanation of division of\n the problem into subproblems you made (if applicable), and\n interesting highlights of the solution would all be good things\n to have in there.\n\nAnd as a topic goes through iterations, it gives you a good place to\nsummarize what changed since the previously reviewed iterations. It\ncould be just a single liner \"addressed all the review comments for\nthe previous iteration\". A well-written multi-patch topic also uses\nthe same after-three-dash technique used for a single-patch topic\n(see below) to summarize what changed since the corresponding patch\nin the series in the previous iteration (or just says \"no changes\nsince the previous round\"---that helps the reviewers a lot).\n \nFor a single-patch topic, there is no place for \"here is an overall\nbirds-eyes-view picture because the changes described in the\nproposed log message of individual patches are so big and complex\".\nA single-patch topic has one patch, that solves one problem and only\nthat problem well. When you want to summarize the changes since the\nprevious iteration, you would write it between the three-dash-line\n(which appears after your sign-off) and the diffstat.\n\nThanks.\n"},{"id":"385579","messageId":"xmqqftj1th93.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"4bea7075cfcac013f5947cd3e9254d38e86e675c.1573016055.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T10:46:00Z","receivedAt":"2019-11-06T10:46:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Utsav Shah via GitGitGadget\" writes:\n\n> From: Utsav Shah \n>\n> The index might be aware that a file hasn't modified via fsmonitor, but\n> unpack-trees did not pay attention to it and checked via ie_match_stat\n> which can be inefficient on certain filesystems. This significantly slows\n> down commands that run oneway_merge, like checkout and reset --hard.\n\ns/hasn't/& been/;\n\nOtherwise, well written.\n\n> This patch makes oneway_merge check whether a file is considered\n> unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n> also now correctly copies over fsmonitor validity state from the source\n> index. Finally, for correctness, we force a refresh of fsmonitor state in\n> tweak_fsmonitor.\n\ns/This patch makes/Make/; order the person who is updating the code\nwhat to do to the codebase in imperative mood.\n\nOtherwise, well written.\n\n> After this change, commands like stash (that use reset --hard\n> internally) go from 8s or more to ~2s on a 250k file repository on a\n> mac.\n>\n> Signed-off-by: Utsav Shah \n> ---\n> fsmonitor.c | 39 ++++++++++++++++++++++++-------------\n> t/t7519-status-fsmonitor.sh | 9 +++++++--\n> unpack-trees.c | 6 +++++-\n> 3 files changed, 37 insertions(+), 17 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 1f4aa1b150..04d6232531 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n> \n> \tif (pos >= istate->cache_nr)\n> \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n> -\t\t (uintmax_t)pos, istate->cache_nr);\n> +\t\t\t(uintmax_t)pos, istate->cache_nr);\n\nUnintended whitespace change?\n\n> @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n> \t}\n> \tistate->fsmonitor_dirty = fsmonitor_dirty;\n> \n> -\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> +\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n> +\n\nThe patch disables this sanity check under \"split index\" mode and it\nmust be done for good reasons, but readers (imagine, somebody found\na bug on this line 6 months down the road, ran \"git blame\" and found\nthis commit and reading it via \"git show\") would want to know why\nthis change was made.\n\nI recall seeing no mention of \"split index\" in the proposed log\nmessage. Is this a fix for unrelated issue that needs to be\nexplained in a separate patch, perhaps?\n\nThe hunk also has the unintended whitespace change, it seems.\n\n> @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> \tuint32_t ewah_size = 0;\n> \tint fixup = 0;\n> \n> -\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> +\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n\nLikewise (both indentation of the second line and the unexplained\nchange to the sanity check condition we saw above).\n\n> @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)\n> \t\t}\n> \t\tif (bol < query_result.len)\n> \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n> +\n> +\t\tif (istate->untracked)\n> +\t\t\tistate->untracked->use_fsmonitor = 1;\n\nUnexplained. We used to do this in tweak_fsmonitor() but now we do\nthis here, as we are making tweak_fsmonitor() to call this function\nanyway. If there are other callers that call refresh_fsmonitor()\nand they did not do this, this would be a behaviour change to them.\nAs there is no explanation why this change is done, readers cannot\ntell if it is a good change. If this were explained like so:\n\n Any caller of refresh_fsmonitor() must set use_fsmonitor bit in\n istate when istate->untracked exists FOR SUCH AND SUCH REASONS.\n Move the code to do so in tweak_fsmonitor() to near the\n beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH\n other callers that forgets to do this.\n\nin the proposed log message, that might help justifying the change.\n\nIf use_fsmonitor is not set, why is the caller calling\nrefresh_fsmonitor() in the first place, by the way? \n\nIsn't it more like \"we are told to use fsmonitor via\nistate->untracked->use_fsmonitor bit being true, so we call\nrefresh_fsmonitor, and if the bit is false, we do not have to bother\nwith fsmonitor and no point calling refresh_fsmonitor\"?\n\nIf a careless caller makes a call to refresh_fsmonitor() when the\nconfiguration tells us not to use fsmonitor, wouldn't this cause us\nto use fsmonitor anyway? Which sounds bad, so perhaps all callers\nare careful to first check if use_fsmonitor is set before deciding\nto call refresh_fsmonitor()---but if that is the case, is there a\npoint in setting it here to true?\n\nPuzzled by an unexplained code...\n\n> \t} else {\n> +\n> +\t\t/* We only want to run the post index changed hook if we've actually changed entries, so keep track\n> +\t\t * if we actually changed entries or not */\n> +\t\tint is_cache_changed = 0;\n> \t\t/* Mark all entries invalid */\n> -\t\tfor (i = 0; i < istate->cache_nr; i++)\n> -\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n> +\t\t\t\tis_cache_changed = 1;\n> +\t\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> +\t\t\t}\n> +\t\t}\n>\n> \t\t/* If we're going to check every file, ensure we save the results */\n> -\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n> +\t\tif (is_cache_changed)\n> +\t\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n\nThis part (and a call to refresh_fsmonitor() we see blow) is the\n\"Finally, we force a refresh\" explained in the proposed log message,\nI presume.\n\n> \t\tif (istate->untracked)\n> \t\t\tistate->untracked->use_fsmonitor = 0;\n> @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)\n> \t\t\t/* Mark all previously saved entries as dirty */\n> \t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> \t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\t\t\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n\nThis shares the same indentation issue but does not disable the\nsanity check for split index case. Intended? Without explanation\nin the proposed log message, readers cannot tell.\n\n> \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> \n> -\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n> -\t\t\tif (istate->untracked)\n> -\t\t\t\tistate->untracked->use_fsmonitor = 1;\n> +\t\t\trefresh_fsmonitor(istate);\n> \t\t}\n> \n> \t\tewah_free(istate->fsmonitor_dirty);\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 33ea7810d8..fc5ceb932c 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \to->merge_size = len;\n> \tmark_all_ce_unused(o->src_index);\n> \n> +\tif (o->src_index->fsmonitor_last_update)\n> +\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n> +\n\nThis is the \"correctly copies\" part, which was well explained.\n\n> \t/*\n> \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n> \t */\n> @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n> \n> \tif (old && same(old, a)) {\n> \t\tint update = 0;\n> -\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> +\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> +\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n\nThis is the \"skip when we know it is valid\" part, which was well\nexplained.\n\n> \t\t\tstruct stat st;\n> \t\t\tif (lstat(old->name, &st) ||\n> \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n\nThanks.\n"},{"id":"385611","messageId":"BN6PR21MB0786BC0CD7ABD6B3EEACE74B91790@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52120","inReplyTo":"CAPYzU3PLVhGWcautao1oenbKbgcOB_99PoyVEcK1hnUk5XErKg@mail.gmail.com","subject":"RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-11-06T17:24:39Z","receivedAt":"2019-11-06T17:24:43Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"> From: Utsav Shah \n> Sent: Tuesday, November 5, 2019 9:36 PM\n> \n> The one part that I don't fully understand if safe is copying over the\n> o->src_index->fsmonitor_last_update to the result index in\n> unpack-trees. I don't understand the implications of that, and if that's the\n> only field worth copying over, or if we should be copying over other fields\n> like the bitmap as well.\n\nThe data from the bitmap has already been applied to the index entries at\nthis point so it is no longer needed after that.\n\nfsmonitor_last_update is really just being used as the flag to fill the bitmap\nand write out the fsmonitor extension. The fill_fsmonitor_bitmap will use\nthe current CE_FSMONITOR_VALID flags for the index entries to create the\nbitmap.\n\nif (istate->fsmonitor_last_update)\n\tfill_fsmonitor_bitmap(istate);\n\nvoid fill_fsmonitor_bitmap(struct index_state *istate)\n{\n\tunsigned int i, skipped = 0;\n\tistate->fsmonitor_dirty = ewah_new();\n\tfor (i = 0; i < istate->cache_nr; i++) {\n\t\tif (istate->cache[i]->ce_flags & CE_REMOVE)\n\t\t\tskipped++;\n\t\telse if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n\t\t\tewah_set(istate->fsmonitor_dirty, i - skipped);\n\t}\n}\n\nif (!strip_extensions && istate->fsmonitor_last_update) {\n\tstruct strbuf sb = STRBUF_INIT;\n\n\twrite_fsmonitor_extension(&sb, istate);\n\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0\n\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n\tstrbuf_release(&sb);\n\tif (err)\n\t\treturn -1;\n}\n"},{"id":"385632","messageId":"CAPYzU3P6M0s365gOEz360B0QqRN6JKaUB2Zc8+-XnVei0WF6WA@mail.gmail.com","threadId":"52120","inReplyTo":"xmqqftj1th93.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-11-06T22:33:18Z","receivedAt":"2019-11-06T22:33:34Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano wrote:\n>\n> \"Utsav Shah via GitGitGadget\" writes:\n>\n> > From: Utsav Shah \n> >\n> > The index might be aware that a file hasn't modified via fsmonitor, but\n> > unpack-trees did not pay attention to it and checked via ie_match_stat\n> > which can be inefficient on certain filesystems. This significantly slows\n> > down commands that run oneway_merge, like checkout and reset --hard.\n>\n> s/hasn't/& been/;\n>\n> Otherwise, well written.\n>\n> > This patch makes oneway_merge check whether a file is considered\n> > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n> > also now correctly copies over fsmonitor validity state from the source\n> > index. Finally, for correctness, we force a refresh of fsmonitor state in\n> > tweak_fsmonitor.\n>\n> s/This patch makes/Make/; order the person who is updating the code\n> what to do to the codebase in imperative mood.\n>\n> Otherwise, well written.\n>\n> > After this change, commands like stash (that use reset --hard\n> > internally) go from 8s or more to ~2s on a 250k file repository on a\n> > mac.\n> >\n> > Signed-off-by: Utsav Shah \n> > ---\n> > fsmonitor.c | 39 ++++++++++++++++++++++++-------------\n> > t/t7519-status-fsmonitor.sh | 9 +++++++--\n> > unpack-trees.c | 6 +++++-\n> > 3 files changed, 37 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/fsmonitor.c b/fsmonitor.c\n> > index 1f4aa1b150..04d6232531 100644\n> > --- a/fsmonitor.c\n> > +++ b/fsmonitor.c\n> > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n> >\n> > if (pos >= istate->cache_nr)\n> > BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n> > - (uintmax_t)pos, istate->cache_nr);\n> > + (uintmax_t)pos, istate->cache_nr);\n>\n> Unintended whitespace change?\n>\n> > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n> > }\n> > istate->fsmonitor_dirty = fsmonitor_dirty;\n> >\n> > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > - BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > + BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n> > +\n>\n> The patch disables this sanity check under \"split index\" mode and it\n> must be done for good reasons, but readers (imagine, somebody found\n> a bug on this line 6 months down the road, ran \"git blame\" and found\n> this commit and reading it via \"git show\") would want to know why\n> this change was made.\n>\n> I recall seeing no mention of \"split index\" in the proposed log\n> message. Is this a fix for unrelated issue that needs to be\n> explained in a separate patch, perhaps?\n>\n> The hunk also has the unintended whitespace change, it seems.\n>\n> > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> > uint32_t ewah_size = 0;\n> > int fixup = 0;\n> >\n> > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > - BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > + BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n>\n> Likewise (both indentation of the second line and the unexplained\n> change to the sanity check condition we saw above).\n>\n> > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)\n> > }\n> > if (bol < query_result.len)\n> > fsmonitor_refresh_callback(istate, buf + bol);\n> > +\n> > + if (istate->untracked)\n> > + istate->untracked->use_fsmonitor = 1;\n>\n> Unexplained. We used to do this in tweak_fsmonitor() but now we do\n> this here, as we are making tweak_fsmonitor() to call this function\n> anyway. If there are other callers that call refresh_fsmonitor()\n> and they did not do this, this would be a behaviour change to them.\n> As there is no explanation why this change is done, readers cannot\n> tell if it is a good change. If this were explained like so:\n>\n> Any caller of refresh_fsmonitor() must set use_fsmonitor bit in\n> istate when istate->untracked exists FOR SUCH AND SUCH REASONS.\n> Move the code to do so in tweak_fsmonitor() to near the\n> beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH\n> other callers that forgets to do this.\n>\n> in the proposed log message, that might help justifying the change.\n>\n> If use_fsmonitor is not set, why is the caller calling\n> refresh_fsmonitor() in the first place, by the way?\n>\n> Isn't it more like \"we are told to use fsmonitor via\n> istate->untracked->use_fsmonitor bit being true, so we call\n> refresh_fsmonitor, and if the bit is false, we do not have to bother\n> with fsmonitor and no point calling refresh_fsmonitor\"?\n>\n> If a careless caller makes a call to refresh_fsmonitor() when the\n> configuration tells us not to use fsmonitor, wouldn't this cause us\n> to use fsmonitor anyway? Which sounds bad, so perhaps all callers\n> are careful to first check if use_fsmonitor is set before deciding\n> to call refresh_fsmonitor()---but if that is the case, is there a\n> point in setting it here to true?\n>\n> Puzzled by an unexplained code...\n>\n> > } else {\n> > +\n> > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track\n> > + * if we actually changed entries or not */\n> > + int is_cache_changed = 0;\n> > /* Mark all entries invalid */\n> > - for (i = 0; i < istate->cache_nr; i++)\n> > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> > + for (i = 0; i < istate->cache_nr; i++) {\n> > + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n> > + is_cache_changed = 1;\n> > + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> > + }\n> > + }\n> >\n> > /* If we're going to check every file, ensure we save the results */\n> > - istate->cache_changed |= FSMONITOR_CHANGED;\n> > + if (is_cache_changed)\n> > + istate->cache_changed |= FSMONITOR_CHANGED;\n>\n> This part (and a call to refresh_fsmonitor() we see blow) is the\n> \"Finally, we force a refresh\" explained in the proposed log message,\n> I presume.\n>\n> > if (istate->untracked)\n> > istate->untracked->use_fsmonitor = 0;\n> > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)\n> > /* Mark all previously saved entries as dirty */\n> > if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n>\n> This shares the same indentation issue but does not disable the\n> sanity check for split index case. Intended? Without explanation\n> in the proposed log message, readers cannot tell.\n>\n> > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> >\n> > - /* Now mark the untracked cache for fsmonitor usage */\n> > - if (istate->untracked)\n> > - istate->untracked->use_fsmonitor = 1;\n> > + refresh_fsmonitor(istate);\n> > }\n> >\n> > ewah_free(istate->fsmonitor_dirty);\n> > diff --git a/unpack-trees.c b/unpack-trees.c\n> > index 33ea7810d8..fc5ceb932c 100644\n> > --- a/unpack-trees.c\n> > +++ b/unpack-trees.c\n> > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> > o->merge_size = len;\n> > mark_all_ce_unused(o->src_index);\n> >\n> > + if (o->src_index->fsmonitor_last_update)\n> > + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n> > +\n>\n> This is the \"correctly copies\" part, which was well explained.\n>\n> > /*\n> > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n> > */\n> > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n> >\n> > if (old && same(old, a)) {\n> > int update = 0;\n> > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> > + !(old->ce_flags & CE_FSMONITOR_VALID)) {\n>\n> This is the \"skip when we know it is valid\" part, which was well\n> explained.\n>\n> > struct stat st;\n> > if (lstat(old->name, &st) ||\n> > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n>\n> Thanks.\n\nThanks for the feedback. The lines with the indentation changes had\ntabs and spaces mixed up, but I'll revert those changes.\n"},{"id":"385752","messageId":"CAPYzU3OU45E_OQKOFFohxsqUvmMGXL9NNYNi2i27k3aRZ4oqVg@mail.gmail.com","threadId":"52120","inReplyTo":"CAPYzU3P6M0s365gOEz360B0QqRN6JKaUB2Zc8+-XnVei0WF6WA@mail.gmail.com","subject":"Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-11-08T03:51:04Z","receivedAt":"2019-11-08T03:51:21Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"On Wed, Nov 6, 2019 at 2:33 PM Utsav Shah wrote:\n>\n> On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano wrote:\n> >\n> > \"Utsav Shah via GitGitGadget\" writes:\n> >\n> > > From: Utsav Shah \n> > >\n> > > The index might be aware that a file hasn't modified via fsmonitor, but\n> > > unpack-trees did not pay attention to it and checked via ie_match_stat\n> > > which can be inefficient on certain filesystems. This significantly slows\n> > > down commands that run oneway_merge, like checkout and reset --hard.\n> >\n> > s/hasn't/& been/;\n> >\n> > Otherwise, well written.\n> >\n> > > This patch makes oneway_merge check whether a file is considered\n> > > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n> > > also now correctly copies over fsmonitor validity state from the source\n> > > index. Finally, for correctness, we force a refresh of fsmonitor state in\n> > > tweak_fsmonitor.\n> >\n> > s/This patch makes/Make/; order the person who is updating the code\n> > what to do to the codebase in imperative mood.\n> >\n> > Otherwise, well written.\n> >\n> > > After this change, commands like stash (that use reset --hard\n> > > internally) go from 8s or more to ~2s on a 250k file repository on a\n> > > mac.\n> > >\n> > > Signed-off-by: Utsav Shah \n> > > ---\n> > > fsmonitor.c | 39 ++++++++++++++++++++++++-------------\n> > > t/t7519-status-fsmonitor.sh | 9 +++++++--\n> > > unpack-trees.c | 6 +++++-\n> > > 3 files changed, 37 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/fsmonitor.c b/fsmonitor.c\n> > > index 1f4aa1b150..04d6232531 100644\n> > > --- a/fsmonitor.c\n> > > +++ b/fsmonitor.c\n> > > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n> > >\n> > > if (pos >= istate->cache_nr)\n> > > BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n> > > - (uintmax_t)pos, istate->cache_nr);\n> > > + (uintmax_t)pos, istate->cache_nr);\n> >\n> > Unintended whitespace change?\n> >\n> > > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n> > > }\n> > > istate->fsmonitor_dirty = fsmonitor_dirty;\n> > >\n> > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > > - BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > > + BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n> > > +\n> >\n> > The patch disables this sanity check under \"split index\" mode and it\n> > must be done for good reasons, but readers (imagine, somebody found\n> > a bug on this line 6 months down the road, ran \"git blame\" and found\n> > this commit and reading it via \"git show\") would want to know why\n> > this change was made.\n> >\n> > I recall seeing no mention of \"split index\" in the proposed log\n> > message. Is this a fix for unrelated issue that needs to be\n> > explained in a separate patch, perhaps?\n\nYes. I think this is an important bug to fix, fsmonitor and split\nindices don't seem to be interoperating well after these checks have\nbeen added.\n\n> >\n> > The hunk also has the unintended whitespace change, it seems.\n> >\n> > > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> > > uint32_t ewah_size = 0;\n> > > int fixup = 0;\n> > >\n> > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > > - BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > > + BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n> >\n> > Likewise (both indentation of the second line and the unexplained\n> > change to the sanity check condition we saw above).\n> >\n> > > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate)\n> > > }\n> > > if (bol < query_result.len)\n> > > fsmonitor_refresh_callback(istate, buf + bol);\n> > > +\n> > > + if (istate->untracked)\n> > > + istate->untracked->use_fsmonitor = 1;\n> >\n> > Unexplained. We used to do this in tweak_fsmonitor() but now we do\n> > this here, as we are making tweak_fsmonitor() to call this function\n> > anyway. If there are other callers that call refresh_fsmonitor()\n> > and they did not do this, this would be a behaviour change to them.\n> > As there is no explanation why this change is done, readers cannot\n> > tell if it is a good change. If this were explained like so:\n> >\n> > Any caller of refresh_fsmonitor() must set use_fsmonitor bit in\n> > istate when istate->untracked exists FOR SUCH AND SUCH REASONS.\n> > Move the code to do so in tweak_fsmonitor() to near the\n> > beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH\n> > other callers that forgets to do this.\n> >\n> > in the proposed log message, that might help justifying the change.\n> >\n> > If use_fsmonitor is not set, why is the caller calling\n> > refresh_fsmonitor() in the first place, by the way?\n> >\n> > Isn't it more like \"we are told to use fsmonitor via\n> > istate->untracked->use_fsmonitor bit being true, so we call\n> > refresh_fsmonitor, and if the bit is false, we do not have to bother\n> > with fsmonitor and no point calling refresh_fsmonitor\"?\n> >\n> > If a careless caller makes a call to refresh_fsmonitor() when the\n> > configuration tells us not to use fsmonitor, wouldn't this cause us\n> > to use fsmonitor anyway? Which sounds bad, so perhaps all callers\n> > are careful to first check if use_fsmonitor is set before deciding\n> > to call refresh_fsmonitor()---but if that is the case, is there a\n> > point in setting it here to true?\n> >\n> > Puzzled by an unexplained code...\n\nI might be misunderstanding, but wouldn't the if condition above make\nsure we don't enter this codepath at all?\n\nif (!core_fsmonitor || istate->fsmonitor_has_run_once)\n return;\n\nistate->untracked->use_fsmonitor is only used in the untracked cache\nto skip some lstats AFAICT. So callers refresh_fsmonitor should see no\ndifference.\n\nI agree that this change deserves a better explanation.\n\n> >\n> > > } else {\n> > > +\n> > > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track\n> > > + * if we actually changed entries or not */\n> > > + int is_cache_changed = 0;\n> > > /* Mark all entries invalid */\n> > > - for (i = 0; i < istate->cache_nr; i++)\n> > > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> > > + for (i = 0; i < istate->cache_nr; i++) {\n> > > + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n> > > + is_cache_changed = 1;\n> > > + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> > > + }\n> > > + }\n> > >\n> > > /* If we're going to check every file, ensure we save the results */\n> > > - istate->cache_changed |= FSMONITOR_CHANGED;\n> > > + if (is_cache_changed)\n> > > + istate->cache_changed |= FSMONITOR_CHANGED;\n> >\n> > This part (and a call to refresh_fsmonitor() we see blow) is the\n> > \"Finally, we force a refresh\" explained in the proposed log message,\n> > I presume.\n> >\n> > > if (istate->untracked)\n> > > istate->untracked->use_fsmonitor = 0;\n> > > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate)\n> > > /* Mark all previously saved entries as dirty */\n> > > if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> > > BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> >\n> > This shares the same indentation issue but does not disable the\n> > sanity check for split index case. Intended? Without explanation\n> > in the proposed log message, readers cannot tell.\n\nYes, will add the sanity check here too.\n\n> >\n> > > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> > >\n> > > - /* Now mark the untracked cache for fsmonitor usage */\n> > > - if (istate->untracked)\n> > > - istate->untracked->use_fsmonitor = 1;\n> > > + refresh_fsmonitor(istate);\n> > > }\n> > >\n> > > ewah_free(istate->fsmonitor_dirty);\n> > > diff --git a/unpack-trees.c b/unpack-trees.c\n> > > index 33ea7810d8..fc5ceb932c 100644\n> > > --- a/unpack-trees.c\n> > > +++ b/unpack-trees.c\n> > > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> > > o->merge_size = len;\n> > > mark_all_ce_unused(o->src_index);\n> > >\n> > > + if (o->src_index->fsmonitor_last_update)\n> > > + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n> > > +\n> >\n> > This is the \"correctly copies\" part, which was well explained.\n> >\n> > > /*\n> > > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n> > > */\n> > > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n> > >\n> > > if (old && same(old, a)) {\n> > > int update = 0;\n> > > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n> > > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n> > > + !(old->ce_flags & CE_FSMONITOR_VALID)) {\n> >\n> > This is the \"skip when we know it is valid\" part, which was well\n> > explained.\n> >\n> > > struct stat st;\n> > > if (lstat(old->name, &st) ||\n> > > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n> >\n> > Thanks.\n>\n> Thanks for the feedback. The lines with the indentation changes had\n> tabs and spaces mixed up, but I'll revert those changes.\n\nI will try to figure out how to use gitgitgadget to send a smaller\npatch within this thread to fix the fsmonitor and split index\ninteractions. I tried writing a test today that uses\nt7519/fsmonitor-watchman to simulate the bug, but it is flaky.\n"},{"id":"385753","messageId":"xmqqlfsrnh1i.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"CAPYzU3OU45E_OQKOFFohxsqUvmMGXL9NNYNi2i27k3aRZ4oqVg@mail.gmail.com","subject":"Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T04:11:37Z","receivedAt":"2019-11-08T04:11:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Utsav Shah writes:\n\n> I will try to figure out how to use gitgitgadget to send a smaller\n> patch within this thread to fix the fsmonitor and split index\n> interactions.\n\nIf the current one is trying to handle two issues, then you'd need\nto reduce this one and add another spearate topic, probably.\n\n> I tried writing a test today that uses\n> t7519/fsmonitor-watchman to simulate the bug, but it is flaky.\n\nAre you aware of Kevin Willford's recent work?\n\n<52eaf20f405b53743e27935dfe89f62e6f824a33.1572889841.git.gitgitgadget@gmail.com>\n\n"},{"id":"386620","messageId":"pull.424.v4.git.1574238737.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.v3.git.1573016055.gitgitgadget@gmail.com","subject":"[PATCH v4 0/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-20T08:32:16Z","receivedAt":"2019-11-20T08:32:23Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"The index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat which\ncan be inefficient on certain filesystems. This significantly slows down\ncommands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered unchanged\nthrough fsmonitor and skips ie_match_stat on it. unpack-trees also now\ncorrectly copies over fsmonitor validity state from the source index.\nFinally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard internally) go\nfrom 8s or more to ~2s on a 250k file repository on a mac.\n\nChanges since the last version are:\n\n * The sanity checks around accessing the fsmonitor_dirty bitmap have been\n moved to another patch, which is in message id [1]\n * Unintended indentation changes in fsmonitor have been removed\n * A comment explaining what untracked->use_fsmonitor means has been\n re-added (it was dropped in the previous version)\n * A few \"helped-by\" entries have been added to the patch\n\n1: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)\n\nHelped-by: Junio C Hamano gitster@pobox.com [gitster@pobox.com]Helped-by:\nKevin Willford Kevin.Willford@microsoft.com [Kevin.Willford@microsoft.com]\nSigned-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]\n\nUtsav Shah (1):\n unpack-trees: skip stat on fsmonitor-valid files\n\n fsmonitor.c | 23 +++++++++++++++++------\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 3 files changed, 29 insertions(+), 9 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/424\n\nRange-diff vs v3:\n\n 1: 4bea7075cf ! 1: ea7880f2d0 unpack-trees: skip stat on fsmonitor-valid files\n @@ -17,52 +17,29 @@\n internally) go from 8s or more to ~2s on a 250k file repository on a\n mac.\n \n + Changes since the last version are:\n + * The sanity checks around accessing the fsmonitor_dirty bitmap have\n + been moved to another patch, which is in message id [1]\n + * Unintended indentation changes in fsmonitor have been removed\n + * A comment explaining what untracked->use_fsmonitor means has been\n + re-added (it was dropped in the previous version)\n + * A few \"helped-by\" entries have been added to the patch\n +\n + [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)\n +\n + Helped-by: Junio C Hamano \n + Helped-by: Kevin Willford \n Signed-off-by: Utsav Shah \n \n diff --git a/fsmonitor.c b/fsmonitor.c\n --- a/fsmonitor.c\n +++ b/fsmonitor.c\n @@\n - \n - \tif (pos >= istate->cache_nr)\n - \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n --\t\t (uintmax_t)pos, istate->cache_nr);\n -+\t\t\t(uintmax_t)pos, istate->cache_nr);\n - \n - \tce = istate->cache[pos];\n - \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n -@@\n - \t}\n - \tistate->fsmonitor_dirty = fsmonitor_dirty;\n - \n --\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n --\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n --\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n -+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n -+\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n -+\n - \n - \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n - \treturn 0;\n -@@\n - \tuint32_t ewah_size = 0;\n - \tint fixup = 0;\n - \n --\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n --\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n --\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n -+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n -+\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n - \n - \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n - \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n -@@\n \t\t}\n \t\tif (bol < query_result.len)\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n +\n ++\t\t/* Now mark the untracked cache for fsmonitor usage */\n +\t\tif (istate->untracked)\n +\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n @@ -88,11 +65,7 @@\n \t\tif (istate->untracked)\n \t\t\tistate->untracked->use_fsmonitor = 0;\n @@\n - \t\t\t/* Mark all previously saved entries as dirty */\n - \t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n - \t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n --\t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n -+\t\t\t\t\t(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n + \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n -\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n\n-- \ngitgitgadget\n"},{"id":"386621","messageId":"ea7880f2d07ef65f8b41f6786ff789f7a0ff8fd7.1574238737.git.gitgitgadget@gmail.com","threadId":"52120","inReplyTo":"pull.424.v4.git.1574238737.gitgitgadget@gmail.com","subject":"[PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-20T08:32:17Z","receivedAt":"2019-11-20T08:32:24Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah \n\nThe index might be aware that a file hasn't modified via fsmonitor, but\nunpack-trees did not pay attention to it and checked via ie_match_stat\nwhich can be inefficient on certain filesystems. This significantly slows\ndown commands that run oneway_merge, like checkout and reset --hard.\n\nThis patch makes oneway_merge check whether a file is considered\nunchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\nalso now correctly copies over fsmonitor validity state from the source\nindex. Finally, for correctness, we force a refresh of fsmonitor state in\ntweak_fsmonitor.\n\nAfter this change, commands like stash (that use reset --hard\ninternally) go from 8s or more to ~2s on a 250k file repository on a\nmac.\n\nChanges since the last version are:\n* The sanity checks around accessing the fsmonitor_dirty bitmap have\nbeen moved to another patch, which is in message id [1]\n* Unintended indentation changes in fsmonitor have been removed\n* A comment explaining what untracked->use_fsmonitor means has been\nre-added (it was dropped in the previous version)\n* A few \"helped-by\" entries have been added to the patch\n\n[1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)\n\nHelped-by: Junio C Hamano \nHelped-by: Kevin Willford \nSigned-off-by: Utsav Shah \n---\n fsmonitor.c | 23 +++++++++++++++++------\n t/t7519-status-fsmonitor.sh | 9 +++++++--\n unpack-trees.c | 6 +++++-\n 3 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..0d270da80f 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate)\n \t\t}\n \t\tif (bol < query_result.len)\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n+\n+\t\t/* Now mark the untracked cache for fsmonitor usage */\n+\t\tif (istate->untracked)\n+\t\t\tistate->untracked->use_fsmonitor = 1;\n \t} else {\n+\n+\t\t/* We only want to run the post index changed hook if we've actually changed entries, so keep track\n+\t\t * if we actually changed entries or not */\n+\t\tint is_cache_changed = 0;\n \t\t/* Mark all entries invalid */\n-\t\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\t\tif (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {\n+\t\t\t\tis_cache_changed = 1;\n+\t\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n+\t\t\t}\n+\t\t}\n \n \t\t/* If we're going to check every file, ensure we save the results */\n-\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n+\t\tif (is_cache_changed)\n+\t\t\tistate->cache_changed |= FSMONITOR_CHANGED;\n \n \t\tif (istate->untracked)\n \t\t\tistate->untracked->use_fsmonitor = 0;\n@@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n-\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n-\t\t\tif (istate->untracked)\n-\t\t\t\tistate->untracked->use_fsmonitor = 1;\n+\t\t\trefresh_fsmonitor(istate);\n \t\t}\n \n \t\tewah_free(istate->fsmonitor_dirty);\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex d8df990972..69908b6a9b 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -106,6 +106,8 @@ EOF\n \n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n \tgit update-index --fsmonitor-valid dir2/modified &&\n@@ -164,6 +166,8 @@ EOF\n \n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n \tgit add dir2/new &&\n@@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged\n # Ensure commands that call refresh_index() to move the index back in time\n # properly invalidate the fsmonitor cache\n test_expect_success 'refresh_index() invalidates fsmonitor cache' '\n-\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n-\tEOF\n \tclean_repo &&\n \tdirty_repo &&\n+\twrite_integration_script &&\n \tgit add . &&\n+\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tEOF\n \tgit commit -m \"to reset\" &&\n \tgit reset HEAD~1 &&\n \tgit status >actual &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 33ea7810d8..fc5ceb932c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \tmark_all_ce_unused(o->src_index);\n \n+\tif (o->src_index->fsmonitor_last_update)\n+\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n@@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src,\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {\n+\t\tif (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&\n+\t\t\t!(old->ce_flags & CE_FSMONITOR_VALID)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))\n-- \ngitgitgadget\n"},{"id":"386696","messageId":"xmqqd0dlamq2.fsf@gitster-ct.c.googlers.com","threadId":"52120","inReplyTo":"ea7880f2d07ef65f8b41f6786ff789f7a0ff8fd7.1574238737.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T04:15:49Z","receivedAt":"2019-11-21T04:15:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Utsav Shah via GitGitGadget\" writes:\n\n> From: Utsav Shah \n>\n> The index might be aware that a file hasn't modified via fsmonitor, but\n> unpack-trees did not pay attention to it and checked via ie_match_stat\n> which can be inefficient on certain filesystems. This significantly slows\n> down commands that run oneway_merge, like checkout and reset --hard.\n>\n> This patch makes oneway_merge check whether a file is considered\n> unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees\n> also now correctly copies over fsmonitor validity state from the source\n> index. Finally, for correctness, we force a refresh of fsmonitor state in\n> tweak_fsmonitor.\n\nInstead of saying \"also now correctly copies...\" as if it started\nworking correctly by accident, be more assertive and actively make\nit so ;-)\n\n Check if a file is unchanged by fsmonitor in oneway_merge(), and\n avoid unnecessary calls to ie_match_stat(). Copy the fsmonitor\n validity state from the source index to the destination index in\n unpack_trees(). Force a refresh of the fsmonitor state in\n tweak_fsmonitor(), which is called after the index file is read\n from the disk, for correctness.\n\nperhaps.\n\n> After this change, commands like stash (that use reset --hard\n> internally) go from 8s or more to ~2s on a 250k file repository on a\n> mac.\n\nGood.\n\n>\n> Changes since the last version are:\n> * The sanity checks around accessing the fsmonitor_dirty bitmap have\n> been moved to another patch, which is in message id [1]\n> * Unintended indentation changes in fsmonitor have been removed\n> * A comment explaining what untracked->use_fsmonitor means has been\n> re-added (it was dropped in the previous version)\n> * A few \"helped-by\" entries have been added to the patch\n>\n> [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com)\n\nThe above is for the cover letter or after the three-dash lines, and\nnot for the log message.\n\n> Helped-by: Junio C Hamano \n> Helped-by: Kevin Willford \n> Signed-off-by: Utsav Shah \n> ---\n> fsmonitor.c | 23 +++++++++++++++++------\n> t/t7519-status-fsmonitor.sh | 9 +++++++--\n> unpack-trees.c | 6 +++++-\n> 3 files changed, 29 insertions(+), 9 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 1f4aa1b150..0d270da80f 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate)\n> \t\t}\n> \t\tif (bol < query_result.len)\n> \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n> +\n> +\t\t/* Now mark the untracked cache for fsmonitor usage */\n> +\t\tif (istate->untracked)\n> +\t\t\tistate->untracked->use_fsmonitor = 1;\n> \t} else {\n> +\n> +\t\t/* We only want to run the post index changed hook if we've actually changed entries, so keep track\n> +\t\t * if we actually changed entries or not */\n\nMulti-line comment style.\n\n> +\t\tint is_cache_changed = 0;\n> \t\t/* Mark all entries invalid */\n> -\t\tfor (i = 0; i < istate->cache_nr; i++)\n> -\t\t\tistate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;\n> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n\nLack of blank line between the last decl and the first stmt.\nProbably the blank should go before \"/* Mark all ...\".\n\n> @@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate)\n> \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n> \n> -\t\t\t/* Now mark the untracked cache for fsmonitor usage */\n> -\t\t\tif (istate->untracked)\n> -\t\t\t\tistate->untracked->use_fsmonitor = 1;\n> +\t\t\trefresh_fsmonitor(istate);\n> \t\t}\n> \n> \t\tewah_free(istate->fsmonitor_dirty);\n\nLooks good.\n\nThanks.\n\n"}]}