[PATCH/RFC v3 0/9] git checkout: more cleanups, optimisation, less lstat() calls

[PATCH/RFC v3 0/9] git checkout: more cleanups, optimisation, less lstat() calls

[Kjetil Barvik]

Changes since v2

--- patch */9 ---
- Changed the order of some patches.
- Some updates to the commit log messages.

--- patch 2/9 ---
- New patch which let us later use longest_match_lstat_cache(), now
  renamed to longest_path_match(), inside patch 4/9.

--- patch 3/9 ---
- New patch which update the symlinks.c file to be more in line with
  the GIT source code (s/length,string/string,length/ for function
  arguments).

-- patch 4/9 ---
- The new function schedule_dir_for_removal() is placed inside
  symlinks.c instead of unpack-trees.c

-- patch 9/9 ---
- NOTE/NB: this patch is only a debug patch, not be included in the final
  GIT release version.



Kjetil Barvik (9):
  lstat_cache(): small cleanup and optimisation
  lstat_cache(): generalise longest_match_lstat_cache()
  lstat_cache(): swap func(length, string) into func(string, length)
  unlink_entry(): introduce schedule_dir_for_removal()
  create_directories(): remove some memcpy() and strchr() calls
  write_entry(): cleanup of some duplicated code
  write_entry(): use fstat() instead of lstat() when file is open
  show_patch_diff(): remove a call to fstat()
  lstat_cache(): print a warning if doing ping-pong between cache types

 Documentation/CodingGuidelines |    3 +
 builtin-add.c                  |    2 +-
 builtin-apply.c                |    2 +-
 builtin-update-index.c         |    2 +-
 cache.h                        |   10 ++-
 combine-diff.c                 |    4 +-
 diff-lib.c                     |    2 +-
 dir.c                          |    2 +-
 entry.c                        |  108 ++++++++++++-------------
 symlinks.c                     |  177 ++++++++++++++++++++++++++++++----------
 unpack-trees.c                 |   34 ++------
 11 files changed, 208 insertions(+), 138 deletions(-)

[PATCH/RFC v3 1/9] lstat_cache(): small cleanup and optimisation

[Kjetil Barvik]

Simplify the if-else test in longest_match_lstat_cache() such that we
only have one simple if test.  Instead of testing for 'i == cache.len'
or 'i == len', we transform this to a common test for 'i == max_len'.

And to further optimise we use 'i >= max_len' instead of 'i ==
max_len', the reason is that it is now the exact opposite of one part
inside the while-loop termination expression 'i < max_len && name[i]
== cache.path[i]', and then the compiler can probably reuse a test
instruction from it.

We also throw away the arguments to reset_lstat_cache(), such that all
the safeguard logic inside lstat_cache() is handled at one place.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 symlinks.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index f262b7c..ae57e56 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -25,27 +25,30 @@ static inline int longest_match_lstat_cache(int len, const char *name,
 		}
 		i++;
 	}
-	/* Is the cached path string a substring of 'name'? */
-	if (i == cache.len && cache.len < len && name[cache.len] == '/') {
-		match_len_prev = match_len;
-		match_len = cache.len;
-	/* Is 'name' a substring of the cached path string? */
-	} else if ((i == len && len < cache.len && cache.path[len] == '/') ||
-		   (i == len && len == cache.len)) {
+	/*
+	 * Is the cached path string a substring of 'name', is 'name'
+	 * a substring of the cached path string, or is 'name' and the
+	 * cached path string the exact same string?
+	 */
+	if (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||
+			     (len < cache.len && cache.path[len] == '/') ||
+			     (len == cache.len))) {
 		match_len_prev = match_len;
-		match_len = len;
+		match_len = i;
 	}
 	*previous_slash = match_len_prev;
 	return match_len;
 }
 
-static inline void reset_lstat_cache(int track_flags, int prefix_len_stat_func)
+static inline void reset_lstat_cache(void)
 {
 	cache.path[0] = '\0';
 	cache.len = 0;
 	cache.flags = 0;
-	cache.track_flags = track_flags;
-	cache.prefix_len_stat_func = prefix_len_stat_func;
+	/*
+	 * The track_flags and prefix_len_stat_func members is only
+	 * set by the safeguard rule inside lstat_cache()
+	 */
 }
 
 #define FL_DIR      (1 << 0)
