Some rough edges of core.fsmonitor

Some rough edges of core.fsmonitor

[Ævar Arnfjörð Bjarmason]

I just got around to testing this since it landed, for context some
previous poking of mine in [1].

Issues / stuff I've noticed:

1) We end up invalidating the untracked cache because stuff in .git/
changed. For example:

    01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
    01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
    01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
    01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
    01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful

Am I missing something or should we do something like:

    diff --git a/fsmonitor.c b/fsmonitor.c
    index 0af7c4edba..5067b89bda 100644
    --- a/fsmonitor.c
    +++ b/fsmonitor.c
    @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que

     static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
     {
    -       int pos = index_name_pos(istate, name, strlen(name));
    +       int pos;
    +
    +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
    +               return;
    +
    +       pos = index_name_pos(istate, name, strlen(name));

            if (pos >= 0) {
                    struct cache_entry *ce = istate->cache[pos];

With that patch applied status on a large repo[2] goes from a consistent
~180-200ms to ~140-150ms, since we're not invalidating some of the UC
structure

2) We re-write out the index even though we know nothing changed

When you first run with core.fsmonitor it needs to
mark_fsmonitor_clean() for every path, but is there a reason for why we
wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are
marked and we know from the hook that nothing changed? Why write out the
index again?

3) A lot of time spend reading the index (or something..)

While the hook itself takes ~20ms (and watchman itself 1/4 of that)
status as a whole takes much longer. gprof reveals:

    Each sample counts as 0.01 seconds.
      %   cumulative   self              self     total
     time   seconds   seconds    calls  ms/call  ms/call  name
     15.38      0.02     0.02   221690     0.00     0.00  memihash
     15.38      0.04     0.02   221689     0.00     0.00  create_from_disk
      7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32
      7.69      0.06     0.01   222661     0.00     0.00  ce_path_match
      7.69      0.07     0.01   221769     0.00     0.00  hashmap_add
      7.69      0.08     0.01    39941     0.00     0.00  prep_exclude
      7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch
      7.69      0.10     0.01        1    10.00    10.00  read_one
      7.69      0.11     0.01        1    10.00    10.00  refresh_index
      7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor
      7.69      0.13     0.01                             preload_thread

The index is 24M in this case, I guess it's unpacking it, but I wonder
if this couldn't be much faster if we saved away the result of the last
"status" in something that's quick to access, and then if nothing
changed we just report that, and no need to re-write the index (or just
write the "it was clean at this time" part).

4) core.fsmonitor=false behaves unexpectedly

The code that reads this variable just treats it as a string, so we do a
bunch of work for nothing (and nothing warns) if this is set and 'false'
is executed. Any reason we couldn't do our standard boolean parsing
here? You couldn't call your hook 0/1/true/false, but that doesn't seem
like a big loss.

1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/
2. https://github.com/avar/2015-04-03-1M-git

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Sat, Jan 27, 2018 at 7:28 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 3) A lot of time spend reading the index (or something..)

I'm resending a patch from my old index-helper series. It should
measure all big time consuming blocks in git. Maybe we should get it
merged...

> While the hook itself takes ~20ms (and watchman itself 1/4 of that)
> status as a whole takes much longer. gprof reveals:
>
>     Each sample counts as 0.01 seconds.
>       %   cumulative   self              self     total
>      time   seconds   seconds    calls  ms/call  ms/call  name
>      15.38      0.02     0.02   221690     0.00     0.00  memihash

This sounds like name-hash to me.

>      15.38      0.04     0.02   221689     0.00     0.00  create_from_disk
>       7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32
>       7.69      0.06     0.01   222661     0.00     0.00  ce_path_match
>       7.69      0.07     0.01   221769     0.00     0.00  hashmap_add
>       7.69      0.08     0.01    39941     0.00     0.00  prep_exclude
>       7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch
>       7.69      0.10     0.01        1    10.00    10.00  read_one
>       7.69      0.11     0.01        1    10.00    10.00  refresh_index
>       7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor
>       7.69      0.13     0.01                             preload_thread
>
> The index is 24M in this case, I guess it's unpacking it, but I wonder
> if this couldn't be much faster if we saved away the result of the last
> "status" in something that's quick to access, and then if nothing

No we could do better, we could cache parsed index content so
everybody benefits. I demonstrated it with my "index v254" patch a
while back:

https://public-inbox.org/git/1399980019-8706-1-git-send-email-pclouds@gmail.com/

With the patch I'm sending soon, we can see how much time reading an
index take out of that ~140-150ms (and we probably can cut down index
read time to like 10-20% when cached).

> changed we just report that, and no need to re-write the index (or just
> write the "it was clean at this time" part).

Hmm.. does an index write increase that much time?
-- 
Duy

[PATCH] trace: measure where the time is spent in the index-heavy operations

[Nguyễn Thái Ngọc Duy]

All the known heavy code blocks are measured (except object database
access). This should help identify if an optimization is effective or
not. An unoptimized git-status would give something like below (92% of
time is accounted).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This was in my old index-helper series. The series was replaced by
 fsmonitor but perhaps some measurements like this still helps.

 In my old version I measured packed-refs read time too. But
 packed-refs is mmap'd now, no need to worry about it (or at least its
 initial cost).

 diff-lib.c      |  4 ++++
 dir.c           |  2 ++
 name-hash.c     |  3 +++
 preload-index.c |  2 ++
 read-cache.c    | 11 +++++++++++
 5 files changed, 22 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 8104603a3b..a228e1a219 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -92,6 +92,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	int diff_unmerged_stage = revs->max_count;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
+	uint64_t start = getnanotime();
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
@@ -246,6 +247,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	}
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
+	trace_performance_since(start, "diff-files");
 	return 0;
 }
 
@@ -512,6 +514,7 @@ static int diff_cache(struct rev_info *revs,
 int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
+	uint64_t start = getnanotime();
 
 	ent = revs->pending.objects;
 	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
@@ -521,6 +524,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 	diffcore_fix_diff_index(&revs->diffopt);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
+	trace_performance_since(start, "diff-index");
 	return 0;
 }
 
diff --git a/dir.c b/dir.c
index 7c4b45e30e..4479a02a49 100644
--- a/dir.c
+++ b/dir.c
@@ -2248,6 +2248,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
 		   const char *path, int len, const struct pathspec *pathspec)
 {
 	struct untracked_cache_dir *untracked;
+	uint64_t start = getnanotime();
 
 	if (has_symlink_leading_path(path, len))
 		return dir->nr;
@@ -2286,6 +2287,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
 		dir->nr = i;
 	}
 
+	trace_performance_since(start, "read directory %.*s", len, path);
 	if (dir->untracked) {
 		static struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);
 		trace_printf_key(&trace_untracked_stats,
diff --git a/name-hash.c b/name-hash.c
index 45c98db0a0..ada66f066a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -578,6 +578,8 @@ static void threaded_lazy_init_name_hash(
 
 static void lazy_init_name_hash(struct index_state *istate)
 {
+	uint64_t start = getnanotime();
+
 	if (istate->name_hash_initialized)
 		return;
 	hashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);
@@ -600,6 +602,7 @@ static void lazy_init_name_hash(struct index_state *istate)
 	}
 
 	istate->name_hash_initialized = 1;
+	trace_performance_since(start, "initialize name hash");
 }
 
 /*
diff --git a/preload-index.c b/preload-index.c
index 2a83255e4e..4d08d44874 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -78,6 +78,7 @@ static void preload_index(struct index_state *index,
 {
 	int threads, i, work, offset;
 	struct thread_data data[MAX_PARALLEL];
+	uint64_t start = getnanotime();
 
 	if (!core_preload_index)
 		return;
@@ -108,6 +109,7 @@ static void preload_index(struct index_state *index,
 		if (pthread_join(p->pthread, NULL))
 			die("unable to join threaded lstat");
 	}
+	trace_performance_since(start, "preload index");
 }
 #endif
 
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..1f00aee6a2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	const char *typechange_fmt;
 	const char *added_fmt;
 	const char *unmerged_fmt;
+	uint64_t start = getnanotime();
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
@@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 
 		replace_index_entry(istate, i, new);
 	}
+	trace_performance_since(start, "refresh index");
 	return has_errors;
 }
 
@@ -1877,12 +1879,15 @@ int read_index_from(struct index_state *istate, const char *path)
 	int ret;
 	char *base_sha1_hex;
 	const char *base_path;
+	uint64_t start;
 
 	/* istate->initialized covers both .git/index and .git/sharedindex.xxx */
 	if (istate->initialized)
 		return istate->cache_nr;
 
+	start = getnanotime();
 	ret = do_read_index(istate, path, 0);
+	trace_performance_since(start, "read cache %s", path);
 
 	split_index = istate->split_index;
 	if (!split_index || is_null_sha1(split_index->base_sha1)) {
@@ -1897,6 +1902,7 @@ int read_index_from(struct index_state *istate, const char *path)
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
 	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	start = getnanotime();
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",
@@ -1906,6 +1912,9 @@ int read_index_from(struct index_state *istate, const char *path)
 	freshen_shared_index(base_sha1_hex, 0);
 	merge_base_index(istate);
 	post_read_index_from(istate);
+	trace_performance_since(start, "read cache %s",
+				git_path("sharedindex.%s",
+					 sha1_to_hex(split_index->base_sha1)));
 	return ret;
 }
 
@@ -2244,6 +2253,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	struct ondisk_cache_entry_extended ondisk;
 	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
 	int drop_cache_tree = 0;
+	uint64_t start = getnanotime();
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -2374,6 +2384,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
 	return 0;
 }
 
-- 
2.16.1.205.g271f633410

Re: [PATCH] trace: measure where the time is spent in the index-heavy operations

[Thomas Gummerer]

