[PATCH 0/2] fsmonitor inline / testing cleanup

[PATCH 0/2] fsmonitor inline / testing cleanup

[Nipunn Koorapati via GitGitGadget]

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

[PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Alex Vandiver via GitGitGadget]

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

Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Taylor Blau]

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

Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Junio C Hamano]

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/

Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Taylor Blau]

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

Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Junio C Hamano]

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.

Re: [PATCH 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Nipunn Koorapati]

> 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

[PATCH 2/2] fsmonitor: make output of test-dump-fsmonitor more concise

[Alex Vandiver via GitGitGadget]

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

Re: [PATCH 0/2] fsmonitor inline / testing cleanup

[Taylor Blau]

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

Re: [PATCH 0/2] fsmonitor inline / testing cleanup

[Nipunn Koorapati]

> 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

[PATCH v2 0/2] fsmonitor inline / testing cleanup

[Nipunn Koorapati via GitGitGadget]

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

[PATCH v2 1/2] fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid

[Alex Vandiver via GitGitGadget]

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

[PATCH v2 2/2] fsmonitor: make output of test-dump-fsmonitor more concise

[Alex Vandiver via GitGitGadget]

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

Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

[Taylor Blau]

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

Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

[Junio C Hamano]

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

Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

[Taylor Blau]

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

Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

[Junio C Hamano]

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.

Re: [PATCH v2 0/2] fsmonitor inline / testing cleanup

[Nipunn Koorapati]

> 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