@@ -77,11 +80,13 @@ static int lstat_cache(int len, const char *name,
 	if (cache.track_flags != track_flags ||
 	    cache.prefix_len_stat_func != prefix_len_stat_func) {
 		/*
-		 * As a safeguard we clear the cache if the values of
-		 * track_flags and/or prefix_len_stat_func does not
-		 * match with the last supplied values.
+		 * As a safeguard rule we clear the cache if the
+		 * values of track_flags and/or prefix_len_stat_func
+		 * does not match with the last supplied values.
 		 */
-		reset_lstat_cache(track_flags, prefix_len_stat_func);
+		reset_lstat_cache();
+		cache.track_flags = track_flags;
+		cache.prefix_len_stat_func = prefix_len_stat_func;
 		match_len = last_slash = 0;
 	} else {
 		/*
@@ -153,7 +158,7 @@ static int lstat_cache(int len, const char *name,
 		cache.path[last_slash] = '\0';
 		cache.len = last_slash;
 		cache.flags = save_flags;
-	} else if (track_flags & FL_DIR &&
+	} else if ((track_flags & FL_DIR) &&
 		   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
 		/*
 		 * We have a separate test for the directory case,
@@ -170,7 +175,7 @@ static int lstat_cache(int len, const char *name,
 		cache.len = last_slash_dir;
 		cache.flags = FL_DIR;
 	} else {
-		reset_lstat_cache(track_flags, prefix_len_stat_func);
+		reset_lstat_cache();
 	}
 	return ret_flags;
 }
@@ -190,8 +195,7 @@ void invalidate_lstat_cache(int len, const char *name)
 			cache.len = previous_slash;
 			cache.flags = FL_DIR;
 		} else
-			reset_lstat_cache(cache.track_flags,
-					  cache.prefix_len_stat_func);
+			reset_lstat_cache();
 	}
 }
 
@@ -200,7 +204,7 @@ void invalidate_lstat_cache(int len, const char *name)
  */
 void clear_lstat_cache(void)
 {
-	reset_lstat_cache(0, 0);
+	reset_lstat_cache();
 }
 
 #define USE_ONLY_LSTAT  0
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 2/9] lstat_cache(): generalise longest_match_lstat_cache()

[Kjetil Barvik]

Rename the function to longst_path_match() and generalise it such that
it can also be used by other functions.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 symlinks.c |   46 ++++++++++++++++++++++++----------------------
 1 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index ae57e56..4596aee 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -1,38 +1,30 @@
 #include "cache.h"
 
-static struct cache_def {
-	char path[PATH_MAX + 1];
-	int len;
-	int flags;
-	int track_flags;
-	int prefix_len_stat_func;
-} cache;
-
 /*
  * Returns the length (on a path component basis) of the longest
- * common prefix match of 'name' and the cached path string.
+ * common prefix match of 'name_a' and 'name_b'.
  */
-static inline int longest_match_lstat_cache(int len, const char *name,
-					    int *previous_slash)
+static int longest_path_match(const char *name_a, int len_a,
+			      const char *name_b, int len_b,
+			      int *previous_slash)
 {
 	int max_len, match_len = 0, match_len_prev = 0, i = 0;
 
-	max_len = len < cache.len ? len : cache.len;
-	while (i < max_len && name[i] == cache.path[i]) {
-		if (name[i] == '/') {
+	max_len = len_a < len_b ? len_a : len_b;
+	while (i < max_len && name_a[i] == name_b[i]) {
+		if (name_a[i] == '/') {
 			match_len_prev = match_len;
 			match_len = i;
 		}
 		i++;
 	}
 	/*
-	 * Is the cached path string a substring of 'name', is 'name'
-	 * a substring of the cached path string, or is 'name' and the
-	 * cached path string the exact same string?
+	 * Is 'name_b' a substring of 'name_a', the other way around,
+	 * or is 'name_a' and 'name_b' the exact same string?
 	 */
-	if (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||
-			     (len < cache.len && cache.path[len] == '/') ||
-			     (len == cache.len))) {
+	if (i >= max_len && ((len_a > len_b && name_a[len_b] == '/') ||
+			     (len_a < len_b && name_b[len_a] == '/') ||
+			     (len_a == len_b))) {
 		match_len_prev = match_len;
 		match_len = i;
 	}
@@ -40,6 +32,14 @@ static inline int longest_match_lstat_cache(int len, const char *name,
 	return match_len;
 }
 
+static struct cache_def {
+	char path[PATH_MAX + 1];
+	int len;
+	int flags;
+	int track_flags;
+	int prefix_len_stat_func;
+} cache;
+
 static inline void reset_lstat_cache(void)
 {
 	cache.path[0] = '\0';
@@ -94,7 +94,8 @@ static int lstat_cache(int len, const char *name,
 		 * the 2 "excluding" path types.
 		 */
 		match_len = last_slash =
-			longest_match_lstat_cache(len, name, &previous_slash);
+			longest_path_match(name, len, cache.path, cache.len,
+					   &previous_slash);
 		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
 		if (match_flags && match_len == cache.len)
 			return match_flags;
@@ -188,7 +189,8 @@ void invalidate_lstat_cache(int len, const char *name)
 {
 	int match_len, previous_slash;
 
-	match_len = longest_match_lstat_cache(len, name, &previous_slash);
+	match_len = longest_path_match(name, len, cache.path, cache.len,
+				       &previous_slash);
 	if (len == match_len) {
 		if ((cache.track_flags & FL_DIR) && previous_slash > 0) {
 			cache.path[previous_slash] = '\0';
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 3/9] lstat_cache(): swap func(length, string) into func(string, length)

[Kjetil Barvik]

Swap function argument pair (length, string) into (string, length) to
conform with the commonly used order inside the GIT source code.

Also, add a note about this fact into the coding guidelines.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 Documentation/CodingGuidelines |    3 +++
 builtin-add.c                  |    2 +-
 builtin-apply.c                |    2 +-
 builtin-update-index.c         |    2 +-
 cache.h                        |    8 ++++----
 diff-lib.c                     |    2 +-
 dir.c                          |    2 +-
 entry.c                        |    2 +-
 symlinks.c                     |   16 ++++++++--------
 unpack-trees.c                 |    4 ++--
 10 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 0d7fa9c..b8bf618 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -129,3 +129,6 @@ For C programs:
    used in the git core command set (unless your command is clearly
    separate from it, such as an importer to convert random-scm-X
    repositories to git).
+
+ - When we pass <string, length> pair to functions, we should try to
+   pass them in that order.
diff --git a/builtin-add.c b/builtin-add.c
index ac98c83..a23ad96 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -148,7 +148,7 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p
 	if (pathspec) {
 		const char **p;
 		for (p = pathspec; *p; p++) {
-			if (has_symlink_leading_path(strlen(*p), *p)) {
+			if (has_symlink_leading_path(*p, strlen(*p))) {
 				int len = prefix ? strlen(prefix) : 0;
 				die("'%s' is beyond a symbolic link", *p + len);
 			}
diff --git a/builtin-apply.c b/builtin-apply.c
index 1e7f917..a1de3cb 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2360,7 +2360,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 		 * In such a case, path "new_name" does not exist as
 		 * far as git is concerned.
 		 */
-		if (has_symlink_leading_path(strlen(new_name), new_name))
+		if (has_symlink_leading_path(new_name, strlen(new_name)))
 			return 0;
 
 		return error("%s: already exists in working directory", new_name);
diff --git a/builtin-update-index.c b/builtin-update-index.c
index 5604977..6c55527 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -195,7 +195,7 @@ static int process_path(const char *path)
 	struct stat st;
 
 	len = strlen(path);
-	if (has_symlink_leading_path(len, path))
+	if (has_symlink_leading_path(path, len))
 		return error("'%s' is beyond a symbolic link", path);
 
 	/*
diff --git a/cache.h b/cache.h
index 45e713e..30039ac 100644
--- a/cache.h
+++ b/cache.h
@@ -721,10 +721,10 @@ struct checkout {
 };
 
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
-extern int has_symlink_leading_path(int len, const char *name);
-extern int has_symlink_or_noent_leading_path(int len, const char *name);
-extern int has_dirs_only_path(int len, const char *name, int prefix_len);
-extern void invalidate_lstat_cache(int len, const char *name);
+extern int has_symlink_leading_path(const char *name, int len);
+extern int has_symlink_or_noent_leading_path(const char *name, int len);
+extern int has_dirs_only_path(const char *name, int len, int prefix_len);
+extern void invalidate_lstat_cache(const char *name, int len);
 extern void clear_lstat_cache(void);
 
 extern struct alternate_object_database {
diff --git a/diff-lib.c b/diff-lib.c
index a41e1ec..a3ba20e 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -31,7 +31,7 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)
 			return -1;
 		return 1;
 	}
-	if (has_symlink_leading_path(ce_namelen(ce), ce->name))
+	if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
 		return 1;
 	if (S_ISDIR(st->st_mode)) {
 		unsigned char sub[20];
diff --git a/dir.c b/dir.c
index cfd1ea5..8fb5226 100644
--- a/dir.c
+++ b/dir.c
@@ -720,7 +720,7 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
 {
 	struct path_simplify *simplify;
 
-	if (has_symlink_leading_path(strlen(path), path))
+	if (has_symlink_leading_path(path, strlen(path)))
 		return dir->nr;
 
 	simplify = create_simplify(pathspec);
diff --git a/entry.c b/entry.c
index 05aa58d..bb6bdb9 100644
--- a/entry.c
+++ b/entry.c
@@ -20,7 +20,7 @@ static void create_directories(const char *path, const struct checkout *state)
 		 * we test the path components of the prefix with the
 		 * stat() function instead of the lstat() function.
 		 */
-		if (has_dirs_only_path(len, buf, state->base_dir_len))
+		if (has_dirs_only_path(buf, len, state->base_dir_len))
 			continue; /* ok, it is already a directory. */
 
 		/*
diff --git a/symlinks.c b/symlinks.c
index 4596aee..5167286 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -70,7 +70,7 @@ static inline void reset_lstat_cache(void)
  * of the prefix, where the cache should use the stat() function
  * instead of the lstat() function to test each path component.
  */
-static int lstat_cache(int len, const char *name,
+static int lstat_cache(const char *name, int len,
 		       int track_flags, int prefix_len_stat_func)
 {
 	int match_len, last_slash, last_slash_dir, previous_slash;
@@ -185,7 +185,7 @@ static int lstat_cache(int len, const char *name,
  * Invalidate the given 'name' from the cache, if 'name' matches
  * completely with the cache.
  */
-void invalidate_lstat_cache(int len, const char *name)
+void invalidate_lstat_cache(const char *name, int len)
 {
 	int match_len, previous_slash;
 
@@ -214,9 +214,9 @@ void clear_lstat_cache(void)
 /*
  * Return non-zero if path 'name' has a leading symlink component
  */
-int has_symlink_leading_path(int len, const char *name)
+int has_symlink_leading_path(const char *name, int len)
 {
-	return lstat_cache(len, name,
+	return lstat_cache(name, len,
 			   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &
 		FL_SYMLINK;
 }
@@ -225,9 +225,9 @@ int has_symlink_leading_path(int len, const char *name)
  * Return non-zero if path 'name' has a leading symlink component or
  * if some leading path component does not exists.
  */
-int has_symlink_or_noent_leading_path(int len, const char *name)
+int has_symlink_or_noent_leading_path(const char *name, int len)
 {
-	return lstat_cache(len, name,
+	return lstat_cache(name, len,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &
 		(FL_SYMLINK|FL_NOENT);
 }
@@ -239,9 +239,9 @@ int has_symlink_or_noent_leading_path(int len, const char *name)
  * 'prefix_len', thus we then allow for symlinks in the prefix part as
  * long as those points to real existing directories.
  */
-int has_dirs_only_path(int len, const char *name, int prefix_len)
+int has_dirs_only_path(const char *name, int len, int prefix_len)
 {
-	return lstat_cache(len, name,
+	return lstat_cache(name, len,
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index e547282..2293158 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -61,7 +61,7 @@ static void unlink_entry(struct cache_entry *ce)
 	char *cp, *prev;
 	char *name = ce->name;
 
-	if (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))
+	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
 		return;
 	if (unlink(name))
 		return;
@@ -583,7 +583,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 	if (o->index_only || o->reset || !o->update)
 		return 0;
 
-	if (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))
+	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
 		return 0;
 
 	if (!lstat(ce->name, &st)) {
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()

[Kjetil Barvik]

Currently inside unlink_entry() if we get a successful removal of one
file with unlink(), we try to remove the leading directories each and
every time.  So if one directory containing 200 files is moved to an
other location we get 199 failed calls to rmdir() and 1 successful
call.

To fix this and avoid some unnecessary calls to rmdir(), we schedule
each directory for removal and wait much longer before we do the real
call to rmdir().

Since the unlink_entry() function is called with alphabetically sorted
names, this new function end up being very effective to avoid
unnecessary calls to rmdir().  In some cases over 95% of all calls to
rmdir() is removed with this patch.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 cache.h        |    2 +
 symlinks.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c |   30 +++++----------------------
 3 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/cache.h b/cache.h
index 30039ac..717dd84 100644
--- a/cache.h
+++ b/cache.h
@@ -726,6 +726,8 @@ extern int has_symlink_or_noent_leading_path(const char *name, int len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
 extern void invalidate_lstat_cache(const char *name, int len);
 extern void clear_lstat_cache(void);
+extern void schedule_dir_for_removal(const char *name, int len);
+extern void remove_scheduled_dirs(void);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;
diff --git a/symlinks.c b/symlinks.c
index 5167286..215d049 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -245,3 +245,63 @@ int has_dirs_only_path(const char *name, int len, int prefix_len)
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;
 }
+
+static struct removal_def {
+	char path[PATH_MAX];
+	int len;
+} removal;
+
+static void do_remove_scheduled_dirs(int new_len)
+{
+	while (removal.len > new_len) {
+		removal.path[removal.len] = '\0';
+		if (rmdir(removal.path))
+			break;
+		do {
+			removal.len--;
+		} while (removal.len > new_len &&
+			 removal.path[removal.len] != '/');
+	}
+	removal.len = new_len;
+	return;
+}
+
+void schedule_dir_for_removal(const char *name, int len)
+{
+	int match_len, last_slash, i, previous_slash;
+
+	match_len = last_slash = i =
+		longest_path_match(name, len, removal.path, removal.len,
+				   &previous_slash);
+	/* Find last slash inside 'name' */
+	while (i < len) {
+		if (name[i] == '/')
+			last_slash = i;
+		i++;
+	}
+
+	/*
+	 * If we are about to go down the directory tree, we check if
+	 * we must first go upwards the tree, such that we then can
+	 * remove possible empty directories as we go upwards.
+	 */
+	if (match_len < last_slash && match_len < removal.len)
+		do_remove_scheduled_dirs(match_len);
+	/*
+	 * If we go deeper down the directory tree, we only need to
+	 * save the new path components as we go down.
+	 */
+	if (match_len < last_slash) {
+		memmove(&removal.path[match_len], &name[match_len],
+			last_slash - match_len);
+		removal.len = last_slash;
+	}
+	return;
+}
+
+#define ALL_DIRS 0
+void remove_scheduled_dirs(void)
+{
+	do_remove_scheduled_dirs(ALL_DIRS);
+	return;
+}
diff --git a/unpack-trees.c b/unpack-trees.c
index 2293158..e3c3fa1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,36 +52,17 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);
 }
 
-/* Unlink the last component and attempt to remove leading
- * directories, in case this unlink is the removal of the
- * last entry in the directory -- empty directories are removed.
+/*
+ * Unlink the last component and schedule the leading directories for
+ * removal, such that empty directories get removed.
  */
 static void unlink_entry(struct cache_entry *ce)
 {
-	char *cp, *prev;
-	char *name = ce->name;
-
 	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
 		return;
-	if (unlink(name))
+	if (unlink(ce->name))
 		return;
-	prev = NULL;
-	while (1) {
-		int status;
-		cp = strrchr(name, '/');
-		if (prev)
-			*prev = '/';
-		if (!cp)
-			break;
-
-		*cp = 0;
-		status = rmdir(name);
-		if (status) {
-			*cp = '/';
-			break;
-		}
-		prev = cp;
-	}
+	schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
 static struct checkout state;
@@ -117,6 +98,7 @@ static int check_updates(struct unpack_trees_options *o)
 			continue;
 		}
 	}
+	remove_scheduled_dirs();
 
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
-- 
1.6.1.349.g99fa5

Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Since the unlink_entry() function is called with alphabetically sorted
> names, this new function end up being very effective to avoid
> unnecessary calls to rmdir().  In some cases over 95% of all calls to
> rmdir() is removed with this patch.

I first feared that this will badly break D/F conflicting cases where we
remove a file D/F that is the last remaining file in directory D and rely
on the successful removal of the directory to create a file D, but the
check_updates() function already is structured to allow this late removal
easily.

I think the patch is sensible modulo minor nits.

 * The new global path_buf/path_len in unpack-trees.c are named too
   generic.  It is not just a buffer anybody can use for anything, but
   is used to keep track for a specific purpose of doing something, so
   please name them after that something.

 * "#define ALL_DIRS 0" is useless, given the way how the parameter to
   do_remove_scheduled_dirs() is named;

Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Since the unlink_entry() function is called with alphabetically sorted
> names, this new function end up being very effective to avoid
> unnecessary calls to rmdir().  In some cases over 95% of all calls to
> rmdir() is removed with this patch.

I first feared that this will badly break D/F conflicting cases where we
remove a file D/F that is the last remaining file in directory D and rely
on the successful removal of the directory to create a file D, but the
check_updates() function already is structured to allow this late removal
easily.

I think the patch is sensible modulo minor nits.

 * The new global path_buf/path_len in unpack-trees.c are named too
   generic.  It is not just a buffer anybody can use for anything, but
   is used to keep track for a specific purpose of doing something, so
   please name them after that something.

 * "#define ALL_DIRS 0" is useless, given the way how the parameter to
   do_remove_scheduled_dirs() is named;

Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Kjetil Barvik <barvik@broadpark.no> writes:
>
>> Since the unlink_entry() function is called with alphabetically sorted
>> names, this new function end up being very effective to avoid
>> unnecessary calls to rmdir().  In some cases over 95% of all calls to
>> rmdir() is removed with this patch.
>
> I first feared that this will badly break D/F conflicting cases where we
> remove a file D/F that is the last remaining file in directory D and rely
> on the successful removal of the directory to create a file D, but the
> check_updates() function already is structured to allow this late removal
> easily.
>
> I think the patch is sensible modulo minor nits.

  Yes, if the 2 for-loop's inside check_updates() is rewritten into 1
  for-loop, then you could end up with such a conflict.  Should I add a
  comment about this inside check_updates()?

>  * The new global path_buf/path_len in unpack-trees.c are named too
>    generic.  It is not just a buffer anybody can use for anything, but
>    is used to keep track for a specific purpose of doing something, so
>    please name them after that something.

   Already done in v3 (this version), since the functions
   schedule_dir_for_removal() and remove_scheduled_dirs() is now
   implemented in the symlinks.c file, where I have made the following:

static struct removal_def {
	char path[PATH_MAX];
	int len;
} removal;

>  * "#define ALL_DIRS 0" is useless, given the way how the parameter to
>    do_remove_scheduled_dirs() is named;

  Ok, I can do a rebase of this patch (4/9) and resend the patches.

  -- kjetil

[PATCH/RFC v3 5/9] create_directories(): remove some memcpy() and strchr() calls

[Kjetil Barvik]

Remove the call to memcpy() and strchr() for each path component
tested, and instead add each path component as we go forward inside
the while-loop.

Impact: small optimisation

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 entry.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/entry.c b/entry.c
index bb6bdb9..cc8f0c6 100644
--- a/entry.c
+++ b/entry.c
@@ -2,15 +2,19 @@
 #include "blob.h"
 #include "dir.h"
 
-static void create_directories(const char *path, const struct checkout *state)
+static void create_directories(const char *path, int path_len,
+			       const struct checkout *state)
 {
-	int len = strlen(path);
-	char *buf = xmalloc(len + 1);
-	const char *slash = path;
-
-	while ((slash = strchr(slash+1, '/')) != NULL) {
-		len = slash - path;
-		memcpy(buf, path, len);
+	char *buf = xmalloc(path_len + 1);
+	int len = 0;
+
+	while (len < path_len) {
+		do {
+			buf[len] = path[len];
+			len++;
+		} while (len < path_len && path[len] != '/');
+		if (len >= path_len)
+			break;
 		buf[len] = 0;
 
 		/*
@@ -190,6 +194,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 
 	memcpy(path, state->base_dir, len);
 	strcpy(path + len, ce->name);
+	len += ce_namelen(ce);
 
 	if (!lstat(path, &st)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);
@@ -218,6 +223,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 			return error("unable to unlink old '%s' (%s)", path, strerror(errno));
 	} else if (state->not_new)
 		return 0;
-	create_directories(path, state);
+	create_directories(path, len, state);
 	return write_entry(ce, path, state, 0);
 }
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 6/9] write_entry(): cleanup of some duplicated code

[Kjetil Barvik]

The switch-cases for S_IFREG and S_IFLNK was so similar that it will
be better to do some cleanup and use the common parts of it.

And the entry.c file should now be clean for 'gcc -Wextra' warnings.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 entry.c |   75 +++++++++++++++++++++++++-------------------------------------
 1 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/entry.c b/entry.c
index cc8f0c6..1f53588 100644
--- a/entry.c
+++ b/entry.c
@@ -78,7 +78,7 @@ static int create_file(const char *path, unsigned int mode)
 	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
 }
 
-static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned long *size)
+static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)
 {
 	enum object_type type;
 	void *new = read_sha1_file(ce->sha1, &type, size);
@@ -93,36 +93,51 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned
 
 static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
 {
-	int fd;
-	long wrote;
-
-	switch (ce->ce_mode & S_IFMT) {
-		char *new;
-		struct strbuf buf;
-		unsigned long size;
-
+	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
+	int fd, ret;
+	char *new;
+	struct strbuf buf = STRBUF_INIT;
+	unsigned long size;
+	size_t wrote, newsize = 0;
+
+	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
-		new = read_blob_entry(ce, path, &size);
+	case S_IFLNK:
+		new = read_blob_entry(ce, &size);
 		if (!new)
 			return error("git checkout-index: unable to read sha1 file of %s (%s)",
 				path, sha1_to_hex(ce->sha1));
 
+		if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
+			ret = symlink(new, path);
+			free(new);
+			if (ret)
+				return error("git checkout-index: unable to create symlink %s (%s)",
+					     path, strerror(errno));
+			break;
+		}
+
 		/*
 		 * Convert from git internal format to working tree format
 		 */
-		strbuf_init(&buf, 0);
-		if (convert_to_working_tree(ce->name, new, size, &buf)) {
-			size_t newsize = 0;
+		if (ce_mode_s_ifmt == S_IFREG &&
+		    convert_to_working_tree(ce->name, new, size, &buf)) {
 			free(new);
 			new = strbuf_detach(&buf, &newsize);
 			size = newsize;
 		}
 
 		if (to_tempfile) {
-			strcpy(path, ".merge_file_XXXXXX");
+			if (ce_mode_s_ifmt == S_IFREG)
+				strcpy(path, ".merge_file_XXXXXX");
+			else
+				strcpy(path, ".merge_link_XXXXXX");
 			fd = mkstemp(path);
-		} else
+		} else if (ce_mode_s_ifmt == S_IFREG) {
 			fd = create_file(path, ce->ce_mode);
+		} else {
+			fd = create_file(path, 0666);
+		}
 		if (fd < 0) {
 			free(new);
 			return error("git checkout-index: unable to create file %s (%s)",
@@ -135,36 +150,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		if (wrote != size)
 			return error("git checkout-index: unable to write file %s", path);
 		break;
-	case S_IFLNK:
-		new = read_blob_entry(ce, path, &size);
-		if (!new)
-			return error("git checkout-index: unable to read sha1 file of %s (%s)",
-				path, sha1_to_hex(ce->sha1));
-		if (to_tempfile || !has_symlinks) {
-			if (to_tempfile) {
-				strcpy(path, ".merge_link_XXXXXX");
-				fd = mkstemp(path);
-			} else
-				fd = create_file(path, 0666);
-			if (fd < 0) {
-				free(new);
-				return error("git checkout-index: unable to create "
-						 "file %s (%s)", path, strerror(errno));
-			}
-			wrote = write_in_full(fd, new, size);
-			close(fd);
-			free(new);
-			if (wrote != size)
-				return error("git checkout-index: unable to write file %s",
-					path);
-		} else {
-			wrote = symlink(new, path);
-			free(new);
-			if (wrote)
-				return error("git checkout-index: unable to create "
-						 "symlink %s (%s)", path, strerror(errno));
-		}
-		break;
 	case S_IFGITLINK:
 		if (to_tempfile)
 			return error("git checkout-index: cannot create temporary subproject %s", path);
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Currently inside write_entry() we do an lstat(path, &st) call on a
file which have just been opened inside the exact same function.  It
should be better to call fstat(fd, &st) on the file while it is open,
and it should be at least as fast as the lstat() method.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 entry.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/entry.c b/entry.c
index 1f53588..5daacc2 100644
--- a/entry.c
+++ b/entry.c
@@ -94,11 +94,12 @@ static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)
 static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
 {
 	unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
-	int fd, ret;
+	int fd, ret, fstat_done = 0;
 	char *new;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned long size;
 	size_t wrote, newsize = 0;
+	struct stat st;
 
 	switch (ce_mode_s_ifmt) {
 	case S_IFREG:
@@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		}
 
 		wrote = write_in_full(fd, new, size);
+		/* use fstat() only when path == ce->name */
+		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
+			fstat(fd, &st);
+			fstat_done = 1;
+		}
 		close(fd);
 		free(new);
 		if (wrote != size)
@@ -161,8 +167,8 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	}
 
 	if (state->refresh_cache) {
-		struct stat st;
-		lstat(ce->name, &st);
+		if (!fstat_done)
+			lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 	}
 	return 0;
-- 
1.6.1.349.g99fa5

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Sixt]

Kjetil Barvik schrieb:
> Currently inside write_entry() we do an lstat(path, &st) call on a
> file which have just been opened inside the exact same function.  It
> should be better to call fstat(fd, &st) on the file while it is open,
> and it should be at least as fast as the lstat() method.
...
> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>  		}
>  
>  		wrote = write_in_full(fd, new, size);
> +		/* use fstat() only when path == ce->name */
> +		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
> +			fstat(fd, &st);
> +			fstat_done = 1;
> +		}
>  		close(fd);

I've a bad gut feeling about this: It may not work as expected on Windows
because there is this statement in the documentation:

  "The only guarantee about a file timestamp is that the file time is
   correctly reflected when the handle that makes the change is closed."

(http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)

We are operating on a temporary file. It could happen that the fstat()
returns the time when the file was created, as opposed to when the
write_in_full() was completed successfully. The fstat()ed time ends up in
the index, but it can be different from what later lstat() calls report
(and the file would be regarded as modified).

I have the suspicion that the gain from this patch is minimal. Would you
mind playing it safe and drop this patch?

-- Hannes

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Kjetil Barvik schrieb:
>> Currently inside write_entry() we do an lstat(path, &st) call on a
>> file which have just been opened inside the exact same function.  It
>> should be better to call fstat(fd, &st) on the file while it is open,
>> and it should be at least as fast as the lstat() method.
> ...
>> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>>  		}
>>  
>>  		wrote = write_in_full(fd, new, size);
>> +		/* use fstat() only when path == ce->name */
>> +		if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
>> +			fstat(fd, &st);
>> +			fstat_done = 1;
>> +		}
>>  		close(fd);
>
> I've a bad gut feeling about this: It may not work as expected on Windows
> because there is this statement in the documentation:
>
>   "The only guarantee about a file timestamp is that the file time is
>    correctly reflected when the handle that makes the change is closed."
>
> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)
>
> We are operating on a temporary file. It could happen that the fstat()
> returns the time when the file was created, as opposed to when the
> write_in_full() was completed successfully. The fstat()ed time ends up in
> the index, but it can be different from what later lstat() calls report
> (and the file would be regarded as modified).
>
> I have the suspicion that the gain from this patch is minimal. Would you
> mind playing it safe and drop this patch?

Hmm, write_entry() is actually called once per one path we write out, and
the fstat() is added to the common case (no --tempfile, no --prefix=<dir>
checkout), which would mean that if there were any performance gain from
this change, it was obtained by trading correctness away.  Sad.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j.sixt@viscovery.net> writes:
<snip>
>> I've a bad gut feeling about this: It may not work as expected on Windows
>> because there is this statement in the documentation:
>>
>>   "The only guarantee about a file timestamp is that the file time is
>>    correctly reflected when the handle that makes the change is closed."
>>
>> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)

  Ok, I admit I was not aware of this Windows fact.

>> We are operating on a temporary file. It could happen that the fstat()
>> returns the time when the file was created, as opposed to when the
>> write_in_full() was completed successfully. The fstat()ed time ends up in
>> the index, but it can be different from what later lstat() calls report
>> (and the file would be regarded as modified).
>>
>> I have the suspicion that the gain from this patch is minimal. Would you
>> mind playing it safe and drop this patch?
>
> Hmm, write_entry() is actually called once per one path we write out, and
> the fstat() is added to the common case (no --tempfile, no --prefix=<dir>
> checkout), 

  Yes, I had to make sure that the path string and ce->name was the
  exact same string, so therefore I had to add the test "&& !to_tempfile
  && !state->base_dir_len" to the if-test.

> which would mean that if there were any performance gain from
> this change, it was obtained by trading correctness away.  Sad.

  Sorry about this.  Yes, I agree that we should drop this patch.

  And, yes, since each lstat() call cost approximately 44 microseconds
  compared to 12-16 for each lstat() on my Linux box, there was a little
  performance gain from this patch.

  Junio, is it OK to ask that you drop this patch if/when you update the
  pu branch, such that I do not have to resend the patch series almost
  unchanged to the mailinglist (except for one missing patch)?

  Ok, maybe wait one day, just in case there will be more comments.

  And, thanks for the review!

  -- kjetil

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

>   Junio, is it OK to ask that you drop this patch if/when you update the
>   pu branch, such that I do not have to resend the patch series almost
>   unchanged to the mailinglist (except for one missing patch)?

Surely.  Although I've replaced the topic with the updated series, I am
not in the reintegration phase yet, so not much work is lost.  I was
planning to merge this to 'next' today after one last round of eyeballing.

Thanks.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Sixt]

Kjetil Barvik schrieb:
>   And, yes, since each lstat() call cost approximately 44 microseconds
>   compared to 12-16 for each lstat() on my Linux box, there was a little
                               ^^^^^^^ fstat()?
>   performance gain from this patch.

This does look like a good gain. But do you have hard numbers that back
the claim?

If you can squeeze out a 10% improvement on Linux with this patch, we
should take it, and if it turns out that it doesn't work on Windows, we
could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
is affected, too) that skips the fstat() optimization.

But we really should have numbers that justify this effort.

BTW, the statement from the Windows documentation was:

  "The only guarantee about a file timestamp is that the file time is
   correctly reflected when the handle that makes the change is closed."

In the case of this patch, the timestamp is queried via the handle that
made the change, and in this case special case the timestamp could be
correct nevertheless. The guarantee doesn't cover this case, but it would
be natural, and perhaps it Just Works?

-- Hannes

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Schindelin]

Hi,

On Thu, 5 Feb 2009, Johannes Sixt wrote:

> Kjetil Barvik schrieb:
> >   And, yes, since each lstat() call cost approximately 44 microseconds
> >   compared to 12-16 for each lstat() on my Linux box, there was a little
>                                ^^^^^^^ fstat()?
> >   performance gain from this patch.
> 
> This does look like a good gain. But do you have hard numbers that back
> the claim?
> 
> If you can squeeze out a 10% improvement on Linux with this patch, we
> should take it, and if it turns out that it doesn't work on Windows, we
> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
> is affected, too) that skips the fstat() optimization.

Of course, what we _really_ would do is to provide a flag, say, 
FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
for the appropriate platforms).

Ciao,
Dscho

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Of course, what we _really_ would do is to provide a flag, say, 
> FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
> for the appropriate platforms).

Sounds sensible.  POSIX of course requires write(), pwrite() and friends
to mark the mtime for update and fstat() to be reliable, but there may be
"not quite POSIX" systems.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

* On Thu, 5 Feb 2009, Johannes Sixt wrote:
|
|> Kjetil Barvik schrieb:
|> >   And, yes, since each lstat() call cost approximately 44 microseconds
|> >   compared to 12-16 for each lstat() on my Linux box, there was a little
|>                                ^^^^^^^ fstat()?
|> >   performance gain from this patch.
|> 
|> This does look like a good gain. But do you have hard numbers that back
|> the claim?

  __________________
  Test description

  I have used the following 8 git binaries:

$ for g in ./git_t*; do printf "$g => $($g --version)\n"; done
./git_t0 => git version 1.6.1.80.g7eb5
./git_t1 => git version 1.6.1.85.gbda6       <= added lstat_cache()
./git_t2 => git version 1.6.1.2.306.gc0f6f
./git_t3 => git version 1.6.1.2.307.g55385
./git_t4 => git version 1.6.1.2.308.g052a
./git_t5 => git version 1.6.1.2.310.g40dd2   <= added schedule_dir_for_removal()
./git_t6 => git version 1.6.1.2.313.g9deee
./git_t7 => git version 1.6.1.2.314.g0ad9    <= added this patch (7/9)

  Except for git_t7 all commits should be in origin/pu, so people should
  be able to do git show/diff/log on the last hex chars on the version-
  strings to see the differences.

  CFLAGS="-mtune=core2 -O2 -fomit-frame-pointer -fno-stack-protector -g0 -s"

  Each git_t* binary is run with args "checkout -q my-v.2.6.2[57]" for a
  total of 100 times (50/50 to my-v2.6.25/my-v2.6.27).  Before each run
  the test script sleeps 20 seconds to allow the disk to finish and
  being idle.  Time is collected by /usr/bin/time -o output-file prog.

  While the test script was run, the laptop with an Intel Core2 Duo 2
  GHz processor, was run without X and was otherwise idle.  The test
  script took 9 hours and 45 minutes to complete.

  __________________
  Test numbers

$ for ((t=0; $t<=7; t++)); do echo "git_t$t => $(parse_usr_bin_time_lines.pl git_t$t*)"; done
git_t0 =>  5.646 user  8.322 sys  25.579 real  54.6% CPU  faults:  0 major 39587 minor
git_t1 =>  5.631 user  6.826 sys  23.941 real  52.1% CPU  faults:  0 major 39901 minor
git_t2 =>  5.622 user  6.854 sys  24.036 real  52.1% CPU  faults:  0 major 39298 minor
git_t3 =>  5.636 user  6.867 sys  24.088 real  52.0% CPU  faults:  0 major 39786 minor
git_t4 =>  5.640 user  6.818 sys  24.006 real  52.0% CPU  faults:  0 major 39629 minor
git_t5 =>  5.642 user  6.552 sys  23.805 real  51.4% CPU  faults:  0 major 39707 minor
git_t6 =>  5.629 user  6.518 sys  22.981 real  53.0% CPU  faults:  0 major 40029 minor
git_t7 =>  5.629 user  6.051 sys  23.013 real  50.9% CPU  faults:  0 major 39451 minor

|> If you can squeeze out a 10% improvement on Linux with this patch, we
|> should take it, and if it turns out that it doesn't work on Windows, we
|> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
|> is affected, too) that skips the fstat() optimization.

  For the system used time, the improvement was (- 6.518 6.051) = 0.467
  seconds, or (/ (* (- 6.518 6.051) 100.0) 6.518) = 7.2%, so not 10%.

  Funny to see that in:
     http://article.gmane.org/gmane.comp.version-control.git/107281

  I guessed the improvement to be (quote):
     "(* 14403 (- 44 12)) = 460 896 microseconds system time"

  So I missed only by a little over 6ms. :-)

* Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
| Of course, what we _really_ would do is to provide a flag, say, 
| FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
| for the appropriate platforms).

  Or, maybe

     #define FSTAT_RELIABLE 1

  for Linux only?  Then we can change the if-test inside this patch to
  the following:

-  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
+  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && 
+      FSTAT_RELIABLE) {

  Then we do not have to have #if-defines inside the source code, only
  in one header file.

  But, question: is this patch worth the extra lines added to the source
  code?

  -- kjetil

  PS!  Junio, I think this patch series should be inside pu some days
       more, since things obviously needs more refinement and tests.

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Kjetil Barvik wrote:

> * Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> | Of course, what we _really_ would do is to provide a flag, say, 
> | FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile 
> | for the appropriate platforms).
> 
>   Or, maybe
> 
>      #define FSTAT_RELIABLE 1
> 
>   for Linux only?

No, I think that would be wrong.  Especially after Junio's remarks that 
fstat() is actually required to behave as you expected it, and only 
Windows (surprise, surprise) has problems following the standard.

> Then we can change the if-test inside this patch to the following:
> 
> -  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {
> +  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && 
> +      FSTAT_RELIABLE) {
> 
>   Then we do not have to have #if-defines inside the source code, only
>   in one header file.

In the spirit of consistency, I would not do that.

>   But, question: is this patch worth the extra lines added to the source
>   code?

You seemed to get a nice speedup on Linux.  If Windows cannot follow suit, 
too bad.  But I do not want to be punished because other people's OS is 
not as good as mine, so I _want_ fstat().

And those few lines will not hurt, they'll be readable enough.

Ciao,
Dscho

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Kjetil Barvik schrieb:
>>   And, yes, since each lstat() call cost approximately 44 microseconds
>>   compared to 12-16 for each lstat() on my Linux box, there was a little
>                                ^^^^^^^ fstat()?
>>   performance gain from this patch.
>
> This does look like a good gain. But do you have hard numbers that back
> the claim?

  OK, I have done some testing/profiling with oprofile(1), and one thing
  I found out was that my Linux kernel was built with SLUB debug, and of
  course it cost some system time to run the VM debug code.  After I
  turned this off, the total system time when down from aprox 6 to 3
  seconds for the 'git checkout -q my-v2.6.25/7' test.

  Also, from strace output each lstat() call now take around 16
  microseconds, and each fstat() call around 12 microseconds, so for
  aprox 14000 changed calls (lstat() => fstat()) the performance gain
  should now only be (* 14000 (- 16 12)) = 56 ms, compared to 467 ms,
  which I reported before.

  -- kjetil

  1) http://oprofile.sourceforge.net/about/

[PATCH/RFC v3 8/9] show_patch_diff(): remove a call to fstat()

[Kjetil Barvik]

Currently inside show_patch_diff() we have an fstat() call after an
ok lstat() call.  Since before the call to fstat() we have already
tested for the link case with S_ISLNK(), the fstat() can be removed.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 combine-diff.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index bccc018..4300319 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -713,9 +713,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			result_size = buf.len;
 			result = strbuf_detach(&buf, NULL);
 			elem->mode = canon_mode(st.st_mode);
-		}
-		else if (0 <= (fd = open(elem->path, O_RDONLY)) &&
-			 !fstat(fd, &st)) {
+		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
 			size_t len = xsize_t(st.st_size);
 			ssize_t done;
 			int is_file, i;
-- 
1.6.1.349.g99fa5

[PATCH/RFC v3 9/9] lstat_cache(): print a warning if doing ping-pong between cache types

[Kjetil Barvik]

This is a debug patch which is only to be used while the lstat_cache()
is in the test stage, and should be removed/reverted before the final
relase.

I think it should be useful to catch these warnings, as I it could be
an indication of that the cache would not be very effective if it is
doing ping-pong by switching between different cache types too many
times.

Also, if someone is experimenting with the lstat_cache(), this patch
will maybe be useful while debugging.

If someone is able to trigger the warning, then send a mail to the GIT
mailing list, containing the first 15 lines of the warning, and a
short description of the GIT commands to trigger the warnings.

I hope someone is willing to use this patch for a while, to be able to
catch possible ping-pong's.

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
 symlinks.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 215d049..0ad8e39 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -51,6 +51,11 @@ static inline void reset_lstat_cache(void)
 	 */
 }
 
+#define SWITCHES_BEFORE_WARNING 10
+static unsigned int cache_switches, number_of_warnings;
+static unsigned int current_cache_func, last_cache_func;
+static unsigned int total_calls;
+
 #define FL_DIR      (1 << 0)
 #define FL_NOENT    (1 << 1)
 #define FL_SYMLINK  (1 << 2)
@@ -77,6 +82,7 @@ static int lstat_cache(const char *name, int len,
 	int match_flags, ret_flags, save_flags, max_len, ret;
 	struct stat st;
 
+	total_calls++;
 	if (cache.track_flags != track_flags ||
 	    cache.prefix_len_stat_func != prefix_len_stat_func) {
 		/*
@@ -88,6 +94,17 @@ static int lstat_cache(const char *name, int len,
 		cache.track_flags = track_flags;
 		cache.prefix_len_stat_func = prefix_len_stat_func;
 		match_len = last_slash = 0;
+		cache_switches++;
+		if (cache_switches > SWITCHES_BEFORE_WARNING) {
+			if (number_of_warnings < 10 || number_of_warnings % 1000 == 0)
+				printf("warning from %s:%d cache_switches:%u > %u "\
+				       "(current:%u last:%u total:%u)\n",
+				       __FILE__, __LINE__,
+				       cache_switches, SWITCHES_BEFORE_WARNING,
+				       current_cache_func, last_cache_func,
+				       total_calls);
+			number_of_warnings++;
+		}
 	} else {
 		/*
 		 * Check to see if we have a match from the cache for
@@ -216,6 +233,8 @@ void clear_lstat_cache(void)
  */
 int has_symlink_leading_path(const char *name, int len)
 {
+	last_cache_func = current_cache_func;
+	current_cache_func = 1;
 	return lstat_cache(name, len,
 			   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &
 		FL_SYMLINK;
@@ -227,6 +246,8 @@ int has_symlink_leading_path(const char *name, int len)
  */
 int has_symlink_or_noent_leading_path(const char *name, int len)
 {
+	last_cache_func = current_cache_func;
+	current_cache_func = 2;
 	return lstat_cache(name, len,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &
 		(FL_SYMLINK|FL_NOENT);
@@ -241,6 +262,8 @@ int has_symlink_or_noent_leading_path(const char *name, int len)
  */
 int has_dirs_only_path(const char *name, int len, int prefix_len)
 {
+	last_cache_func = current_cache_func;
+	current_cache_func = 3;
 	return lstat_cache(name, len,
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;
-- 
1.6.1.349.g99fa5

Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open

[Kjetil Barvik]

* Kjetil Barvik schrieb:
| And, yes, since each lstat() call cost approximately 44 microseconds
| compared to 12-16 for each lstat() on my Linux box, there was a little
                             ^^^^^^^ fstat()?

  Yes, is is the fstat() call which takes 12-16 microseconds on my Linux
  box.

| performance gain from this patch.

* Johannes Sixt <j.sixt@viscovery.net>
| This does look like a good gain. But do you have hard numbers that back
| the claim?
|
| If you can squeeze out a 10% improvement on Linux with this patch, we
| should take it, and if it turns out that it doesn't work on Windows, we
| could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin
| is affected, too) that skips the fstat() optimization.
| 
| But we really should have numbers that justify this effort.

  I have been working on a long running test script latly, but then I
  started to play with the 'git repack' command, so it was not top
  priority.  But, I can finish the script today, and run it while I am
  sleeping tonight.

| BTW, the statement from the Windows documentation was:
|
|  "The only guarantee about a file timestamp is that the file time is
|   correctly reflected when the handle that makes the change is closed."
|
| In the case of this patch, the timestamp is queried via the handle
| that made the change, and in this case special case the timestamp
| could be correct nevertheless. The guarantee doesn't cover this case,
| but it would be natural, and perhaps it Just Works?

  Yes it could perhaps "just works".  But, then I guess that when it
  does not work, the user would not notice it _except_ for more time
  used.  Since I can to this:

kjetil@localhost ~/git/git $ git status 
# On branch lstat_fstat_v6
nothing to commit (working directory clean)
kjetil@localhost ~/git/git $ touch var.c
kjetil@localhost ~/git/git $ git status 
# On branch lstat_fstat_v6
nothing to commit (working directory clean)
kjetil@localhost ~/git/git $ 

  ... I think that git will have to check the content and read each byte
  and do a SHA1 of the file var.c in this case (since the timestamps do
  not match), which is a more time and CPU hungry opperation, to decide
  if there is a difference inside the file or not.

  And, maybe some other platform also have problems with this trick?

  OK, I do the time tests and let the numbers talk.

  -- kjetil