fsmonitor inline / testing cleanup

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/[email protected]/
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

From: Alex Vandiver <[email protected]>

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").

---
 t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 975f0ac890..a42e402bf8 100644
@@ -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

From: Alex Vandiver <[email protected]>

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.

---
 fsmonitor.c | 19 +++++++++++++++++++
 fsmonitor.h | 18 ++----------------
 2 files changed, 21 insertions(+), 16 deletions(-)

index ca031c3abb..e120b3c5a9 100644
@@ -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;

index 739318ab6d..6249020692 100644
@@ -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

Hi Nipunn,

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

Letting the compiler inline these is fine, but...

Possibly due to the recent discussion?

https://lore.kernel.org/git/[email protected]/

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

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.

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

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

From: Alex Vandiver <[email protected]>

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").

---
 t/helper/test-dump-fsmonitor.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 975f0ac890..a42e402bf8 100644
@@ -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

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/[email protected]/
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

From: Alex Vandiver <[email protected]>

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.

---
 fsmonitor.c | 19 +++++++++++++++++++
 fsmonitor.h | 19 ++-----------------
 2 files changed, 21 insertions(+), 17 deletions(-)

index ca031c3abb..e120b3c5a9 100644
@@ -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;

index 739318ab6d..313a35fdc8 100644
@@ -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

Hi Nipunn,

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:

Thanks,
Taylor

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

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....

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.

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.

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

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.

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