On 01/27, Nguyễn Thái Ngọc Duy wrote:
> All the known heavy code blocks are measured (except object database
> access). This should help identify if an optimization is effective or
> not. An unoptimized git-status would give something like below (92% of
> time is accounted).
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  This was in my old index-helper series. The series was replaced by
>  fsmonitor but perhaps some measurements like this still helps.
> 
>  In my old version I measured packed-refs read time too. But
>  packed-refs is mmap'd now, no need to worry about it (or at least its
>  initial cost).
> 
>  diff-lib.c      |  4 ++++
>  dir.c           |  2 ++
>  name-hash.c     |  3 +++
>  preload-index.c |  2 ++
>  read-cache.c    | 11 +++++++++++
>  5 files changed, 22 insertions(+)
>
> [...]
>  
> diff --git a/read-cache.c b/read-cache.c
> index 2eb81a66b9..1f00aee6a2 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  	const char *typechange_fmt;
>  	const char *added_fmt;
>  	const char *unmerged_fmt;
> +	uint64_t start = getnanotime();
>  
>  	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
>  	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
> @@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  
>  		replace_index_entry(istate, i, new);
>  	}
> +	trace_performance_since(start, "refresh index");
>  	return has_errors;
>  }
>  
> @@ -1877,12 +1879,15 @@ int read_index_from(struct index_state *istate, const char *path)
>  	int ret;
>  	char *base_sha1_hex;
>  	const char *base_path;
> +	uint64_t start;
>  
>  	/* istate->initialized covers both .git/index and .git/sharedindex.xxx */
>  	if (istate->initialized)
>  		return istate->cache_nr;
>  
> +	start = getnanotime();
>  	ret = do_read_index(istate, path, 0);
> +	trace_performance_since(start, "read cache %s", path);
>  
>  	split_index = istate->split_index;
>  	if (!split_index || is_null_sha1(split_index->base_sha1)) {
> @@ -1897,6 +1902,7 @@ int read_index_from(struct index_state *istate, const char *path)
>  
>  	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
>  	base_path = git_path("sharedindex.%s", base_sha1_hex);
> +	start = getnanotime();
>  	ret = do_read_index(split_index->base, base_path, 1);
>  	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
>  		die("broken index, expect %s in %s, got %s",
> @@ -1906,6 +1912,9 @@ int read_index_from(struct index_state *istate, const char *path)
>  	freshen_shared_index(base_sha1_hex, 0);
>  	merge_base_index(istate);
>  	post_read_index_from(istate);
> +	trace_performance_since(start, "read cache %s",
> +				git_path("sharedindex.%s",
> +					 sha1_to_hex(split_index->base_sha1)));

Would it be worth doing this on top of tg/split-index-fixes?  OTOH
this will only give a wrong output when tracing performance is on, and
it should be easy enough to figure out where the sharedindex actually
is.  So it might be better to keep this separate, and then just add a
patch on top for fixing the path later, which might be less work for
Junio.

So dunno what the best way is, just wanted to mention it.

>  	return ret;
>  }
>  
> @@ -2244,6 +2253,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  	struct ondisk_cache_entry_extended ondisk;
>  	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
>  	int drop_cache_tree = 0;
> +	uint64_t start = getnanotime();
>  
>  	for (i = removed = extended = 0; i < entries; i++) {
>  		if (cache[i]->ce_flags & CE_REMOVE)
> @@ -2374,6 +2384,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>  		return -1;
>  	istate->timestamp.sec = (unsigned int)st.st_mtime;
>  	istate->timestamp.nsec = ST_MTIME_NSEC(st);
> +	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
>  	return 0;
>  }
>  
> -- 
> 2.16.1.205.g271f633410
> 

[PATCH v2] trace: measure where the time is spent in the index-heavy operations

[Nguyễn Thái Ngọc Duy]

All the known heavy code blocks are measured (except object database
access). This should help identify if an optimization is effective or
not. An unoptimized git-status would give something like below:

    0.001791141 s: read cache ...
    0.004011363 s: preload index
    0.000516161 s: refresh index
    0.003139257 s: git command: ... 'status' '--porcelain=2'
    0.006788129 s: diff-files
    0.002090267 s: diff-index
    0.001885735 s: initialize name hash
    0.032013138 s: read directory
    0.051781209 s: git command: './git' 'status'

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 > Would it be worth doing this on top of tg/split-index-fixes?  OTOH
 > this will only give a wrong output when tracing performance is on, and
 > it should be easy enough to figure out where the sharedindex actually
 > is.  So it might be better to keep this separate, and then just add a
 > patch on top for fixing the path later, which might be less work for
 > Junio.

 I updated the patch a bit to avoid git_path(). A merge on 'pu' still
 conflicts, but it's much easier to resolve by making sure free() is
 called after the trace_performance_since() line in read_index_from().

 It's technically dangerous to re-use base_path again this way, too
 far away from its assignment since 4 other git_path() calls may have
 been done and changed base_path value. But since tg/split-index-fixes
 should enter 'master' eventually and make it safe to re-use
 base_path, I think it's ok.

 diff-lib.c      | 4 ++++
 dir.c           | 2 ++
 name-hash.c     | 3 +++
 preload-index.c | 2 ++
 read-cache.c    | 7 +++++++
 5 files changed, 18 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 8104603a3b..a228e1a219 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -92,6 +92,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	int diff_unmerged_stage = revs->max_count;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
+	uint64_t start = getnanotime();
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
@@ -246,6 +247,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	}
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
+	trace_performance_since(start, "diff-files");
 	return 0;
 }
 
@@ -512,6 +514,7 @@ static int diff_cache(struct rev_info *revs,
 int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
+	uint64_t start = getnanotime();
 
 	ent = revs->pending.objects;
 	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
@@ -521,6 +524,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 	diffcore_fix_diff_index(&revs->diffopt);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
+	trace_performance_since(start, "diff-index");
 	return 0;
 }
 
diff --git a/dir.c b/dir.c
index 7c4b45e30e..4479a02a49 100644
--- a/dir.c
+++ b/dir.c
@@ -2248,6 +2248,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
 		   const char *path, int len, const struct pathspec *pathspec)
 {
 	struct untracked_cache_dir *untracked;
+	uint64_t start = getnanotime();
 
 	if (has_symlink_leading_path(path, len))
 		return dir->nr;
@@ -2286,6 +2287,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
 		dir->nr = i;
 	}
 
+	trace_performance_since(start, "read directory %.*s", len, path);
 	if (dir->untracked) {
 		static struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);
 		trace_printf_key(&trace_untracked_stats,
diff --git a/name-hash.c b/name-hash.c
index 45c98db0a0..ada66f066a 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -578,6 +578,8 @@ static void threaded_lazy_init_name_hash(
 
 static void lazy_init_name_hash(struct index_state *istate)
 {
+	uint64_t start = getnanotime();
+
 	if (istate->name_hash_initialized)
 		return;
 	hashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);
@@ -600,6 +602,7 @@ static void lazy_init_name_hash(struct index_state *istate)
 	}
 
 	istate->name_hash_initialized = 1;
+	trace_performance_since(start, "initialize name hash");
 }
 
 /*
diff --git a/preload-index.c b/preload-index.c
index 2a83255e4e..4d08d44874 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -78,6 +78,7 @@ static void preload_index(struct index_state *index,
 {
 	int threads, i, work, offset;
 	struct thread_data data[MAX_PARALLEL];
+	uint64_t start = getnanotime();
 
 	if (!core_preload_index)
 		return;
@@ -108,6 +109,7 @@ static void preload_index(struct index_state *index,
 		if (pthread_join(p->pthread, NULL))
 			die("unable to join threaded lstat");
 	}
+	trace_performance_since(start, "preload index");
 }
 #endif
 
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..eac74bc9f1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1372,6 +1372,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	const char *typechange_fmt;
 	const char *added_fmt;
 	const char *unmerged_fmt;
+	uint64_t start = getnanotime();
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
@@ -1442,6 +1443,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 
 		replace_index_entry(istate, i, new);
 	}
+	trace_performance_since(start, "refresh index");
 	return has_errors;
 }
 
@@ -1873,6 +1875,7 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 
 int read_index_from(struct index_state *istate, const char *path)
 {
+	uint64_t start = getnanotime();
 	struct split_index *split_index;
 	int ret;
 	char *base_sha1_hex;
@@ -1883,6 +1886,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		return istate->cache_nr;
 
 	ret = do_read_index(istate, path, 0);
+	trace_performance_since(start, "read cache %s", path);
 
 	split_index = istate->split_index;
 	if (!split_index || is_null_sha1(split_index->base_sha1)) {
@@ -1906,6 +1910,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	freshen_shared_index(base_sha1_hex, 0);
 	merge_base_index(istate);
 	post_read_index_from(istate);
+	trace_performance_since(start, "read cache %s", base_path);
 	return ret;
 }
 
@@ -2234,6 +2239,7 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
 static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 			  int strip_extensions)
 {
+	uint64_t start = getnanotime();
 	int newfd = tempfile->fd;
 	git_SHA_CTX c;
 	struct cache_header hdr;
@@ -2374,6 +2380,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
+	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
 	return 0;
 }
 
-- 
2.16.1.205.g271f633410

Re: Some rough edges of core.fsmonitor

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 27 2018, Duy Nguyen jotted:

> On Sat, Jan 27, 2018 at 7:28 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> 3) A lot of time spend reading the index (or something..)
>
> I'm resending a patch from my old index-helper series. It should
> measure all big time consuming blocks in git. Maybe we should get it
> merged...
>
>> While the hook itself takes ~20ms (and watchman itself 1/4 of that)
>> status as a whole takes much longer. gprof reveals:
>>
>>     Each sample counts as 0.01 seconds.
>>       %   cumulative   self              self     total
>>      time   seconds   seconds    calls  ms/call  ms/call  name
>>      15.38      0.02     0.02   221690     0.00     0.00  memihash
>
> This sounds like name-hash to me.
>
>>      15.38      0.04     0.02   221689     0.00     0.00  create_from_disk
>>       7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32
>>       7.69      0.06     0.01   222661     0.00     0.00  ce_path_match
>>       7.69      0.07     0.01   221769     0.00     0.00  hashmap_add
>>       7.69      0.08     0.01    39941     0.00     0.00  prep_exclude
>>       7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch
>>       7.69      0.10     0.01        1    10.00    10.00  read_one
>>       7.69      0.11     0.01        1    10.00    10.00  refresh_index
>>       7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor
>>       7.69      0.13     0.01                             preload_thread
>>
>> The index is 24M in this case, I guess it's unpacking it, but I wonder
>> if this couldn't be much faster if we saved away the result of the last
>> "status" in something that's quick to access, and then if nothing
>
> No we could do better, we could cache parsed index content so
> everybody benefits. I demonstrated it with my "index v254" patch a
> while back:
>
> https://public-inbox.org/git/1399980019-8706-1-git-send-email-pclouds@gmail.com/
>
> With the patch I'm sending soon, we can see how much time reading an
> index take out of that ~140-150ms (and we probably can cut down index
> read time to like 10-20% when cached).
>
>> changed we just report that, and no need to re-write the index (or just
>> write the "it was clean at this time" part).
>
> Hmm.. does an index write increase that much time?

Your patch is very useful. Here's (with gcc -03) some runtimes. This
also includes my .git exclusion patch.

These are all best out of 5, and with the top (until <0.5% time) of
strace -c output (run as another invocation, timing not done with
strace)::

a) no fsmonitor

    $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
    12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index
    12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index
    12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index
    12:32:44.979083 diff-lib.c:250          performance: 0.004649994 s: diff-files
    12:32:44.982511 diff-lib.c:527          performance: 0.002918416 s: diff-index
    12:32:45.037880 dir.c:2290              performance: 0.055331063 s: read directory
    On branch master
    Your branch is up to date with 'origin/master'.

    nothing to commit, working tree clean
    12:32:45.040666 trace.c:417             performance: 0.146724289 s: git command: '/home/aearnfjord/g/git/git-status'

    real    0m0.153s
    user    0m0.110s
    sys     0m0.354s
    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     59.93    0.031924           1     39978         9 stat
     35.86    0.019104        6368         3           futex
      0.84    0.000446          12        36           mprotect
      0.73    0.000389          13        29           munmap
      0.66    0.000349           6        62           mmap
      0.53    0.000285          14        20           clone

b) with fsmonitor

    $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
    12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
    12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index
    12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
    12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index
    12:34:23.876427 diff-lib.c:250          performance: 0.004731427 s: diff-files
    12:34:23.880669 diff-lib.c:527          performance: 0.003944422 s: diff-index
    12:34:23.899225 dir.c:2290              performance: 0.018509066 s: read directory
    On branch master
    Your branch is up to date with 'origin/master'.

    nothing to commit, working tree clean
    12:34:23.901914 trace.c:417             performance: 0.118250995 s: git command: '/home/aearnfjord/g/git/git-status'

    real    0m0.125s
    user    0m0.086s
    sys     0m0.043s
    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     36.61    0.001281          61        21           clone
     33.84    0.001184          41        29           munmap
      5.09    0.000178           5        36           mprotect
      4.34    0.000152           0       619           brk
      4.17    0.000146           2        62           mmap
      3.34    0.000117           2        55        20 open
      3.00    0.000105           2        60        27 lstat
      1.77    0.000062           2        37         9 stat
      1.60    0.000056           1        51           read
      1.57    0.000055           5        12           write
      1.17    0.000041          41         1           wait4
      0.83    0.000029           1        41           close
      0.83    0.000029           1        22           getcwd
      0.80    0.000028           1        34           fstat

c) with fsmonitor + don't write index

    $ time GIT_TRACE_PERFORMANCE=1 GIT_OPTIONAL_LOCKS=0 ~/g/git/git-status
    12:36:03.176866 read-cache.c:1890       performance: 0.048292088 s: read cache .git/index
    12:36:03.181006 preload-index.c:112     performance: 0.001343593 s: preload index
    12:36:03.200970 fsmonitor.c:170         performance: 0.019936338 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
    12:36:03.210556 read-cache.c:1446       performance: 0.029525531 s: refresh index
    12:36:03.213366 diff-lib.c:250          performance: 0.002718730 s: diff-files
    12:36:03.216273 diff-lib.c:527          performance: 0.002666604 s: diff-index
    12:36:03.233579 dir.c:2290              performance: 0.017261702 s: read directory
    On branch master
    Your branch is up to date with 'origin/master'.

    nothing to commit, working tree clean
    12:36:03.233733 trace.c:417             performance: 0.105632105 s: git command: '/home/aearnfjord/g/git/git-status'

    real    0m0.111s
    user    0m0.073s
    sys     0m0.044s
    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     24.42    0.001081          37        29           munmap
     20.74    0.000918          44        21           clone
      7.63    0.000338           5        62           mmap
      7.43    0.000329           6        54        20 open
      6.05    0.000268           7        36           mprotect
      5.99    0.000265           0       619           brk
      4.99    0.000221           4        60        27 lstat
      4.13    0.000183           4        51           read
      3.68    0.000163           9        19        10 access
      3.25    0.000144           4        34           fstat
      2.78    0.000123           3        40           close
      2.48    0.000110           3        37         9 stat
      2.24    0.000099           5        21           getcwd
      1.15    0.000051           4        12           write
      0.99    0.000044          22         2           poll

For comparison just the output from the hook:

    $ time ('.git/hooks/fsmonitor-watchman' '1' '1517052856494406191')
    .git
    real    0m0.017s
    user    0m0.011s
    sys     0m0.003s

And this is what goes on behind the scenes after we get rid of the
.git/hooks/fsmonitor-watchman overhead:
    
    $ time (echo '["query", "/home/aearnfjord/git_tree/2015-04-03-1M-git", {
                            "since": 1517052856,
                            "fields": ["name"],
                            "expression": ["not", ["allof", ["since", 1517052856, "cclock"], ["not", "exists"]]]
                    }]' | watchman -j)
    {
        "version": "4.9.0",
        "is_fresh_instance": false,
        "clock": "c:1517006351:31165:4:1113968",
        "files": [
            ".git"
        ]
    }
    
    real    0m0.003s
    user    0m0.000s
    sys     0m0.004s

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> a) no fsmonitor
>
>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index
>     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index
>     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index
>
> ...
>
> b) with fsmonitor
>
>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
>     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index
>     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
>     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index

Hmm.. why does refresh take longer with fsmonitor/watchman? With the
help from watchman, we know what files are modified. We don't need
manual stat()'ing and this line should be lower than the "no
fsmonitor" case, which is 0.006230611s.

>     12:34:23.876427 diff-lib.c:250          performance: 0.004731427 s: diff-files
>     12:34:23.880669 diff-lib.c:527          performance: 0.003944422 s: diff-index
>     12:34:23.899225 dir.c:2290              performance: 0.018509066 s: read directory
>     12:34:23.901914 trace.c:417             performance: 0.118250995 s: git command: '/home/aearnfjord/g/git/git-status'

I don't see any "write index" line here, which is interesting since
your case c) is about "don't write index".

> c) with fsmonitor + don't write index
-- 
Duy

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:
> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > a) no fsmonitor
> >
> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
> >     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index
> >     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index
> >     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index
> >
> > ...
> >
> > b) with fsmonitor
> >
> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
> >     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
> >     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index
> >     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
> >     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index
> 
> Hmm.. why does refresh take longer with fsmonitor/watchman? With the
> help from watchman, we know what files are modified. We don't need
> manual stat()'ing and this line should be lower than the "no
> fsmonitor" case, which is 0.006230611s.

Ahh.. my patch probably does not see that fsmonitor could be activated
lazily inside refresh_index() call. The patch below should fix it.

But between your normal refresh time (0.020 preload + 0.006 actual
refresh) and fsmonitor taking 0.020 just to talk to watchman, this
repo seems "too small" for fsmonitor/watchman to shine.

I'm still a bit curious that refresh index time, after excluding 0.020
for fsmonitor, is stil 0.012s. What does it do? It should really be
doing nothing. Either way, read index time seems to be the elephant in
the room now.

-- 8< --
diff --git a/read-cache.c b/read-cache.c
index eac74bc9f1..d60e0a8480 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	unsigned int options = (CE_MATCH_REFRESH |
 				(really ? CE_MATCH_IGNORE_VALID : 0) |
 				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
+	int ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;
 	const char *modified_fmt;
 	const char *deleted_fmt;
 	const char *typechange_fmt;
 	const char *added_fmt;
 	const char *unmerged_fmt;
-	uint64_t start = getnanotime();
+	uint64_t start;
+
+	/*
+	 * If fsmonitor is used, force its communication early to
+	 * accurately measure how long this function takes without it.
+	 */
+	if (!ignore_fsmonitor)
+		refresh_fsmonitor(istate);
+	start = getnanotime();
 
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
-- 8< --

