{"thread":{"id":"17556","subject":"[PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()","startedAt":"2009-02-04T12:52:45Z","lastAt":"2009-02-14T17:50:34Z","messageCount":23,"participants":["Kjetil Barvik","Johannes Sixt","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":9},"messages":[{"id":"103123","messageId":"cover.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":null,"subject":"[PATCH/RFC v3 0/9] git checkout: more cleanups, optimisation, less lstat() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:45Z","receivedAt":"2009-02-04T12:52:45Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Changes since v2\n\n--- patch */9 ---\n- Changed the order of some patches.\n- Some updates to the commit log messages.\n\n--- patch 2/9 ---\n- New patch which let us later use longest_match_lstat_cache(), now\n  renamed to longest_path_match(), inside patch 4/9.\n\n--- patch 3/9 ---\n- New patch which update the symlinks.c file to be more in line with\n  the GIT source code (s/length,string/string,length/ for function\n  arguments).\n\n-- patch 4/9 ---\n- The new function schedule_dir_for_removal() is placed inside\n  symlinks.c instead of unpack-trees.c\n\n-- patch 9/9 ---\n- NOTE/NB: this patch is only a debug patch, not be included in the final\n  GIT release version.\n\n\n\nKjetil Barvik (9):\n  lstat_cache(): small cleanup and optimisation\n  lstat_cache(): generalise longest_match_lstat_cache()\n  lstat_cache(): swap func(length, string) into func(string, length)\n  unlink_entry(): introduce schedule_dir_for_removal()\n  create_directories(): remove some memcpy() and strchr() calls\n  write_entry(): cleanup of some duplicated code\n  write_entry(): use fstat() instead of lstat() when file is open\n  show_patch_diff(): remove a call to fstat()\n  lstat_cache(): print a warning if doing ping-pong between cache types\n\n Documentation/CodingGuidelines |    3 +\n builtin-add.c                  |    2 +-\n builtin-apply.c                |    2 +-\n builtin-update-index.c         |    2 +-\n cache.h                        |   10 ++-\n combine-diff.c                 |    4 +-\n diff-lib.c                     |    2 +-\n dir.c                          |    2 +-\n entry.c                        |  108 ++++++++++++-------------\n symlinks.c                     |  177 ++++++++++++++++++++++++++++++----------\n unpack-trees.c                 |   34 ++------\n 11 files changed, 208 insertions(+), 138 deletions(-)\n"},{"id":"103126","messageId":"1aaaa97735ca2c9224ea0d198e600c01fbddf854.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 1/9] lstat_cache(): small cleanup and optimisation","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:46Z","receivedAt":"2009-02-04T12:52:46Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Simplify the if-else test in longest_match_lstat_cache() such that we\nonly have one simple if test.  Instead of testing for 'i == cache.len'\nor 'i == len', we transform this to a common test for 'i == max_len'.\n\nAnd to further optimise we use 'i >= max_len' instead of 'i ==\nmax_len', the reason is that it is now the exact opposite of one part\ninside the while-loop termination expression 'i < max_len && name[i]\n== cache.path[i]', and then the compiler can probably reuse a test\ninstruction from it.\n\nWe also throw away the arguments to reset_lstat_cache(), such that all\nthe safeguard logic inside lstat_cache() is handled at one place.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n symlinks.c |   44 ++++++++++++++++++++++++--------------------\n 1 files changed, 24 insertions(+), 20 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex f262b7c..ae57e56 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -25,27 +25,30 @@ static inline int longest_match_lstat_cache(int len, const char *name,\n \t\t}\n \t\ti++;\n \t}\n-\t/* Is the cached path string a substring of 'name'? */\n-\tif (i == cache.len && cache.len < len && name[cache.len] == '/') {\n-\t\tmatch_len_prev = match_len;\n-\t\tmatch_len = cache.len;\n-\t/* Is 'name' a substring of the cached path string? */\n-\t} else if ((i == len && len < cache.len && cache.path[len] == '/') ||\n-\t\t   (i == len && len == cache.len)) {\n+\t/*\n+\t * Is the cached path string a substring of 'name', is 'name'\n+\t * a substring of the cached path string, or is 'name' and the\n+\t * cached path string the exact same string?\n+\t */\n+\tif (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||\n+\t\t\t     (len < cache.len && cache.path[len] == '/') ||\n+\t\t\t     (len == cache.len))) {\n \t\tmatch_len_prev = match_len;\n-\t\tmatch_len = len;\n+\t\tmatch_len = i;\n \t}\n \t*previous_slash = match_len_prev;\n \treturn match_len;\n }\n \n-static inline void reset_lstat_cache(int track_flags, int prefix_len_stat_func)\n+static inline void reset_lstat_cache(void)\n {\n \tcache.path[0] = '\\0';\n \tcache.len = 0;\n \tcache.flags = 0;\n-\tcache.track_flags = track_flags;\n-\tcache.prefix_len_stat_func = prefix_len_stat_func;\n+\t/*\n+\t * The track_flags and prefix_len_stat_func members is only\n+\t * set by the safeguard rule inside lstat_cache()\n+\t */\n }\n \n #define FL_DIR      (1 << 0)\n@@ -77,11 +80,13 @@ static int lstat_cache(int len, const char *name,\n \tif (cache.track_flags != track_flags ||\n \t    cache.prefix_len_stat_func != prefix_len_stat_func) {\n \t\t/*\n-\t\t * As a safeguard we clear the cache if the values of\n-\t\t * track_flags and/or prefix_len_stat_func does not\n-\t\t * match with the last supplied values.\n+\t\t * As a safeguard rule we clear the cache if the\n+\t\t * values of track_flags and/or prefix_len_stat_func\n+\t\t * does not match with the last supplied values.\n \t\t */\n-\t\treset_lstat_cache(track_flags, prefix_len_stat_func);\n+\t\treset_lstat_cache();\n+\t\tcache.track_flags = track_flags;\n+\t\tcache.prefix_len_stat_func = prefix_len_stat_func;\n \t\tmatch_len = last_slash = 0;\n \t} else {\n \t\t/*\n@@ -153,7 +158,7 @@ static int lstat_cache(int len, const char *name,\n \t\tcache.path[last_slash] = '\\0';\n \t\tcache.len = last_slash;\n \t\tcache.flags = save_flags;\n-\t} else if (track_flags & FL_DIR &&\n+\t} else if ((track_flags & FL_DIR) &&\n \t\t   last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {\n \t\t/*\n \t\t * We have a separate test for the directory case,\n@@ -170,7 +175,7 @@ static int lstat_cache(int len, const char *name,\n \t\tcache.len = last_slash_dir;\n \t\tcache.flags = FL_DIR;\n \t} else {\n-\t\treset_lstat_cache(track_flags, prefix_len_stat_func);\n+\t\treset_lstat_cache();\n \t}\n \treturn ret_flags;\n }\n@@ -190,8 +195,7 @@ void invalidate_lstat_cache(int len, const char *name)\n \t\t\tcache.len = previous_slash;\n \t\t\tcache.flags = FL_DIR;\n \t\t} else\n-\t\t\treset_lstat_cache(cache.track_flags,\n-\t\t\t\t\t  cache.prefix_len_stat_func);\n+\t\t\treset_lstat_cache();\n \t}\n }\n \n@@ -200,7 +204,7 @@ void invalidate_lstat_cache(int len, const char *name)\n  */\n void clear_lstat_cache(void)\n {\n-\treset_lstat_cache(0, 0);\n+\treset_lstat_cache();\n }\n \n #define USE_ONLY_LSTAT  0\n-- \n1.6.1.349.g99fa5\n"},{"id":"103125","messageId":"677cba5ee9c7a89afcc5326251f980109326dfda.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 2/9] lstat_cache(): generalise longest_match_lstat_cache()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:47Z","receivedAt":"2009-02-04T12:52:47Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Rename the function to longst_path_match() and generalise it such that\nit can also be used by other functions.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n symlinks.c |   46 ++++++++++++++++++++++++----------------------\n 1 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex ae57e56..4596aee 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -1,38 +1,30 @@\n #include \"cache.h\"\n \n-static struct cache_def {\n-\tchar path[PATH_MAX + 1];\n-\tint len;\n-\tint flags;\n-\tint track_flags;\n-\tint prefix_len_stat_func;\n-} cache;\n-\n /*\n  * Returns the length (on a path component basis) of the longest\n- * common prefix match of 'name' and the cached path string.\n+ * common prefix match of 'name_a' and 'name_b'.\n  */\n-static inline int longest_match_lstat_cache(int len, const char *name,\n-\t\t\t\t\t    int *previous_slash)\n+static int longest_path_match(const char *name_a, int len_a,\n+\t\t\t      const char *name_b, int len_b,\n+\t\t\t      int *previous_slash)\n {\n \tint max_len, match_len = 0, match_len_prev = 0, i = 0;\n \n-\tmax_len = len < cache.len ? len : cache.len;\n-\twhile (i < max_len && name[i] == cache.path[i]) {\n-\t\tif (name[i] == '/') {\n+\tmax_len = len_a < len_b ? len_a : len_b;\n+\twhile (i < max_len && name_a[i] == name_b[i]) {\n+\t\tif (name_a[i] == '/') {\n \t\t\tmatch_len_prev = match_len;\n \t\t\tmatch_len = i;\n \t\t}\n \t\ti++;\n \t}\n \t/*\n-\t * Is the cached path string a substring of 'name', is 'name'\n-\t * a substring of the cached path string, or is 'name' and the\n-\t * cached path string the exact same string?\n+\t * Is 'name_b' a substring of 'name_a', the other way around,\n+\t * or is 'name_a' and 'name_b' the exact same string?\n \t */\n-\tif (i >= max_len && ((len > cache.len && name[cache.len] == '/') ||\n-\t\t\t     (len < cache.len && cache.path[len] == '/') ||\n-\t\t\t     (len == cache.len))) {\n+\tif (i >= max_len && ((len_a > len_b && name_a[len_b] == '/') ||\n+\t\t\t     (len_a < len_b && name_b[len_a] == '/') ||\n+\t\t\t     (len_a == len_b))) {\n \t\tmatch_len_prev = match_len;\n \t\tmatch_len = i;\n \t}\n@@ -40,6 +32,14 @@ static inline int longest_match_lstat_cache(int len, const char *name,\n \treturn match_len;\n }\n \n+static struct cache_def {\n+\tchar path[PATH_MAX + 1];\n+\tint len;\n+\tint flags;\n+\tint track_flags;\n+\tint prefix_len_stat_func;\n+} cache;\n+\n static inline void reset_lstat_cache(void)\n {\n \tcache.path[0] = '\\0';\n@@ -94,7 +94,8 @@ static int lstat_cache(int len, const char *name,\n \t\t * the 2 \"excluding\" path types.\n \t\t */\n \t\tmatch_len = last_slash =\n-\t\t\tlongest_match_lstat_cache(len, name, &previous_slash);\n+\t\t\tlongest_path_match(name, len, cache.path, cache.len,\n+\t\t\t\t\t   &previous_slash);\n \t\tmatch_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);\n \t\tif (match_flags && match_len == cache.len)\n \t\t\treturn match_flags;\n@@ -188,7 +189,8 @@ void invalidate_lstat_cache(int len, const char *name)\n {\n \tint match_len, previous_slash;\n \n-\tmatch_len = longest_match_lstat_cache(len, name, &previous_slash);\n+\tmatch_len = longest_path_match(name, len, cache.path, cache.len,\n+\t\t\t\t       &previous_slash);\n \tif (len == match_len) {\n \t\tif ((cache.track_flags & FL_DIR) && previous_slash > 0) {\n \t\t\tcache.path[previous_slash] = '\\0';\n-- \n1.6.1.349.g99fa5\n"},{"id":"103122","messageId":"f19eb192deeb73865d91ddb3643378751173f655.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 3/9] lstat_cache(): swap func(length, string) into func(string, length)","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:48Z","receivedAt":"2009-02-04T12:52:48Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Swap function argument pair (length, string) into (string, length) to\nconform with the commonly used order inside the GIT source code.\n\nAlso, add a note about this fact into the coding guidelines.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n Documentation/CodingGuidelines |    3 +++\n builtin-add.c                  |    2 +-\n builtin-apply.c                |    2 +-\n builtin-update-index.c         |    2 +-\n cache.h                        |    8 ++++----\n diff-lib.c                     |    2 +-\n dir.c                          |    2 +-\n entry.c                        |    2 +-\n symlinks.c                     |   16 ++++++++--------\n unpack-trees.c                 |    4 ++--\n 10 files changed, 23 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 0d7fa9c..b8bf618 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -129,3 +129,6 @@ For C programs:\n    used in the git core command set (unless your command is clearly\n    separate from it, such as an importer to convert random-scm-X\n    repositories to git).\n+\n+ - When we pass <string, length> pair to functions, we should try to\n+   pass them in that order.\ndiff --git a/builtin-add.c b/builtin-add.c\nindex ac98c83..a23ad96 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -148,7 +148,7 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p\n \tif (pathspec) {\n \t\tconst char **p;\n \t\tfor (p = pathspec; *p; p++) {\n-\t\t\tif (has_symlink_leading_path(strlen(*p), *p)) {\n+\t\t\tif (has_symlink_leading_path(*p, strlen(*p))) {\n \t\t\t\tint len = prefix ? strlen(prefix) : 0;\n \t\t\t\tdie(\"'%s' is beyond a symbolic link\", *p + len);\n \t\t\t}\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1e7f917..a1de3cb 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2360,7 +2360,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)\n \t\t * In such a case, path \"new_name\" does not exist as\n \t\t * far as git is concerned.\n \t\t */\n-\t\tif (has_symlink_leading_path(strlen(new_name), new_name))\n+\t\tif (has_symlink_leading_path(new_name, strlen(new_name)))\n \t\t\treturn 0;\n \n \t\treturn error(\"%s: already exists in working directory\", new_name);\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 5604977..6c55527 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -195,7 +195,7 @@ static int process_path(const char *path)\n \tstruct stat st;\n \n \tlen = strlen(path);\n-\tif (has_symlink_leading_path(len, path))\n+\tif (has_symlink_leading_path(path, len))\n \t\treturn error(\"'%s' is beyond a symbolic link\", path);\n \n \t/*\ndiff --git a/cache.h b/cache.h\nindex 45e713e..30039ac 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -721,10 +721,10 @@ struct checkout {\n };\n \n extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);\n-extern int has_symlink_leading_path(int len, const char *name);\n-extern int has_symlink_or_noent_leading_path(int len, const char *name);\n-extern int has_dirs_only_path(int len, const char *name, int prefix_len);\n-extern void invalidate_lstat_cache(int len, const char *name);\n+extern int has_symlink_leading_path(const char *name, int len);\n+extern int has_symlink_or_noent_leading_path(const char *name, int len);\n+extern int has_dirs_only_path(const char *name, int len, int prefix_len);\n+extern void invalidate_lstat_cache(const char *name, int len);\n extern void clear_lstat_cache(void);\n \n extern struct alternate_object_database {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a41e1ec..a3ba20e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -31,7 +31,7 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)\n \t\t\treturn -1;\n \t\treturn 1;\n \t}\n-\tif (has_symlink_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 1;\n \tif (S_ISDIR(st->st_mode)) {\n \t\tunsigned char sub[20];\ndiff --git a/dir.c b/dir.c\nindex cfd1ea5..8fb5226 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -720,7 +720,7 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i\n {\n \tstruct path_simplify *simplify;\n \n-\tif (has_symlink_leading_path(strlen(path), path))\n+\tif (has_symlink_leading_path(path, strlen(path)))\n \t\treturn dir->nr;\n \n \tsimplify = create_simplify(pathspec);\ndiff --git a/entry.c b/entry.c\nindex 05aa58d..bb6bdb9 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -20,7 +20,7 @@ static void create_directories(const char *path, const struct checkout *state)\n \t\t * we test the path components of the prefix with the\n \t\t * stat() function instead of the lstat() function.\n \t\t */\n-\t\tif (has_dirs_only_path(len, buf, state->base_dir_len))\n+\t\tif (has_dirs_only_path(buf, len, state->base_dir_len))\n \t\t\tcontinue; /* ok, it is already a directory. */\n \n \t\t/*\ndiff --git a/symlinks.c b/symlinks.c\nindex 4596aee..5167286 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -70,7 +70,7 @@ static inline void reset_lstat_cache(void)\n  * of the prefix, where the cache should use the stat() function\n  * instead of the lstat() function to test each path component.\n  */\n-static int lstat_cache(int len, const char *name,\n+static int lstat_cache(const char *name, int len,\n \t\t       int track_flags, int prefix_len_stat_func)\n {\n \tint match_len, last_slash, last_slash_dir, previous_slash;\n@@ -185,7 +185,7 @@ static int lstat_cache(int len, const char *name,\n  * Invalidate the given 'name' from the cache, if 'name' matches\n  * completely with the cache.\n  */\n-void invalidate_lstat_cache(int len, const char *name)\n+void invalidate_lstat_cache(const char *name, int len)\n {\n \tint match_len, previous_slash;\n \n@@ -214,9 +214,9 @@ void clear_lstat_cache(void)\n /*\n  * Return non-zero if path 'name' has a leading symlink component\n  */\n-int has_symlink_leading_path(int len, const char *name)\n+int has_symlink_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_cache(name, len,\n \t\t\t   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n }\n@@ -225,9 +225,9 @@ int has_symlink_leading_path(int len, const char *name)\n  * Return non-zero if path 'name' has a leading symlink component or\n  * if some leading path component does not exists.\n  */\n-int has_symlink_or_noent_leading_path(int len, const char *name)\n+int has_symlink_or_noent_leading_path(const char *name, int len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_cache(name, len,\n \t\t\t   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &\n \t\t(FL_SYMLINK|FL_NOENT);\n }\n@@ -239,9 +239,9 @@ int has_symlink_or_noent_leading_path(int len, const char *name)\n  * 'prefix_len', thus we then allow for symlinks in the prefix part as\n  * long as those points to real existing directories.\n  */\n-int has_dirs_only_path(int len, const char *name, int prefix_len)\n+int has_dirs_only_path(const char *name, int len, int prefix_len)\n {\n-\treturn lstat_cache(len, name,\n+\treturn lstat_cache(name, len,\n \t\t\t   FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..2293158 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -61,7 +61,7 @@ static void unlink_entry(struct cache_entry *ce)\n \tchar *cp, *prev;\n \tchar *name = ce->name;\n \n-\tif (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn;\n \tif (unlink(name))\n \t\treturn;\n@@ -583,7 +583,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \tif (o->index_only || o->reset || !o->update)\n \t\treturn 0;\n \n-\tif (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))\n+\tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n-- \n1.6.1.349.g99fa5\n"},{"id":"103121","messageId":"5e00462034e14b628ad6744edc1fad7daee78c5c.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:49Z","receivedAt":"2009-02-04T12:52:49Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside unlink_entry() if we get a successful removal of one\nfile with unlink(), we try to remove the leading directories each and\nevery time.  So if one directory containing 200 files is moved to an\nother location we get 199 failed calls to rmdir() and 1 successful\ncall.\n\nTo fix this and avoid some unnecessary calls to rmdir(), we schedule\neach directory for removal and wait much longer before we do the real\ncall to rmdir().\n\nSince the unlink_entry() function is called with alphabetically sorted\nnames, this new function end up being very effective to avoid\nunnecessary calls to rmdir().  In some cases over 95% of all calls to\nrmdir() is removed with this patch.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n cache.h        |    2 +\n symlinks.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c |   30 +++++----------------------\n 3 files changed, 68 insertions(+), 24 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 30039ac..717dd84 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -726,6 +726,8 @@ extern int has_symlink_or_noent_leading_path(const char *name, int len);\n extern int has_dirs_only_path(const char *name, int len, int prefix_len);\n extern void invalidate_lstat_cache(const char *name, int len);\n extern void clear_lstat_cache(void);\n+extern void schedule_dir_for_removal(const char *name, int len);\n+extern void remove_scheduled_dirs(void);\n \n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\ndiff --git a/symlinks.c b/symlinks.c\nindex 5167286..215d049 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -245,3 +245,63 @@ int has_dirs_only_path(const char *name, int len, int prefix_len)\n \t\t\t   FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n }\n+\n+static struct removal_def {\n+\tchar path[PATH_MAX];\n+\tint len;\n+} removal;\n+\n+static void do_remove_scheduled_dirs(int new_len)\n+{\n+\twhile (removal.len > new_len) {\n+\t\tremoval.path[removal.len] = '\\0';\n+\t\tif (rmdir(removal.path))\n+\t\t\tbreak;\n+\t\tdo {\n+\t\t\tremoval.len--;\n+\t\t} while (removal.len > new_len &&\n+\t\t\t removal.path[removal.len] != '/');\n+\t}\n+\tremoval.len = new_len;\n+\treturn;\n+}\n+\n+void schedule_dir_for_removal(const char *name, int len)\n+{\n+\tint match_len, last_slash, i, previous_slash;\n+\n+\tmatch_len = last_slash = i =\n+\t\tlongest_path_match(name, len, removal.path, removal.len,\n+\t\t\t\t   &previous_slash);\n+\t/* Find last slash inside 'name' */\n+\twhile (i < len) {\n+\t\tif (name[i] == '/')\n+\t\t\tlast_slash = i;\n+\t\ti++;\n+\t}\n+\n+\t/*\n+\t * If we are about to go down the directory tree, we check if\n+\t * we must first go upwards the tree, such that we then can\n+\t * remove possible empty directories as we go upwards.\n+\t */\n+\tif (match_len < last_slash && match_len < removal.len)\n+\t\tdo_remove_scheduled_dirs(match_len);\n+\t/*\n+\t * If we go deeper down the directory tree, we only need to\n+\t * save the new path components as we go down.\n+\t */\n+\tif (match_len < last_slash) {\n+\t\tmemmove(&removal.path[match_len], &name[match_len],\n+\t\t\tlast_slash - match_len);\n+\t\tremoval.len = last_slash;\n+\t}\n+\treturn;\n+}\n+\n+#define ALL_DIRS 0\n+void remove_scheduled_dirs(void)\n+{\n+\tdo_remove_scheduled_dirs(ALL_DIRS);\n+\treturn;\n+}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2293158..e3c3fa1 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -52,36 +52,17 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);\n }\n \n-/* Unlink the last component and attempt to remove leading\n- * directories, in case this unlink is the removal of the\n- * last entry in the directory -- empty directories are removed.\n+/*\n+ * Unlink the last component and schedule the leading directories for\n+ * removal, such that empty directories get removed.\n  */\n static void unlink_entry(struct cache_entry *ce)\n {\n-\tchar *cp, *prev;\n-\tchar *name = ce->name;\n-\n \tif (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))\n \t\treturn;\n-\tif (unlink(name))\n+\tif (unlink(ce->name))\n \t\treturn;\n-\tprev = NULL;\n-\twhile (1) {\n-\t\tint status;\n-\t\tcp = strrchr(name, '/');\n-\t\tif (prev)\n-\t\t\t*prev = '/';\n-\t\tif (!cp)\n-\t\t\tbreak;\n-\n-\t\t*cp = 0;\n-\t\tstatus = rmdir(name);\n-\t\tif (status) {\n-\t\t\t*cp = '/';\n-\t\t\tbreak;\n-\t\t}\n-\t\tprev = cp;\n-\t}\n+\tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n }\n \n static struct checkout state;\n@@ -117,6 +98,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\tcontinue;\n \t\t}\n \t}\n+\tremove_scheduled_dirs();\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n-- \n1.6.1.349.g99fa5\n"},{"id":"103127","messageId":"4e4df31abdedfdb7cdca587491acf2849074d19c.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 5/9] create_directories(): remove some memcpy() and strchr() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:50Z","receivedAt":"2009-02-04T12:52:50Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Remove the call to memcpy() and strchr() for each path component\ntested, and instead add each path component as we go forward inside\nthe while-loop.\n\nImpact: small optimisation\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n entry.c |   23 ++++++++++++++---------\n 1 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex bb6bdb9..cc8f0c6 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,15 +2,19 @@\n #include \"blob.h\"\n #include \"dir.h\"\n \n-static void create_directories(const char *path, const struct checkout *state)\n+static void create_directories(const char *path, int path_len,\n+\t\t\t       const struct checkout *state)\n {\n-\tint len = strlen(path);\n-\tchar *buf = xmalloc(len + 1);\n-\tconst char *slash = path;\n-\n-\twhile ((slash = strchr(slash+1, '/')) != NULL) {\n-\t\tlen = slash - path;\n-\t\tmemcpy(buf, path, len);\n+\tchar *buf = xmalloc(path_len + 1);\n+\tint len = 0;\n+\n+\twhile (len < path_len) {\n+\t\tdo {\n+\t\t\tbuf[len] = path[len];\n+\t\t\tlen++;\n+\t\t} while (len < path_len && path[len] != '/');\n+\t\tif (len >= path_len)\n+\t\t\tbreak;\n \t\tbuf[len] = 0;\n \n \t\t/*\n@@ -190,6 +194,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \n \tmemcpy(path, state->base_dir, len);\n \tstrcpy(path + len, ce->name);\n+\tlen += ce_namelen(ce);\n \n \tif (!lstat(path, &st)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);\n@@ -218,6 +223,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n-\tcreate_directories(path, state);\n+\tcreate_directories(path, len, state);\n \treturn write_entry(ce, path, state, 0);\n }\n-- \n1.6.1.349.g99fa5\n"},{"id":"103128","messageId":"a9e25f77aadd46fffb5db44d70995ab6cad8457d.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 6/9] write_entry(): cleanup of some duplicated code","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:52:51Z","receivedAt":"2009-02-04T12:52:51Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"The switch-cases for S_IFREG and S_IFLNK was so similar that it will\nbe better to do some cleanup and use the common parts of it.\n\nAnd the entry.c file should now be clean for 'gcc -Wextra' warnings.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n entry.c |   75 +++++++++++++++++++++++++-------------------------------------\n 1 files changed, 30 insertions(+), 45 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex cc8f0c6..1f53588 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -78,7 +78,7 @@ static int create_file(const char *path, unsigned int mode)\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n \n-static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned long *size)\n+static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)\n {\n \tenum object_type type;\n \tvoid *new = read_sha1_file(ce->sha1, &type, size);\n@@ -93,36 +93,51 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned\n \n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n-\tint fd;\n-\tlong wrote;\n-\n-\tswitch (ce->ce_mode & S_IFMT) {\n-\t\tchar *new;\n-\t\tstruct strbuf buf;\n-\t\tunsigned long size;\n-\n+\tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n+\tint fd, ret;\n+\tchar *new;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tunsigned long size;\n+\tsize_t wrote, newsize = 0;\n+\n+\tswitch (ce_mode_s_ifmt) {\n \tcase S_IFREG:\n-\t\tnew = read_blob_entry(ce, path, &size);\n+\tcase S_IFLNK:\n+\t\tnew = read_blob_entry(ce, &size);\n \t\tif (!new)\n \t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, sha1_to_hex(ce->sha1));\n \n+\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n+\t\t\tret = symlink(new, path);\n+\t\t\tfree(new);\n+\t\t\tif (ret)\n+\t\t\t\treturn error(\"git checkout-index: unable to create symlink %s (%s)\",\n+\t\t\t\t\t     path, strerror(errno));\n+\t\t\tbreak;\n+\t\t}\n+\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tstrbuf_init(&buf, 0);\n-\t\tif (convert_to_working_tree(ce->name, new, size, &buf)) {\n-\t\t\tsize_t newsize = 0;\n+\t\tif (ce_mode_s_ifmt == S_IFREG &&\n+\t\t    convert_to_working_tree(ce->name, new, size, &buf)) {\n \t\t\tfree(new);\n \t\t\tnew = strbuf_detach(&buf, &newsize);\n \t\t\tsize = newsize;\n \t\t}\n \n \t\tif (to_tempfile) {\n-\t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n+\t\t\tif (ce_mode_s_ifmt == S_IFREG)\n+\t\t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n+\t\t\telse\n+\t\t\t\tstrcpy(path, \".merge_link_XXXXXX\");\n \t\t\tfd = mkstemp(path);\n-\t\t} else\n+\t\t} else if (ce_mode_s_ifmt == S_IFREG) {\n \t\t\tfd = create_file(path, ce->ce_mode);\n+\t\t} else {\n+\t\t\tfd = create_file(path, 0666);\n+\t\t}\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n \t\t\treturn error(\"git checkout-index: unable to create file %s (%s)\",\n@@ -135,36 +150,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tif (wrote != size)\n \t\t\treturn error(\"git checkout-index: unable to write file %s\", path);\n \t\tbreak;\n-\tcase S_IFLNK:\n-\t\tnew = read_blob_entry(ce, path, &size);\n-\t\tif (!new)\n-\t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n-\t\t\t\tpath, sha1_to_hex(ce->sha1));\n-\t\tif (to_tempfile || !has_symlinks) {\n-\t\t\tif (to_tempfile) {\n-\t\t\t\tstrcpy(path, \".merge_link_XXXXXX\");\n-\t\t\t\tfd = mkstemp(path);\n-\t\t\t} else\n-\t\t\t\tfd = create_file(path, 0666);\n-\t\t\tif (fd < 0) {\n-\t\t\t\tfree(new);\n-\t\t\t\treturn error(\"git checkout-index: unable to create \"\n-\t\t\t\t\t\t \"file %s (%s)\", path, strerror(errno));\n-\t\t\t}\n-\t\t\twrote = write_in_full(fd, new, size);\n-\t\t\tclose(fd);\n-\t\t\tfree(new);\n-\t\t\tif (wrote != size)\n-\t\t\t\treturn error(\"git checkout-index: unable to write file %s\",\n-\t\t\t\t\tpath);\n-\t\t} else {\n-\t\t\twrote = symlink(new, path);\n-\t\t\tfree(new);\n-\t\t\tif (wrote)\n-\t\t\t\treturn error(\"git checkout-index: unable to create \"\n-\t\t\t\t\t\t \"symlink %s (%s)\", path, strerror(errno));\n-\t\t}\n-\t\tbreak;\n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n \t\t\treturn error(\"git checkout-index: cannot create temporary subproject %s\", path);\n-- \n1.6.1.349.g99fa5\n"},{"id":"103129","messageId":"21073c1f3f6c2c81b26a632f495325f5e7a7de5a.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:53:18Z","receivedAt":"2009-02-04T12:53:18Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside write_entry() we do an lstat(path, &st) call on a\nfile which have just been opened inside the exact same function.  It\nshould be better to call fstat(fd, &st) on the file while it is open,\nand it should be at least as fast as the lstat() method.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n entry.c |   12 +++++++++---\n 1 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 1f53588..5daacc2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -94,11 +94,12 @@ static void *read_blob_entry(struct cache_entry *ce, unsigned long *size)\n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n-\tint fd, ret;\n+\tint fd, ret, fstat_done = 0;\n \tchar *new;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned long size;\n \tsize_t wrote, newsize = 0;\n+\tstruct stat st;\n \n \tswitch (ce_mode_s_ifmt) {\n \tcase S_IFREG:\n@@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t}\n \n \t\twrote = write_in_full(fd, new, size);\n+\t\t/* use fstat() only when path == ce->name */\n+\t\tif (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n+\t\t\tfstat(fd, &st);\n+\t\t\tfstat_done = 1;\n+\t\t}\n \t\tclose(fd);\n \t\tfree(new);\n \t\tif (wrote != size)\n@@ -161,8 +167,8 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t}\n \n \tif (state->refresh_cache) {\n-\t\tstruct stat st;\n-\t\tlstat(ce->name, &st);\n+\t\tif (!fstat_done)\n+\t\t\tlstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n \t}\n \treturn 0;\n-- \n1.6.1.349.g99fa5\n"},{"id":"103130","messageId":"8221299c3dcfc7c2a874ebfe2a2826ee1be7ff84.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 8/9] show_patch_diff(): remove a call to fstat()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:53:19Z","receivedAt":"2009-02-04T12:53:19Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside show_patch_diff() we have an fstat() call after an\nok lstat() call.  Since before the call to fstat() we have already\ntested for the link case with S_ISLNK(), the fstat() can be removed.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n combine-diff.c |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bccc018..4300319 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -713,9 +713,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult_size = buf.len;\n \t\t\tresult = strbuf_detach(&buf, NULL);\n \t\t\telem->mode = canon_mode(st.st_mode);\n-\t\t}\n-\t\telse if (0 <= (fd = open(elem->path, O_RDONLY)) &&\n-\t\t\t !fstat(fd, &st)) {\n+\t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n \t\t\tssize_t done;\n \t\t\tint is_file, i;\n-- \n1.6.1.349.g99fa5\n"},{"id":"103124","messageId":"4f1c56791fb6b40ce54a3b6a5ad873ab78c0d194.1233751281.git.barvik@broadpark.no","threadId":"17556","inReplyTo":"cover.1233751281.git.barvik@broadpark.no","subject":"[PATCH/RFC v3 9/9] lstat_cache(): print a warning if doing ping-pong between cache types","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T12:53:20Z","receivedAt":"2009-02-04T12:53:20Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"This is a debug patch which is only to be used while the lstat_cache()\nis in the test stage, and should be removed/reverted before the final\nrelase.\n\nI think it should be useful to catch these warnings, as I it could be\nan indication of that the cache would not be very effective if it is\ndoing ping-pong by switching between different cache types too many\ntimes.\n\nAlso, if someone is experimenting with the lstat_cache(), this patch\nwill maybe be useful while debugging.\n\nIf someone is able to trigger the warning, then send a mail to the GIT\nmailing list, containing the first 15 lines of the warning, and a\nshort description of the GIT commands to trigger the warnings.\n\nI hope someone is willing to use this patch for a while, to be able to\ncatch possible ping-pong's.\n\nSigned-off-by: Kjetil Barvik <barvik@broadpark.no>\n---\n symlinks.c |   23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex 215d049..0ad8e39 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -51,6 +51,11 @@ static inline void reset_lstat_cache(void)\n \t */\n }\n \n+#define SWITCHES_BEFORE_WARNING 10\n+static unsigned int cache_switches, number_of_warnings;\n+static unsigned int current_cache_func, last_cache_func;\n+static unsigned int total_calls;\n+\n #define FL_DIR      (1 << 0)\n #define FL_NOENT    (1 << 1)\n #define FL_SYMLINK  (1 << 2)\n@@ -77,6 +82,7 @@ static int lstat_cache(const char *name, int len,\n \tint match_flags, ret_flags, save_flags, max_len, ret;\n \tstruct stat st;\n \n+\ttotal_calls++;\n \tif (cache.track_flags != track_flags ||\n \t    cache.prefix_len_stat_func != prefix_len_stat_func) {\n \t\t/*\n@@ -88,6 +94,17 @@ static int lstat_cache(const char *name, int len,\n \t\tcache.track_flags = track_flags;\n \t\tcache.prefix_len_stat_func = prefix_len_stat_func;\n \t\tmatch_len = last_slash = 0;\n+\t\tcache_switches++;\n+\t\tif (cache_switches > SWITCHES_BEFORE_WARNING) {\n+\t\t\tif (number_of_warnings < 10 || number_of_warnings % 1000 == 0)\n+\t\t\t\tprintf(\"warning from %s:%d cache_switches:%u > %u \"\\\n+\t\t\t\t       \"(current:%u last:%u total:%u)\\n\",\n+\t\t\t\t       __FILE__, __LINE__,\n+\t\t\t\t       cache_switches, SWITCHES_BEFORE_WARNING,\n+\t\t\t\t       current_cache_func, last_cache_func,\n+\t\t\t\t       total_calls);\n+\t\t\tnumber_of_warnings++;\n+\t\t}\n \t} else {\n \t\t/*\n \t\t * Check to see if we have a match from the cache for\n@@ -216,6 +233,8 @@ void clear_lstat_cache(void)\n  */\n int has_symlink_leading_path(const char *name, int len)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 1;\n \treturn lstat_cache(name, len,\n \t\t\t   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n@@ -227,6 +246,8 @@ int has_symlink_leading_path(const char *name, int len)\n  */\n int has_symlink_or_noent_leading_path(const char *name, int len)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 2;\n \treturn lstat_cache(name, len,\n \t\t\t   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &\n \t\t(FL_SYMLINK|FL_NOENT);\n@@ -241,6 +262,8 @@ int has_symlink_or_noent_leading_path(const char *name, int len)\n  */\n int has_dirs_only_path(const char *name, int len, int prefix_len)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 3;\n \treturn lstat_cache(name, len,\n \t\t\t   FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n-- \n1.6.1.349.g99fa5\n"},{"id":"103142","messageId":"49899FA4.2020003@viscovery.net","threadId":"17556","inReplyTo":"21073c1f3f6c2c81b26a632f495325f5e7a7de5a.1233751281.git.barvik@broadpark.no","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-02-04T14:01:08Z","receivedAt":"2009-02-04T14:01:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Kjetil Barvik schrieb:\n> Currently inside write_entry() we do an lstat(path, &st) call on a\n> file which have just been opened inside the exact same function.  It\n> should be better to call fstat(fd, &st) on the file while it is open,\n> and it should be at least as fast as the lstat() method.\n...\n> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n>  \t\t}\n>  \n>  \t\twrote = write_in_full(fd, new, size);\n> +\t\t/* use fstat() only when path == ce->name */\n> +\t\tif (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n> +\t\t\tfstat(fd, &st);\n> +\t\t\tfstat_done = 1;\n> +\t\t}\n>  \t\tclose(fd);\n\nI've a bad gut feeling about this: It may not work as expected on Windows\nbecause there is this statement in the documentation:\n\n  \"The only guarantee about a file timestamp is that the file time is\n   correctly reflected when the handle that makes the change is closed.\"\n\n(http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)\n\nWe are operating on a temporary file. It could happen that the fstat()\nreturns the time when the file was created, as opposed to when the\nwrite_in_full() was completed successfully. The fstat()ed time ends up in\nthe index, but it can be different from what later lstat() calls report\n(and the file would be regarded as modified).\n\nI have the suspicion that the gain from this patch is minimal. Would you\nmind playing it safe and drop this patch?\n\n-- Hannes\n"},{"id":"103180","messageId":"7vfxiut57t.fsf@gitster.siamese.dyndns.org","threadId":"17556","inReplyTo":"49899FA4.2020003@viscovery.net","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-04T18:41:26Z","receivedAt":"2009-02-04T18:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Kjetil Barvik schrieb:\n>> Currently inside write_entry() we do an lstat(path, &st) call on a\n>> file which have just been opened inside the exact same function.  It\n>> should be better to call fstat(fd, &st) on the file while it is open,\n>> and it should be at least as fast as the lstat() method.\n> ...\n>> @@ -145,6 +146,11 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n>>  \t\t}\n>>  \n>>  \t\twrote = write_in_full(fd, new, size);\n>> +\t\t/* use fstat() only when path == ce->name */\n>> +\t\tif (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n>> +\t\t\tfstat(fd, &st);\n>> +\t\t\tfstat_done = 1;\n>> +\t\t}\n>>  \t\tclose(fd);\n>\n> I've a bad gut feeling about this: It may not work as expected on Windows\n> because there is this statement in the documentation:\n>\n>   \"The only guarantee about a file timestamp is that the file time is\n>    correctly reflected when the handle that makes the change is closed.\"\n>\n> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)\n>\n> We are operating on a temporary file. It could happen that the fstat()\n> returns the time when the file was created, as opposed to when the\n> write_in_full() was completed successfully. The fstat()ed time ends up in\n> the index, but it can be different from what later lstat() calls report\n> (and the file would be regarded as modified).\n>\n> I have the suspicion that the gain from this patch is minimal. Would you\n> mind playing it safe and drop this patch?\n\nHmm, write_entry() is actually called once per one path we write out, and\nthe fstat() is added to the common case (no --tempfile, no --prefix=<dir>\ncheckout), which would mean that if there were any performance gain from\nthis change, it was obtained by trading correctness away.  Sad.\n"},{"id":"103189","messageId":"86tz7ayo51.fsf_-_@broadpark.no","threadId":"17556","inReplyTo":"7vfxiut57t.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T19:53:46Z","receivedAt":"2009-02-04T19:53:46Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n<snip>\n>> I've a bad gut feeling about this: It may not work as expected on Windows\n>> because there is this statement in the documentation:\n>>\n>>   \"The only guarantee about a file timestamp is that the file time is\n>>    correctly reflected when the handle that makes the change is closed.\"\n>>\n>> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)\n\n  Ok, I admit I was not aware of this Windows fact.\n\n>> We are operating on a temporary file. It could happen that the fstat()\n>> returns the time when the file was created, as opposed to when the\n>> write_in_full() was completed successfully. The fstat()ed time ends up in\n>> the index, but it can be different from what later lstat() calls report\n>> (and the file would be regarded as modified).\n>>\n>> I have the suspicion that the gain from this patch is minimal. Would you\n>> mind playing it safe and drop this patch?\n>\n> Hmm, write_entry() is actually called once per one path we write out, and\n> the fstat() is added to the common case (no --tempfile, no --prefix=<dir>\n> checkout), \n\n  Yes, I had to make sure that the path string and ce->name was the\n  exact same string, so therefore I had to add the test \"&& !to_tempfile\n  && !state->base_dir_len\" to the if-test.\n\n> which would mean that if there were any performance gain from\n> this change, it was obtained by trading correctness away.  Sad.\n\n  Sorry about this.  Yes, I agree that we should drop this patch.\n\n  And, yes, since each lstat() call cost approximately 44 microseconds\n  compared to 12-16 for each lstat() on my Linux box, there was a little\n  performance gain from this patch.\n\n  Junio, is it OK to ask that you drop this patch if/when you update the\n  pu branch, such that I do not have to resend the patch series almost\n  unchanged to the mailinglist (except for one missing patch)?\n\n  Ok, maybe wait one day, just in case there will be more comments.\n\n  And, thanks for the review!\n\n  -- kjetil\n"},{"id":"103196","messageId":"7vbptiosgj.fsf@gitster.siamese.dyndns.org","threadId":"17556","inReplyTo":"86tz7ayo51.fsf_-_@broadpark.no","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-04T20:30:36Z","receivedAt":"2009-02-04T20:30:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik <barvik@broadpark.no> writes:\n\n>   Junio, is it OK to ask that you drop this patch if/when you update the\n>   pu branch, such that I do not have to resend the patch series almost\n>   unchanged to the mailinglist (except for one missing patch)?\n\nSurely.  Although I've replaced the topic with the updated series, I am\nnot in the reintegration phase yet, so not much work is lost.  I was\nplanning to merge this to 'next' today after one last round of eyeballing.\n\nThanks.\n"},{"id":"103205","messageId":"7vmyd1orcq.fsf@gitster.siamese.dyndns.org","threadId":"17556","inReplyTo":"5e00462034e14b628ad6744edc1fad7daee78c5c.1233751281.git.barvik@broadpark.no","subject":"Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-04T20:54:29Z","receivedAt":"2009-02-04T20:54:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik <barvik@broadpark.no> writes:\n\n> Since the unlink_entry() function is called with alphabetically sorted\n> names, this new function end up being very effective to avoid\n> unnecessary calls to rmdir().  In some cases over 95% of all calls to\n> rmdir() is removed with this patch.\n\nI first feared that this will badly break D/F conflicting cases where we\nremove a file D/F that is the last remaining file in directory D and rely\non the successful removal of the directory to create a file D, but the\ncheck_updates() function already is structured to allow this late removal\neasily.\n\nI think the patch is sensible modulo minor nits.\n\n * The new global path_buf/path_len in unpack-trees.c are named too\n   generic.  It is not just a buffer anybody can use for anything, but\n   is used to keep track for a specific purpose of doing something, so\n   please name them after that something.\n\n * \"#define ALL_DIRS 0\" is useless, given the way how the parameter to\n   do_remove_scheduled_dirs() is named;\n"},{"id":"103206","messageId":"7vfxitorb7.fsf@gitster.siamese.dyndns.org","threadId":"17556","inReplyTo":"5e00462034e14b628ad6744edc1fad7daee78c5c.1233751281.git.barvik@broadpark.no","subject":"Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-04T20:55:24Z","receivedAt":"2009-02-04T20:55:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik <barvik@broadpark.no> writes:\n\n> Since the unlink_entry() function is called with alphabetically sorted\n> names, this new function end up being very effective to avoid\n> unnecessary calls to rmdir().  In some cases over 95% of all calls to\n> rmdir() is removed with this patch.\n\nI first feared that this will badly break D/F conflicting cases where we\nremove a file D/F that is the last remaining file in directory D and rely\non the successful removal of the directory to create a file D, but the\ncheck_updates() function already is structured to allow this late removal\neasily.\n\nI think the patch is sensible modulo minor nits.\n\n * The new global path_buf/path_len in unpack-trees.c are named too\n   generic.  It is not just a buffer anybody can use for anything, but\n   is used to keep track for a specific purpose of doing something, so\n   please name them after that something.\n\n * \"#define ALL_DIRS 0\" is useless, given the way how the parameter to\n   do_remove_scheduled_dirs() is named;\n"},{"id":"103210","messageId":"86myd1zy4x.fsf@broadpark.no","threadId":"17556","inReplyTo":"7vfxitorb7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC v3 4/9] unlink_entry(): introduce schedule_dir_for_removal()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-04T21:32:30Z","receivedAt":"2009-02-04T21:32:30Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Kjetil Barvik <barvik@broadpark.no> writes:\n>\n>> Since the unlink_entry() function is called with alphabetically sorted\n>> names, this new function end up being very effective to avoid\n>> unnecessary calls to rmdir().  In some cases over 95% of all calls to\n>> rmdir() is removed with this patch.\n>\n> I first feared that this will badly break D/F conflicting cases where we\n> remove a file D/F that is the last remaining file in directory D and rely\n> on the successful removal of the directory to create a file D, but the\n> check_updates() function already is structured to allow this late removal\n> easily.\n>\n> I think the patch is sensible modulo minor nits.\n\n  Yes, if the 2 for-loop's inside check_updates() is rewritten into 1\n  for-loop, then you could end up with such a conflict.  Should I add a\n  comment about this inside check_updates()?\n\n>  * The new global path_buf/path_len in unpack-trees.c are named too\n>    generic.  It is not just a buffer anybody can use for anything, but\n>    is used to keep track for a specific purpose of doing something, so\n>    please name them after that something.\n\n   Already done in v3 (this version), since the functions\n   schedule_dir_for_removal() and remove_scheduled_dirs() is now\n   implemented in the symlinks.c file, where I have made the following:\n\nstatic struct removal_def {\n\tchar path[PATH_MAX];\n\tint len;\n} removal;\n\n>  * \"#define ALL_DIRS 0\" is useless, given the way how the parameter to\n>    do_remove_scheduled_dirs() is named;\n\n  Ok, I can do a rebase of this patch (4/9) and resend the patches.\n\n  -- kjetil\n"},{"id":"103301","messageId":"498A9FD3.2020601@viscovery.net","threadId":"17556","inReplyTo":"86tz7ayo51.fsf_-_@broadpark.no","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-02-05T08:14:11Z","receivedAt":"2009-02-05T08:14:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Kjetil Barvik schrieb:\n>   And, yes, since each lstat() call cost approximately 44 microseconds\n>   compared to 12-16 for each lstat() on my Linux box, there was a little\n                               ^^^^^^^ fstat()?\n>   performance gain from this patch.\n\nThis does look like a good gain. But do you have hard numbers that back\nthe claim?\n\nIf you can squeeze out a 10% improvement on Linux with this patch, we\nshould take it, and if it turns out that it doesn't work on Windows, we\ncould trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin\nis affected, too) that skips the fstat() optimization.\n\nBut we really should have numbers that justify this effort.\n\nBTW, the statement from the Windows documentation was:\n\n  \"The only guarantee about a file timestamp is that the file time is\n   correctly reflected when the handle that makes the change is closed.\"\n\nIn the case of this patch, the timestamp is queried via the handle that\nmade the change, and in this case special case the timestamp could be\ncorrect nevertheless. The guarantee doesn't cover this case, but it would\nbe natural, and perhaps it Just Works?\n\n-- Hannes\n"},{"id":"103321","messageId":"alpine.DEB.1.00.0902051202440.7491@intel-tinevez-2-302","threadId":"17556","inReplyTo":"498A9FD3.2020601@viscovery.net","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-05T11:03:47Z","receivedAt":"2009-02-05T11:03:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Feb 2009, Johannes Sixt wrote:\n\n> Kjetil Barvik schrieb:\n> >   And, yes, since each lstat() call cost approximately 44 microseconds\n> >   compared to 12-16 for each lstat() on my Linux box, there was a little\n>                                ^^^^^^^ fstat()?\n> >   performance gain from this patch.\n> \n> This does look like a good gain. But do you have hard numbers that back\n> the claim?\n> \n> If you can squeeze out a 10% improvement on Linux with this patch, we\n> should take it, and if it turns out that it doesn't work on Windows, we\n> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin\n> is affected, too) that skips the fstat() optimization.\n\nOf course, what we _really_ would do is to provide a flag, say, \nFSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile \nfor the appropriate platforms).\n\nCiao,\nDscho\n"},{"id":"103363","messageId":"7veiyceq1p.fsf@gitster.siamese.dyndns.org","threadId":"17556","inReplyTo":"alpine.DEB.1.00.0902051202440.7491@intel-tinevez-2-302","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-05T17:45:06Z","receivedAt":"2009-02-05T17:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Of course, what we _really_ would do is to provide a flag, say, \n> FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile \n> for the appropriate platforms).\n\nSounds sensible.  POSIX of course requires write(), pwrite() and friends\nto mark the mtime for update and fstat() to be reliable, but there may be\n\"not quite POSIX\" systems.\n"},{"id":"103452","messageId":"868wojq0xa.fsf@broadpark.no","threadId":"17556","inReplyTo":"alpine.DEB.1.00.0902051202440.7491@intel-tinevez-2-302","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-06T11:06:57Z","receivedAt":"2009-02-06T11:06:57Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"* On Thu, 5 Feb 2009, Johannes Sixt wrote:\n|\n|> Kjetil Barvik schrieb:\n|> >   And, yes, since each lstat() call cost approximately 44 microseconds\n|> >   compared to 12-16 for each lstat() on my Linux box, there was a little\n|>                                ^^^^^^^ fstat()?\n|> >   performance gain from this patch.\n|> \n|> This does look like a good gain. But do you have hard numbers that back\n|> the claim?\n\n  __________________\n  Test description\n\n  I have used the following 8 git binaries:\n\n$ for g in ./git_t*; do printf \"$g => $($g --version)\\n\"; done\n./git_t0 => git version 1.6.1.80.g7eb5\n./git_t1 => git version 1.6.1.85.gbda6       <= added lstat_cache()\n./git_t2 => git version 1.6.1.2.306.gc0f6f\n./git_t3 => git version 1.6.1.2.307.g55385\n./git_t4 => git version 1.6.1.2.308.g052a\n./git_t5 => git version 1.6.1.2.310.g40dd2   <= added schedule_dir_for_removal()\n./git_t6 => git version 1.6.1.2.313.g9deee\n./git_t7 => git version 1.6.1.2.314.g0ad9    <= added this patch (7/9)\n\n  Except for git_t7 all commits should be in origin/pu, so people should\n  be able to do git show/diff/log on the last hex chars on the version-\n  strings to see the differences.\n\n  CFLAGS=\"-mtune=core2 -O2 -fomit-frame-pointer -fno-stack-protector -g0 -s\"\n\n  Each git_t* binary is run with args \"checkout -q my-v.2.6.2[57]\" for a\n  total of 100 times (50/50 to my-v2.6.25/my-v2.6.27).  Before each run\n  the test script sleeps 20 seconds to allow the disk to finish and\n  being idle.  Time is collected by /usr/bin/time -o output-file prog.\n\n  While the test script was run, the laptop with an Intel Core2 Duo 2\n  GHz processor, was run without X and was otherwise idle.  The test\n  script took 9 hours and 45 minutes to complete.\n\n  __________________\n  Test numbers\n\n$ for ((t=0; $t<=7; t++)); do echo \"git_t$t => $(parse_usr_bin_time_lines.pl git_t$t*)\"; done\ngit_t0 =>  5.646 user  8.322 sys  25.579 real  54.6% CPU  faults:  0 major 39587 minor\ngit_t1 =>  5.631 user  6.826 sys  23.941 real  52.1% CPU  faults:  0 major 39901 minor\ngit_t2 =>  5.622 user  6.854 sys  24.036 real  52.1% CPU  faults:  0 major 39298 minor\ngit_t3 =>  5.636 user  6.867 sys  24.088 real  52.0% CPU  faults:  0 major 39786 minor\ngit_t4 =>  5.640 user  6.818 sys  24.006 real  52.0% CPU  faults:  0 major 39629 minor\ngit_t5 =>  5.642 user  6.552 sys  23.805 real  51.4% CPU  faults:  0 major 39707 minor\ngit_t6 =>  5.629 user  6.518 sys  22.981 real  53.0% CPU  faults:  0 major 40029 minor\ngit_t7 =>  5.629 user  6.051 sys  23.013 real  50.9% CPU  faults:  0 major 39451 minor\n\n|> If you can squeeze out a 10% improvement on Linux with this patch, we\n|> should take it, and if it turns out that it doesn't work on Windows, we\n|> could trivially throw in an #ifdef MINGW (or even #ifdef WIN32 if Cygwin\n|> is affected, too) that skips the fstat() optimization.\n\n  For the system used time, the improvement was (- 6.518 6.051) = 0.467\n  seconds, or (/ (* (- 6.518 6.051) 100.0) 6.518) = 7.2%, so not 10%.\n\n  Funny to see that in:\n     http://article.gmane.org/gmane.comp.version-control.git/107281\n\n  I guessed the improvement to be (quote):\n     \"(* 14403 (- 44 12)) = 460 896 microseconds system time\"\n\n  So I missed only by a little over 6ms. :-)\n\n* Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n| Of course, what we _really_ would do is to provide a flag, say, \n| FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile \n| for the appropriate platforms).\n\n  Or, maybe\n\n     #define FSTAT_RELIABLE 1\n\n  for Linux only?  Then we can change the if-test inside this patch to\n  the following:\n\n-  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n+  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && \n+      FSTAT_RELIABLE) {\n\n  Then we do not have to have #if-defines inside the source code, only\n  in one header file.\n\n  But, question: is this patch worth the extra lines added to the source\n  code?\n\n  -- kjetil\n\n  PS!  Junio, I think this patch series should be inside pu some days\n       more, since things obviously needs more refinement and tests.\n"},{"id":"103458","messageId":"alpine.DEB.1.00.0902061221150.7377@intel-tinevez-2-302","threadId":"17556","inReplyTo":"868wojq0xa.fsf@broadpark.no","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-06T11:26:09Z","receivedAt":"2009-02-06T11:26:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 6 Feb 2009, Kjetil Barvik wrote:\n\n> * Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> | Of course, what we _really_ would do is to provide a flag, say, \n> | FSTAT_UNRELIABLE and test for _that_ (after defining it in the Makefile \n> | for the appropriate platforms).\n> \n>   Or, maybe\n> \n>      #define FSTAT_RELIABLE 1\n> \n>   for Linux only?\n\nNo, I think that would be wrong.  Especially after Junio's remarks that \nfstat() is actually required to behave as you expected it, and only \nWindows (surprise, surprise) has problems following the standard.\n\n> Then we can change the if-test inside this patch to the following:\n> \n> -  if (state->refresh_cache && !to_tempfile && !state->base_dir_len) {\n> +  if (state->refresh_cache && !to_tempfile && !state->base_dir_len && \n> +      FSTAT_RELIABLE) {\n> \n>   Then we do not have to have #if-defines inside the source code, only\n>   in one header file.\n\nIn the spirit of consistency, I would not do that.\n\n>   But, question: is this patch worth the extra lines added to the source\n>   code?\n\nYou seemed to get a nice speedup on Linux.  If Windows cannot follow suit, \ntoo bad.  But I do not want to be punished because other people's OS is \nnot as good as mine, so I _want_ fstat().\n\nAnd those few lines will not hurt, they'll be readable enough.\n\nCiao,\nDscho\n"},{"id":"104653","messageId":"86ab8olxg5.fsf@broadpark.no","threadId":"17556","inReplyTo":"498A9FD3.2020601@viscovery.net","subject":"Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-14T17:50:34Z","receivedAt":"2009-02-14T17:50:34Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Kjetil Barvik schrieb:\n>>   And, yes, since each lstat() call cost approximately 44 microseconds\n>>   compared to 12-16 for each lstat() on my Linux box, there was a little\n>                                ^^^^^^^ fstat()?\n>>   performance gain from this patch.\n>\n> This does look like a good gain. But do you have hard numbers that back\n> the claim?\n\n  OK, I have done some testing/profiling with oprofile(1), and one thing\n  I found out was that my Linux kernel was built with SLUB debug, and of\n  course it cost some system time to run the VM debug code.  After I\n  turned this off, the total system time when down from aprox 6 to 3\n  seconds for the 'git checkout -q my-v2.6.25/7' test.\n\n  Also, from strace output each lstat() call now take around 16\n  microseconds, and each fstat() call around 12 microseconds, so for\n  aprox 14000 changed calls (lstat() => fstat()) the performance gain\n  should now only be (* 14000 (- 16 12)) = 56 ms, compared to 467 ms,\n  which I reported before.\n\n  -- kjetil\n\n  1) http://oprofile.sourceforge.net/about/\n"}]}