{"thread":{"id":"17488","subject":"[PATCH/RFC v2 0/7] git checkout: more cleanups, optimisation, less lstat() calls","startedAt":"2009-02-01T20:23:08Z","lastAt":"2009-02-01T20:23:39Z","messageCount":8,"participants":["Kjetil Barvik"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"102764","messageId":"cover.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":null,"subject":"[PATCH/RFC v2 0/7] git checkout: more cleanups, optimisation, less lstat() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:08Z","receivedAt":"2009-02-01T20:23:08Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Changes since v1\n\n--- patch 1/7 ---\nKept the \"match_len >= cache.len\" trick only inside the\nlongest_match_lstat_cache() function.  Also removed the arguments to\nreset_lstat_cache()\n\n--- patch 2/7 ---\nApplied all comments from Junio\n\n--- patch 3/7 ---\nKept long format strings in printf() calls, even if source lines get\nlonger than 80 chars.  The patch touch less source lines because of\nthis.\n\n--- patch 4/7 ---\nAdded sanity check to the if-test under which fstat() is called.\n\n--- patch 5/7 ---\nUpdated commit log message\n\n--- patch 7/7 ---\nNew patch which reduces the number of rmdir() calls significantly.\nWhen tested with 'git chekcout -q my-v2.6.27' (checkout from Linux\nv2.6.25 to v2.6.27), the numbers look like this before this patch:\n\nrmdir          4532   3.314% OK:   174 NOT:  4358   0.263268 sec   58 usec/call\n    strings    4532 tot    354 uniq  12.802 /uniq   0.263268 sec   58 usec/call\n    errors     4358 tot    347 uniq  12.559 /uniq   0.249116 sec   57 usec/call\n                233   5.141% OK:     1 NOT:   232  \"include/asm-powerpc\"\n                187   4.126% OK:     1 NOT:   186  \"include/asm-sh\"\n                172   3.795% OK:     0 NOT:   172  \"include/asm-arm\"\n                169   3.729% OK:     1 NOT:   168  \"include/asm-sparc64\"\n                161   3.553% OK:     1 NOT:   160  \"include/asm-sparc\"\n                144   3.177% OK:     1 NOT:   143  \"include/asm-ia64\"\n                143   3.155% OK:     1 NOT:   142  \"include/asm-m68knommu\"\n                133   2.935% OK:     1 NOT:   132  \"include/asm-alpha\"\n                126   2.780% OK:     1 NOT:   125  \"include/asm-s390\"\n                122   2.692% OK:     1 NOT:   121  \"include/asm-v850\"\n\nNotice the nice change in the top 10 list after this patch:\n\nrmdir           331   0.255% OK:   174 NOT:   157   0.033435 sec  101 usec/call\n    strings     331 tot    331 uniq   1.000 /uniq   0.033435 sec  101 usec/call\n    errors      157 tot    157 uniq   1.000 /uniq   0.016300 sec  104 usec/call\n                  1   0.302% OK:     0 NOT:     1  \"Documentation\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/filesystems/configfs\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/firmware_class\"\n                  1   0.302% OK:     1 NOT:     0  \"Documentation/hrtimer\"\n                  1   0.302% OK:     1 NOT:     0  \"Documentation/hrtimers\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/i2c/busses\"\n                  1   0.302% OK:     1 NOT:     0  \"Documentation/i386\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/networking\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/power\"\n                  1   0.302% OK:     0 NOT:     1  \"Documentation/powerpc\"\n\n\n\nKjetil Barvik (7):\n  symlinks.c: small cleanup and optimisation\n  remove some memcpy() and strchr() calls inside create_directories()\n  cleanup of write_entry() in entry.c\n  write_entry(): use fstat() instead of lstat() when file is open\n  combine-diff.c: remove a call to fstat() inside show_patch_diff()\n  lstat_cache(): print a warning if doing ping-pong between cache types\n  unpack-trees.c: introduce schedule_dir_for_removal()\n\n combine-diff.c |    4 +--\n entry.c        |  106 +++++++++++++++++++++++++++-----------------------------\n symlinks.c     |   67 +++++++++++++++++++++++++----------\n unpack-trees.c |   97 ++++++++++++++++++++++++++++++++++++++-------------\n 4 files changed, 172 insertions(+), 102 deletions(-)\n"},{"id":"102766","messageId":"8f32b315490bb0c48263a0be34b9ea4f9d6fbbbb.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 1/7] symlinks.c: small cleanup and optimisation","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:09Z","receivedAt":"2009-02-01T20:23:09Z","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":"102767","messageId":"f395c63c938c6be861614df6989a696a71599641.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 2/7] remove some memcpy() and strchr() calls inside create_directories()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:10Z","receivedAt":"2009-02-01T20:23:10Z","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\n* Kjetil Barvik <barvik@broadpark.no> writes:\n| OK, maybe I instead should have tried to merge the function\n| create_directories() with the safe_create_leading_directories() and\n| *_const() functions?  What do pepople think?\n  \n* Junio C Hamano <gitster@pobox.com> writes\n| Strictly speaking, the safe_create_leading_* functions are meant to\n| work on paths inside $GIT_DIR and are not meant for paths inside the\n| work tree, which is this function is about.  Their semantics are\n| different with respect to adjust_shared_perm().\n\n  Ok, I think I keep the create_directories() function as is.  Also\n  because I think a shared \"create_directories()\" function could maybe\n  make the calls to the lstat_cache() function less likely to be\n  sorted alphabetically, and therefore be less cache effective.\n\n| I see existing (mis)uses of the safe_create_leading_* in\n| builtin-apply.c, builtin-clone.c (the one that creates the\n| work_tree, the other one is Ok), merge-recursive.c (both call sites)\n| that are used to touch the work tree, but all places that create\n| regular files in the work tree do not run adjust_shared_perm() but\n| simply honor the user's umask.\n\n  Inside update_file_flags() in merge-recursive.c I see a call to\n  make_room_for_path() (if update_wd is non-zero) which calls\n  safe_create_leading_directories_const().  \n\n  If we get lots of calls to make_room_for_path() in some cases, maybe\n  we will also save some stat() calls if we fix the call to\n  safe_create_*() at this place?\n\n  Remember that safe_create_*() calls stat() on each path component to\n  make sure that the component exists, same as create_directories()\n  used to do before.\n\n\n\n\n entry.c |   23 ++++++++++++++---------\n 1 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 05aa58d..1879b83 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":"102765","messageId":"add4deb5cb16ea9e4b41375b2b1ed201a6bdd000.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 3/7] cleanup of write_entry() in entry.c","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:11Z","receivedAt":"2009-02-01T20:23:11Z","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 1879b83..0f1f3f0 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":"102769","messageId":"4ec475af53f8dbbeb5688183232d390503a2d3f3.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 4/7] write_entry(): use fstat() instead of lstat() when file is open","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:12Z","receivedAt":"2009-02-01T20:23:12Z","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 0f1f3f0..35054a5 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":"102768","messageId":"1f6efca31aee90211b1d6f77919405c990f96d4b.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 5/7] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:13Z","receivedAt":"2009-02-01T20:23:13Z","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":"102770","messageId":"a0e2ed1f3dbdbdb0fe3ceb8bf030bbe7c98235f8.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 6/7] lstat_cache(): print a warning if doing ping-pong between cache types","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:38Z","receivedAt":"2009-02-01T20:23:38Z","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 ae57e56..fe4b188 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(int len, const char *name,\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(int len, const char *name,\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@@ -214,6 +231,8 @@ void clear_lstat_cache(void)\n  */\n int has_symlink_leading_path(int len, const char *name)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 1;\n \treturn lstat_cache(len, name,\n \t\t\t   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &\n \t\tFL_SYMLINK;\n@@ -225,6 +244,8 @@ int has_symlink_leading_path(int len, const char *name)\n  */\n int has_symlink_or_noent_leading_path(int len, const char *name)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 2;\n \treturn lstat_cache(len, name,\n \t\t\t   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &\n \t\t(FL_SYMLINK|FL_NOENT);\n@@ -239,6 +260,8 @@ int has_symlink_or_noent_leading_path(int len, const char *name)\n  */\n int has_dirs_only_path(int len, const char *name, int prefix_len)\n {\n+\tlast_cache_func = current_cache_func;\n+\tcurrent_cache_func = 3;\n \treturn lstat_cache(len, name,\n \t\t\t   FL_DIR|FL_FULLPATH, prefix_len) &\n \t\tFL_DIR;\n-- \n1.6.1.349.g99fa5\n"},{"id":"102771","messageId":"02194022d0e7bfaef7eb9d98be30292954090a2a.1233499703.git.barvik@broadpark.no","threadId":"17488","inReplyTo":"cover.1233499703.git.barvik@broadpark.no","subject":"[PATCH/RFC v2 7/7] unpack-trees.c: introduce schedule_dir_for_removal()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-02-01T20:23:39Z","receivedAt":"2009-02-01T20:23:39Z","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 to avoid some unnecessary calls to rmdir(), we only\nschedule each directory for removal and we wait much longer before we\ndo the actually call 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\n  I noticed that I had to copy-and-paste almost all lines from the\n  longest_match_lstat_cache() in symlinks.c into this new function, so\n  maybe I should do the following:\n\n  Move the 3 new functions below inside the same file (dir.c?) as the\n  lstat_cache() functions, rename longest_match_lstat_cache() to\n  longest_dir_match(), do some cleanup/fix'es, and let it be called\n  from the 3 functions which currently need it.\n\n  Good idea?\n\n\n unpack-trees.c |   97 ++++++++++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 73 insertions(+), 24 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..5c12eca 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -52,36 +52,84 @@ 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+static char path_buf[PATH_MAX];\n+static unsigned int path_len;\n+\n+static void do_remove_scheduled_dirs(unsigned int new_len)\n+{\n+\twhile (path_len > new_len) {\n+\t\tpath_buf[path_len] = '\\0';\n+\t\tif (rmdir(path_buf))\n+\t\t\tbreak;\n+\t\tdo {\n+\t\t\tpath_len--;\n+\t\t} while (path_len > new_len && path_buf[path_len] != '/');\n+\t}\n+\tpath_len = new_len;\n+\treturn;\n+}\n+\n+static void schedule_dir_for_removal(const char *name, unsigned int len)\n+{\n+\tunsigned int i = 0, match_len = 0, max_len, last_slash;\n+\n+\t/* Find longest dir-match between 'name' and 'path' */\n+\tmax_len = len < path_len ? len : path_len;\n+\twhile (i < max_len && name[i] == path_buf[i]) {\n+\t\tif (name[i] == '/')\n+\t\t\tmatch_len = i;\n+\t\ti++;\n+\t}\n+\tif (i >= max_len && ((len > path_len && name[path_len] == '/') ||\n+\t\t\t     (len < path_len && path_buf[len] == '/') ||\n+\t\t\t     (len == path_len)))\n+\t\tmatch_len = i;\n+\n+\t/* Find the last slash inside 'name' */\n+\tlast_slash = match_len;\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 < path_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(&path_buf[match_len], &name[match_len],\n+\t\t\tlast_slash - match_len);\n+\t\tpath_len = last_slash;\n+\t}\n+\treturn;\n+}\n+\n+#define ALL_DIRS 0\n+static void remove_scheduled_dirs(void)\n+{\n+\tdo_remove_scheduled_dirs(ALL_DIRS);\n+\treturn;\n+}\n+\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_namelen(ce), ce->name))\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 +165,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"}]}