Re: Some rough edges of core.fsmonitor

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 27 2018, Duy Nguyen jotted:

> On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:
>> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> > a) no fsmonitor
>> >
>> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>> >     12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index
>> >     12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index
>> >     12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index
>> >
>> > ...
>> >
>> > b) with fsmonitor
>> >
>> >     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>> >     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
>> >     12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index
>> >     12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
>> >     12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index
>>
>> Hmm.. why does refresh take longer with fsmonitor/watchman? With the
>> help from watchman, we know what files are modified. We don't need
>> manual stat()'ing and this line should be lower than the "no
>> fsmonitor" case, which is 0.006230611s.
>
> Ahh.. my patch probably does not see that fsmonitor could be activated
> lazily inside refresh_index() call. The patch below should fix it.

Will have to get those numbers to you later, or alternatively clone
https://github.com/avar/2015-04-03-1M-git (or some other test repo) and
test it yourself, sorry. Don't have time to follow-up much this weekend.

> But between your normal refresh time (0.020 preload + 0.006 actual
> refresh) and fsmonitor taking 0.020 just to talk to watchman, this
> repo seems "too small" for fsmonitor/watchman to shine.

Surely that's an implementation limitation and not something inherent,
given that watchman itself returns in 5ms?

I.e. status could work like this, no?:

 1. At start, record the timestamp & find out canonical state via some
    expansive method.
 2. Print out xyz changed, abc added etc.
 3. Record *just* what status would report about xyz, abc etc.
 4. On subsequent git status, just amend that information, e.g. if
    watchman says nothing changed $(cat .git/last-status-output).

We shouldn't need to be reading the entire index in the common case
where just a few things change.

There's also a lot of things that use status to just check "are we
clean?", those would only need to record the last known timestamp when
the tree was clean, and then ask watchman if there were any changes, if
not we're done.

> I'm still a bit curious that refresh index time, after excluding 0.020
> for fsmonitor, is stil 0.012s. What does it do? It should really be
> doing nothing. Either way, read index time seems to be the elephant in
> the room now.
>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index eac74bc9f1..d60e0a8480 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  	unsigned int options = (CE_MATCH_REFRESH |
>  				(really ? CE_MATCH_IGNORE_VALID : 0) |
>  				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
> +	int ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;
>  	const char *modified_fmt;
>  	const char *deleted_fmt;
>  	const char *typechange_fmt;
>  	const char *added_fmt;
>  	const char *unmerged_fmt;
> -	uint64_t start = getnanotime();
> +	uint64_t start;
> +
> +	/*
> +	 * If fsmonitor is used, force its communication early to
> +	 * accurately measure how long this function takes without it.
> +	 */
> +	if (!ignore_fsmonitor)
> +		refresh_fsmonitor(istate);
> +	start = getnanotime();
>
>  	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
>  	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
> -- 8< --

Re: Some rough edges of core.fsmonitor

[Ben Peart]

On 1/27/2018 2:01 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Jan 27 2018, Duy Nguyen jotted:
> 
>> On Sat, Jan 27, 2018 at 07:39:27PM +0700, Duy Nguyen wrote:
>>> On Sat, Jan 27, 2018 at 6:43 PM, Ævar Arnfjörð Bjarmason
>>> <avarab@gmail.com> wrote:
>>>> a) no fsmonitor
>>>>
>>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>>>      12:32:44.947651 read-cache.c:1890       performance: 0.053153609 s: read cache .git/index
>>>>      12:32:44.967943 preload-index.c:112     performance: 0.020161093 s: preload index
>>>>      12:32:44.974217 read-cache.c:1446       performance: 0.006230611 s: refresh index
>>>>
>>>> ...
>>>>
>>>> b) with fsmonitor
>>>>
>>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
>>>>      12:34:23.838622 preload-index.c:112     performance: 0.001221197 s: preload index
>>>>      12:34:23.858723 fsmonitor.c:170         performance: 0.020059647 s: fsmonitor process '.git/hooks/fsmonitor-watchman'
>>>>      12:34:23.871532 read-cache.c:1446       performance: 0.032870818 s: refresh index
>>>
>>> Hmm.. why does refresh take longer with fsmonitor/watchman? With the
>>> help from watchman, we know what files are modified. We don't need
>>> manual stat()'ing and this line should be lower than the "no
>>> fsmonitor" case, which is 0.006230611s.
>>
>> Ahh.. my patch probably does not see that fsmonitor could be activated
>> lazily inside refresh_index() call. The patch below should fix it.
> 
> Will have to get those numbers to you later, or alternatively clone
> https://github.com/avar/2015-04-03-1M-git (or some other test repo) and
> test it yourself, sorry. Don't have time to follow-up much this weekend.
> 
>> But between your normal refresh time (0.020 preload + 0.006 actual
>> refresh) and fsmonitor taking 0.020 just to talk to watchman, this
>> repo seems "too small" for fsmonitor/watchman to shine.
> 
> Surely that's an implementation limitation and not something inherent,
> given that watchman itself returns in 5ms?
> 
> I.e. status could work like this, no?:
> 
>   1. At start, record the timestamp & find out canonical state via some
>      expansive method.
>   2. Print out xyz changed, abc added etc.
>   3. Record *just* what status would report about xyz, abc etc.
>   4. On subsequent git status, just amend that information, e.g. if
>      watchman says nothing changed $(cat .git/last-status-output).
> 
> We shouldn't need to be reading the entire index in the common case
> where just a few things change.
> 

I agree that reading the entire index in the common case is rather 
expensive. It is, however, the model we have today and all the code in 
git assumes all cache entries are in memory.

We are interested in pursuing a patch series that would enable higher 
performance especially with large and/or sparse repos by making the 
index sparse, hierarchical, and incrementally readable/writable.  As you 
might expect, that is a lot of work and is far beyond what we can 
address in this patch series.

> There's also a lot of things that use status to just check "are we
> clean?", those would only need to record the last known timestamp when
> the tree was clean, and then ask watchman if there were any changes, if
> not we're done.
> 
>> I'm still a bit curious that refresh index time, after excluding 0.020
>> for fsmonitor, is stil 0.012s. What does it do? It should really be
>> doing nothing. Either way, read index time seems to be the elephant in
>> the room now.
>>
>> -- 8< --
>> diff --git a/read-cache.c b/read-cache.c
>> index eac74bc9f1..d60e0a8480 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1367,12 +1367,21 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>>   	unsigned int options = (CE_MATCH_REFRESH |
>>   				(really ? CE_MATCH_IGNORE_VALID : 0) |
>>   				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
>> +	int ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;
>>   	const char *modified_fmt;
>>   	const char *deleted_fmt;
>>   	const char *typechange_fmt;
>>   	const char *added_fmt;
>>   	const char *unmerged_fmt;
>> -	uint64_t start = getnanotime();
>> +	uint64_t start;
>> +
>> +	/*
>> +	 * If fsmonitor is used, force its communication early to
>> +	 * accurately measure how long this function takes without it.
>> +	 */
>> +	if (!ignore_fsmonitor)
>> +		refresh_fsmonitor(istate);
>> +	start = getnanotime();
>>
>>   	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
>>   	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
>> -- 8< --

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
> b) with fsmonitor
> 
>     $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>     12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index

This is sort of off topic but may be interesting for big repo guys. It
looks like read cache's time is partially dominated by malloc().

This is the performance breakdown of do_read_index()

$ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache
0.000078489 s: open/mmap/close
0.038915239 s: main entries
0.018983150 s: ext TREE
0.012667080 s: ext UNTR
0.000005372 s: ext FSMN
0.001473470 s: munmap
0.072386911 s: read cache .git/index

Reading main index entries takes like half of the time (0.038 vs
0.072). With the below patch to take out hundred thousands of malloc()
we have this, loading main entries now only takes 0.012s:

$ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache
0.000046587 s: open/mmap/close
0.012077300 s: main entries
0.020477683 s: ext TREE
0.010259998 s: ext UNTR
0.000010250 s: ext FSMN
0.000753854 s: munmap
0.043906473 s: read cache .git/index

We used to do less malloc until debed2a629 (read-cache.c: allocate
index entries individually - 2011-10-24) but I don't think we can
simply revert that (not worth the extra complexity of the old way).

Now "TREE" and "UNTR" extensions become a bigger problem.

