* [PATCH 0/2] fsmonitor inline / testing cleanup @ 2020-10-21 18:04 Nipunn Koorapati via GitGitGadget 2020-10-21 18:04 ` [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Nipunn Koorapati via GitGitGadget @ 2020-10-21 18:04 UTC (permalink / raw) To: git; +Cc: Nipunn Koorapati Credit to alexmv again - I'm rebasing these changes from a couple years ago for contribution. Full comments are available here - https://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/ To summarize the relevant points Re: Inlining mark_fsmonitor_[in]valid peartben said I'm fine with these not being inline. I was attempting to minimize the performance impact of the fsmonitor code when it was not even turned on. Inlineing these functions allowed it to be kept to a simple test but I suspect (especially with modern optimizing compilers) that the overhead of calling a function to do that test is negligible. Re test-dump-fsmonitor peartben suggested keeping the +- syntax as well as the summary counts dscho suggested dumping the invalid entries Alex Vandiver (2): fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid fsmonitor: make output of test-dump-fsmonitor more concise fsmonitor.c | 19 +++++++++++++++++++ fsmonitor.h | 18 ++---------------- t/helper/test-dump-fsmonitor.c | 14 ++++++++++++-- 3 files changed, 33 insertions(+), 18 deletions(-) base-commit: 69986e19ffcfb9af674ae5180689ab7bbf92ed28 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-767%2Fnipunn1313%2Ffsmonitor-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-767/nipunn1313/fsmonitor-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/767 -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 18:04 [PATCH 0/2] fsmonitor inline / testing cleanup Nipunn Koorapati via GitGitGadget @ 2020-10-21 18:04 ` Alex Vandiver via GitGitGadget 2020-10-21 20:55 ` Taylor Blau 2020-10-21 18:04 ` [PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Alex Vandiver via GitGitGadget @ 2020-10-21 18:04 UTC (permalink / raw) To: git; +Cc: Nipunn Koorapati, Alex Vandiver From: Alex Vandiver <alexmv@dropbox.com> These were inline'd when they were first introduced, presumably as an optimization for cases when they were called in tight loops. This complicates using these functions, as untracked_cache_invalidate_path is defined in dir.h. Leave the inline'ing up to the compiler's decision, for ease of use. Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> --- fsmonitor.c | 19 +++++++++++++++++++ fsmonitor.h | 18 ++---------------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ca031c3abb..e120b3c5a9 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -287,6 +287,25 @@ void refresh_fsmonitor(struct index_state *istate) istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL); } +void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) +{ + if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { + istate->cache_changed = 1; + ce->ce_flags |= CE_FSMONITOR_VALID; + trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); + } +} + +void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce) +{ + if (core_fsmonitor) { + ce->ce_flags &= ~CE_FSMONITOR_VALID; + untracked_cache_invalidate_path(istate, ce->name, 1); + trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); + } +} + + void add_fsmonitor(struct index_state *istate) { unsigned int i; diff --git a/fsmonitor.h b/fsmonitor.h index 739318ab6d..6249020692 100644 --- a/fsmonitor.h +++ b/fsmonitor.h @@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate); * called any time the cache entry has been updated to reflect the * current state of the file on disk. */ -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) -{ - if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { - istate->cache_changed = 1; - ce->ce_flags |= CE_FSMONITOR_VALID; - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); - } -} +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce); /* * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate @@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache * trigger an lstat() or invalidate the untracked cache for the * corresponding directory */ -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce) -{ - if (core_fsmonitor) { - ce->ce_flags &= ~CE_FSMONITOR_VALID; - untracked_cache_invalidate_path(istate, ce->name, 1); - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); - } -} +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce); #endif -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 18:04 ` [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget @ 2020-10-21 20:55 ` Taylor Blau 2020-10-21 21:24 ` Junio C Hamano 2020-10-21 23:22 ` Nipunn Koorapati 0 siblings, 2 replies; 18+ messages in thread From: Taylor Blau @ 2020-10-21 20:55 UTC (permalink / raw) To: Alex Vandiver via GitGitGadget; +Cc: git, Nipunn Koorapati, Alex Vandiver On Wed, Oct 21, 2020 at 06:04:33PM +0000, Alex Vandiver via GitGitGadget wrote: > From: Alex Vandiver <alexmv@dropbox.com> > > These were inline'd when they were first introduced, presumably as an > optimization for cases when they were called in tight loops. This > complicates using these functions, as untracked_cache_invalidate_path > is defined in dir.h. > > Leave the inline'ing up to the compiler's decision, for ease of use. Letting the compiler inline these is fine, but... > diff --git a/fsmonitor.h b/fsmonitor.h > index 739318ab6d..6249020692 100644 > --- a/fsmonitor.h > +++ b/fsmonitor.h > @@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate); > * called any time the cache entry has been updated to reflect the > * current state of the file on disk. > */ > -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) > -{ > - if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { > - istate->cache_changed = 1; > - ce->ce_flags |= CE_FSMONITOR_VALID; > - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); > - } > -} > +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce); > > /* > * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate > @@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache > * trigger an lstat() or invalidate the untracked cache for the > * corresponding directory > */ > -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce) > -{ > - if (core_fsmonitor) { > - ce->ce_flags &= ~CE_FSMONITOR_VALID; > - untracked_cache_invalidate_path(istate, ce->name, 1); > - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); > - } > -} > +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce); > > #endif Any reason that these need to be externed explicitly? Note that these functions are already externed by default since you haven't said otherwise (and for no other reason than this'd be the only explicitly externed function in fsmonitor.h). Thanks, Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 20:55 ` Taylor Blau @ 2020-10-21 21:24 ` Junio C Hamano 2020-10-21 21:31 ` Taylor Blau 2020-10-21 23:22 ` Nipunn Koorapati 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2020-10-21 21:24 UTC (permalink / raw) To: Taylor Blau Cc: Alex Vandiver via GitGitGadget, git, Nipunn Koorapati, Alex Vandiver Taylor Blau <me@ttaylorr.com> writes: >> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce); > ... > Any reason that these need to be externed explicitly? Note that these > functions are already externed by default since you haven't said > otherwise (and for no other reason than this'd be the only explicitly > externed function in fsmonitor.h). Possibly due to the recent discussion? https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 21:24 ` Junio C Hamano @ 2020-10-21 21:31 ` Taylor Blau 2020-10-21 21:38 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Taylor Blau @ 2020-10-21 21:31 UTC (permalink / raw) To: Junio C Hamano Cc: Taylor Blau, Alex Vandiver via GitGitGadget, git, Nipunn Koorapati, Alex Vandiver On Wed, Oct 21, 2020 at 02:24:22PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > Any reason that these need to be externed explicitly? Note that these > > functions are already externed by default since you haven't said > > otherwise (and for no other reason than this'd be the only explicitly > > externed function in fsmonitor.h). > > Possibly due to the recent discussion? > > https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/ Ah, thanks. I remember the thread, but I wasn't sure where the discussion ended up. After re-reading it, it sounds like new function declarations in header files should be prefixed with 'extern' (making this patch correct as it already is). Tangential to this discussion: are you still expecting a tree-wide change to start use extern everywhere? Thanks, Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 21:31 ` Taylor Blau @ 2020-10-21 21:38 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2020-10-21 21:38 UTC (permalink / raw) To: Taylor Blau Cc: Alex Vandiver via GitGitGadget, git, Nipunn Koorapati, Alex Vandiver Taylor Blau <me@ttaylorr.com> writes: > Tangential to this discussion: are you still expecting a tree-wide > change to start use extern everywhere? I think before we start opening the tree for new topics is the best time to do so, if we were to follow through, but after we have dealt with brown-paper-bag fixes to the release. The Makefile tweak for the skip-dashed thing is the only one for 2.29, I think, so ... Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-21 20:55 ` Taylor Blau 2020-10-21 21:24 ` Junio C Hamano @ 2020-10-21 23:22 ` Nipunn Koorapati 1 sibling, 0 replies; 18+ messages in thread From: Nipunn Koorapati @ 2020-10-21 23:22 UTC (permalink / raw) To: Taylor Blau; +Cc: Alex Vandiver via GitGitGadget, git, Alex Vandiver > Letting the compiler inline these is fine, but... > > Any reason that these need to be externed explicitly? Note that these > functions are already externed by default since you haven't said > otherwise (and for no other reason than this'd be the only explicitly > externed function in fsmonitor.h). Did not have a reason or strong opinion here. It was this way, because this was the way alexmv used it originally - but it does compile in either manner. The thread Junio linked does seem to indicate preference for extern to avoid confusion. --Nipunn ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise 2020-10-21 18:04 [PATCH 0/2] fsmonitor inline / testing cleanup Nipunn Koorapati via GitGitGadget 2020-10-21 18:04 ` [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget @ 2020-10-21 18:04 ` Alex Vandiver via GitGitGadget 2020-10-21 20:52 ` [PATCH 0/2] fsmonitor inline / testing cleanup Taylor Blau 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget 3 siblings, 0 replies; 18+ messages in thread From: Alex Vandiver via GitGitGadget @ 2020-10-21 18:04 UTC (permalink / raw) To: git; +Cc: Nipunn Koorapati, Alex Vandiver From: Alex Vandiver <alexmv@dropbox.com> After displaying one very long line, summarize the contents of that line. The tests do not currently rely on any content except the first line ("no fsmonitor" / "fsmonitor last update"). Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> --- t/helper/test-dump-fsmonitor.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c index 975f0ac890..a42e402bf8 100644 --- a/t/helper/test-dump-fsmonitor.c +++ b/t/helper/test-dump-fsmonitor.c @@ -4,7 +4,7 @@ int cmd__dump_fsmonitor(int ac, const char **av) { struct index_state *istate = the_repository->index; - int i; + int i, valid = 0; setup_git_directory(); if (do_read_index(istate, the_repository->index_file, 0) < 0) @@ -15,8 +15,18 @@ int cmd__dump_fsmonitor(int ac, const char **av) } printf("fsmonitor last update %s\n", istate->fsmonitor_last_update); - for (i = 0; i < istate->cache_nr; i++) + for (i = 0; i < istate->cache_nr; i++) { printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-"); + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) + valid++; + } + + printf("\n valid: %d\n", valid); + printf(" invalid: %d\n", istate->cache_nr - valid); + + for (i = 0; i < istate->cache_nr; i++) + if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)) + printf(" - %s\n", istate->cache[i]->name); return 0; } -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] fsmonitor inline / testing cleanup 2020-10-21 18:04 [PATCH 0/2] fsmonitor inline / testing cleanup Nipunn Koorapati via GitGitGadget 2020-10-21 18:04 ` [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget 2020-10-21 18:04 ` [PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget @ 2020-10-21 20:52 ` Taylor Blau 2020-10-21 23:15 ` Nipunn Koorapati 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget 3 siblings, 1 reply; 18+ messages in thread From: Taylor Blau @ 2020-10-21 20:52 UTC (permalink / raw) To: Nipunn Koorapati via GitGitGadget; +Cc: git, Nipunn Koorapati Hi Nipunn, On Wed, Oct 21, 2020 at 06:04:32PM +0000, Nipunn Koorapati via GitGitGadget wrote: > Credit to alexmv again - I'm rebasing these changes from a couple years ago > for contribution. > > Full comments are available here - > https://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/ > To summarize the relevant points I'm fine with both of these patches, but it may help to have a bit more information about how they will be used. Presumably more patches are coming that make use of the new public functions, but it'd be good to know a little bit of why these changes are necessary. Thanks, Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] fsmonitor inline / testing cleanup 2020-10-21 20:52 ` [PATCH 0/2] fsmonitor inline / testing cleanup Taylor Blau @ 2020-10-21 23:15 ` Nipunn Koorapati 0 siblings, 0 replies; 18+ messages in thread From: Nipunn Koorapati @ 2020-10-21 23:15 UTC (permalink / raw) To: Taylor Blau; +Cc: Nipunn Koorapati via GitGitGadget, git > I'm fine with both of these patches, but it may help to have a bit > more information about how they will be used. Presumably more patches > are coming that make use of the new public functions, but it'd be good > to know a little bit of why these changes are necessary. I believe the externs are just there to avoid pulling in `dir.h` from the include file - since it's only needed in the implementation. I believe at the time (12/2017) `dir.h` was not imported from `fsmonitor.h`, but it does appear imported now. I've eliminated the import of `dir.h` as it no longer appears necessary - which I will include in the next iteration of this diff. The test helper merely makes it easier to debug fsmonitor tests - will be useful to any developer working on fsmonitor related changes. I have an upcoming one related to fsmonitor in git checkout, which I've also revived, but I'm not 100% sure I'll get it through, but regardless, I believe this test-debugging change will be good. --Nipunn ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-21 18:04 [PATCH 0/2] fsmonitor inline / testing cleanup Nipunn Koorapati via GitGitGadget ` (2 preceding siblings ...) 2020-10-21 20:52 ` [PATCH 0/2] fsmonitor inline / testing cleanup Taylor Blau @ 2020-10-22 0:21 ` Nipunn Koorapati via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget ` (2 more replies) 3 siblings, 3 replies; 18+ messages in thread From: Nipunn Koorapati via GitGitGadget @ 2020-10-22 0:21 UTC (permalink / raw) To: git; +Cc: Taylor Blau, Nipunn Koorapati, Nipunn Koorapati UPDATE SINCE v1 * Removed include of dir.h from fsmonitor.h as it's no longer needed Credit to alexmv again - I'm rebasing these changes from a couple years ago for contribution. Full comments are available here - https://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/ To summarize the relevant points Re: Inlining mark_fsmonitor_[in]valid peartben said I'm fine with these not being inline. I was attempting to minimize the performance impact of the fsmonitor code when it was not even turned on. Inlineing these functions allowed it to be kept to a simple test but I suspect (especially with modern optimizing compilers) that the overhead of calling a function to do that test is negligible. Re test-dump-fsmonitor peartben suggested keeping the +- syntax as well as the summary counts dscho suggested dumping the invalid entries Alex Vandiver (2): fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid fsmonitor: make output of test-dump-fsmonitor more concise fsmonitor.c | 19 +++++++++++++++++++ fsmonitor.h | 19 ++----------------- t/helper/test-dump-fsmonitor.c | 14 ++++++++++++-- 3 files changed, 33 insertions(+), 19 deletions(-) base-commit: 69986e19ffcfb9af674ae5180689ab7bbf92ed28 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-767%2Fnipunn1313%2Ffsmonitor-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-767/nipunn1313/fsmonitor-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/767 Range-diff vs v1: 1: 049989652c ! 1: ab9c330ca8 fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid @@ fsmonitor.c: void refresh_fsmonitor(struct index_state *istate) unsigned int i; ## fsmonitor.h ## +@@ + #define FSMONITOR_H + + #include "cache.h" +-#include "dir.h" + + extern struct trace_key trace_fsmonitor; + @@ fsmonitor.h: void refresh_fsmonitor(struct index_state *istate); * called any time the cache entry has been updated to reflect the * current state of the file on disk. 2: 598521091a = 2: 8ff657ded1 fsmonitor: make output of test-dump-fsmonitor more concise -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget @ 2020-10-22 0:21 ` Alex Vandiver via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget 2020-10-22 17:40 ` [PATCH v2 0/2] fsmonitor inline / testing cleanup Taylor Blau 2 siblings, 0 replies; 18+ messages in thread From: Alex Vandiver via GitGitGadget @ 2020-10-22 0:21 UTC (permalink / raw) To: git; +Cc: Taylor Blau, Nipunn Koorapati, Nipunn Koorapati, Alex Vandiver From: Alex Vandiver <alexmv@dropbox.com> These were inline'd when they were first introduced, presumably as an optimization for cases when they were called in tight loops. This complicates using these functions, as untracked_cache_invalidate_path is defined in dir.h. Leave the inline'ing up to the compiler's decision, for ease of use. Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> --- fsmonitor.c | 19 +++++++++++++++++++ fsmonitor.h | 19 ++----------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ca031c3abb..e120b3c5a9 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -287,6 +287,25 @@ void refresh_fsmonitor(struct index_state *istate) istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL); } +void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) +{ + if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { + istate->cache_changed = 1; + ce->ce_flags |= CE_FSMONITOR_VALID; + trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); + } +} + +void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce) +{ + if (core_fsmonitor) { + ce->ce_flags &= ~CE_FSMONITOR_VALID; + untracked_cache_invalidate_path(istate, ce->name, 1); + trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); + } +} + + void add_fsmonitor(struct index_state *istate) { unsigned int i; diff --git a/fsmonitor.h b/fsmonitor.h index 739318ab6d..313a35fdc8 100644 --- a/fsmonitor.h +++ b/fsmonitor.h @@ -2,7 +2,6 @@ #define FSMONITOR_H #include "cache.h" -#include "dir.h" extern struct trace_key trace_fsmonitor; @@ -49,14 +48,7 @@ void refresh_fsmonitor(struct index_state *istate); * called any time the cache entry has been updated to reflect the * current state of the file on disk. */ -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) -{ - if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { - istate->cache_changed = 1; - ce->ce_flags |= CE_FSMONITOR_VALID; - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); - } -} +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce); /* * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate @@ -65,13 +57,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache * trigger an lstat() or invalidate the untracked cache for the * corresponding directory */ -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce) -{ - if (core_fsmonitor) { - ce->ce_flags &= ~CE_FSMONITOR_VALID; - untracked_cache_invalidate_path(istate, ce->name, 1); - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); - } -} +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce); #endif -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget @ 2020-10-22 0:21 ` Alex Vandiver via GitGitGadget 2020-10-22 17:40 ` [PATCH v2 0/2] fsmonitor inline / testing cleanup Taylor Blau 2 siblings, 0 replies; 18+ messages in thread From: Alex Vandiver via GitGitGadget @ 2020-10-22 0:21 UTC (permalink / raw) To: git; +Cc: Taylor Blau, Nipunn Koorapati, Nipunn Koorapati, Alex Vandiver From: Alex Vandiver <alexmv@dropbox.com> After displaying one very long line, summarize the contents of that line. The tests do not currently rely on any content except the first line ("no fsmonitor" / "fsmonitor last update"). Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> --- t/helper/test-dump-fsmonitor.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c index 975f0ac890..a42e402bf8 100644 --- a/t/helper/test-dump-fsmonitor.c +++ b/t/helper/test-dump-fsmonitor.c @@ -4,7 +4,7 @@ int cmd__dump_fsmonitor(int ac, const char **av) { struct index_state *istate = the_repository->index; - int i; + int i, valid = 0; setup_git_directory(); if (do_read_index(istate, the_repository->index_file, 0) < 0) @@ -15,8 +15,18 @@ int cmd__dump_fsmonitor(int ac, const char **av) } printf("fsmonitor last update %s\n", istate->fsmonitor_last_update); - for (i = 0; i < istate->cache_nr; i++) + for (i = 0; i < istate->cache_nr; i++) { printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-"); + if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) + valid++; + } + + printf("\n valid: %d\n", valid); + printf(" invalid: %d\n", istate->cache_nr - valid); + + for (i = 0; i < istate->cache_nr; i++) + if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)) + printf(" - %s\n", istate->cache[i]->name); return 0; } -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget @ 2020-10-22 17:40 ` Taylor Blau 2020-10-22 18:32 ` Junio C Hamano 2 siblings, 1 reply; 18+ messages in thread From: Taylor Blau @ 2020-10-22 17:40 UTC (permalink / raw) To: Nipunn Koorapati via GitGitGadget; +Cc: git, Nipunn Koorapati Hi Nipunn, On Thu, Oct 22, 2020 at 12:21:04AM +0000, Nipunn Koorapati via GitGitGadget wrote: > UPDATE SINCE v1 > > * Removed include of dir.h from fsmonitor.h as it's no longer needed This version all looks sensible to me. I'm still iffy on whether or not this series makes sense to apply without the rest of the code that depends on it, but I'll leave that up to Junio whether he wants to take the series as it is now, or wait for other patches to come in on top. In either case, these two patches are: Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-22 17:40 ` [PATCH v2 0/2] fsmonitor inline / testing cleanup Taylor Blau @ 2020-10-22 18:32 ` Junio C Hamano 2020-10-22 18:38 ` Taylor Blau 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2020-10-22 18:32 UTC (permalink / raw) To: Taylor Blau; +Cc: Nipunn Koorapati via GitGitGadget, git, Nipunn Koorapati Taylor Blau <me@ttaylorr.com> writes: > Hi Nipunn, > > On Thu, Oct 22, 2020 at 12:21:04AM +0000, Nipunn Koorapati via GitGitGadget wrote: >> UPDATE SINCE v1 >> >> * Removed include of dir.h from fsmonitor.h as it's no longer needed > > This version all looks sensible to me. > > I'm still iffy on whether or not this series makes sense to apply > without the rest of the code that depends on it, but I'll leave that up > to Junio whether he wants to take the series as it is now, or wait for > other patches to come in on top. Sorry but I am not sure what you mean by "the code that depends on it". Are these two functions unused anywhere in the code? If so, the right way to clean them up may not be to turn them from inline to a proper definition, but to remove them ;-). If they have existing callers and it can be demonstrated that their callers do not benefit from them being inline, that by itself is a worthy clean-up, without adding any more callers, no? Confused... > In either case, these two patches are: > > Reviewed-by: Taylor Blau <me@ttaylorr.com> > > Thanks, > Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-22 18:32 ` Junio C Hamano @ 2020-10-22 18:38 ` Taylor Blau 2020-10-22 19:14 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Taylor Blau @ 2020-10-22 18:38 UTC (permalink / raw) To: Junio C Hamano Cc: Taylor Blau, Nipunn Koorapati via GitGitGadget, git, Nipunn Koorapati On Thu, Oct 22, 2020 at 11:32:51AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > I'm still iffy on whether or not this series makes sense to apply > > without the rest of the code that depends on it, but I'll leave that up > > to Junio whether he wants to take the series as it is now, or wait for > > other patches to come in on top. > > Sorry but I am not sure what you mean by "the code that depends on > it". Are these two functions unused anywhere in the code? If so, > the right way to clean them up may not be to turn them from inline > to a proper definition, but to remove them ;-). > > If they have existing callers and it can be demonstrated that their > callers do not benefit from them being inline, that by itself is a > worthy clean-up, without adding any more callers, no? > > Confused... Sorry for the confusion. I mean the following: - These functions have existing callers that Nipunn claims do not need to be explicitly inlined. - These functions are being moved to be part of the fsmonitor public interface (presumably so that new callers can be added). ...And I was wondering whether you wanted to wait for new callers before applying these to your tree. Thanks, Taylor ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-22 18:38 ` Taylor Blau @ 2020-10-22 19:14 ` Junio C Hamano 2020-10-22 20:59 ` Nipunn Koorapati 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2020-10-22 19:14 UTC (permalink / raw) To: Taylor Blau; +Cc: Nipunn Koorapati via GitGitGadget, git, Nipunn Koorapati Taylor Blau <me@ttaylorr.com> writes: > Sorry for the confusion. I mean the following: > > - These functions have existing callers that Nipunn claims do not need > to be explicitly inlined. I guess "claims" is the key phrase in your responsehere. Do you feel that the claim is not sufficiently substantiated? Those without fsmonitor would pay the call/return cost for no good reason if core_fsmonitor is not set, and checking that on the caller side may make a big difference. How big? That needs measurement. This is a tangent, but with or without inlining, I find it iffy to see that untracked_cache_invalidate_path() is called only when fsmonitor is in use. Does untracked_cache depend on fsmonitor for its correct operation? Why is it OK not to invlidate when the caller would tell fsmonitor that a path is invalid if fsmonitor were in use? The call is a statement of fact that the path is no longer valid, and that bit of information would be useful to the parts of the system outside fsmonitor, no? Puzzled.... > - These functions are being moved to be part of the fsmonitor public > interface (presumably so that new callers can be added). They used to be implemented as static inline functions in the fsmonitor.h header file, so they have been part of the public interface anyway. Anybody that includes fsmonitor.h can use it, with or without the patch. So I think this one would not be a problem. > ...And I was wondering whether you wanted to wait for new callers > before applying these to your tree. Thanks. I still do not know about the "should the inline be kept" question. The proposed log message for the commit does not explain (let alone justify) why "optimization" is unneeded for the fuctions in the first place, which does not help. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup 2020-10-22 19:14 ` Junio C Hamano @ 2020-10-22 20:59 ` Nipunn Koorapati 0 siblings, 0 replies; 18+ messages in thread From: Nipunn Koorapati @ 2020-10-22 20:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, Nipunn Koorapati via GitGitGadget, git > from Taylor > I'm still iffy on whether or not this series makes sense to apply > without the rest of the code that depends on it Sorry for confusion. I don't think we should assume there is more code coming related to this. I think this is intended to stand on its own. It's not a required dependency either. Rather, it's motivated by simplicity - remove the dir.h dependency from fsmonitor.h. - Keep implementation in fsmonitor.c and definitions in fsmonitor.h > From Junio > Those without fsmonitor would pay the call/return cost for no good > reason if core_fsmonitor is not set, and checking that on the caller > side may make a big difference. How big? That needs measurement. Noted! This is not called out or measured - it is simply assumed based on earlier conversation. I should be able to run the fsmonitor perf suite before/after this change and include the results in the commit message. > This is a tangent, but with or without inlining, I find it iffy to > see that untracked_cache_invalidate_path() is called only when > fsmonitor is in use. Does untracked_cache depend on fsmonitor for > its correct operation? Why is it OK not to invlidate when the > caller would tell fsmonitor that a path is invalid if fsmonitor were > in use? The call is a statement of fact that the path is no longer > valid, and that bit of information would be useful to the parts of > the system outside fsmonitor, no? Puzzled.... I did some source diving in an attempt to understand what's happening here. I believe that untracked_cache_invalidate_path() is called in dir.c whenever an entry is added or removed from a directory. This is an additional call when fsmonitor is enabled - because fsmonitor's whole purpose is to avoid the lstat on the other path. There is a nice explanation in the original commit message Commit 883e248b (fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files., 2017-09-22) --Nipunn ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-22 20:59 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-21 18:04 [PATCH 0/2] fsmonitor inline / testing cleanup Nipunn Koorapati via GitGitGadget 2020-10-21 18:04 ` [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget 2020-10-21 20:55 ` Taylor Blau 2020-10-21 21:24 ` Junio C Hamano 2020-10-21 21:31 ` Taylor Blau 2020-10-21 21:38 ` Junio C Hamano 2020-10-21 23:22 ` Nipunn Koorapati 2020-10-21 18:04 ` [PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget 2020-10-21 20:52 ` [PATCH 0/2] fsmonitor inline / testing cleanup Taylor Blau 2020-10-21 23:15 ` Nipunn Koorapati 2020-10-22 0:21 ` [PATCH v2 " Nipunn Koorapati via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver via GitGitGadget 2020-10-22 0:21 ` [PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise Alex Vandiver via GitGitGadget 2020-10-22 17:40 ` [PATCH v2 0/2] fsmonitor inline / testing cleanup Taylor Blau 2020-10-22 18:32 ` Junio C Hamano 2020-10-22 18:38 ` Taylor Blau 2020-10-22 19:14 ` Junio C Hamano 2020-10-22 20:59 ` Nipunn Koorapati
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).