* [PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor @ 2019-10-25 15:23 Utsav Shah via GitGitGadget 2019-10-25 15:23 ` [PATCH 1/1] unpack-trees: skip lstat " Utsav Shah via GitGitGadget 2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget 0 siblings, 2 replies; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-10-25 15:23 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano git stash runs git reset --hard as its final step, which can be fairly slow on a large repository. This change lets us skip that if fsmonitor thinks those files aren't modified. git stash goes from ~8s -> 2s on my repository after this change. Utsav Shah (1): unpack-trees: skip lstat based on fsmonitor unpack-trees.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/424 -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-25 15:23 [PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor Utsav Shah via GitGitGadget @ 2019-10-25 15:23 ` Utsav Shah via GitGitGadget 2019-10-28 3:37 ` Junio C Hamano 2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget 1 sibling, 1 reply; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-10-25 15:23 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano, Utsav Shah From: Utsav Shah <utsav@dropbox.com> git stash runs git reset --hard as its final step, which can be fairly slow on a large repository. This change lets us skip that if fsmonitor thinks those files aren't modified. git stash goes from ~8s -> 2s on my repository after this change. Signed-off-by: Utsav Shah <utsav@dropbox.com> --- unpack-trees.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/unpack-trees.c b/unpack-trees.c index 33ea7810d8..5d07a2d0ea 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2384,7 +2384,8 @@ int oneway_merge(const struct cache_entry * const *src, if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && + !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-25 15:23 ` [PATCH 1/1] unpack-trees: skip lstat " Utsav Shah via GitGitGadget @ 2019-10-28 3:37 ` Junio C Hamano 2019-10-28 6:39 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2019-10-28 3:37 UTC (permalink / raw) To: Utsav Shah via GitGitGadget; +Cc: git, Utsav Shah, Utsav Shah "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Utsav Shah <utsav@dropbox.com> > > git stash runs git reset --hard as its final step, which can be fairly slow on a large repository. > This change lets us skip that if fsmonitor thinks those files aren't modified. > > git stash goes from ~8s -> 2s on my repository after this change. Please line-wrap overlong lines. More importantly, "stash" may be a mere symptom that does not deserve this much emphasis. What you improved directly is "git reset --hard" isn't it? The fsmonitor may know that a path hasn't been modified but "git reset --hard" did not pay attention to it and performed its own check based on ie_match_stat(), which was inefficient. or something like that? > if (old && same(old, a)) { > int update = 0; > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && > + !(old->ce_flags & CE_FSMONITOR_VALID)) { I wonder if !ce_uptodate(old) should say "this one is up to date and not modified" when CE_FSMONITOR_VALID bit is set. Are there other codepaths that use ce_uptodate(ce) to decide to do X without paying attention to CE_FSMONITOR_VALID bit? If there are, are they buggy in the same way as you found this instance, or do they have legitimate reason why they only check ce_uptodate(ce) and ignore fsmonitor? If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID bit and have fsmonitor directly set CE_UPTODATE bit instead? That would make this fix unnecessary and fix other codepaths that check only ce_uptodate() without checking fsmonitor. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-28 3:37 ` Junio C Hamano @ 2019-10-28 6:39 ` Utsav Shah 2019-10-28 19:23 ` Kevin Willford 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-10-28 6:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah Thanks for the review. On Sun, Oct 27, 2019 at 8:37 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Utsav Shah <utsav@dropbox.com> > > > > git stash runs git reset --hard as its final step, which can be fairly slow on a large repository. > > This change lets us skip that if fsmonitor thinks those files aren't modified. > > > > git stash goes from ~8s -> 2s on my repository after this change. > > Please line-wrap overlong lines. > > More importantly, "stash" may be a mere symptom that does not > deserve this much emphasis. What you improved directly is "git > reset --hard" isn't it? > > The fsmonitor may know that a path hasn't been modified but > "git reset --hard" did not pay attention to it and performed > its own check based on ie_match_stat(), which was inefficient. > > or something like that? > > > if (old && same(old, a)) { > > int update = 0; > > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { > > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && > > + !(old->ce_flags & CE_FSMONITOR_VALID)) { > > I wonder if !ce_uptodate(old) should say "this one is up to date and > not modified" when CE_FSMONITOR_VALID bit is set. Are there other > codepaths that use ce_uptodate(ce) to decide to do X without paying > attention to CE_FSMONITOR_VALID bit? If there are, are they buggy > in the same way as you found this instance, or do they have legitimate > reason why they only check ce_uptodate(ce) and ignore fsmonitor? > Yes, there are other code paths as well. After reading the code some more, it seems like there's no legitimate need to ignore fsmonitor. > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID > bit and have fsmonitor directly set CE_UPTODATE bit instead? That > would make this fix unnecessary and fix other codepaths that check > only ce_uptodate() without checking fsmonitor. > There's a few issues with replacing it entirely that I've found. One is the "CE_MATCH_IGNORE_FSMONITOR" flag. This flag can be set to let ie_match_stat skip calling refresh_fsmonitor repeatedly. This is set only in one place right now in preload-index, and it's unclear how necessary this optimization even is, given that refresh_fsmonitor has a check whether it's been called already, and returns if true. The second is that git ls-files has an "f" option that makes it "use lowercase letters for 'fsmonitor clean' files". I think this can simply be replaced by checking if a file is up to date instead of specifically via fsmonitor. If we do go ahead with the replace, we will have to be diligent about calling refresh_fsmonitor everywhere, or we will have correctness issues. I patched git locally to do this, and immediately saw a bug in git stash where the underlying git reset --hard skipped modifying a file it should have. In my opinion refresh_fsmonitor should be called somewhere top level, like an initialization, but I'm not sure if that makes sense for all git subcommands. Do you think it's worth cleaning up and sending this patch instead? It will reduce the surface area of bugs and remove a bunch of functions like mark_fsmonitor_valid/mark_fsmonitor_invalid ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-28 6:39 ` Utsav Shah @ 2019-10-28 19:23 ` Kevin Willford 2019-10-29 19:06 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Kevin Willford @ 2019-10-28 19:23 UTC (permalink / raw) To: Utsav Shah, Junio C Hamano; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com> wrote: > > I wonder if !ce_uptodate(old) should say "this one is up to date and > > not modified" when CE_FSMONITOR_VALID bit is set. Are there other > > codepaths that use ce_uptodate(ce) to decide to do X without paying > > attention to CE_FSMONITOR_VALID bit? If there are, are they buggy in > > the same way as you found this instance, or do they have legitimate > > reason why they only check ce_uptodate(ce) and ignore fsmonitor? > > > > Yes, there are other code paths as well. After reading the code some more, it > seems like there's no legitimate need to ignore fsmonitor. > > > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID > > bit and have fsmonitor directly set CE_UPTODATE bit instead? That > > would make this fix unnecessary and fix other codepaths that check > > only ce_uptodate() without checking fsmonitor. > > > I would need to go back and see if there was some reasoning why the new flag was added but using CE_UPTODATE makes sense especially when most calls to ce_mark_uptodate is followed directly by a call to mark_fsmonitor_valid. There is a little more going on in the mark_fsmonitor_X than just setting the bit though and the invalid calls are not matched with code to clear the CE_UPTODATE flag. The change to use CE_UPTODATE would have more extensive effects and like you said we would need to make sure it would not cause correctness issues in some corner case. Did you run all the git tests with GIT_TEST_FSMONITOR set to t/t7519/fsmonitor-all? This will run the tests with fsmonitor on. I was getting multiple failures with this change and fsmonitor on. I added the refresh_fsmonitor call to the tweak_fsmonitor after using the bitmap to set the dirty entries. This fixed most of the test failures but there are still some failures that I haven't tracked down the reason for. I will do some more digging and testing to see what other pitfalls there might be with this change. Thanks, Kevin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-28 19:23 ` Kevin Willford @ 2019-10-29 19:06 ` Utsav Shah 2019-10-29 20:12 ` Kevin Willford 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-10-29 19:06 UTC (permalink / raw) To: Kevin Willford Cc: Junio C Hamano, Utsav Shah via GitGitGadget, git, Utsav Shah On Mon, Oct 28, 2019 at 12:23 PM Kevin Willford <Kevin.Willford@microsoft.com> wrote: > > On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com> > wrote: > > > > I wonder if !ce_uptodate(old) should say "this one is up to date and > > > not modified" when CE_FSMONITOR_VALID bit is set. Are there other > > > codepaths that use ce_uptodate(ce) to decide to do X without paying > > > attention to CE_FSMONITOR_VALID bit? If there are, are they buggy in > > > the same way as you found this instance, or do they have legitimate > > > reason why they only check ce_uptodate(ce) and ignore fsmonitor? > > > > > > > Yes, there are other code paths as well. After reading the code some more, it > > seems like there's no legitimate need to ignore fsmonitor. > > > > > If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID > > > bit and have fsmonitor directly set CE_UPTODATE bit instead? That > > > would make this fix unnecessary and fix other codepaths that check > > > only ce_uptodate() without checking fsmonitor. > > > > > > > I would need to go back and see if there was some reasoning why the > new flag was added but using CE_UPTODATE makes sense especially when > most calls to ce_mark_uptodate is followed directly by a call to > mark_fsmonitor_valid. I've been playing around with the patch and trying to get the tests pass. I've found that we set CE_UPTODATE when we try to skip worktrees to stat in the case of sparse checkouts, and there are cases where we mark cache entries up to date without consulting fsmonitor or stating them. It seems like making fsmonitor only modify CE_UPTODATE makes it hard to verify and test correct fsmonitor behavior and debugging fsmonitor with git ls-files -f. I think the patch also makes things overall slightly more complicated. There is a little more going on in the > mark_fsmonitor_X than just setting the bit though and the invalid > calls are not matched with code to clear the CE_UPTODATE flag. Yeah. The patch to replace CE_FSMONITOR_VALID doesn't remove the need for calling mark_fsmonitor_valid/mark_fsmonitor_invalid, since there's special behavior like modifying the untracked cache which doesn't make sense in a more general mark_ce_not_uptodate function. > > The change to use CE_UPTODATE would have > more extensive effects and like you said we would need to make sure it > would not cause correctness issues in some corner case. > > Did you run all the git tests with GIT_TEST_FSMONITOR set to > t/t7519/fsmonitor-all? This will run the tests with fsmonitor on. I was > getting multiple failures with this change and fsmonitor on. > > I added the refresh_fsmonitor call to the tweak_fsmonitor after > using the bitmap to set the dirty entries. This fixed most of the test > failures but there are still some failures that I haven't tracked down the > reason for. I'm getting the same test failures with or without GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor in tweak_fsmonitor. Could you share your patch? I'm probably messing something up, and I can try taking a look at fixing test cases as well. > > I will do some more digging and testing to see what other pitfalls there > might be with this change. > > Thanks, > Kevin ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-29 19:06 ` Utsav Shah @ 2019-10-29 20:12 ` Kevin Willford 2019-10-29 23:50 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Kevin Willford @ 2019-10-29 20:12 UTC (permalink / raw) To: Utsav Shah; +Cc: Junio C Hamano, Utsav Shah via GitGitGadget, git, Utsav Shah > On Tuesday, October 29, 2019 1:07 PM Utsav Shah <utsav@dropbox.com> > wrote: > > I'm getting the same test failures with or without > GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor > in tweak_fsmonitor. Could you share your patch? I'm probably messing > something up, and I can try taking a look at fixing test cases as well. I have the tests passing with the following commit. https://github.com/kewillford/git/commit/3b1fdf5a4b1cd1d654b1733ce058faa4f087f75f Things to note: 1. Not sure if fsmonitor was tested with split index so for now I removed that from the check of entries in fsmonitor bitmap vs the number of cache entries 2. With these changes update-index was triggering the post-index-change hook with the updated_skipworktree flag set which it wasn't before. 3. Copied the fsmonitor_last_update to the result index so the fsmonitor data will be carried over to the new index in unpack_trees. This is to make sure that the next call to git will have the fsmonitor data to use. We found that running `git status` after any command that ran unpack_trees (checkout, reset --hard, etc.) was very slow the first call but and subsequent calls were fast. I'm still testing and reviewing these changes to make sure there isn't something I have missed and that I made the right changes to the tests that were failing. Kevin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-29 20:12 ` Kevin Willford @ 2019-10-29 23:50 ` Utsav Shah 2019-10-30 0:21 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-10-29 23:50 UTC (permalink / raw) To: Kevin Willford Cc: Junio C Hamano, Utsav Shah via GitGitGadget, git, Utsav Shah Thanks for testing it out. The unpack_trees bugfix is especially useful. There's tons of places where we're using ce_uptodate(ce) that could be optimized by checking CE_FSMONITOR_VALID. One example is in run_diff_files in diff-lib.c Should we add a check for CE_FSMONITOR_VALID in all of them? Should we do that in this patch? Or should we take the time to refactor and flesh out bugs in unifying it with CE_UPTODATE? It would be nice to get more opinions. I've taken a look and believe that it will make things a little more complicated to merge it with CE_UPTODATE, especially since it's used in a few places for other reasons like sparse checkouts. On the other hand, I'm a first time contributor, so my perspective towards a large refactor like might be too conservative. On Tue, Oct 29, 2019 at 1:12 PM Kevin Willford <Kevin.Willford@microsoft.com> wrote: > > > On Tuesday, October 29, 2019 1:07 PM Utsav Shah <utsav@dropbox.com> > > wrote: > > > > I'm getting the same test failures with or without > > GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor > > in tweak_fsmonitor. Could you share your patch? I'm probably messing > > something up, and I can try taking a look at fixing test cases as well. > > I have the tests passing with the following commit. > > https://github.com/kewillford/git/commit/3b1fdf5a4b1cd1d654b1733ce058faa4f087f75f > > Things to note: > 1. Not sure if fsmonitor was tested with split index so for now I removed that from the > check of entries in fsmonitor bitmap vs the number of cache entries > 2. With these changes update-index was triggering the post-index-change hook with the > updated_skipworktree flag set which it wasn't before. > 3. Copied the fsmonitor_last_update to the result index so the fsmonitor data will be > carried over to the new index in unpack_trees. This is to make sure that the next call > to git will have the fsmonitor data to use. We found that running `git status` after any > command that ran unpack_trees (checkout, reset --hard, etc.) was very slow the first > call but and subsequent calls were fast. > > I'm still testing and reviewing these changes to make sure there isn't something I > have missed and that I made the right changes to the tests that were failing. > > Kevin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-29 23:50 ` Utsav Shah @ 2019-10-30 0:21 ` Junio C Hamano 2019-10-30 16:41 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2019-10-30 0:21 UTC (permalink / raw) To: Utsav Shah; +Cc: Kevin Willford, Utsav Shah via GitGitGadget, git, Utsav Shah Utsav Shah <utsav@dropbox.com> writes: > Thanks for testing it out. The unpack_trees bugfix is especially useful. > > There's tons of places where we're using ce_uptodate(ce) that could be > optimized by checking CE_FSMONITOR_VALID. One example is in > run_diff_files in diff-lib.c > > Should we add a check for CE_FSMONITOR_VALID in all of them? Should we > do that in this patch? Or should we take the time to refactor and > flesh out bugs in unifying it with CE_UPTODATE? If we rephrase the first question slightly, i.e. "should these places all be avoiding lstat() based check when fsmonitor says the path is up to date?", I would imagine the answer is absolutely yes. I would further imagine that the implementation of the interface to external fsmonitor itself may have to distinguish "we know/have known this path is clean" vs "we just got told by fsmonitor that this path is clean", so losing FSMONITOR_VALID bit might not be an easy or clean conversion, in which case my earlier "can we perhaps lose it and have fsmonitor interfacing code to directly set UPTODATE bit?" would lead us in a wrong direction. But ce_uptodate(ce) being the primary way for the callers that care about "is the path known to be up to date?", it is unsatisfying that all of them have to ask if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID)) ... process ce that is not up-to-date ... So I would say that the longer term goal should be to let them ask ce_uptodate(ce) and have that macro automatically take FSMONITOR bit into account (in other words, those who want to know if ce is fresh should not have to even know about what fsmonitor is). Perhaps we can take a polished version of this "'reset --hard' can and should notice paths known-to-be-uptodate via fsmonitor" as an independent patch (to reduce the number of things we have to worry by one) for now? Taking this patch means we would now have one more place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if we would be auditing all such places before we can decide what the best way to reach the goal of allowing them to just say ce_uptodate() without having to spell FSMONITOR_VALID, that probably is a cost worth paying. Thanks for working on this topic. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-30 0:21 ` Junio C Hamano @ 2019-10-30 16:41 ` Utsav Shah 2019-11-04 6:02 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-10-30 16:41 UTC (permalink / raw) To: Junio C Hamano Cc: Kevin Willford, Utsav Shah via GitGitGadget, git, Utsav Shah Thanks, that makes a lot of sense. ce_uptodate doesn't have too many callers either, so modifying it and checking CE_FSMONITOR_VALID there should not be hard to audit. On Tue, Oct 29, 2019 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote: > > Utsav Shah <utsav@dropbox.com> writes: > > > Thanks for testing it out. The unpack_trees bugfix is especially useful. > > > > There's tons of places where we're using ce_uptodate(ce) that could be > > optimized by checking CE_FSMONITOR_VALID. One example is in > > run_diff_files in diff-lib.c > > > > Should we add a check for CE_FSMONITOR_VALID in all of them? Should we > > do that in this patch? Or should we take the time to refactor and > > flesh out bugs in unifying it with CE_UPTODATE? > > If we rephrase the first question slightly, i.e. "should these > places all be avoiding lstat() based check when fsmonitor says the > path is up to date?", I would imagine the answer is absolutely yes. > > I would further imagine that the implementation of the interface to > external fsmonitor itself may have to distinguish "we know/have known > this path is clean" vs "we just got told by fsmonitor that this path > is clean", so losing FSMONITOR_VALID bit might not be an easy or > clean conversion, in which case my earlier "can we perhaps lose it > and have fsmonitor interfacing code to directly set UPTODATE bit?" > would lead us in a wrong direction. > > But ce_uptodate(ce) being the primary way for the callers that care > about "is the path known to be up to date?", it is unsatisfying that > all of them have to ask > > if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID)) > ... process ce that is not up-to-date ... > > So I would say that the longer term goal should be to let them ask > ce_uptodate(ce) and have that macro automatically take FSMONITOR bit > into account (in other words, those who want to know if ce is fresh > should not have to even know about what fsmonitor is). > > Perhaps we can take a polished version of this "'reset --hard' can > and should notice paths known-to-be-uptodate via fsmonitor" as an > independent patch (to reduce the number of things we have to worry > by one) for now? Taking this patch means we would now have one more > place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if > we would be auditing all such places before we can decide what the > best way to reach the goal of allowing them to just say ce_uptodate() > without having to spell FSMONITOR_VALID, that probably is a cost > worth paying. > > Thanks for working on this topic. > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor 2019-10-30 16:41 ` Utsav Shah @ 2019-11-04 6:02 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2019-11-04 6:02 UTC (permalink / raw) To: Utsav Shah; +Cc: Kevin Willford, Utsav Shah via GitGitGadget, git, Utsav Shah Utsav Shah <utsav@dropbox.com> writes: [jc: we avoid top-posting for readability, so swapped paragraphs in this quote] >> Perhaps we can take a polished version of this "'reset --hard' can >> and should notice paths known-to-be-uptodate via fsmonitor" as an >> independent patch (to reduce the number of things we have to worry >> by one) for now? Taking this patch means we would now have one more >> place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if >> we would be auditing all such places before we can decide what the >> best way to reach the goal of allowing them to just say ce_uptodate() >> without having to spell FSMONITOR_VALID, that probably is a cost >> worth paying. >> >> Thanks for working on this topic. > > Thanks, that makes a lot of sense. ce_uptodate doesn't have too many > callers either, so modifying it and checking CE_FSMONITOR_VALID there > should not be hard to audit. OK, so let's see an updated and hopefully final version of [*1*], perhaps with Kevin's help you mentioned in [*2*] for now? [References] *1* <pull.424.git.1572017008.gitgitgadget@gmail.com> *2* <CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files 2019-10-25 15:23 [PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor Utsav Shah via GitGitGadget 2019-10-25 15:23 ` [PATCH 1/1] unpack-trees: skip lstat " Utsav Shah via GitGitGadget @ 2019-11-05 15:27 ` Utsav Shah via GitGitGadget 2019-11-05 15:27 ` [PATCH v2 1/1] " Utsav Shah via GitGitGadget 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget 1 sibling, 2 replies; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-05 15:27 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com] Utsav Shah (1): unpack-trees: skip stat on fsmonitor-valid files fsmonitor.c | 20 +++++++++++--------- t/t7113-post-index-change-hook.sh | 3 --- t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 4 files changed, 23 insertions(+), 15 deletions(-) base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/424 Range-diff vs v1: 1: 609c7c5047 ! 1: f76ba554ed unpack-trees: skip lstat based on fsmonitor @@ -1,24 +1,147 @@ Author: Utsav Shah <utsav@dropbox.com> - unpack-trees: skip lstat based on fsmonitor + unpack-trees: skip stat on fsmonitor-valid files - git stash runs git reset --hard as its final step, which can be fairly slow on a large repository. - This change lets us skip that if fsmonitor thinks those files aren't modified. + The index might be aware that a file hasn't modified via fsmonitor, but + unpack-trees did not pay attention to it and checked via ie_match_stat + which can be inefficient on certain filesystems. This significantly slows + down commands that run oneway_merge, like checkout and reset --hard. - git stash goes from ~8s -> 2s on my repository after this change. + This patch makes oneway_merge check whether a file is considered + unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees + also now correctly copies over fsmonitor validity state from the source + index. Finally, for correctness, we force a refresh of fsmonitor state in + tweak_fsmonitor. + + After this change, commands like stash (that use reset --hard + internally) go from 8s or more to ~2s on a 250k file repository on a + mac. Signed-off-by: Utsav Shah <utsav@dropbox.com> + diff --git a/fsmonitor.c b/fsmonitor.c + --- a/fsmonitor.c + +++ b/fsmonitor.c +@@ + } + istate->fsmonitor_dirty = fsmonitor_dirty; + +- if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) +- BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", +- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ++ if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) ++ BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", ++ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); ++ + + trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful"); + return 0; +@@ + uint32_t ewah_size = 0; + int fixup = 0; + +- if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) +- BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", +- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ++ if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) ++ BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", ++ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); + + put_be32(&hdr_version, INDEX_EXTENSION_VERSION); + strbuf_add(sb, &hdr_version, sizeof(uint32_t)); +@@ + } + if (bol < query_result.len) + fsmonitor_refresh_callback(istate, buf + bol); ++ ++ if (istate->untracked) ++ istate->untracked->use_fsmonitor = 1; + } else { + /* Mark all entries invalid */ + for (i = 0; i < istate->cache_nr; i++) +@@ + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); + +- /* Now mark the untracked cache for fsmonitor usage */ +- if (istate->untracked) +- istate->untracked->use_fsmonitor = 1; ++ refresh_fsmonitor(istate); + } + + ewah_free(istate->fsmonitor_dirty); + + diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh + --- a/t/t7113-post-index-change-hook.sh + +++ b/t/t7113-post-index-change-hook.sh +@@ + git checkout -- dir1/file1.txt && + test_path_is_file testsuccess && rm -f testsuccess && + test_path_is_missing testfailure && +- git update-index && +- test_path_is_missing testsuccess && +- test_path_is_missing testfailure && + git reset --soft && + test_path_is_missing testsuccess && + test_path_is_missing testfailure + + diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh + --- a/t/t7519-status-fsmonitor.sh + +++ b/t/t7519-status-fsmonitor.sh +@@ + + # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit + test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' ' ++ write_script .git/hooks/fsmonitor-test<<-\EOF && ++ EOF + git update-index --fsmonitor && + git update-index --fsmonitor-valid dir1/modified && + git update-index --fsmonitor-valid dir2/modified && +@@ + + # test that newly added files are marked valid + test_expect_success 'newly added files are marked valid' ' ++ write_script .git/hooks/fsmonitor-test<<-\EOF && ++ EOF + git add new && + git add dir1/new && + git add dir2/new && +@@ + # Ensure commands that call refresh_index() to move the index back in time + # properly invalidate the fsmonitor cache + test_expect_success 'refresh_index() invalidates fsmonitor cache' ' +- write_script .git/hooks/fsmonitor-test<<-\EOF && +- EOF + clean_repo && ++ write_integration_script && + dirty_repo && + git add . && ++ write_script .git/hooks/fsmonitor-test<<-\EOF && ++ EOF + git commit -m "to reset" && + git reset HEAD~1 && + git status >actual && + diff --git a/unpack-trees.c b/unpack-trees.c --- a/unpack-trees.c +++ b/unpack-trees.c @@ + o->merge_size = len; + mark_all_ce_unused(o->src_index); + ++ if (o->src_index->fsmonitor_last_update) ++ o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; ++ + /* + * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries + */ +@@ if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && -+ !(old->ce_flags & CE_FSMONITOR_VALID)) { ++ !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget @ 2019-11-05 15:27 ` Utsav Shah via GitGitGadget 2019-11-05 21:40 ` Kevin Willford 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget 1 sibling, 1 reply; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-05 15:27 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano, Utsav Shah From: Utsav Shah <utsav@dropbox.com> The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Signed-off-by: Utsav Shah <utsav@dropbox.com> --- fsmonitor.c | 20 +++++++++++--------- t/t7113-post-index-change-hook.sh | 3 --- t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index 1f4aa1b150..4362bc6ee9 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, } istate->fsmonitor_dirty = fsmonitor_dirty; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); + trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful"); return 0; @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) uint32_t ewah_size = 0; int fixup = 0; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); put_be32(&hdr_version, INDEX_EXTENSION_VERSION); strbuf_add(sb, &hdr_version, sizeof(uint32_t)); @@ -189,6 +190,9 @@ void refresh_fsmonitor(struct index_state *istate) } if (bol < query_result.len) fsmonitor_refresh_callback(istate, buf + bol); + + if (istate->untracked) + istate->untracked->use_fsmonitor = 1; } else { /* Mark all entries invalid */ for (i = 0; i < istate->cache_nr; i++) @@ -257,9 +261,7 @@ void tweak_fsmonitor(struct index_state *istate) (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); - /* Now mark the untracked cache for fsmonitor usage */ - if (istate->untracked) - istate->untracked->use_fsmonitor = 1; + refresh_fsmonitor(istate); } ewah_free(istate->fsmonitor_dirty); diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh index f011ad7eec..5ca2279d0d 100755 --- a/t/t7113-post-index-change-hook.sh +++ b/t/t7113-post-index-change-hook.sh @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags git checkout -- dir1/file1.txt && test_path_is_file testsuccess && rm -f testsuccess && test_path_is_missing testfailure && - git update-index && - test_path_is_missing testsuccess && - test_path_is_missing testfailure && git reset --soft && test_path_is_missing testsuccess && test_path_is_missing testfailure diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index d8df990972..9cac3d3d8e 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -106,6 +106,8 @@ EOF # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git update-index --fsmonitor && git update-index --fsmonitor-valid dir1/modified && git update-index --fsmonitor-valid dir2/modified && @@ -164,6 +166,8 @@ EOF # test that newly added files are marked valid test_expect_success 'newly added files are marked valid' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git add new && git add dir1/new && git add dir2/new && @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged # Ensure commands that call refresh_index() to move the index back in time # properly invalidate the fsmonitor cache test_expect_success 'refresh_index() invalidates fsmonitor cache' ' - write_script .git/hooks/fsmonitor-test<<-\EOF && - EOF clean_repo && + write_integration_script && dirty_repo && git add . && + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git commit -m "to reset" && git reset HEAD~1 && git status >actual && diff --git a/unpack-trees.c b/unpack-trees.c index 33ea7810d8..fc5ceb932c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->merge_size = len; mark_all_ce_unused(o->src_index); + if (o->src_index->fsmonitor_last_update) + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; + /* * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries */ @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && + !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-05 15:27 ` [PATCH v2 1/1] " Utsav Shah via GitGitGadget @ 2019-11-05 21:40 ` Kevin Willford 2019-11-06 4:36 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Kevin Willford @ 2019-11-05 21:40 UTC (permalink / raw) To: Utsav Shah via GitGitGadget, git; +Cc: Utsav Shah, Junio C Hamano, Utsav Shah > Sent: Tuesday, November 5, 2019 8:27 AM > From: Utsav Shah <utsav@dropbox.com> > > diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change- > hook.sh > index f011ad7eec..5ca2279d0d 100755 > --- a/t/t7113-post-index-change-hook.sh > +++ b/t/t7113-post-index-change-hook.sh > @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others > trigger hook without flags > git checkout -- dir1/file1.txt && > test_path_is_file testsuccess && rm -f testsuccess && > test_path_is_missing testfailure && > - git update-index && > - test_path_is_missing testsuccess && > - test_path_is_missing testfailure && > git reset --soft && > test_path_is_missing testsuccess && > test_path_is_missing testfailure Looking into this change and I wonder if instead we should be updating refresh_fsmonitor to only update istate->cache_changed if there was an entry where CE_FSMONITOR_VALID was turned off. The reason I bring this up is because with this change, the post-index-change hook will behave differently depending on fsmonitor. It will pass if GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set to fsmonitor-all it will fail because it is going down the code path that invalidates all the entries and sets istate->cache_changed. > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index > d8df990972..9cac3d3d8e 100755 > --- a/t/t7519-status-fsmonitor.sh > +++ b/t/t7519-status-fsmonitor.sh > @@ -106,6 +106,8 @@ EOF > > # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit > test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor > valid bit' ' > + write_script .git/hooks/fsmonitor-test<<-\EOF && > + EOF > git update-index --fsmonitor && > git update-index --fsmonitor-valid dir1/modified && > git update-index --fsmonitor-valid dir2/modified && @@ -164,6 > +166,8 @@ EOF > > # test that newly added files are marked valid test_expect_success 'newly > added files are marked valid' ' > + write_script .git/hooks/fsmonitor-test<<-\EOF && > + EOF > git add new && > git add dir1/new && > git add dir2/new && > @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the > integration script get flagged # Ensure commands that call refresh_index() to > move the index back in time # properly invalidate the fsmonitor cache > test_expect_success 'refresh_index() invalidates fsmonitor cache' ' > - write_script .git/hooks/fsmonitor-test<<-\EOF && > - EOF > clean_repo && > + write_integration_script && > dirty_repo && > git add . && > + write_script .git/hooks/fsmonitor-test<<-\EOF && > + EOF > git commit -m "to reset" && > git reset HEAD~1 && > git status >actual && We need to take a close look at all the tests in t7519-status-fsmonitor.sh and see if we are doing the right thing with these changes because before most commands that read the index would only apply the bits from the fsmonitor bitmap to the cache entries. Whereas now, it does that but also applies what the fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is now affecting tests and commands where it was not before. So if .git/hooks/fsmonitor-test has paths even git ls-files gets those paths marked dirty and that command is being used to validate the state of the CE_FSMONITOR_VALID. So I think in most cases for these tests we want the .git/hooks/fsmonitor-test to be empty before calling git ls-files so that doesn't change the index state. > > if (old && same(old, a)) { > int update = 0; > - if (o->reset && o->update && !ce_uptodate(old) && > !ce_skip_worktree(old)) { > + if (o->reset && o->update && !ce_uptodate(old) && > !ce_skip_worktree(old) && > + !(old->ce_flags & CE_FSMONITOR_VALID)) { > struct stat st; > if (lstat(old->name, &st) || > ie_match_stat(o->src_index, old, &st, > CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) FYI I have been testing with the ce_uptodate macro checking the CE_FSMONITOR_VALID flag instead and only have failures when using the fsmonitor-watchman script which I'm not sure if all the tests were ever passing using it. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-05 21:40 ` Kevin Willford @ 2019-11-06 4:36 ` Utsav Shah 2019-11-06 17:24 ` Kevin Willford 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-11-06 4:36 UTC (permalink / raw) To: Kevin Willford Cc: Utsav Shah via GitGitGadget, git, Utsav Shah, Junio C Hamano On Tue, Nov 5, 2019 at 1:40 PM Kevin Willford <Kevin.Willford@microsoft.com> wrote: > > > Sent: Tuesday, November 5, 2019 8:27 AM > > From: Utsav Shah <utsav@dropbox.com> > > > > diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change- > > hook.sh > > index f011ad7eec..5ca2279d0d 100755 > > --- a/t/t7113-post-index-change-hook.sh > > +++ b/t/t7113-post-index-change-hook.sh > > @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others > > trigger hook without flags > > git checkout -- dir1/file1.txt && > > test_path_is_file testsuccess && rm -f testsuccess && > > test_path_is_missing testfailure && > > - git update-index && > > - test_path_is_missing testsuccess && > > - test_path_is_missing testfailure && > > git reset --soft && > > test_path_is_missing testsuccess && > > test_path_is_missing testfailure > > Looking into this change and I wonder if instead we should be updating > refresh_fsmonitor to only update istate->cache_changed if there was an > entry where CE_FSMONITOR_VALID was turned off. > > The reason I bring this up is because with this change, the post-index-change > hook will behave differently depending on fsmonitor. It will pass if > GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set > to fsmonitor-all it will fail because it is going down the code path that > invalidates all the entries and sets istate->cache_changed. Thanks, this observation was correct. v3 of this patch will check if the index actually needs to mark its cache as changed, and this test passes without modification. > > > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index > > d8df990972..9cac3d3d8e 100755 > > --- a/t/t7519-status-fsmonitor.sh > > +++ b/t/t7519-status-fsmonitor.sh > > @@ -106,6 +106,8 @@ EOF > > > > # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit > > test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor > > valid bit' ' > > + write_script .git/hooks/fsmonitor-test<<-\EOF && > > + EOF > > git update-index --fsmonitor && > > git update-index --fsmonitor-valid dir1/modified && > > git update-index --fsmonitor-valid dir2/modified && @@ -164,6 > > +166,8 @@ EOF > > > > # test that newly added files are marked valid test_expect_success 'newly > > added files are marked valid' ' > > + write_script .git/hooks/fsmonitor-test<<-\EOF && > > + EOF > > git add new && > > git add dir1/new && > > git add dir2/new && > > @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the > > integration script get flagged # Ensure commands that call refresh_index() to > > move the index back in time # properly invalidate the fsmonitor cache > > test_expect_success 'refresh_index() invalidates fsmonitor cache' ' > > - write_script .git/hooks/fsmonitor-test<<-\EOF && > > - EOF > > clean_repo && > > + write_integration_script && > > dirty_repo && > > git add . && > > + write_script .git/hooks/fsmonitor-test<<-\EOF && > > + EOF > > git commit -m "to reset" && > > git reset HEAD~1 && > > git status >actual && > > We need to take a close look at all the tests in > t7519-status-fsmonitor.sh and see if we are doing the right thing with > these changes because before most commands that read the > index would only apply the bits from the fsmonitor bitmap to > the cache entries. Whereas now, it does that but also applies what the > fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is > now affecting tests and commands where it was not before. > > So if .git/hooks/fsmonitor-test has paths even git ls-files gets those > paths marked dirty and that command is being used to validate the state of > the CE_FSMONITOR_VALID. So I think in most cases for these tests we > want the .git/hooks/fsmonitor-test to be empty before calling git ls-files > so that doesn't change the index state. I audited these tests very closely, and to the best of my knowledge, the modifications made are valid. For test failures of test_expect_success 'update-index --fsmonitor-valid sets the fsmonitor valid bit' test_expect_success 'newly added files are marked valid' It's relatively straightforward that our patch now runs the fsmonitor hook so we need to make sure the hook doesn't return anything. The trickiest case was "refresh_index()" test, and I've made a slight change to make it clearer why that test was failing. @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged # Ensure commands that call refresh_index() to move the index back in time # properly invalidate the fsmonitor cache test_expect_success 'refresh_index() invalidates fsmonitor cache' ' - write_script .git/hooks/fsmonitor-test<<-\EOF && - EOF clean_repo && dirty_repo && + write_integration_script && git add . && + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git commit -m "to reset" && git reset HEAD~1 && git status >actual && With patch v2, git add was failing to add all files, since it now consults the fsmonitor hook which wrongly implied that no files were modified. This was rectified by the write_integration_script. After that, we immediately ensure that the test fsmonitor doesn't return any files, and the test passes. > > > > > if (old && same(old, a)) { > > int update = 0; > > - if (o->reset && o->update && !ce_uptodate(old) && > > !ce_skip_worktree(old)) { > > + if (o->reset && o->update && !ce_uptodate(old) && > > !ce_skip_worktree(old) && > > + !(old->ce_flags & CE_FSMONITOR_VALID)) { > > struct stat st; > > if (lstat(old->name, &st) || > > ie_match_stat(o->src_index, old, &st, > > CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) > > FYI I have been testing with the ce_uptodate macro checking the > CE_FSMONITOR_VALID flag instead and only have failures when using > the fsmonitor-watchman script which I'm not sure if all the tests were > ever passing using it. > Yeah, I see the same results. The one part that I don't fully understand if safe is copying over the o->src_index->fsmonitor_last_update to the result index in unpack-trees. I don't understand the implications of that, and if that's the only field worth copying over, or if we should be copying over other fields like the bitmap as well. ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 4:36 ` Utsav Shah @ 2019-11-06 17:24 ` Kevin Willford 0 siblings, 0 replies; 26+ messages in thread From: Kevin Willford @ 2019-11-06 17:24 UTC (permalink / raw) To: Utsav Shah; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah, Junio C Hamano > From: Utsav Shah <utsav@dropbox.com> > Sent: Tuesday, November 5, 2019 9:36 PM > > The one part that I don't fully understand if safe is copying over the > o->src_index->fsmonitor_last_update to the result index in > unpack-trees. I don't understand the implications of that, and if that's the > only field worth copying over, or if we should be copying over other fields > like the bitmap as well. The data from the bitmap has already been applied to the index entries at this point so it is no longer needed after that. fsmonitor_last_update is really just being used as the flag to fill the bitmap and write out the fsmonitor extension. The fill_fsmonitor_bitmap will use the current CE_FSMONITOR_VALID flags for the index entries to create the bitmap. if (istate->fsmonitor_last_update) fill_fsmonitor_bitmap(istate); void fill_fsmonitor_bitmap(struct index_state *istate) { unsigned int i, skipped = 0; istate->fsmonitor_dirty = ewah_new(); for (i = 0; i < istate->cache_nr; i++) { if (istate->cache[i]->ce_flags & CE_REMOVE) skipped++; else if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)) ewah_set(istate->fsmonitor_dirty, i - skipped); } } if (!strip_extensions && istate->fsmonitor_last_update) { struct strbuf sb = STRBUF_INIT; write_fsmonitor_extension(&sb, istate); err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0 || ce_write(&c, newfd, sb.buf, sb.len) < 0; strbuf_release(&sb); if (err) return -1; } ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget 2019-11-05 15:27 ` [PATCH v2 1/1] " Utsav Shah via GitGitGadget @ 2019-11-06 4:54 ` Utsav Shah via GitGitGadget 2019-11-06 4:54 ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget ` (2 more replies) 1 sibling, 3 replies; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-06 4:54 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com] Utsav Shah (1): unpack-trees: skip stat on fsmonitor-valid files fsmonitor.c | 39 ++++++++++++++++++++++++------------- t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 3 files changed, 37 insertions(+), 17 deletions(-) base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/424 Range-diff vs v2: 1: f76ba554ed ! 1: 4bea7075cf unpack-trees: skip stat on fsmonitor-valid files @@ -23,6 +23,15 @@ --- a/fsmonitor.c +++ b/fsmonitor.c @@ + + if (pos >= istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", +- (uintmax_t)pos, istate->cache_nr); ++ (uintmax_t)pos, istate->cache_nr); + + ce = istate->cache[pos]; + ce->ce_flags &= ~CE_FSMONITOR_VALID; +@@ } istate->fsmonitor_dirty = fsmonitor_dirty; @@ -31,7 +40,7 @@ - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", -+ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); ++ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); + trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful"); @@ -45,7 +54,7 @@ - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", -+ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); ++ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); put_be32(&hdr_version, INDEX_EXTENSION_VERSION); strbuf_add(sb, &hdr_version, sizeof(uint32_t)); @@ -57,10 +66,33 @@ + if (istate->untracked) + istate->untracked->use_fsmonitor = 1; } else { ++ ++ /* We only want to run the post index changed hook if we've actually changed entries, so keep track ++ * if we actually changed entries or not */ ++ int is_cache_changed = 0; /* Mark all entries invalid */ - for (i = 0; i < istate->cache_nr; i++) +- for (i = 0; i < istate->cache_nr; i++) +- istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; ++ for (i = 0; i < istate->cache_nr; i++) { ++ if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { ++ is_cache_changed = 1; ++ istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; ++ } ++ } + + /* If we're going to check every file, ensure we save the results */ +- istate->cache_changed |= FSMONITOR_CHANGED; ++ if (is_cache_changed) ++ istate->cache_changed |= FSMONITOR_CHANGED; + + if (istate->untracked) + istate->untracked->use_fsmonitor = 0; @@ - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + /* Mark all previously saved entries as dirty */ + if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", +- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ++ (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); - /* Now mark the untracked cache for fsmonitor usage */ @@ -71,20 +103,6 @@ ewah_free(istate->fsmonitor_dirty); - diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-hook.sh - --- a/t/t7113-post-index-change-hook.sh - +++ b/t/t7113-post-index-change-hook.sh -@@ - git checkout -- dir1/file1.txt && - test_path_is_file testsuccess && rm -f testsuccess && - test_path_is_missing testfailure && -- git update-index && -- test_path_is_missing testsuccess && -- test_path_is_missing testfailure && - git reset --soft && - test_path_is_missing testsuccess && - test_path_is_missing testfailure - diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -113,8 +131,8 @@ - write_script .git/hooks/fsmonitor-test<<-\EOF && - EOF clean_repo && -+ write_integration_script && dirty_repo && ++ write_integration_script && git add . && + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget @ 2019-11-06 4:54 ` Utsav Shah via GitGitGadget 2019-11-06 10:46 ` Junio C Hamano 2019-11-06 10:16 ` [PATCH v3 0/1] " Junio C Hamano 2019-11-20 8:32 ` [PATCH v4 " Utsav Shah via GitGitGadget 2 siblings, 1 reply; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-06 4:54 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano, Utsav Shah From: Utsav Shah <utsav@dropbox.com> The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Signed-off-by: Utsav Shah <utsav@dropbox.com> --- fsmonitor.c | 39 ++++++++++++++++++++++++------------- t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index 1f4aa1b150..04d6232531 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) if (pos >= istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", - (uintmax_t)pos, istate->cache_nr); + (uintmax_t)pos, istate->cache_nr); ce = istate->cache[pos]; ce->ce_flags &= ~CE_FSMONITOR_VALID; @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, } istate->fsmonitor_dirty = fsmonitor_dirty; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); + trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful"); return 0; @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) uint32_t ewah_size = 0; int fixup = 0; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); put_be32(&hdr_version, INDEX_EXTENSION_VERSION); strbuf_add(sb, &hdr_version, sizeof(uint32_t)); @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate) } if (bol < query_result.len) fsmonitor_refresh_callback(istate, buf + bol); + + if (istate->untracked) + istate->untracked->use_fsmonitor = 1; } else { + + /* We only want to run the post index changed hook if we've actually changed entries, so keep track + * if we actually changed entries or not */ + int is_cache_changed = 0; /* Mark all entries invalid */ - for (i = 0; i < istate->cache_nr; i++) - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; + for (i = 0; i < istate->cache_nr; i++) { + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { + is_cache_changed = 1; + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; + } + } /* If we're going to check every file, ensure we save the results */ - istate->cache_changed |= FSMONITOR_CHANGED; + if (is_cache_changed) + istate->cache_changed |= FSMONITOR_CHANGED; if (istate->untracked) istate->untracked->use_fsmonitor = 0; @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate) /* Mark all previously saved entries as dirty */ if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); - /* Now mark the untracked cache for fsmonitor usage */ - if (istate->untracked) - istate->untracked->use_fsmonitor = 1; + refresh_fsmonitor(istate); } ewah_free(istate->fsmonitor_dirty); diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index d8df990972..69908b6a9b 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -106,6 +106,8 @@ EOF # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git update-index --fsmonitor && git update-index --fsmonitor-valid dir1/modified && git update-index --fsmonitor-valid dir2/modified && @@ -164,6 +166,8 @@ EOF # test that newly added files are marked valid test_expect_success 'newly added files are marked valid' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git add new && git add dir1/new && git add dir2/new && @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged # Ensure commands that call refresh_index() to move the index back in time # properly invalidate the fsmonitor cache test_expect_success 'refresh_index() invalidates fsmonitor cache' ' - write_script .git/hooks/fsmonitor-test<<-\EOF && - EOF clean_repo && dirty_repo && + write_integration_script && git add . && + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git commit -m "to reset" && git reset HEAD~1 && git status >actual && diff --git a/unpack-trees.c b/unpack-trees.c index 33ea7810d8..fc5ceb932c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->merge_size = len; mark_all_ce_unused(o->src_index); + if (o->src_index->fsmonitor_last_update) + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; + /* * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries */ @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && + !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 4:54 ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget @ 2019-11-06 10:46 ` Junio C Hamano 2019-11-06 22:33 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2019-11-06 10:46 UTC (permalink / raw) To: Utsav Shah via GitGitGadget; +Cc: git, Utsav Shah, Utsav Shah "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Utsav Shah <utsav@dropbox.com> > > The index might be aware that a file hasn't modified via fsmonitor, but > unpack-trees did not pay attention to it and checked via ie_match_stat > which can be inefficient on certain filesystems. This significantly slows > down commands that run oneway_merge, like checkout and reset --hard. s/hasn't/& been/; Otherwise, well written. > This patch makes oneway_merge check whether a file is considered > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees > also now correctly copies over fsmonitor validity state from the source > index. Finally, for correctness, we force a refresh of fsmonitor state in > tweak_fsmonitor. s/This patch makes/Make/; order the person who is updating the code what to do to the codebase in imperative mood. Otherwise, well written. > After this change, commands like stash (that use reset --hard > internally) go from 8s or more to ~2s on a 250k file repository on a > mac. > > Signed-off-by: Utsav Shah <utsav@dropbox.com> > --- > fsmonitor.c | 39 ++++++++++++++++++++++++------------- > t/t7519-status-fsmonitor.sh | 9 +++++++-- > unpack-trees.c | 6 +++++- > 3 files changed, 37 insertions(+), 17 deletions(-) > > diff --git a/fsmonitor.c b/fsmonitor.c > index 1f4aa1b150..04d6232531 100644 > --- a/fsmonitor.c > +++ b/fsmonitor.c > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) > > if (pos >= istate->cache_nr) > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", > - (uintmax_t)pos, istate->cache_nr); > + (uintmax_t)pos, istate->cache_nr); Unintended whitespace change? > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, > } > istate->fsmonitor_dirty = fsmonitor_dirty; > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); > + The patch disables this sanity check under "split index" mode and it must be done for good reasons, but readers (imagine, somebody found a bug on this line 6 months down the road, ran "git blame" and found this commit and reading it via "git show") would want to know why this change was made. I recall seeing no mention of "split index" in the proposed log message. Is this a fix for unrelated issue that needs to be explained in a separate patch, perhaps? The hunk also has the unintended whitespace change, it seems. > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) > uint32_t ewah_size = 0; > int fixup = 0; > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); Likewise (both indentation of the second line and the unexplained change to the sanity check condition we saw above). > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate) > } > if (bol < query_result.len) > fsmonitor_refresh_callback(istate, buf + bol); > + > + if (istate->untracked) > + istate->untracked->use_fsmonitor = 1; Unexplained. We used to do this in tweak_fsmonitor() but now we do this here, as we are making tweak_fsmonitor() to call this function anyway. If there are other callers that call refresh_fsmonitor() and they did not do this, this would be a behaviour change to them. As there is no explanation why this change is done, readers cannot tell if it is a good change. If this were explained like so: Any caller of refresh_fsmonitor() must set use_fsmonitor bit in istate when istate->untracked exists FOR SUCH AND SUCH REASONS. Move the code to do so in tweak_fsmonitor() to near the beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH other callers that forgets to do this. in the proposed log message, that might help justifying the change. If use_fsmonitor is not set, why is the caller calling refresh_fsmonitor() in the first place, by the way? Isn't it more like "we are told to use fsmonitor via istate->untracked->use_fsmonitor bit being true, so we call refresh_fsmonitor, and if the bit is false, we do not have to bother with fsmonitor and no point calling refresh_fsmonitor"? If a careless caller makes a call to refresh_fsmonitor() when the configuration tells us not to use fsmonitor, wouldn't this cause us to use fsmonitor anyway? Which sounds bad, so perhaps all callers are careful to first check if use_fsmonitor is set before deciding to call refresh_fsmonitor()---but if that is the case, is there a point in setting it here to true? Puzzled by an unexplained code... > } else { > + > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track > + * if we actually changed entries or not */ > + int is_cache_changed = 0; > /* Mark all entries invalid */ > - for (i = 0; i < istate->cache_nr; i++) > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > + for (i = 0; i < istate->cache_nr; i++) { > + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { > + is_cache_changed = 1; > + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > + } > + } > > /* If we're going to check every file, ensure we save the results */ > - istate->cache_changed |= FSMONITOR_CHANGED; > + if (is_cache_changed) > + istate->cache_changed |= FSMONITOR_CHANGED; This part (and a call to refresh_fsmonitor() we see blow) is the "Finally, we force a refresh" explained in the proposed log message, I presume. > if (istate->untracked) > istate->untracked->use_fsmonitor = 0; > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate) > /* Mark all previously saved entries as dirty */ > if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); This shares the same indentation issue but does not disable the sanity check for split index case. Intended? Without explanation in the proposed log message, readers cannot tell. > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); > > - /* Now mark the untracked cache for fsmonitor usage */ > - if (istate->untracked) > - istate->untracked->use_fsmonitor = 1; > + refresh_fsmonitor(istate); > } > > ewah_free(istate->fsmonitor_dirty); > diff --git a/unpack-trees.c b/unpack-trees.c > index 33ea7810d8..fc5ceb932c 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options > o->merge_size = len; > mark_all_ce_unused(o->src_index); > > + if (o->src_index->fsmonitor_last_update) > + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; > + This is the "correctly copies" part, which was well explained. > /* > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries > */ > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, > > if (old && same(old, a)) { > int update = 0; > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && > + !(old->ce_flags & CE_FSMONITOR_VALID)) { This is the "skip when we know it is valid" part, which was well explained. > struct stat st; > if (lstat(old->name, &st) || > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 10:46 ` Junio C Hamano @ 2019-11-06 22:33 ` Utsav Shah 2019-11-08 3:51 ` Utsav Shah 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-11-06 22:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Utsav Shah <utsav@dropbox.com> > > > > The index might be aware that a file hasn't modified via fsmonitor, but > > unpack-trees did not pay attention to it and checked via ie_match_stat > > which can be inefficient on certain filesystems. This significantly slows > > down commands that run oneway_merge, like checkout and reset --hard. > > s/hasn't/& been/; > > Otherwise, well written. > > > This patch makes oneway_merge check whether a file is considered > > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees > > also now correctly copies over fsmonitor validity state from the source > > index. Finally, for correctness, we force a refresh of fsmonitor state in > > tweak_fsmonitor. > > s/This patch makes/Make/; order the person who is updating the code > what to do to the codebase in imperative mood. > > Otherwise, well written. > > > After this change, commands like stash (that use reset --hard > > internally) go from 8s or more to ~2s on a 250k file repository on a > > mac. > > > > Signed-off-by: Utsav Shah <utsav@dropbox.com> > > --- > > fsmonitor.c | 39 ++++++++++++++++++++++++------------- > > t/t7519-status-fsmonitor.sh | 9 +++++++-- > > unpack-trees.c | 6 +++++- > > 3 files changed, 37 insertions(+), 17 deletions(-) > > > > diff --git a/fsmonitor.c b/fsmonitor.c > > index 1f4aa1b150..04d6232531 100644 > > --- a/fsmonitor.c > > +++ b/fsmonitor.c > > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) > > > > if (pos >= istate->cache_nr) > > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", > > - (uintmax_t)pos, istate->cache_nr); > > + (uintmax_t)pos, istate->cache_nr); > > Unintended whitespace change? > > > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, > > } > > istate->fsmonitor_dirty = fsmonitor_dirty; > > > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); > > + > > The patch disables this sanity check under "split index" mode and it > must be done for good reasons, but readers (imagine, somebody found > a bug on this line 6 months down the road, ran "git blame" and found > this commit and reading it via "git show") would want to know why > this change was made. > > I recall seeing no mention of "split index" in the proposed log > message. Is this a fix for unrelated issue that needs to be > explained in a separate patch, perhaps? > > The hunk also has the unintended whitespace change, it seems. > > > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) > > uint32_t ewah_size = 0; > > int fixup = 0; > > > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); > > Likewise (both indentation of the second line and the unexplained > change to the sanity check condition we saw above). > > > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate) > > } > > if (bol < query_result.len) > > fsmonitor_refresh_callback(istate, buf + bol); > > + > > + if (istate->untracked) > > + istate->untracked->use_fsmonitor = 1; > > Unexplained. We used to do this in tweak_fsmonitor() but now we do > this here, as we are making tweak_fsmonitor() to call this function > anyway. If there are other callers that call refresh_fsmonitor() > and they did not do this, this would be a behaviour change to them. > As there is no explanation why this change is done, readers cannot > tell if it is a good change. If this were explained like so: > > Any caller of refresh_fsmonitor() must set use_fsmonitor bit in > istate when istate->untracked exists FOR SUCH AND SUCH REASONS. > Move the code to do so in tweak_fsmonitor() to near the > beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH > other callers that forgets to do this. > > in the proposed log message, that might help justifying the change. > > If use_fsmonitor is not set, why is the caller calling > refresh_fsmonitor() in the first place, by the way? > > Isn't it more like "we are told to use fsmonitor via > istate->untracked->use_fsmonitor bit being true, so we call > refresh_fsmonitor, and if the bit is false, we do not have to bother > with fsmonitor and no point calling refresh_fsmonitor"? > > If a careless caller makes a call to refresh_fsmonitor() when the > configuration tells us not to use fsmonitor, wouldn't this cause us > to use fsmonitor anyway? Which sounds bad, so perhaps all callers > are careful to first check if use_fsmonitor is set before deciding > to call refresh_fsmonitor()---but if that is the case, is there a > point in setting it here to true? > > Puzzled by an unexplained code... > > > } else { > > + > > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track > > + * if we actually changed entries or not */ > > + int is_cache_changed = 0; > > /* Mark all entries invalid */ > > - for (i = 0; i < istate->cache_nr; i++) > > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > > + for (i = 0; i < istate->cache_nr; i++) { > > + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { > > + is_cache_changed = 1; > > + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > > + } > > + } > > > > /* If we're going to check every file, ensure we save the results */ > > - istate->cache_changed |= FSMONITOR_CHANGED; > > + if (is_cache_changed) > > + istate->cache_changed |= FSMONITOR_CHANGED; > > This part (and a call to refresh_fsmonitor() we see blow) is the > "Finally, we force a refresh" explained in the proposed log message, > I presume. > > > if (istate->untracked) > > istate->untracked->use_fsmonitor = 0; > > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate) > > /* Mark all previously saved entries as dirty */ > > if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > This shares the same indentation issue but does not disable the > sanity check for split index case. Intended? Without explanation > in the proposed log message, readers cannot tell. > > > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); > > > > - /* Now mark the untracked cache for fsmonitor usage */ > > - if (istate->untracked) > > - istate->untracked->use_fsmonitor = 1; > > + refresh_fsmonitor(istate); > > } > > > > ewah_free(istate->fsmonitor_dirty); > > diff --git a/unpack-trees.c b/unpack-trees.c > > index 33ea7810d8..fc5ceb932c 100644 > > --- a/unpack-trees.c > > +++ b/unpack-trees.c > > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options > > o->merge_size = len; > > mark_all_ce_unused(o->src_index); > > > > + if (o->src_index->fsmonitor_last_update) > > + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; > > + > > This is the "correctly copies" part, which was well explained. > > > /* > > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries > > */ > > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, > > > > if (old && same(old, a)) { > > int update = 0; > > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { > > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && > > + !(old->ce_flags & CE_FSMONITOR_VALID)) { > > This is the "skip when we know it is valid" part, which was well > explained. > > > struct stat st; > > if (lstat(old->name, &st) || > > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) > > Thanks. Thanks for the feedback. The lines with the indentation changes had tabs and spaces mixed up, but I'll revert those changes. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 22:33 ` Utsav Shah @ 2019-11-08 3:51 ` Utsav Shah 2019-11-08 4:11 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah @ 2019-11-08 3:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah On Wed, Nov 6, 2019 at 2:33 PM Utsav Shah <utsav@dropbox.com> wrote: > > On Wed, Nov 6, 2019 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > > > From: Utsav Shah <utsav@dropbox.com> > > > > > > The index might be aware that a file hasn't modified via fsmonitor, but > > > unpack-trees did not pay attention to it and checked via ie_match_stat > > > which can be inefficient on certain filesystems. This significantly slows > > > down commands that run oneway_merge, like checkout and reset --hard. > > > > s/hasn't/& been/; > > > > Otherwise, well written. > > > > > This patch makes oneway_merge check whether a file is considered > > > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees > > > also now correctly copies over fsmonitor validity state from the source > > > index. Finally, for correctness, we force a refresh of fsmonitor state in > > > tweak_fsmonitor. > > > > s/This patch makes/Make/; order the person who is updating the code > > what to do to the codebase in imperative mood. > > > > Otherwise, well written. > > > > > After this change, commands like stash (that use reset --hard > > > internally) go from 8s or more to ~2s on a 250k file repository on a > > > mac. > > > > > > Signed-off-by: Utsav Shah <utsav@dropbox.com> > > > --- > > > fsmonitor.c | 39 ++++++++++++++++++++++++------------- > > > t/t7519-status-fsmonitor.sh | 9 +++++++-- > > > unpack-trees.c | 6 +++++- > > > 3 files changed, 37 insertions(+), 17 deletions(-) > > > > > > diff --git a/fsmonitor.c b/fsmonitor.c > > > index 1f4aa1b150..04d6232531 100644 > > > --- a/fsmonitor.c > > > +++ b/fsmonitor.c > > > @@ -18,7 +18,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) > > > > > > if (pos >= istate->cache_nr) > > > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", > > > - (uintmax_t)pos, istate->cache_nr); > > > + (uintmax_t)pos, istate->cache_nr); > > > > Unintended whitespace change? > > > > > @@ -55,9 +55,10 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, > > > } > > > istate->fsmonitor_dirty = fsmonitor_dirty; > > > > > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); > > > + > > > > The patch disables this sanity check under "split index" mode and it > > must be done for good reasons, but readers (imagine, somebody found > > a bug on this line 6 months down the road, ran "git blame" and found > > this commit and reading it via "git show") would want to know why > > this change was made. > > > > I recall seeing no mention of "split index" in the proposed log > > message. Is this a fix for unrelated issue that needs to be > > explained in a separate patch, perhaps? Yes. I think this is an important bug to fix, fsmonitor and split indices don't seem to be interoperating well after these checks have been added. > > > > The hunk also has the unintended whitespace change, it seems. > > > > > @@ -83,9 +84,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) > > > uint32_t ewah_size = 0; > > > int fixup = 0; > > > > > > - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > > - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > > + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > > + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", > > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); > > > > Likewise (both indentation of the second line and the unexplained > > change to the sanity check condition we saw above). > > > > > @@ -189,13 +190,25 @@ void refresh_fsmonitor(struct index_state *istate) > > > } > > > if (bol < query_result.len) > > > fsmonitor_refresh_callback(istate, buf + bol); > > > + > > > + if (istate->untracked) > > > + istate->untracked->use_fsmonitor = 1; > > > > Unexplained. We used to do this in tweak_fsmonitor() but now we do > > this here, as we are making tweak_fsmonitor() to call this function > > anyway. If there are other callers that call refresh_fsmonitor() > > and they did not do this, this would be a behaviour change to them. > > As there is no explanation why this change is done, readers cannot > > tell if it is a good change. If this were explained like so: > > > > Any caller of refresh_fsmonitor() must set use_fsmonitor bit in > > istate when istate->untracked exists FOR SUCH AND SUCH REASONS. > > Move the code to do so in tweak_fsmonitor() to near the > > beginning of refresh_fsmonitor(), which would fix SUCH AND SUCH > > other callers that forgets to do this. > > > > in the proposed log message, that might help justifying the change. > > > > If use_fsmonitor is not set, why is the caller calling > > refresh_fsmonitor() in the first place, by the way? > > > > Isn't it more like "we are told to use fsmonitor via > > istate->untracked->use_fsmonitor bit being true, so we call > > refresh_fsmonitor, and if the bit is false, we do not have to bother > > with fsmonitor and no point calling refresh_fsmonitor"? > > > > If a careless caller makes a call to refresh_fsmonitor() when the > > configuration tells us not to use fsmonitor, wouldn't this cause us > > to use fsmonitor anyway? Which sounds bad, so perhaps all callers > > are careful to first check if use_fsmonitor is set before deciding > > to call refresh_fsmonitor()---but if that is the case, is there a > > point in setting it here to true? > > > > Puzzled by an unexplained code... I might be misunderstanding, but wouldn't the if condition above make sure we don't enter this codepath at all? if (!core_fsmonitor || istate->fsmonitor_has_run_once) return; istate->untracked->use_fsmonitor is only used in the untracked cache to skip some lstats AFAICT. So callers refresh_fsmonitor should see no difference. I agree that this change deserves a better explanation. > > > > > } else { > > > + > > > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track > > > + * if we actually changed entries or not */ > > > + int is_cache_changed = 0; > > > /* Mark all entries invalid */ > > > - for (i = 0; i < istate->cache_nr; i++) > > > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > > > + for (i = 0; i < istate->cache_nr; i++) { > > > + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { > > > + is_cache_changed = 1; > > > + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > > > + } > > > + } > > > > > > /* If we're going to check every file, ensure we save the results */ > > > - istate->cache_changed |= FSMONITOR_CHANGED; > > > + if (is_cache_changed) > > > + istate->cache_changed |= FSMONITOR_CHANGED; > > > > This part (and a call to refresh_fsmonitor() we see blow) is the > > "Finally, we force a refresh" explained in the proposed log message, > > I presume. > > > > > if (istate->untracked) > > > istate->untracked->use_fsmonitor = 0; > > > @@ -254,12 +267,10 @@ void tweak_fsmonitor(struct index_state *istate) > > > /* Mark all previously saved entries as dirty */ > > > if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) > > > BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", > > > - (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > > + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > > > > This shares the same indentation issue but does not disable the > > sanity check for split index case. Intended? Without explanation > > in the proposed log message, readers cannot tell. Yes, will add the sanity check here too. > > > > > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); > > > > > > - /* Now mark the untracked cache for fsmonitor usage */ > > > - if (istate->untracked) > > > - istate->untracked->use_fsmonitor = 1; > > > + refresh_fsmonitor(istate); > > > } > > > > > > ewah_free(istate->fsmonitor_dirty); > > > diff --git a/unpack-trees.c b/unpack-trees.c > > > index 33ea7810d8..fc5ceb932c 100644 > > > --- a/unpack-trees.c > > > +++ b/unpack-trees.c > > > @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options > > > o->merge_size = len; > > > mark_all_ce_unused(o->src_index); > > > > > > + if (o->src_index->fsmonitor_last_update) > > > + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; > > > + > > > > This is the "correctly copies" part, which was well explained. > > > > > /* > > > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries > > > */ > > > @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, > > > > > > if (old && same(old, a)) { > > > int update = 0; > > > - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { > > > + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && > > > + !(old->ce_flags & CE_FSMONITOR_VALID)) { > > > > This is the "skip when we know it is valid" part, which was well > > explained. > > > > > struct stat st; > > > if (lstat(old->name, &st) || > > > ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) > > > > Thanks. > > Thanks for the feedback. The lines with the indentation changes had > tabs and spaces mixed up, but I'll revert those changes. I will try to figure out how to use gitgitgadget to send a smaller patch within this thread to fix the fsmonitor and split index interactions. I tried writing a test today that uses t7519/fsmonitor-watchman to simulate the bug, but it is flaky. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-08 3:51 ` Utsav Shah @ 2019-11-08 4:11 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2019-11-08 4:11 UTC (permalink / raw) To: Utsav Shah; +Cc: Utsav Shah via GitGitGadget, git, Utsav Shah Utsav Shah <utsav@dropbox.com> writes: > I will try to figure out how to use gitgitgadget to send a smaller > patch within this thread to fix the fsmonitor and split index > interactions. If the current one is trying to handle two issues, then you'd need to reduce this one and add another spearate topic, probably. > I tried writing a test today that uses > t7519/fsmonitor-watchman to simulate the bug, but it is flaky. Are you aware of Kevin Willford's recent work? <52eaf20f405b53743e27935dfe89f62e6f824a33.1572889841.git.gitgitgadget@gmail.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget 2019-11-06 4:54 ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget @ 2019-11-06 10:16 ` Junio C Hamano 2019-11-20 8:32 ` [PATCH v4 " Utsav Shah via GitGitGadget 2 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2019-11-06 10:16 UTC (permalink / raw) To: Utsav Shah via GitGitGadget; +Cc: git, Utsav Shah "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > The index might be aware that a file hasn't modified via fsmonitor, but > unpack-trees did not pay attention to it and checked via ie_match_stat which > can be inefficient on certain filesystems. This significantly slows down > commands that run oneway_merge, like checkout and reset --hard. > > This patch makes oneway_merge check whether a file is considered unchanged > through fsmonitor and skips ie_match_stat on it. unpack-trees also now > correctly copies over fsmonitor validity state from the source index. > Finally, for correctness, we force a refresh of fsmonitor state in > tweak_fsmonitor. > > After this change, commands like stash (that use reset --hard internally) go > from 8s or more to ~2s on a 250k file repository on a mac. If the above is verbatim copy of what you wrote in 1/1, that is very much unappreciated X-<. A cover letter for a topic serves primarily just one purpose: It is a good place to present a birds-eye-view of a multi-patch topic; a high level description of the problem (e.g. how the issue manifests to the end users), an explanation of division of the problem into subproblems you made (if applicable), and interesting highlights of the solution would all be good things to have in there. And as a topic goes through iterations, it gives you a good place to summarize what changed since the previously reviewed iterations. It could be just a single liner "addressed all the review comments for the previous iteration". A well-written multi-patch topic also uses the same after-three-dash technique used for a single-patch topic (see below) to summarize what changed since the corresponding patch in the series in the previous iteration (or just says "no changes since the previous round"---that helps the reviewers a lot). For a single-patch topic, there is no place for "here is an overall birds-eyes-view picture because the changes described in the proposed log message of individual patches are so big and complex". A single-patch topic has one patch, that solves one problem and only that problem well. When you want to summarize the changes since the previous iteration, you would write it between the three-dash-line (which appears after your sign-off) and the diffstat. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 0/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget 2019-11-06 4:54 ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget 2019-11-06 10:16 ` [PATCH v3 0/1] " Junio C Hamano @ 2019-11-20 8:32 ` Utsav Shah via GitGitGadget 2019-11-20 8:32 ` [PATCH v4 1/1] " Utsav Shah via GitGitGadget 2 siblings, 1 reply; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-20 8:32 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Changes since the last version are: * The sanity checks around accessing the fsmonitor_dirty bitmap have been moved to another patch, which is in message id [1] * Unintended indentation changes in fsmonitor have been removed * A comment explaining what untracked->use_fsmonitor means has been re-added (it was dropped in the previous version) * A few "helped-by" entries have been added to the patch 1: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com) Helped-by: Junio C Hamano gitster@pobox.com [gitster@pobox.com]Helped-by: Kevin Willford Kevin.Willford@microsoft.com [Kevin.Willford@microsoft.com] Signed-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com] Utsav Shah (1): unpack-trees: skip stat on fsmonitor-valid files fsmonitor.c | 23 +++++++++++++++++------ t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 3 files changed, 29 insertions(+), 9 deletions(-) base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-424%2FUtsav2%2Fskip-lstat-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-424/Utsav2/skip-lstat-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/424 Range-diff vs v3: 1: 4bea7075cf ! 1: ea7880f2d0 unpack-trees: skip stat on fsmonitor-valid files @@ -17,52 +17,29 @@ internally) go from 8s or more to ~2s on a 250k file repository on a mac. + Changes since the last version are: + * The sanity checks around accessing the fsmonitor_dirty bitmap have + been moved to another patch, which is in message id [1] + * Unintended indentation changes in fsmonitor have been removed + * A comment explaining what untracked->use_fsmonitor means has been + re-added (it was dropped in the previous version) + * A few "helped-by" entries have been added to the patch + + [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com) + + Helped-by: Junio C Hamano <gitster@pobox.com> + Helped-by: Kevin Willford <Kevin.Willford@microsoft.com> Signed-off-by: Utsav Shah <utsav@dropbox.com> diff --git a/fsmonitor.c b/fsmonitor.c --- a/fsmonitor.c +++ b/fsmonitor.c @@ - - if (pos >= istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", -- (uintmax_t)pos, istate->cache_nr); -+ (uintmax_t)pos, istate->cache_nr); - - ce = istate->cache[pos]; - ce->ce_flags &= ~CE_FSMONITOR_VALID; -@@ - } - istate->fsmonitor_dirty = fsmonitor_dirty; - -- if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) -- BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", -- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); -+ if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) -+ BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", -+ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); -+ - - trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful"); - return 0; -@@ - uint32_t ewah_size = 0; - int fixup = 0; - -- if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) -- BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", -- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); -+ if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) -+ BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")", -+ (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr); - - put_be32(&hdr_version, INDEX_EXTENSION_VERSION); - strbuf_add(sb, &hdr_version, sizeof(uint32_t)); -@@ } if (bol < query_result.len) fsmonitor_refresh_callback(istate, buf + bol); + ++ /* Now mark the untracked cache for fsmonitor usage */ + if (istate->untracked) + istate->untracked->use_fsmonitor = 1; } else { @@ -88,11 +65,7 @@ if (istate->untracked) istate->untracked->use_fsmonitor = 0; @@ - /* Mark all previously saved entries as dirty */ - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) - BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", -- (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); -+ (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); + (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); - /* Now mark the untracked cache for fsmonitor usage */ -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-20 8:32 ` [PATCH v4 " Utsav Shah via GitGitGadget @ 2019-11-20 8:32 ` Utsav Shah via GitGitGadget 2019-11-21 4:15 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Utsav Shah via GitGitGadget @ 2019-11-20 8:32 UTC (permalink / raw) To: git; +Cc: Utsav Shah, Junio C Hamano, Utsav Shah From: Utsav Shah <utsav@dropbox.com> The index might be aware that a file hasn't modified via fsmonitor, but unpack-trees did not pay attention to it and checked via ie_match_stat which can be inefficient on certain filesystems. This significantly slows down commands that run oneway_merge, like checkout and reset --hard. This patch makes oneway_merge check whether a file is considered unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees also now correctly copies over fsmonitor validity state from the source index. Finally, for correctness, we force a refresh of fsmonitor state in tweak_fsmonitor. After this change, commands like stash (that use reset --hard internally) go from 8s or more to ~2s on a 250k file repository on a mac. Changes since the last version are: * The sanity checks around accessing the fsmonitor_dirty bitmap have been moved to another patch, which is in message id [1] * Unintended indentation changes in fsmonitor have been removed * A comment explaining what untracked->use_fsmonitor means has been re-added (it was dropped in the previous version) * A few "helped-by" entries have been added to the patch [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com) Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Kevin Willford <Kevin.Willford@microsoft.com> Signed-off-by: Utsav Shah <utsav@dropbox.com> --- fsmonitor.c | 23 +++++++++++++++++------ t/t7519-status-fsmonitor.sh | 9 +++++++-- unpack-trees.c | 6 +++++- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index 1f4aa1b150..0d270da80f 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate) } if (bol < query_result.len) fsmonitor_refresh_callback(istate, buf + bol); + + /* Now mark the untracked cache for fsmonitor usage */ + if (istate->untracked) + istate->untracked->use_fsmonitor = 1; } else { + + /* We only want to run the post index changed hook if we've actually changed entries, so keep track + * if we actually changed entries or not */ + int is_cache_changed = 0; /* Mark all entries invalid */ - for (i = 0; i < istate->cache_nr; i++) - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; + for (i = 0; i < istate->cache_nr; i++) { + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) { + is_cache_changed = 1; + istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; + } + } /* If we're going to check every file, ensure we save the results */ - istate->cache_changed |= FSMONITOR_CHANGED; + if (is_cache_changed) + istate->cache_changed |= FSMONITOR_CHANGED; if (istate->untracked) istate->untracked->use_fsmonitor = 0; @@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate) (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); - /* Now mark the untracked cache for fsmonitor usage */ - if (istate->untracked) - istate->untracked->use_fsmonitor = 1; + refresh_fsmonitor(istate); } ewah_free(istate->fsmonitor_dirty); diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index d8df990972..69908b6a9b 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -106,6 +106,8 @@ EOF # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git update-index --fsmonitor && git update-index --fsmonitor-valid dir1/modified && git update-index --fsmonitor-valid dir2/modified && @@ -164,6 +166,8 @@ EOF # test that newly added files are marked valid test_expect_success 'newly added files are marked valid' ' + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git add new && git add dir1/new && git add dir2/new && @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the integration script get flagged # Ensure commands that call refresh_index() to move the index back in time # properly invalidate the fsmonitor cache test_expect_success 'refresh_index() invalidates fsmonitor cache' ' - write_script .git/hooks/fsmonitor-test<<-\EOF && - EOF clean_repo && dirty_repo && + write_integration_script && git add . && + write_script .git/hooks/fsmonitor-test<<-\EOF && + EOF git commit -m "to reset" && git reset HEAD~1 && git status >actual && diff --git a/unpack-trees.c b/unpack-trees.c index 33ea7810d8..fc5ceb932c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1504,6 +1504,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->merge_size = len; mark_all_ce_unused(o->src_index); + if (o->src_index->fsmonitor_last_update) + o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; + /* * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries */ @@ -2384,7 +2387,8 @@ int oneway_merge(const struct cache_entry * const *src, if (old && same(old, a)) { int update = 0; - if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { + if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) && + !(old->ce_flags & CE_FSMONITOR_VALID)) { struct stat st; if (lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/1] unpack-trees: skip stat on fsmonitor-valid files 2019-11-20 8:32 ` [PATCH v4 1/1] " Utsav Shah via GitGitGadget @ 2019-11-21 4:15 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2019-11-21 4:15 UTC (permalink / raw) To: Utsav Shah via GitGitGadget; +Cc: git, Utsav Shah, Utsav Shah "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Utsav Shah <utsav@dropbox.com> > > The index might be aware that a file hasn't modified via fsmonitor, but > unpack-trees did not pay attention to it and checked via ie_match_stat > which can be inefficient on certain filesystems. This significantly slows > down commands that run oneway_merge, like checkout and reset --hard. > > This patch makes oneway_merge check whether a file is considered > unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees > also now correctly copies over fsmonitor validity state from the source > index. Finally, for correctness, we force a refresh of fsmonitor state in > tweak_fsmonitor. Instead of saying "also now correctly copies..." as if it started working correctly by accident, be more assertive and actively make it so ;-) Check if a file is unchanged by fsmonitor in oneway_merge(), and avoid unnecessary calls to ie_match_stat(). Copy the fsmonitor validity state from the source index to the destination index in unpack_trees(). Force a refresh of the fsmonitor state in tweak_fsmonitor(), which is called after the index file is read from the disk, for correctness. perhaps. > After this change, commands like stash (that use reset --hard > internally) go from 8s or more to ~2s on a 250k file repository on a > mac. Good. > > Changes since the last version are: > * The sanity checks around accessing the fsmonitor_dirty bitmap have > been moved to another patch, which is in message id [1] > * Unintended indentation changes in fsmonitor have been removed > * A comment explaining what untracked->use_fsmonitor means has been > re-added (it was dropped in the previous version) > * A few "helped-by" entries have been added to the patch > > [1]: (xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com) The above is for the cover letter or after the three-dash lines, and not for the log message. > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Kevin Willford <Kevin.Willford@microsoft.com> > Signed-off-by: Utsav Shah <utsav@dropbox.com> > --- > fsmonitor.c | 23 +++++++++++++++++------ > t/t7519-status-fsmonitor.sh | 9 +++++++-- > unpack-trees.c | 6 +++++- > 3 files changed, 29 insertions(+), 9 deletions(-) > > diff --git a/fsmonitor.c b/fsmonitor.c > index 1f4aa1b150..0d270da80f 100644 > --- a/fsmonitor.c > +++ b/fsmonitor.c > @@ -189,13 +189,26 @@ void refresh_fsmonitor(struct index_state *istate) > } > if (bol < query_result.len) > fsmonitor_refresh_callback(istate, buf + bol); > + > + /* Now mark the untracked cache for fsmonitor usage */ > + if (istate->untracked) > + istate->untracked->use_fsmonitor = 1; > } else { > + > + /* We only want to run the post index changed hook if we've actually changed entries, so keep track > + * if we actually changed entries or not */ Multi-line comment style. > + int is_cache_changed = 0; > /* Mark all entries invalid */ > - for (i = 0; i < istate->cache_nr; i++) > - istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID; > + for (i = 0; i < istate->cache_nr; i++) { Lack of blank line between the last decl and the first stmt. Probably the blank should go before "/* Mark all ...". > @@ -257,9 +270,7 @@ void tweak_fsmonitor(struct index_state *istate) > (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); > > - /* Now mark the untracked cache for fsmonitor usage */ > - if (istate->untracked) > - istate->untracked->use_fsmonitor = 1; > + refresh_fsmonitor(istate); > } > > ewah_free(istate->fsmonitor_dirty); Looks good. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-11-21 4:15 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-25 15:23 [PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor Utsav Shah via GitGitGadget 2019-10-25 15:23 ` [PATCH 1/1] unpack-trees: skip lstat " Utsav Shah via GitGitGadget 2019-10-28 3:37 ` Junio C Hamano 2019-10-28 6:39 ` Utsav Shah 2019-10-28 19:23 ` Kevin Willford 2019-10-29 19:06 ` Utsav Shah 2019-10-29 20:12 ` Kevin Willford 2019-10-29 23:50 ` Utsav Shah 2019-10-30 0:21 ` Junio C Hamano 2019-10-30 16:41 ` Utsav Shah 2019-11-04 6:02 ` Junio C Hamano 2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget 2019-11-05 15:27 ` [PATCH v2 1/1] " Utsav Shah via GitGitGadget 2019-11-05 21:40 ` Kevin Willford 2019-11-06 4:36 ` Utsav Shah 2019-11-06 17:24 ` Kevin Willford 2019-11-06 4:54 ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget 2019-11-06 4:54 ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget 2019-11-06 10:46 ` Junio C Hamano 2019-11-06 22:33 ` Utsav Shah 2019-11-08 3:51 ` Utsav Shah 2019-11-08 4:11 ` Junio C Hamano 2019-11-06 10:16 ` [PATCH v3 0/1] " Junio C Hamano 2019-11-20 8:32 ` [PATCH v4 " Utsav Shah via GitGitGadget 2019-11-20 8:32 ` [PATCH v4 1/1] " Utsav Shah via GitGitGadget 2019-11-21 4:15 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).