-- 8< --
diff --git a/read-cache.c b/read-cache.c
index d60e0a8480..88f4213c99 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1622,7 +1622,12 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on
 						   const char *name,
 						   size_t len)
 {
+#if 0
 	struct cache_entry *ce = xmalloc(cache_entry_size(len));
+#else
+	static char buf[1024];
+	struct cache_entry *ce = (struct cache_entry *)buf;
+#endif
 
 	ce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);
 	ce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 48255eef31..e1d21d17a3 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -8,7 +8,9 @@ int cmd_main(int argc, const char **argv)
 	setup_git_directory();
 	for (i = 0; i < cnt; i++) {
 		read_cache();
+#if 0
 		discard_cache();
+#endif
 	}
 	return 0;
 }
-- 8< --

Re: Some rough edges of core.fsmonitor

[Ben Peart]

On 1/29/2018 4:40 AM, Duy Nguyen wrote:
> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> b) with fsmonitor
>>
>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s: read cache .git/index
> 
> This is sort of off topic but may be interesting for big repo guys. It
> looks like read cache's time is partially dominated by malloc().
> 

That is correct.  We have tried a few different ways to address this. 
First was my patch series [1] that would parallelize all of the read 
cache code.

We quickly found that malloc() was the biggest culprit and by speeding 
that up, we got most of the wins.  At Peff's recommendation [2], we 
looked into using tcmalloc but found that 1) it has bugs on Windows and 
2) it isn't being actively maintained so it didn't seem those bugs would 
ever get fixed.

We are currently working on a patch that will use a refactored version 
of the mem_pool in fast-import.c to do block allocations of the cache 
entries which is giving us about a 22% improvement in "git status" 
times.  One challenge has been ensuring that cache entries are not 
passed from one index/mem_pool to another which could cause access after 
free bugs.

[1] 
https://public-inbox.org/git/20171109141737.47976-1-benpeart@microsoft.com/
[2] 
https://public-inbox.org/git/20171120153846.v5b7ho42yzrznqoh@sigill.intra.peff.net/


> This is the performance breakdown of do_read_index()
> 
> $ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache
> 0.000078489 s: open/mmap/close
> 0.038915239 s: main entries
> 0.018983150 s: ext TREE
> 0.012667080 s: ext UNTR
> 0.000005372 s: ext FSMN
> 0.001473470 s: munmap
> 0.072386911 s: read cache .git/index
> 
> Reading main index entries takes like half of the time (0.038 vs
> 0.072). With the below patch to take out hundred thousands of malloc()
> we have this, loading main entries now only takes 0.012s:
> 
> $ GIT_TRACE_PERFORMANCE=2 ~/w/git/t/helper/test-read-cache
> 0.000046587 s: open/mmap/close
> 0.012077300 s: main entries
> 0.020477683 s: ext TREE
> 0.010259998 s: ext UNTR
> 0.000010250 s: ext FSMN
> 0.000753854 s: munmap
> 0.043906473 s: read cache .git/index
> 
> We used to do less malloc until debed2a629 (read-cache.c: allocate
> index entries individually - 2011-10-24) but I don't think we can
> simply revert that (not worth the extra complexity of the old way).
> 
> Now "TREE" and "UNTR" extensions become a bigger problem.
> 
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index d60e0a8480..88f4213c99 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1622,7 +1622,12 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on
>   						   const char *name,
>   						   size_t len)
>   {
> +#if 0
>   	struct cache_entry *ce = xmalloc(cache_entry_size(len));
> +#else
> +	static char buf[1024];
> +	struct cache_entry *ce = (struct cache_entry *)buf;
> +#endif
>   
>   	ce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);
>   	ce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);
> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
> index 48255eef31..e1d21d17a3 100644
> --- a/t/helper/test-read-cache.c
> +++ b/t/helper/test-read-cache.c
> @@ -8,7 +8,9 @@ int cmd_main(int argc, const char **argv)
>   	setup_git_directory();
>   	for (i = 0; i < cnt; i++) {
>   		read_cache();
> +#if 0
>   		discard_cache();
> +#endif
>   	}
>   	return 0;
>   }
> -- 8< --
> 

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Tue, Jan 30, 2018 at 6:16 AM, Ben Peart <peartben@gmail.com> wrote:
>
>
> On 1/29/2018 4:40 AM, Duy Nguyen wrote:
>>
>> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> b) with fsmonitor
>>>
>>>      $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>>      12:34:23.833625 read-cache.c:1890       performance: 0.049485685 s:
>>> read cache .git/index
>>
>>
>> This is sort of off topic but may be interesting for big repo guys. It
>> looks like read cache's time is partially dominated by malloc().
>>
>
> That is correct.  We have tried a few different ways to address this. First
> was my patch series [1] that would parallelize all of the read cache code.
>
> We quickly found that malloc() was the biggest culprit and by speeding that
> up, we got most of the wins.  At Peff's recommendation [2], we looked into
> using tcmalloc but found that 1) it has bugs on Windows and 2) it isn't
> being actively maintained so it didn't seem those bugs would ever get fixed.
>
> We are currently working on a patch that will use a refactored version of
> the mem_pool in fast-import.c to do block allocations of the cache entries
> which is giving us about a 22% improvement in "git status" times.  One

My apologies if this has been discussed in the second half of 2017
which I have no idea what happened.

I just wonder if it's possible to design a "file format" that is
basically a memory dump of lots of struct cache_entry? This "file"
will stay in a shared memory somewhere and never get written down to
disk. Since it's very close to the data structure we have in core, the
most we need to do after mmap'ing it (and keeping it mmap'd until the
end) is adjust some pointers. These entries are of course read-only.
When you modify/create new entries, they are created new, using the
old malloc(). We just need to make sure not free the read-only
cache_entry entries and munmap() the whole thing when we
discard_index().

This opens up another option to deal with the large UNTR and TREE
extensions in a similar way. These will be your next headache after
you have reduced parse time for main entries.

> challenge has been ensuring that cache entries are not passed from one
> index/mem_pool to another which could cause access after free bugs.

We kind of have something close to that, but not entirely the same.
When split index is used, the same cache_entry can appear in two
index_state structs. Of course you can free only one of them (and you
can only do so when you know both index_state are gone). I see some
code cleanup opportunity :)

>
> [1]
> https://public-inbox.org/git/20171109141737.47976-1-benpeart@microsoft.com/
> [2]
> https://public-inbox.org/git/20171120153846.v5b7ho42yzrznqoh@sigill.intra.peff.net/
-- 
Duy

Re: Some rough edges of core.fsmonitor

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

Hi,

