| Message ID | 609c7c5047719a619ba22425dafc6ecd105e2cda.1572017008.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-trees: skip lstat on files based on fsmonitor | expand |
"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.
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
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
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
> 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
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
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.
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. > > >
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>
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))