On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> I just got around to testing this since it landed, for context some
> previous poking of mine in [1].
> 
> Issues / stuff I've noticed:
> 
> 1) We end up invalidating the untracked cache because stuff in .git/
> changed. For example:
> 
>     01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>     01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>     01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>     01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>     01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
> 
> Am I missing something or should we do something like:
> 
>     diff --git a/fsmonitor.c b/fsmonitor.c
>     index 0af7c4edba..5067b89bda 100644
>     --- a/fsmonitor.c
>     +++ b/fsmonitor.c
>     @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
> 
>      static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>      {
>     -       int pos = index_name_pos(istate, name, strlen(name));
>     +       int pos;
>     +
>     +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>     +               return;
>     +
>     +       pos = index_name_pos(istate, name, strlen(name));

I would much rather have the fsmonitor hook already exclude those.

If you *must* add these comparisons, you have to use fspathcmp() and
fspathncmp() instead (because case-insensitivity).

Ciao,
Dscho

Re: Some rough edges of core.fsmonitor

[Ævar Arnfjörð Bjarmason]

On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> I just got around to testing this since it landed, for context some
>> previous poking of mine in [1].
>>
>> Issues / stuff I've noticed:
>>
>> 1) We end up invalidating the untracked cache because stuff in .git/
>> changed. For example:
>>
>>     01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>>     01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>>     01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>>     01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>>     01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
>>
>> Am I missing something or should we do something like:
>>
>>     diff --git a/fsmonitor.c b/fsmonitor.c
>>     index 0af7c4edba..5067b89bda 100644
>>     --- a/fsmonitor.c
>>     +++ b/fsmonitor.c
>>     @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>>
>>      static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>>      {
>>     -       int pos = index_name_pos(istate, name, strlen(name));
>>     +       int pos;
>>     +
>>     +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>     +               return;
>>     +
>>     +       pos = index_name_pos(istate, name, strlen(name));
>
> I would much rather have the fsmonitor hook already exclude those.

As documented the fsmonitor-watchman hook we ship doesn't work as
described in githooks(5), unless "files in the working directory" is
taken to include .git/, but I haven't seen that ever used.

On the other hand relying on arbitrary user-provided hooks to do the
right thing at the cost of silent performance degradation is bad. If
we're going to expect the hook to remove these we should probably
warn/die here if it does send us .git/* files.

> If you *must* add these comparisons, you have to use fspathcmp() and
> fspathncmp() instead (because case-insensitivity).

Thanks.

Re: Some rough edges of core.fsmonitor

[Ben Peart]

On 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi,
>>
>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>>
>>> I just got around to testing this since it landed, for context some
>>> previous poking of mine in [1].
>>>
>>> Issues / stuff I've noticed:
>>>
>>> 1) We end up invalidating the untracked cache because stuff in .git/
>>> changed. For example:
>>>
>>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
>>>
>>> Am I missing something or should we do something like:
>>>
>>>      diff --git a/fsmonitor.c b/fsmonitor.c
>>>      index 0af7c4edba..5067b89bda 100644
>>>      --- a/fsmonitor.c
>>>      +++ b/fsmonitor.c
>>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>>>
>>>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>>>       {
>>>      -       int pos = index_name_pos(istate, name, strlen(name));
>>>      +       int pos;
>>>      +
>>>      +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>>      +               return;
>>>      +
>>>      +       pos = index_name_pos(istate, name, strlen(name));
>>
>> I would much rather have the fsmonitor hook already exclude those.
> 
> As documented the fsmonitor-watchman hook we ship doesn't work as
> described in githooks(5), unless "files in the working directory" is
> taken to include .git/, but I haven't seen that ever used.
> 
> On the other hand relying on arbitrary user-provided hooks to do the
> right thing at the cost of silent performance degradation is bad. If
> we're going to expect the hook to remove these we should probably
> warn/die here if it does send us .git/* files.
> 

I'm not sure how often something is modified in the git directory when 
nothing was modified in the working directory but this seems like a nice 
optimization.

We can't just blindly ignore changes under ".git" as the git directory 
may have been moved somewhere else.  Instead we'd need to use get_git_dir().

Rather than assuming the hook will optimize for this particular case, I 
think a better solution would be to update 
untracked_cache_invalidate_path() so that it doesn't invalidate the 
untracked cache and mark the index as dirty when it was asked to 
invalidate a path under GIT_DIR.  I can't think of a case when that 
would be the desired behavior.

Somewhat off topic but related to the overall performance discussion: 
I've also thought the untracked cache shouldn't mark the index as dirty 
except in the case where the extension is being added or removed.  We've 
observed that it causes unnecessary index writes that actually slows 
down overall performance.

Since it is a cache, it does not require the index to be written out for 
correctness, it can simply update the cache again the next time it is 
needed. This is typically faster than the cost of the index write so 
makes things faster overall.  I adopted this same model with the 
fsmonitor extension.

>> If you *must* add these comparisons, you have to use fspathcmp() and
>> fspathncmp() instead (because case-insensitivity).
> 
> Thanks.
> 

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Tue, Jan 30, 2018 at 8:21 AM, Ben Peart <peartben@gmail.com> wrote:
>
>
> On 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>>
>>> Hi,
>>>
>>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>>>
>>>> I just got around to testing this since it landed, for context some
>>>> previous poking of mine in [1].
>>>>
>>>> Issues / stuff I've noticed:
>>>>
>>>> 1) We end up invalidating the untracked cache because stuff in .git/
>>>> changed. For example:
>>>>
>>>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process
>>>> '.git/hooks/fsmonitor-watchman' returned success
>>>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git'
>>>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git/config'
>>>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback
>>>> '.git/index'
>>>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension
>>>> successful
>>>>
>>>> Am I missing something or should we do something like:
>>>>
>>>>      diff --git a/fsmonitor.c b/fsmonitor.c
>>>>      index 0af7c4edba..5067b89bda 100644
>>>>      --- a/fsmonitor.c
>>>>      +++ b/fsmonitor.c
>>>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version,
>>>> uint64_t last_update, struct strbuf *que
>>>>
>>>>       static void fsmonitor_refresh_callback(struct index_state *istate,
>>>> const char *name)
>>>>       {
>>>>      -       int pos = index_name_pos(istate, name, strlen(name));
>>>>      +       int pos;
>>>>      +
>>>>      +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>>>      +               return;
>>>>      +
>>>>      +       pos = index_name_pos(istate, name, strlen(name));
>>>
>>>
>>> I would much rather have the fsmonitor hook already exclude those.
>>
>>
>> As documented the fsmonitor-watchman hook we ship doesn't work as
>> described in githooks(5), unless "files in the working directory" is
>> taken to include .git/, but I haven't seen that ever used.
>>
>> On the other hand relying on arbitrary user-provided hooks to do the
>> right thing at the cost of silent performance degradation is bad. If
>> we're going to expect the hook to remove these we should probably
>> warn/die here if it does send us .git/* files.
>>
>
> I'm not sure how often something is modified in the git directory when
> nothing was modified in the working directory but this seems like a nice
> optimization.
>
> We can't just blindly ignore changes under ".git" as the git directory may
> have been moved somewhere else.  Instead we'd need to use get_git_dir().

In theory. But we do blindly ignore changes under ".git" in some
cases, see treat_path() in dir.c for example.

> Rather than assuming the hook will optimize for this particular case, I
> think a better solution would be to update untracked_cache_invalidate_path()
> so that it doesn't invalidate the untracked cache and mark the index as
> dirty when it was asked to invalidate a path under GIT_DIR.  I can't think
> of a case when that would be the desired behavior.

You see, my only problem with this is tying the check with $GIT_DIR,
which may involve normalizing the path and all that stuff because the
current code base is not prepared to deal with that. Simply ignoring
anything in ".git" may work too. Not pretty but it's more in line with
what we have. Though I'm still not sure how it interacts with ".git"
from submodules which is why I still have not sent a patch to update
untracked_cache_invalidate_path() because it does make sense to fix it
in there.

> Somewhat off topic but related to the overall performance discussion: I've
> also thought the untracked cache shouldn't mark the index as dirty except in
> the case where the extension is being added or removed.  We've observed that
> it causes unnecessary index writes that actually slows down overall
> performance.
>
> Since it is a cache, it does not require the index to be written out for
> correctness, it can simply update the cache again the next time it is
> needed. This is typically faster than the cost of the index write so makes
> things faster overall.  I adopted this same model with the fsmonitor
> extension.

If you turn on split index, the write cost should be much much less
(but I think read cost increases a bit due to merging the two indexes
in core; I noticed this but haven't really dug down). You basically
pay writing modified index entries and extensions.

But yeah not writing is possible. The index's dirty flag can show that
only untracked cache extension is dirty, then it's a judgement call
whether to write it down or drop it. You still need to occasionally
write it down though. Dirty directories will fall back to slow path.
If you don't write it down, the set of dirty paths keeps increasing
and will start to slow git-status down. I don't know at what point we
should write it down though if you choose not to go the split-index
route.
-- 
Duy

[PATCH] dir.c: ignore paths containing .git when invalidating untracked cache

[Nguyễn Thái Ngọc Duy]

read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.

This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".

Without this patch, we invalidate the root directory unncessarily,
which:

- makes read_directory() fall back to slow path for root directory
  (slower)

- makes the index dirty (because UNTR extension is updated). Depending
  on the index size, writing it down could also be slow.

Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Sorry for the resend, I forgot git@vger.

 dir.c             | 13 ++++++++++++-
 git-compat-util.h |  2 ++
 wrapper.c         | 12 ++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 7c4b45e30e..f8b4cabba9 100644
--- a/dir.c
+++ b/dir.c
@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 	if (!de)
 		return treat_path_fast(dir, untracked, cdir, istate, path,
 				       baselen, pathspec);
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+	if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
 		return path_none;
 	strbuf_setlen(path, baselen);
 	strbuf_addstr(path, de->d_name);
@@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct untracked_cache *uc,
 void untracked_cache_invalidate_path(struct index_state *istate,
 				     const char *path)
 {
+	const char *end;
+	int skipped;
+
 	if (!istate->untracked || !istate->untracked->root)
 		return;
+	if (!fspathcmp(path, ".git"))
+		return;
+	if (ignore_case)
+		skipped = skip_caseprefix(path, "/.git", &end);
+	else
+		skipped = skip_prefix(path, "/.git", &end);
+	if (skipped && (*end == '\0' || *end == '/'))
+		return;
 	invalidate_one_component(istate->untracked, istate->untracked->root,
 				 path, strlen(path));
 }
diff --git a/git-compat-util.h b/git-compat-util.h
index 68b2ad531e..27e0b761a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -484,6 +484,8 @@ static inline int skip_prefix(const char *str, const char *prefix,
 	return 0;
 }
 
+int skip_caseprefix(const char *str, const char *prefix, const char **out);
+
 /*
  * If the string "str" is the same as the string in "prefix", then the "arg"
  * parameter is set to the "def" parameter and 1 is returned.
diff --git a/wrapper.c b/wrapper.c
index d20356a776..bb888d9401 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -690,3 +690,15 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }
+
+int skip_caseprefix(const char *str, const char *prefix, const char **out)
+{
+	do {
+		if (!*prefix) {
+			*out = str;
+			return 1;
+		}
+	} while (tolower(*str++) == tolower(*prefix++));
+
+	return 0;
+}
-- 
2.16.1.207.gedba492059

Re: [PATCH] dir.c: ignore paths containing .git when invalidating untracked cache

[Ben Peart]

On 2/4/2018 4:38 AM, Nguyễn Thái Ngọc Duy wrote:
> read_directory() code ignores all paths named ".git" even if it's not
> a valid git repository. See treat_path() for details. Since ".git" is
> basically invisible to read_directory(), when we are asked to
> invalidate a path that contains ".git", we can safely ignore it
> because the slow path would not consider it anyway.
> 
> This helps when fsmonitor is used and we have a real ".git" repo at
> worktree top. Occasionally .git/index will be updated and if the
> fsmonitor hook does not filter it, untracked cache is asked to
> invalidate the path ".git/index".
> 
> Without this patch, we invalidate the root directory unncessarily,
> which:
> 
> - makes read_directory() fall back to slow path for root directory
>    (slower)
> 
> - makes the index dirty (because UNTR extension is updated). Depending
>    on the index size, writing it down could also be slow.
> 
> Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   Sorry for the resend, I forgot git@vger.
> 
>   dir.c             | 13 ++++++++++++-
>   git-compat-util.h |  2 ++
>   wrapper.c         | 12 ++++++++++++
>   3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 7c4b45e30e..f8b4cabba9 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
>   	if (!de)
>   		return treat_path_fast(dir, untracked, cdir, istate, path,
>   				       baselen, pathspec);
> -	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
> +	if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
>   		return path_none;
>   	strbuf_setlen(path, baselen);
>   	strbuf_addstr(path, de->d_name);
> @@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct untracked_cache *uc,
>   void untracked_cache_invalidate_path(struct index_state *istate,
>   				     const char *path)
>   {
> +	const char *end;
> +	int skipped;
> +
>   	if (!istate->untracked || !istate->untracked->root)
>   		return;

Thank you for this patch!  It's great to see people helping improve the 
performance of git.

I ran a quick test and this is not sufficient to prevent the index from 
getting flagged as dirty and written out to disk when fsmonitor detects 
changes for files under the .git folder.  What I'm seeing is that when 
".git/index" is returned, the test below doesn't catch them and end 
early as would be expected.

As a result, invalidate_one_component() is called which calls 
invalidate_one_directory() which increments dir_invalidated counter and 
the regular dirty process continues which triggers the index to be 
written to disk unnecessarily.

> +	if (!fspathcmp(path, ".git"))
> +		return;
> +	if (ignore_case)
> +		skipped = skip_caseprefix(path, "/.git", &end);
> +	else
> +		skipped = skip_prefix(path, "/.git", &end);
> +	if (skipped && (*end == '\0' || *end == '/'))
> +		return;

If I replace the above lines with:

	if (ignore_case)
		skipped = skip_caseprefix(path, ".git", &end);
	else
		skipped = skip_prefix(path, ".git", &end);

Then it correctly skips _all_ files under ".git".  I'm not sure if by 
removing the leading slash, I'm breaking some other case but I was not 
able to find where that was expected or needed. Removing the leading 
slash also allows me to remove the fsmpathcmp() call as it is now redundant.

I wondered what would happen if there was some other directory named 
".git" and how that would behave.  With or without this patch and with 
or without the untracked cache, a file "dir1/.git/foo" is ignored by git 
so no change in behavior there either.

>   	invalidate_one_component(istate->untracked, istate->untracked->root,
>   				 path, strlen(path));
>   }
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 68b2ad531e..27e0b761a3 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -484,6 +484,8 @@ static inline int skip_prefix(const char *str, const char *prefix,
>   	return 0;
>   }
>   
> +int skip_caseprefix(const char *str, const char *prefix, const char **out);
> +
>   /*
>    * If the string "str" is the same as the string in "prefix", then the "arg"
>    * parameter is set to the "def" parameter and 1 is returned.
> diff --git a/wrapper.c b/wrapper.c
> index d20356a776..bb888d9401 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -690,3 +690,15 @@ int xgethostname(char *buf, size_t len)
>   		buf[len - 1] = 0;
>   	return ret;
>   }
> +
> +int skip_caseprefix(const char *str, const char *prefix, const char **out)
> +{
> +	do {
> +		if (!*prefix) {
> +			*out = str;
> +			return 1;
> +		}
> +	} while (tolower(*str++) == tolower(*prefix++));
> +
> +	return 0;
> +}
> 

Re: [PATCH] dir.c: ignore paths containing .git when invalidating untracked cache

[Duy Nguyen]

On Tue, Feb 6, 2018 at 12:44 AM, Ben Peart <peartben@gmail.com> wrote:
>
>
> On 2/4/2018 4:38 AM, Nguyễn Thái Ngọc Duy wrote:
>>
>> read_directory() code ignores all paths named ".git" even if it's not
>> a valid git repository. See treat_path() for details. Since ".git" is
>> basically invisible to read_directory(), when we are asked to
>> invalidate a path that contains ".git", we can safely ignore it
>> because the slow path would not consider it anyway.
>>
>> This helps when fsmonitor is used and we have a real ".git" repo at
>> worktree top. Occasionally .git/index will be updated and if the
>> fsmonitor hook does not filter it, untracked cache is asked to
>> invalidate the path ".git/index".
>>
>> Without this patch, we invalidate the root directory unncessarily,
>> which:
>>
>> - makes read_directory() fall back to slow path for root directory
>>    (slower)
>>
>> - makes the index dirty (because UNTR extension is updated). Depending
>>    on the index size, writing it down could also be slow.
>>
>> Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>   Sorry for the resend, I forgot git@vger.
>>
>>   dir.c             | 13 ++++++++++++-
>>   git-compat-util.h |  2 ++
>>   wrapper.c         | 12 ++++++++++++
>>   3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 7c4b45e30e..f8b4cabba9 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct
>> dir_struct *dir,
>>         if (!de)
>>                 return treat_path_fast(dir, untracked, cdir, istate, path,
>>                                        baselen, pathspec);
>> -       if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
>> +       if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name,
>> ".git"))
>>                 return path_none;
>>         strbuf_setlen(path, baselen);
>>         strbuf_addstr(path, de->d_name);
>> @@ -2970,8 +2970,19 @@ static int invalidate_one_component(struct
>> untracked_cache *uc,
>>   void untracked_cache_invalidate_path(struct index_state *istate,
>>                                      const char *path)
>>   {
>> +       const char *end;
>> +       int skipped;
>> +
>>         if (!istate->untracked || !istate->untracked->root)
>>                 return;
>
>
> Thank you for this patch!  It's great to see people helping improve the
> performance of git.
>
> I ran a quick test and this is not sufficient to prevent the index from
> getting flagged as dirty and written out to disk when fsmonitor detects
> changes for files under the .git folder.  What I'm seeing is that when
> ".git/index" is returned, the test below doesn't catch them and end early as
> would be expected.

Right. I focused too much on ".git" in the middle and the end of the
path and forgot that it's also at the beginning.

> As a result, invalidate_one_component() is called which calls
> invalidate_one_directory() which increments dir_invalidated counter and the
> regular dirty process continues which triggers the index to be written to
> disk unnecessarily.
>
>> +       if (!fspathcmp(path, ".git"))
>> +               return;
>> +       if (ignore_case)
>> +               skipped = skip_caseprefix(path, "/.git", &end);
>> +       else
>> +               skipped = skip_prefix(path, "/.git", &end);
>> +       if (skipped && (*end == '\0' || *end == '/'))
>> +               return;
>
>
> If I replace the above lines with:
>
>         if (ignore_case)
>                 skipped = skip_caseprefix(path, ".git", &end);
>         else
>                 skipped = skip_prefix(path, ".git", &end);
>
> Then it correctly skips _all_ files under ".git".  I'm not sure if by
> removing the leading slash, I'm breaking some other case but I was not able
> to find where that was expected or needed. Removing the leading slash also
> allows me to remove the fsmpathcmp() call as it is now redundant.

Removing "/" could catch things like abc/lala.git/def, which
treat_path does not consider special and may show up as untracked
entries. In that sense, the updated invalidate_... is broken if we
don't invalidate them properly.

I think what we need here is something like

    if (!fspathcmp(path, ".git") || starts_with(path, ".git/"))

but can handle case-insensitivity as well (starts_with can't).

> I wondered what would happen if there was some other directory named ".git"
> and how that would behave.  With or without this patch and with or without
> the untracked cache, a file "dir1/.git/foo" is ignored by git so no change
> in behavior there either.

That's what I meant by treat_path() and invisibility in my commit
message. We always ignore directories (or even files) named ".git". It
does not matter if it contains anything remotely related to git. I'm
not saying it's a good thing, but it's how it is and changing that
requires a lot more work, possibly some performance degradation if you
start to check if ".git" is a valid repo before ignoring. So I focus
on improving this function alone first.
-- 
Duy

[PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Nguyễn Thái Ngọc Duy]

read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.

This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".

Without this patch, we invalidate the root directory unncessarily,
which:

- makes read_directory() fall back to slow path for root directory
  (slower)

- makes the index dirty (because UNTR extension is updated). Depending
  on the index size, writing it down could also be slow.

A note about the new "safe_path" knob. Since this new check could be
relatively expensive, avoid it when we know it's not needed. If the
path comes from the index, it can't contain ".git". If it does
contain, we may be screwed up at many more levels, not just this one.

Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 My v1 was rubbish. It's no wonder Ben didn't see my intention. v2
 corrects the "is .git in a given path?" logic and adds a test to
 verify it.

 dir.c                       | 10 ++++++----
 dir.h                       |  2 +-
 fsmonitor.c                 |  2 +-
 fsmonitor.h                 |  2 +-
 t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++
 unpack-trees.c              |  2 +-
 6 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/dir.c b/dir.c
index 7c4b45e30e..fce45fc55e 100644
--- a/dir.c
+++ b/dir.c
@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 	if (!de)
 		return treat_path_fast(dir, untracked, cdir, istate, path,
 				       baselen, pathspec);
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+	if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
 		return path_none;
 	strbuf_setlen(path, baselen);
 	strbuf_addstr(path, de->d_name);
@@ -2968,10 +2968,12 @@ static int invalidate_one_component(struct untracked_cache *uc,
 }
 
 void untracked_cache_invalidate_path(struct index_state *istate,
-				     const char *path)
+				     const char *path, int safe_path)
 {
 	if (!istate->untracked || !istate->untracked->root)
 		return;
+	if (!safe_path && !verify_path(path))
+		return;
 	invalidate_one_component(istate->untracked, istate->untracked->root,
 				 path, strlen(path));
 }
@@ -2979,13 +2981,13 @@ void untracked_cache_invalidate_path(struct index_state *istate,
 void untracked_cache_remove_from_index(struct index_state *istate,
 				       const char *path)
 {
-	untracked_cache_invalidate_path(istate, path);
+	untracked_cache_invalidate_path(istate, path, 1);
 }
 
 void untracked_cache_add_to_index(struct index_state *istate,
 				  const char *path)
 {
-	untracked_cache_invalidate_path(istate, path);
+	untracked_cache_invalidate_path(istate, path, 1);
 }
 
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
diff --git a/dir.h b/dir.h
index 11a047ba48..06df057054 100644
--- a/dir.h
+++ b/dir.h
@@ -350,7 +350,7 @@ static inline int dir_path_match(const struct dir_entry *ent,
 int cmp_dir_entry(const void *p1, const void *p2);
 int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);
 
-void untracked_cache_invalidate_path(struct index_state *, const char *);
+void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edba..6d7bcd5d0e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -130,7 +130,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 	 * as it could be a new untracked file.
 	 */
 	trace_printf_key(&trace_fsmonitor, "fsmonitor_refresh_callback '%s'", name);
-	untracked_cache_invalidate_path(istate, name);
+	untracked_cache_invalidate_path(istate, name, 0);
 }
 
 void refresh_fsmonitor(struct index_state *istate)
diff --git a/fsmonitor.h b/fsmonitor.h
index cd3cc0ccf2..65f3743636 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac
 {
 	if (core_fsmonitor) {
 		ce->ce_flags &= ~CE_FSMONITOR_VALID;
-		untracked_cache_invalidate_path(istate, ce->name);
+		untracked_cache_invalidate_path(istate, ce->name, 1);
 		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
 	}
 }
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbcf..756beb0d8e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the same state' '
 	test_cmp expect actual
 '
 
+test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' '
+	test_create_repo dot-git &&
+	(
+		cd dot-git &&
+		mkdir -p .git/hooks &&
+		: >tracked &&
+		: >modified &&
+		mkdir dir1 &&
+		: >dir1/tracked &&
+		: >dir1/modified &&
+		mkdir dir2 &&
+		: >dir2/tracked &&
+		: >dir2/modified &&
+		write_integration_script &&
+		git config core.fsmonitor .git/hooks/fsmonitor-test &&
+		git update-index --untracked-cache &&
+		git update-index --fsmonitor &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-before" \
+		git status &&
+		test-dump-untracked-cache >../before
+	) &&
+	cat >>dot-git/.git/hooks/fsmonitor-test <<-\EOF &&
+	printf ".git\0"
+	printf ".git/index\0"
+	printf "dir1/.git\0"
+	printf "dir1/.git/index\0"
+	EOF
+	(
+		cd dot-git &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-after" \
+		git status &&
+		test-dump-untracked-cache >../after
+	) &&
+	grep "directory invalidation" trace-before >>before &&
+	grep "directory invalidation" trace-after >>after &&
+	# UNTR extension unchanged, dir invalidation count unchanged
+	test_cmp before after
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 96c3327f19..9a327696c5 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1506,7 +1506,7 @@ static void invalidate_ce_path(const struct cache_entry *ce,
 	if (!ce)
 		return;
 	cache_tree_invalidate_path(o->src_index, ce->name);
-	untracked_cache_invalidate_path(o->src_index, ce->name);
+	untracked_cache_invalidate_path(o->src_index, ce->name, 1);
 }
 
 /*
-- 
2.16.1.207.gedba492059

[PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Nguyễn Thái Ngọc Duy]

read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.

This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".

Without this patch, we invalidate the root directory unncessarily,
which:

- makes read_directory() fall back to slow path for root directory
  (slower)

- makes the index dirty (because UNTR extension is updated). Depending
  on the index size, writing it down could also be slow.

A note about the new "safe_path" knob. Since this new check could be
relatively expensive, avoid it when we know it's not needed. If the
path comes from the index, it can't contain ".git". If it does
contain, we may be screwed up at many more levels, not just this one.

Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 My v1 was rubbish. It's no wonder Ben didn't see my intention. v2
 corrects the "is .git in a given path?" logic and adds a test to
 verify it.

 dir.c                       | 10 ++++++----
 dir.h                       |  2 +-
 fsmonitor.c                 |  2 +-
 fsmonitor.h                 |  2 +-
 t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++
 unpack-trees.c              |  2 +-
 6 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/dir.c b/dir.c
index 7c4b45e30e..fce45fc55e 100644
--- a/dir.c
+++ b/dir.c
@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 	if (!de)
 		return treat_path_fast(dir, untracked, cdir, istate, path,
 				       baselen, pathspec);
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+	if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
 		return path_none;
 	strbuf_setlen(path, baselen);
 	strbuf_addstr(path, de->d_name);
@@ -2968,10 +2968,12 @@ static int invalidate_one_component(struct untracked_cache *uc,
 }
 
 void untracked_cache_invalidate_path(struct index_state *istate,
-				     const char *path)
+				     const char *path, int safe_path)
 {
 	if (!istate->untracked || !istate->untracked->root)
 		return;
+	if (!safe_path && !verify_path(path))
+		return;
 	invalidate_one_component(istate->untracked, istate->untracked->root,
 				 path, strlen(path));
 }
@@ -2979,13 +2981,13 @@ void untracked_cache_invalidate_path(struct index_state *istate,
 void untracked_cache_remove_from_index(struct index_state *istate,
 				       const char *path)
 {
-	untracked_cache_invalidate_path(istate, path);
+	untracked_cache_invalidate_path(istate, path, 1);
 }
 
 void untracked_cache_add_to_index(struct index_state *istate,
 				  const char *path)
 {
-	untracked_cache_invalidate_path(istate, path);
+	untracked_cache_invalidate_path(istate, path, 1);
 }
 
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
diff --git a/dir.h b/dir.h
index 11a047ba48..06df057054 100644
--- a/dir.h
+++ b/dir.h
@@ -350,7 +350,7 @@ static inline int dir_path_match(const struct dir_entry *ent,
 int cmp_dir_entry(const void *p1, const void *p2);
 int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry *in);
 
-void untracked_cache_invalidate_path(struct index_state *, const char *);
+void untracked_cache_invalidate_path(struct index_state *, const char *, int safe_path);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edba..6d7bcd5d0e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -130,7 +130,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 	 * as it could be a new untracked file.
 	 */
 	trace_printf_key(&trace_fsmonitor, "fsmonitor_refresh_callback '%s'", name);
-	untracked_cache_invalidate_path(istate, name);
+	untracked_cache_invalidate_path(istate, name, 0);
 }
 
 void refresh_fsmonitor(struct index_state *istate)
diff --git a/fsmonitor.h b/fsmonitor.h
index cd3cc0ccf2..65f3743636 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac
 {
 	if (core_fsmonitor) {
 		ce->ce_flags &= ~CE_FSMONITOR_VALID;
-		untracked_cache_invalidate_path(istate, ce->name);
+		untracked_cache_invalidate_path(istate, ce->name, 1);
 		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
 	}
 }
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbcf..756beb0d8e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the same state' '
 	test_cmp expect actual
 '
 
+test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' '
+	test_create_repo dot-git &&
+	(
+		cd dot-git &&
+		mkdir -p .git/hooks &&
+		: >tracked &&
+		: >modified &&
+		mkdir dir1 &&
+		: >dir1/tracked &&
+		: >dir1/modified &&
+		mkdir dir2 &&
+		: >dir2/tracked &&
+		: >dir2/modified &&
+		write_integration_script &&
+		git config core.fsmonitor .git/hooks/fsmonitor-test &&
+		git update-index --untracked-cache &&
+		git update-index --fsmonitor &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-before" \
+		git status &&
+		test-dump-untracked-cache >../before
+	) &&
+	cat >>dot-git/.git/hooks/fsmonitor-test <<-\EOF &&
+	printf ".git\0"
+	printf ".git/index\0"
+	printf "dir1/.git\0"
+	printf "dir1/.git/index\0"
+	EOF
+	(
+		cd dot-git &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-after" \
+		git status &&
+		test-dump-untracked-cache >../after
+	) &&
+	grep "directory invalidation" trace-before >>before &&
+	grep "directory invalidation" trace-after >>after &&
+	# UNTR extension unchanged, dir invalidation count unchanged
+	test_cmp before after
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 96c3327f19..9a327696c5 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1506,7 +1506,7 @@ static void invalidate_ce_path(const struct cache_entry *ce,
 	if (!ce)
 		return;
 	cache_tree_invalidate_path(o->src_index, ce->name);
-	untracked_cache_invalidate_path(o->src_index, ce->name);
+	untracked_cache_invalidate_path(o->src_index, ce->name, 1);
 }
 
 /*
-- 
2.16.1.207.gedba492059

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Ben Peart]

On 2/7/2018 4:21 AM, Nguyễn Thái Ngọc Duy wrote:
> read_directory() code ignores all paths named ".git" even if it's not
> a valid git repository. See treat_path() for details. Since ".git" is
> basically invisible to read_directory(), when we are asked to
> invalidate a path that contains ".git", we can safely ignore it
> because the slow path would not consider it anyway.
> 
> This helps when fsmonitor is used and we have a real ".git" repo at
> worktree top. Occasionally .git/index will be updated and if the
> fsmonitor hook does not filter it, untracked cache is asked to
> invalidate the path ".git/index".
> 
> Without this patch, we invalidate the root directory unncessarily,
> which:
> 
> - makes read_directory() fall back to slow path for root directory
>    (slower)
> 
> - makes the index dirty (because UNTR extension is updated). Depending
>    on the index size, writing it down could also be slow.
> 

Thank you again, this patch makes much more sense to me.

> A note about the new "safe_path" knob. Since this new check could be
> relatively expensive, avoid it when we know it's not needed. If the
> path comes from the index, it can't contain ".git". If it does
> contain, we may be screwed up at many more levels, not just this one.
> 

I do have a simplifying suggestion to make.  I noticed that other uses 
of verify_path() check when the potentially erroneous path is passed in 
and then all the underlying code can assume it is valid.  I think that 
makes sense here as well and it makes for a smaller patch.


> diff --git a/fsmonitor.h b/fsmonitor.h
> index cd3cc0ccf2..65f3743636 100644
> --- a/fsmonitor.h
> +++ b/fsmonitor.h
> @@ -65,7 +65,7 @@ static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cac
>   {
>   	if (core_fsmonitor) {
>   		ce->ce_flags &= ~CE_FSMONITOR_VALID;
> -		untracked_cache_invalidate_path(istate, ce->name);
> +		untracked_cache_invalidate_path(istate, ce->name, 1);

This test isn't needed because we're pulling the name right out of the 
cache entry so it doesn't need to be verified.

Here is a modified version of your patch for consideration:

================

read_directory() code ignores all paths named ".git" even if it's not
a valid git repository. See treat_path() for details. Since ".git" is
basically invisible to read_directory(), when we are asked to
invalidate a path that contains ".git", we can safely ignore it
because the slow path would not consider it anyway.

This helps when fsmonitor is used and we have a real ".git" repo at
worktree top. Occasionally .git/index will be updated and if the
fsmonitor hook does not filter it, untracked cache is asked to
invalidate the path ".git/index".

Without this patch, we invalidate the root directory unnecessarily,
which:

- makes read_directory() fall back to slow path for root directory
   (slower)

- makes the index dirty (because UNTR extension is updated). Depending
   on the index size, writing it down could also be slow.

Noticed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---

Notes:
     Base Ref: master
     Web-Diff: https://github.com/benpeart/git/commit/218a577618
     Checkout: git fetch https://github.com/benpeart/git verify_path-v1 
&& git checkout 218a577618

  dir.c                       |  2 +-
  fsmonitor.c                 |  6 +++++-
  t/t7519-status-fsmonitor.sh | 39 +++++++++++++++++++++++++++++++++++++++
  3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 7c4b45e30e..d431da46f5 100644
--- a/dir.c
+++ b/dir.c
@@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct 
dir_struct *dir,
  	if (!de)
  		return treat_path_fast(dir, untracked, cdir, istate, path,
  				       baselen, pathspec);
-	if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
+	if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
  		return path_none;
  	strbuf_setlen(path, baselen);
  	strbuf_addstr(path, de->d_name);
diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edba..019576f306 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -118,8 +118,12 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que

  static void fsmonitor_refresh_callback(struct index_state *istate, 
const char *name)
  {
-	int pos = index_name_pos(istate, name, strlen(name));
+	int pos;

+	if (!verify_path(name))
+		return;
+
+	pos = index_name_pos(istate, name, strlen(name));
  	if (pos >= 0) {
  		struct cache_entry *ce = istate->cache[pos];
  		ce->ce_flags &= ~CE_FSMONITOR_VALID;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbcf..756beb0d8e 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in 
the same state' '
  	test_cmp expect actual
  '

+test_expect_success UNTRACKED_CACHE 'ignore .git changes when 
invalidating UNTR' '
+	test_create_repo dot-git &&
+	(
+		cd dot-git &&
+		mkdir -p .git/hooks &&
+		: >tracked &&
+		: >modified &&
+		mkdir dir1 &&
+		: >dir1/tracked &&
+		: >dir1/modified &&
+		mkdir dir2 &&
+		: >dir2/tracked &&
+		: >dir2/modified &&
+		write_integration_script &&
+		git config core.fsmonitor .git/hooks/fsmonitor-test &&
+		git update-index --untracked-cache &&
+		git update-index --fsmonitor &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-before" \
+		git status &&
+		test-dump-untracked-cache >../before
+	) &&
+	cat >>dot-git/.git/hooks/fsmonitor-test <<-\EOF &&
+	printf ".git\0"
+	printf ".git/index\0"
+	printf "dir1/.git\0"
+	printf "dir1/.git/index\0"
+	EOF
+	(
+		cd dot-git &&
+		GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-after" \
+		git status &&
+		test-dump-untracked-cache >../after
+	) &&
+	grep "directory invalidation" trace-before >>before &&
+	grep "directory invalidation" trace-after >>after &&
+	# UNTR extension unchanged, dir invalidation count unchanged
+	test_cmp before after
+'
+
  test_done

base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
-- 
2.15.0.windows.1

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Duy Nguyen]

On Wed, Feb 7, 2018 at 11:59 PM, Ben Peart <peartben@gmail.com> wrote:
> diff --git a/dir.c b/dir.c
> index 7c4b45e30e..d431da46f5 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1773,7 +1773,7 @@ static enum path_treatment treat_path(struct
> dir_struct *dir,
>         if (!de)
>                 return treat_path_fast(dir, untracked, cdir, istate, path,
>                                        baselen, pathspec);
> -       if (is_dot_or_dotdot(de->d_name) || !strcmp(de->d_name, ".git"))
> +       if (is_dot_or_dotdot(de->d_name) || !fspathcmp(de->d_name, ".git"))
>                 return path_none;
>         strbuf_setlen(path, baselen);
>         strbuf_addstr(path, de->d_name);
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 0af7c4edba..019576f306 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -118,8 +118,12 @@ static int query_fsmonitor(int version, uint64_t
> last_update, struct strbuf *que
>
>  static void fsmonitor_refresh_callback(struct index_state *istate, const
> char *name)
>  {
> -       int pos = index_name_pos(istate, name, strlen(name));
> +       int pos;
>
> +       if (!verify_path(name))
> +               return;
> +
> +       pos = index_name_pos(istate, name, strlen(name));
>         if (pos >= 0) {
>                 struct cache_entry *ce = istate->cache[pos];
>                 ce->ce_flags &= ~CE_FSMONITOR_VALID;
>

It's very tempting considering that the amount of changes is much
smaller. But I think we should go with my version. The hope is when a
_new_ call site appears, the author would think twice before passing
zero or one to the safe_path argument.

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> index eb2d13bbcf..756beb0d8e 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -314,4 +314,43 @@ test_expect_success 'splitting the index results in the
> same state' '
>         test_cmp expect actual
>  '
>
> +test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating
> UNTR' '
> +       test_create_repo dot-git &&
> +       (
> +               cd dot-git &&
> +               mkdir -p .git/hooks &&
> +               : >tracked &&
> +               : >modified &&
> +               mkdir dir1 &&
> +               : >dir1/tracked &&
> +               : >dir1/modified &&
> +               mkdir dir2 &&
> +               : >dir2/tracked &&
> +               : >dir2/modified &&
> +               write_integration_script &&
> +               git config core.fsmonitor .git/hooks/fsmonitor-test &&
> +               git update-index --untracked-cache &&
> +               git update-index --fsmonitor &&
> +               GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-before" \
> +               git status &&
> +               test-dump-untracked-cache >../before
> +       ) &&
> +       cat >>dot-git/.git/hooks/fsmonitor-test <<-\EOF &&
> +       printf ".git\0"
> +       printf ".git/index\0"
> +       printf "dir1/.git\0"
> +       printf "dir1/.git/index\0"
> +       EOF
> +       (
> +               cd dot-git &&
> +               GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace-after" \
> +               git status &&
> +               test-dump-untracked-cache >../after
> +       ) &&
> +       grep "directory invalidation" trace-before >>before &&
> +       grep "directory invalidation" trace-after >>after &&
> +       # UNTR extension unchanged, dir invalidation count unchanged
> +       test_cmp before after
> +'
> +
>  test_done
>
> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29
> --
> 2.15.0.windows.1
>



-- 
Duy

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> It's very tempting considering that the amount of changes is much
> smaller. But I think we should go with my version. The hope is when a
> _new_ call site appears, the author would think twice before passing
> zero or one to the safe_path argument.

Wouldn't it be a better API if the author of new callsite does not
have to think twice and can instead rely on the called function
untracked_cache_invalidate_path() to always do the right thing?

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Duy Nguyen]

On Wed, Feb 14, 2018 at 12:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> It's very tempting considering that the amount of changes is much
>> smaller. But I think we should go with my version. The hope is when a
>> _new_ call site appears, the author would think twice before passing
>> zero or one to the safe_path argument.
>
> Wouldn't it be a better API if the author of new callsite does not
> have to think twice and can instead rely on the called function
> untracked_cache_invalidate_path() to always do the right thing?

I am worried that always doing the right thing may carry performance
penalty (this is based purely on reading verify_path() code, no actual
benchmarking). For safety, you can always set safe_path to zero. But
if you do a lot of invalidation and something starts to slow down,
then you can consider setting safe_path to 1 (if it's actually safe to
do so). I think we do mass invalidation in some case, so I will try to
actually benchmark that and see if this safe_path argument is
justified or if we can always call verify_path().
-- 
Duy

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Junio C Hamano]

On Tue, Feb 13, 2018 at 5:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> I am worried that always doing the right thing may carry performance
> penalty (this is based purely on reading verify_path() code, no actual
> benchmarking). For safety, you can always set safe_path to zero. But
> if you do a lot of invalidation and something starts to slow down,
> then you can consider setting safe_path to 1 (if it's actually safe to
> do so).

Fair enough. Thanks for articulating the reasoning.

Re: Some rough edges of core.fsmonitor

[Ben Peart]

While some of these issues have been discussed in other threads, I 
thought I'd summarize my thoughts here.

On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:
> I just got around to testing this since it landed, for context some
> previous poking of mine in [1].
> 
> Issues / stuff I've noticed:
> 
> 1) We end up invalidating the untracked cache because stuff in .git/
> changed. For example:
> 
>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
> 
> Am I missing something or should we do something like:
> 
>      diff --git a/fsmonitor.c b/fsmonitor.c
>      index 0af7c4edba..5067b89bda 100644
>      --- a/fsmonitor.c
>      +++ b/fsmonitor.c
>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
> 
>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>       {
>      -       int pos = index_name_pos(istate, name, strlen(name));
>      +       int pos;
>      +
>      +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>      +               return;
>      +
>      +       pos = index_name_pos(istate, name, strlen(name));
> 
>              if (pos >= 0) {
>                      struct cache_entry *ce = istate->cache[pos];
> 
> With that patch applied status on a large repo[2] goes from a consistent
> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC
> structure
> 

I favor making this optimization by updating 
untracked_cache_invalidate_path() so that it ignores paths under 
get_git_dir() and doesn't invalidate the untracked cache or flag the 
index as dirty.

> 2) We re-write out the index even though we know nothing changed
> 
> When you first run with core.fsmonitor it needs to
> mark_fsmonitor_clean() for every path, but is there a reason for why we
> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are
> marked and we know from the hook that nothing changed? Why write out the
> index again?
> 

Writing out the index when core.fsmonitor is first turned on is 
necessary to get the index extension added with the current state of the 
dirty flags.  Given it is a one time cost, I don't think we have 
anything worth trying to optimize here.

> 3) A lot of time spend reading the index (or something..)
> 
> While the hook itself takes ~20ms (and watchman itself 1/4 of that)
> status as a whole takes much longer. gprof reveals:
> 
>      Each sample counts as 0.01 seconds.
>        %   cumulative   self              self     total
>       time   seconds   seconds    calls  ms/call  ms/call  name
>       15.38      0.02     0.02   221690     0.00     0.00  memihash
>       15.38      0.04     0.02   221689     0.00     0.00  create_from_disk
>        7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32
>        7.69      0.06     0.01   222661     0.00     0.00  ce_path_match
>        7.69      0.07     0.01   221769     0.00     0.00  hashmap_add
>        7.69      0.08     0.01    39941     0.00     0.00  prep_exclude
>        7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch
>        7.69      0.10     0.01        1    10.00    10.00  read_one
>        7.69      0.11     0.01        1    10.00    10.00  refresh_index
>        7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor
>        7.69      0.13     0.01                             preload_thread
> 
> The index is 24M in this case, I guess it's unpacking it, but I wonder
> if this couldn't be much faster if we saved away the result of the last
> "status" in something that's quick to access, and then if nothing
> changed we just report that, and no need to re-write the index (or just
> write the "it was clean at this time" part).

Yes, reading the index is slow.  We've made some improvements (not 
computing the SHA, not validating the sort order, etc) and have one more 
in progress that will reduce the malloc() cost.  I haven't found any 
other easy optimizations but it would be great if you could find more! 
To make significant improvements, I'm afraid it will take more 
substantial changes to the in memory and on disk formats and updates to 
the code to take advantage of those changes.

> 
> 4) core.fsmonitor=false behaves unexpectedly
> 
> The code that reads this variable just treats it as a string, so we do a
> bunch of work for nothing (and nothing warns) if this is set and 'false'
> is executed. Any reason we couldn't do our standard boolean parsing
> here? You couldn't call your hook 0/1/true/false, but that doesn't seem
> like a big loss.
> 
> 1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/
> 2. https://github.com/avar/2015-04-03-1M-git
> 

I'm torn on this one.  The core.fsmontior setting isn't a boolean value, 
its a string that is the command to run when we need file system 
changes.  It would be pretty simple to add a call to 
git_parse_maybe_bool_text() to treat "false," "no," or "off" the same as 
an empty string but that makes it look even more like a boolean when it 
isn't.

Re: Some rough edges of core.fsmonitor

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 30 2018, Ben Peart jotted:

> While some of these issues have been discussed in other threads, I
> thought I'd summarize my thoughts here.

Thanks for this & your other reply. I'm going to get to testing some of
Duy's patches soon, and if you have some other relevant WIP I'd be happy
to try them, but meanwhile replying to a few of these:

> On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:
>> I just got around to testing this since it landed, for context some
>> previous poking of mine in [1].
>>
>> Issues / stuff I've noticed:
>>
>> 1) We end up invalidating the untracked cache because stuff in .git/
>> changed. For example:
>>
>>      01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>>      01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>>      01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>>      01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>>      01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
>>
>> Am I missing something or should we do something like:
>>
>>      diff --git a/fsmonitor.c b/fsmonitor.c
>>      index 0af7c4edba..5067b89bda 100644
>>      --- a/fsmonitor.c
>>      +++ b/fsmonitor.c
>>      @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>>
>>       static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>>       {
>>      -       int pos = index_name_pos(istate, name, strlen(name));
>>      +       int pos;
>>      +
>>      +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>      +               return;
>>      +
>>      +       pos = index_name_pos(istate, name, strlen(name));
>>
>>              if (pos >= 0) {
>>                      struct cache_entry *ce = istate->cache[pos];
>>
>> With that patch applied status on a large repo[2] goes from a consistent
>> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC
>> structure
>>
>
> I favor making this optimization by updating
> untracked_cache_invalidate_path() so that it ignores paths under
> get_git_dir() and doesn't invalidate the untracked cache or flag the
> index as dirty.

*nod*

>> 2) We re-write out the index even though we know nothing changed
>>
>> When you first run with core.fsmonitor it needs to
>> mark_fsmonitor_clean() for every path, but is there a reason for why we
>> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are
>> marked and we know from the hook that nothing changed? Why write out the
>> index again?
>>
>
> Writing out the index when core.fsmonitor is first turned on is
> necessary to get the index extension added with the current state of
> the dirty flags.  Given it is a one time cost, I don't think we have
> anything worth trying to optimize here.

Indeed, that makes sense. What I was showing here is even after the
initial setup we continue to write it out when we know nothing changed.

We do that anyway without fsmonitor, but this seemed like a worthwhile
thing to optimize.

>> 3) A lot of time spend reading the index (or something..)
>>
>> While the hook itself takes ~20ms (and watchman itself 1/4 of that)
>> status as a whole takes much longer. gprof reveals:
>>
>>      Each sample counts as 0.01 seconds.
>>        %   cumulative   self              self     total
>>       time   seconds   seconds    calls  ms/call  ms/call  name
>>       15.38      0.02     0.02   221690     0.00     0.00  memihash
>>       15.38      0.04     0.02   221689     0.00     0.00  create_from_disk
>>        7.69      0.05     0.01  2216897     0.00     0.00  git_bswap32
>>        7.69      0.06     0.01   222661     0.00     0.00  ce_path_match
>>        7.69      0.07     0.01   221769     0.00     0.00  hashmap_add
>>        7.69      0.08     0.01    39941     0.00     0.00  prep_exclude
>>        7.69      0.09     0.01    39940     0.00     0.00  strbuf_addch
>>        7.69      0.10     0.01        1    10.00    10.00  read_one
>>        7.69      0.11     0.01        1    10.00    10.00  refresh_index
>>        7.69      0.12     0.01        1    10.00    10.00  tweak_fsmonitor
>>        7.69      0.13     0.01                             preload_thread
>>
>> The index is 24M in this case, I guess it's unpacking it, but I wonder
>> if this couldn't be much faster if we saved away the result of the last
>> "status" in something that's quick to access, and then if nothing
>> changed we just report that, and no need to re-write the index (or just
>> write the "it was clean at this time" part).
>
> Yes, reading the index is slow.  We've made some improvements (not
> computing the SHA, not validating the sort order, etc) and have one
> more in progress that will reduce the malloc() cost.  I haven't found
> any other easy optimizations but it would be great if you could find
> more! To make significant improvements, I'm afraid it will take more
> substantial changes to the in memory and on disk formats and updates
> to the code to take advantage of those changes.

What I was wondering (not very clearly) is whether an easier
optimization for now would be to speed up the case where nothing
changed, that would involve just reading some flag in the index (or
elsewhere) saying nothing changed last time, then the timestamp the
fsmonitor writes, and trusting the hook when it says nothing changed
since that timestamp.

>>
>> 4) core.fsmonitor=false behaves unexpectedly
>>
>> The code that reads this variable just treats it as a string, so we do a
>> bunch of work for nothing (and nothing warns) if this is set and 'false'
>> is executed. Any reason we couldn't do our standard boolean parsing
>> here? You couldn't call your hook 0/1/true/false, but that doesn't seem
>> like a big loss.
>>
>> 1. https://public-inbox.org/git/CACBZZX5a6Op7dH_g9WOFBnejh2zgNK4b34ygxA8daNDqvitFVA@mail.gmail.com/
>> 2. https://github.com/avar/2015-04-03-1M-git
>>
>
> I'm torn on this one.  The core.fsmontior setting isn't a boolean
> value, its a string that is the command to run when we need file
> system changes.  It would be pretty simple to add a call to
> git_parse_maybe_bool_text() to treat "false," "no," or "off" the same
> as an empty string but that makes it look even more like a boolean
> when it isn't.

Yes, that makes sense. I wonder though if we should warn if the hook is
set and returning non-zero, right now that failure case is silent, maybe
the hook would like to return an exit code for "don't ask me", but it
seems better to make that 125 (like git bisect skip) instead of !0,
since that makes the hook failing completely indistinguishable to the
user from the hook doing its job.

Re: Some rough edges of core.fsmonitor

[Ben Peart]

On 1/30/2018 6:16 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jan 30 2018, Ben Peart jotted:
> 
>> While some of these issues have been discussed in other threads, I
>> thought I'd summarize my thoughts here.
> 
> Thanks for this & your other reply. I'm going to get to testing some of
> Duy's patches soon, and if you have some other relevant WIP I'd be happy
> to try them, but meanwhile replying to a few of these:
> 
>> On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:
>>> I just got around to testing this since it landed, for context some
>>> previous poking of mine in [1].
>>>
>>> Issues / stuff I've noticed:
>>>
>>> 1) We end up invalidating the untracked cache because stuff in .git/
>>> changed. For example:
>>>
>>>       01:09:24.975524 fsmonitor.c:173         fsmonitor process '.git/hooks/fsmonitor-watchman' returned success
>>>       01:09:24.975548 fsmonitor.c:138         fsmonitor_refresh_callback '.git'
>>>       01:09:24.975556 fsmonitor.c:138         fsmonitor_refresh_callback '.git/config'
>>>       01:09:24.975568 fsmonitor.c:138         fsmonitor_refresh_callback '.git/index'
>>>       01:09:25.122726 fsmonitor.c:91          write fsmonitor extension successful
>>>
>>> Am I missing something or should we do something like:
>>>
>>>       diff --git a/fsmonitor.c b/fsmonitor.c
>>>       index 0af7c4edba..5067b89bda 100644
>>>       --- a/fsmonitor.c
>>>       +++ b/fsmonitor.c
>>>       @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>>>
>>>        static void fsmonitor_refresh_callback(struct index_state *istate, const char *name)
>>>        {
>>>       -       int pos = index_name_pos(istate, name, strlen(name));
>>>       +       int pos;
>>>       +
>>>       +       if (!strcmp(name, ".git") || starts_with(name, ".git/"))
>>>       +               return;
>>>       +
>>>       +       pos = index_name_pos(istate, name, strlen(name));
>>>
>>>               if (pos >= 0) {
>>>                       struct cache_entry *ce = istate->cache[pos];
>>>
>>> With that patch applied status on a large repo[2] goes from a consistent
>>> ~180-200ms to ~140-150ms, since we're not invalidating some of the UC
>>> structure
>>>
>>
>> I favor making this optimization by updating
>> untracked_cache_invalidate_path() so that it ignores paths under
>> get_git_dir() and doesn't invalidate the untracked cache or flag the
>> index as dirty.
> 
> *nod*
> 
>>> 2) We re-write out the index even though we know nothing changed
>>>
>>> When you first run with core.fsmonitor it needs to
>>> mark_fsmonitor_clean() for every path, but is there a reason for why we
>>> wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are
>>> marked and we know from the hook that nothing changed? Why write out the
>>> index again?
>>>
>>
>> Writing out the index when core.fsmonitor is first turned on is
>> necessary to get the index extension added with the current state of
>> the dirty flags.  Given it is a one time cost, I don't think we have
>> anything worth trying to optimize here.
> 
> Indeed, that makes sense. What I was showing here is even after the
> initial setup we continue to write it out when we know nothing changed.
> 
> We do that anyway without fsmonitor, but this seemed like a worthwhile
> thing to optimize.
> 

There is already logic to avoid writing out the index unless there is 
something that requires it.  In my testing, it was often the untracked 
cache marking the index as dirty that was causing the unexpected writes.

The patch to make the untracked cache only flag the index as dirty when 
the feature is being turned on or off is pretty simple (see below).  The 
challenge was that many of the untracked cache tests assume all changes 
are saved to disk after every command so they fail after making this 
change.  The patch below does a simple hack of checking for a test 
environment variable and only forcing the index writes if it is set.

Internally, we simply turned off the untracked cache as it's usefulness 
in combination with GVFS is limited and without the patch, it actually 
slowed down common operations more than it sped them up.

Typically, changes to the untracked cache don't accumulate long before 
the user does some operation that requires the index to be written out 
at which point the untracked cache is updated as well.


diff --git a/dir.c b/dir.c
index 5e93a1350b..af1d33aae1 100644
--- a/dir.c
+++ b/dir.c
@@ -2256,7 +2256,8 @@ int read_directory(struct dir_struct *dir, struct 
index_state *istate,
                                  dir->untracked->gitignore_invalidated,
                                  dir->untracked->dir_invalidated,
                                  dir->untracked->dir_opened);
-               if (dir->untracked == istate->untracked &&
+               if (getenv("GIT_TEST_UNTRACKED_CACHE") &&
+                       dir->untracked == istate->untracked &&
                     (dir->untracked->dir_opened ||
                      dir->untracked->gitignore_invalidated ||
                      dir->untracked->dir_invalidated))
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index ea9383e8cb..e5811b6ef2 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -14,6 +14,8 @@ test_description='test untracked cache'
  # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know
  # more.

+export GIT_TEST_UNTRACKED_CACHE=true
+
  sync_mtime () {
         if test_have_prereq BUSYBOX
         then