{"thread":{"id":"17382","subject":"[PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation","startedAt":"2009-01-26T21:17:11Z","lastAt":"2009-01-29T14:19:05Z","messageCount":16,"participants":["Kjetil Barvik","Mike Ralphson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"102054","messageId":"1233004637-15112-1-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":null,"subject":"[PATCH/RFC v1 0/6] git checkout: more cleanups, optimisation, less lstat() calls","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:11Z","receivedAt":"2009-01-26T21:17:11Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Here is 6 small patches which should further improve the time it take\nto do 'git checkout'. After the 6 patches, the numbers from the 'git\ncheckout -q my-v2.6.27' test, should now look like this:\n\nTOTAL 136752 100.000% OK:124567 NOT: 12185 8.641698 sec 63 usec/call\nlstat64 55502 40.586% OK: 49122 NOT: 6380 2.463762 sec 44 usec/call\n strings 55502 tot 30163 uniq 1.840 /uniq 2.463762 sec 44 usec/call\n files 47122 tot 23813 uniq 1.979 /uniq 2.070486 sec 44 usec/call\n dirs 2000 tot 1436 uniq 1.393 /uniq 0.087281 sec 44 usec/call\n errors 6380 tot 5187 uniq 1.230 /uniq 0.305995 sec 48 usec/call\n 4 0.007% OK: 3 NOT: 1 \"arch/sh/boards\"\n 3 0.005% OK: 3 NOT: 0 \".git/HEAD\"\n 3 0.005% OK: 3 NOT: 0 \".git/refs/heads/my-v2.6.27\"\n 3 0.005% OK: 3 NOT: 0 \".gitignore\"\n 3 0.005% OK: 3 NOT: 0 \".mailmap\"\n 3 0.005% OK: 3 NOT: 0 \"CREDITS\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/00-INDEX\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/ABI/testing/sysfs-block\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/ABI/testing/sysfs-firmware-acpi\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/CodingStyle\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DMA-API.txt\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DMA-mapping.txt\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/Makefile\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/gadget.tmpl\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/kernel-api.tmpl\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/kernel-locking.tmpl\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/procfs-guide.tmpl\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/procfs_example.c\"\n 3 0.005% OK: 3 NOT: 0 \"Documentation/DocBook/rapidio.tmpl\"\n\nfstat64 14403 10.532% OK: 14403 NOT: 0 0.179938 sec 12 usec/call\n\nSo, since last time, and because of patch 4/6, almost 14 400 of the\nlstat() calls has now become fstat() calls, and it seems we have saved\n(* 14403 (- 44 12)) = 460 896 microseconds system time because of\nthis. I am planing to do a more complete and long-running\n/usr/bin/time test, to get real numbers.\n\nWith both patch-series, the count of lstat() calls for this particular\ntest have dropped from 120 954 to 55 502, which is a total reduction\nof 65 452 calls or 54%.\n\nPlease note that patch 6/6 is only to be a debug patch, to catch a\npossible ping-pong situation inside the lstat_cache(), so I think that\nit should at _most_ be merged to the pu branch, such that people who\nwish to test, can 'git cherry-pick' the patch from there.\n\n\nKjetil Barvik (6):\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 use fstat() instead of lstat() when we have an opened file\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\n combine-diff.c | 5 +-\n entry.c | 144 ++++++++++++++++++++++++++++++--------------------------\n symlinks.c | 48 ++++++++++++++-----\n 3 files changed, 115 insertions(+), 82 deletions(-)\n"},{"id":"102053","messageId":"1233004637-15112-2-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":"1233004637-15112-1-git-send-email-barvik@broadpark.no","subject":"[PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:12Z","receivedAt":"2009-01-26T21:17:12Z","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 hopefully/probably reuse\na test-result from it.\n\nWe do similar transformations inside the lstat_cache() and the\ninvalidate_lstat_cache() functions.\n\nThe result is a little smaller text-size as shown below:\n\n ~/git/git $ size symlinks_??_*\n text\t data\t bss\t dec\t hex\tfilename\n 1282 0\t 4116\t 5398\t 1516\tsymlinks_O2_after.o\n 1378 0\t 4116\t 5494\t 1576\tsymlinks_O2_before.o\n 896 0\t 4116\t 5012\t 1394\tsymlinks_Os_after.o\n 902 0\t 4116\t 5018\t 139a\tsymlinks_Os_before.o\n\n'O2' means that the file is compiled with 'gcc -O2', and similar 'Os'\nmeans that the file is compiled with 'gcc -Os' (gcc 4.3.2).\n\nSigned-off-by: Kjetil Barvik \n---\n symlinks.c | 25 +++++++++++++------------\n 1 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/symlinks.c b/symlinks.c\nindex f262b7c..81f490c 100644\n--- a/symlinks.c\n+++ b/symlinks.c\n@@ -25,15 +25,16 @@ 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 && ((i < len && name[i] == '/') ||\n+\t\t\t (i < cache.len && cache.path[i] == '/') ||\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@@ -91,7 +92,7 @@ static int lstat_cache(int len, const char *name,\n \t\tmatch_len = last_slash =\n \t\t\tlongest_match_lstat_cache(len, name, &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\tif (match_flags && match_len >= cache.len)\n \t\t\treturn match_flags;\n \t\t/*\n \t\t * If we now have match_len > 0, we would know that\n@@ -102,7 +103,7 @@ static int lstat_cache(int len, const char *name,\n \t\t * we can return immediately.\n \t\t */\n \t\tmatch_flags = track_flags & FL_DIR;\n-\t\tif (match_flags && len == match_len)\n+\t\tif (match_flags && match_len >= len)\n \t\t\treturn match_flags;\n \t}\n \n@@ -153,7 +154,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@@ -184,7 +185,7 @@ void invalidate_lstat_cache(int len, const char *name)\n \tint match_len, previous_slash;\n \n \tmatch_len = longest_match_lstat_cache(len, name, &previous_slash);\n-\tif (len == match_len) {\n+\tif (match_len >= len) {\n \t\tif ((cache.track_flags & FL_DIR) && previous_slash > 0) {\n \t\t\tcache.path[previous_slash] = '\\0';\n \t\t\tcache.len = previous_slash;\n-- \n1.6.1.349.g99fa5\n"},{"id":"102055","messageId":"1233004637-15112-3-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":"1233004637-15112-1-git-send-email-barvik@broadpark.no","subject":"[PATCH/RFC v1 2/6] remove some memcpy() and strchr() calls inside create_directories()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:13Z","receivedAt":"2009-01-26T21:17:13Z","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 \n---\n\nOK, maybe I instead should have tried to merge the function\ncreate_directories() with the safe_create_leading_directories() and\n*_const() functions? What do pepople think?\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..c2404ea 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\n+create_directories(int path_len, const char *path, 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(len, path, state);\n \treturn write_entry(ce, path, state, 0);\n }\n-- \n1.6.1.349.g99fa5\n"},{"id":"102058","messageId":"1233004637-15112-4-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":"1233004637-15112-1-git-send-email-barvik@broadpark.no","subject":"[PATCH/RFC v1 3/6] cleanup of write_entry() in entry.c","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:14Z","receivedAt":"2009-01-26T21:17:14Z","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\nAlso fold the longest lines such that no line is longer then 80 chars\nor so.\n\nAnd the entry.c file should now be clean for 'gcc -Wextra' warnings.\n\nSigned-off-by: Kjetil Barvik \n---\n\nIf people do not like this approach I can be willing to drop it from\nthis patch-series, but then I get some source code duplication from\nthe next patch (4/6).\n\n\n entry.c | 114 +++++++++++++++++++++++++++++++-------------------------------\n 1 files changed, 57 insertions(+), 57 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex c2404ea..8543755 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@@ -91,88 +91,86 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned\n \treturn NULL;\n }\n \n-static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n+static int\n+write_entry(struct cache_entry *ce, char *path, const struct checkout *state,\n+\t 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+\t\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t\t \"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t path, 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: \"\\\n+\t\t\t\t\t \"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\tfd = create_file(path, ce->ce_mode);\n+\t\t} else {\n+\t\t\tif (ce_mode_s_ifmt == S_IFREG)\n+\t\t\t\tfd = create_file(path, ce->ce_mode);\n+\t\t\telse\n+\t\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-\t\t\t\tpath, strerror(errno));\n+\t\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t\t \"unable to create file %s (%s)\",\n+\t\t\t\t path, strerror(errno));\n \t\t}\n \n \t\twrote = write_in_full(fd, new, size);\n \t\tclose(fd);\n \t\tfree(new);\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\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t\t \"unable to write file %s\", path);\n+\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+\t\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t\t \"cannot create temporary subproject %s\",\n+\t\t\t\t path);\n \t\tif (mkdir(path, 0777) < 0)\n-\t\t\treturn error(\"git checkout-index: cannot create subproject directory %s\", path);\n+\t\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t\t \"cannot create subproject directory %s (%s)\",\n+\t\t\t\t path, strerror(errno));\n \t\tbreak;\n \tdefault:\n-\t\treturn error(\"git checkout-index: unknown file mode for %s\", path);\n+\t\treturn error(\"git checkout-index: \"\\\n+\t\t\t \"unknown file mode for %s\", path);\n \t}\n \n \tif (state->refresh_cache) {\n@@ -202,7 +200,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\treturn 0;\n \t\tif (!state->force) {\n \t\t\tif (!state->quiet)\n-\t\t\t\tfprintf(stderr, \"git-checkout-index: %s already exists\\n\", path);\n+\t\t\t\tfprintf(stderr, \"git-checkout-index: \"\\\n+\t\t\t\t\t\"%s already exists\\n\", path);\n \t\t\treturn -1;\n \t\t}\n \n@@ -220,7 +219,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\t\treturn error(\"%s is a directory\", path);\n \t\t\tremove_subtree(path);\n \t\t} else if (unlink(path))\n-\t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n+\t\t\treturn error(\"unable to unlink old '%s' (%s)\",\n+\t\t\t\t path, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n \tcreate_directories(len, path, state);\n-- \n1.6.1.349.g99fa5\n"},{"id":"102056","messageId":"1233004637-15112-5-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":"1233004637-15112-1-git-send-email-barvik@broadpark.no","subject":"[PATCH/RFC v1 4/6] use fstat() instead of lstat() when we have an opened file","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:15Z","receivedAt":"2009-01-26T21:17:15Z","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\nopened, and it should be at least as fast as the lstat() method.\n\nSigned-off-by: Kjetil Barvik \n---\n entry.c | 11 ++++++++---\n 1 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 8543755..c932ae8 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -96,11 +96,12 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,\n \t 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@@ -151,6 +152,10 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,\n \t\t}\n \n \t\twrote = write_in_full(fd, new, size);\n+\t\tif (state->refresh_cache) {\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@@ -174,8 +179,8 @@ write_entry(struct cache_entry *ce, char *path, const struct checkout *state,\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":"102057","messageId":"1233004637-15112-6-git-send-email-barvik@broadpark.no","threadId":"17382","inReplyTo":"1233004637-15112-1-git-send-email-barvik@broadpark.no","subject":"[PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-26T21:17:16Z","receivedAt":"2009-01-26T21:17:16Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Currently inside show_patch_diff() we have and fstat() call after an\nok lstat() call. Since we before the call to fstat() have already\ntest for the link case with S_ISLNK() the fstat() can be removed.\n\nSigned-off-by: Kjetil Barvik \n---\n combine-diff.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bccc018..ab4df31 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -713,9 +713,8 @@ 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+\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":"102100","messageId":"e2b179460901270135j6e4cb887radbe664ac98f39f1@mail.gmail.com","threadId":"17382","inReplyTo":"1233004637-15112-6-git-send-email-barvik@broadpark.no","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2009-01-27T09:35:43Z","receivedAt":"2009-01-27T09:35:43Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2009/1/26 Kjetil Barvik :\n> Currently inside show_patch_diff() we have and fstat() call after an\n> ok lstat() call. Since we before the call to fstat() have already\n> test for the link case with S_ISLNK() the fstat() can be removed.\n\ns/have and/have an/ ?\n"},{"id":"102108","messageId":"86myddj6qi.fsf@broadpark.no","threadId":"17382","inReplyTo":"e2b179460901270135j6e4cb887radbe664ac98f39f1@mail.gmail.com","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-27T12:03:49Z","receivedAt":"2009-01-27T12:03:49Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Mike Ralphson writes:\n\n> 2009/1/26 Kjetil Barvik :\n>> Currently inside show_patch_diff() we have and fstat() call after an\n>> ok lstat() call. Since we before the call to fstat() have already\n>> test for the link case with S_ISLNK() the fstat() can be removed.\n>\n> s/have and/have an/ ?\n\n Correct! Will fix. Thanks!\n\n -- kjetil\n"},{"id":"102109","messageId":"e2b179460901270406i3425b99pb412536ce28fb44d@mail.gmail.com","threadId":"17382","inReplyTo":"86myddj6qi.fsf@broadpark.no","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2009-01-27T12:06:52Z","receivedAt":"2009-01-27T12:06:52Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2009/1/27 Kjetil Barvik \n> Mike Ralphson writes:\n>\n> > 2009/1/26 Kjetil Barvik :\n> >> Currently inside show_patch_diff() we have and fstat() call after an\n> >> ok lstat() call. Since we before the call to fstat() have already\n> >> test for the link case with S_ISLNK() the fstat() can be removed.\n> >\n> > s/have and/have an/ ?\n>\n> Correct! Will fix. Thanks!\n\nTypo stood out and blinded me to the rest of the grammar... How about:\n\nCurrently 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\n'Half-a-job' Mike\n"},{"id":"102342","messageId":"7vskn3np6u.fsf@gitster.siamese.dyndns.org","threadId":"17382","inReplyTo":"1233004637-15112-2-git-send-email-barvik@broadpark.no","subject":"??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T20:36:09Z","receivedAt":"2009-01-28T20:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik writes:\n\n> Simplify the if-else test in longest_match_lstat_cache() such that we\n> only have one simple if test. Instead of testing for 'i == cache.len'\n> or 'i == len', we transform this to a common test for 'i == max_len'.\n>\n> And to further optimise we use 'i >= max_len' instead of 'i ==\n> max_len', the reason is that it is now the exact opposite of one part\n> inside the while-loop termination expression 'i < max_len && name[i]\n> == cache.path[i]', and then the compiler can hopefully/probably reuse\n> a test-result from it.\n\nThis is *dense*, and made me wonder if is this really worth doing.\n\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 && ((i < len && name[i] == '/') ||\n> +\t\t\t (i < cache.len && cache.path[i] == '/') ||\n> +\t\t\t (len == cache.len))) {\n\nAs you described in your commit log message, what this really wants to\ncheck is (i == max_len). By saying (i >= max_len) you are losing\nreadability, optimizing for compilers (that do not notice this) and\npessimizing for human readers (like me, who had to spend a few dozens of\nseconds to realize what you are doing, to speculate why you might have\nthought this would be a good idea, and writing this paragraph). I do not\nknow if it is a good trade-off.\n\n> @@ -91,7 +92,7 @@ static int lstat_cache(int len, const char *name,\n> \t\tmatch_len = last_slash =\n> \t\t\tlongest_match_lstat_cache(len, name, &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\tif (match_flags && match_len >= cache.len)\n> \t\t\treturn match_flags;\n> \t\t/*\n> \t\t * If we now have match_len > 0, we would know that\n\nLikewise.\n\n> @@ -102,7 +103,7 @@ static int lstat_cache(int len, const char *name,\n> \t\t * we can return immediately.\n> \t\t */\n> \t\tmatch_flags = track_flags & FL_DIR;\n> -\t\tif (match_flags && len == match_len)\n> +\t\tif (match_flags && match_len >= len)\n> \t\t\treturn match_flags;\n> \t}\n> \n\nLikewise.\n\n> @@ -184,7 +185,7 @@ void invalidate_lstat_cache(int len, const char *name)\n> \tint match_len, previous_slash;\n> \n> \tmatch_len = longest_match_lstat_cache(len, name, &previous_slash);\n> -\tif (len == match_len) {\n> +\tif (match_len >= len) {\n> \t\tif ((cache.track_flags & FL_DIR) && previous_slash > 0) {\n> \t\t\tcache.path[previous_slash] = '\\0';\n> \t\t\tcache.len = previous_slash;\n\nLikewise.\n\nPeople who would want to fix potential bugs in this code 3 months down the\nroad may not have a ready access to your commit log message (they may be\nlooking at an extract from a release tarball, scratching their heads\nwondering why you are not checking for equality). Perhaps the inline\nfunction can have comments that says something like \"when comparing the\nreturn value from this function to see if it is equal to cache.len or len,\nchecking if the returned value is >= might help your compiler optimize it\nbetter\" to help the readers, but still, I do not know if it is a good\ntrade-off.\n\n> @@ -153,7 +154,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\nGood.\n"},{"id":"102345","messageId":"7vd4e7np5i.fsf@gitster.siamese.dyndns.org","threadId":"17382","inReplyTo":"1233004637-15112-3-git-send-email-barvik@broadpark.no","subject":"Re: [PATCH/RFC v1 2/6] remove some memcpy() and strchr() calls inside create_directories()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T20:36:57Z","receivedAt":"2009-01-28T20:36:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik writes:\n\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\nStrictly speaking, the safe_create_leading_* functions are meant to work\non paths inside $GIT_DIR and are not meant for paths inside the work tree,\nwhich is this function is about. Their semantics are different with\nrespect to adjust_shared_perm().\n\nWhich means that you would either have two variants (one for work tree\npaths, and another for paths inside $GIT_DIR), or a unified function that\nhas an argument to specify whether to run adjust_shared_perm().\n\nHOWEVER.\n\nThat is only \"strictly speaking\".\n\nA non-bare repository that is shared feels like an oximoron, but perhaps\nthere is a valid \"shared work area\" workflow that may benefit from such a\nsetup.\n\nI see existing (mis)uses of the safe_create_leading_* in builtin-apply.c,\nbuiltin-clone.c (the one that creates the work_tree, the other one is Ok),\nmerge-recursive.c (both call sites) that are used to touch the work tree,\nbut all places that create regular files in the work tree do not run\nadjust_shared_perm() but simply honor the user's umask.\n\nIf we _were_ to support a \"shared work area\" workflow, having a unified\n\"create leading directory\" function that always calls adjust_shared_perm()\nmay make sense (note that adjust_shared_perm() is a no-op in a non-shared\nrepository). We then need to also call adjust_shared_perm() for codepaths\nthat create regular files as well, though (e.g. write_entry() in entry.c,\nbut there are many others).\n\n> diff --git a/entry.c b/entry.c\n> index 05aa58d..c2404ea 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\n> +create_directories(int path_len, const char *path, const struct checkout *state)\n\nPlease do not split the line like this.\n\nThe existing sources are not laid out to allow \"grep ^funcname(\", nor are\nwe going to reformat all the files to support such a use case.\n\nWhen we pass pair to functions, I think we pass them in\nthe order I just wrote in all the other functions.\n\nThe micro-optimization itself makes sense to me, though.\n"},{"id":"102344","messageId":"7v63jznp58.fsf@gitster.siamese.dyndns.org","threadId":"17382","inReplyTo":"1233004637-15112-6-git-send-email-barvik@broadpark.no","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T20:37:07Z","receivedAt":"2009-01-28T20:37:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik writes:\n\n> Currently inside show_patch_diff() we have and fstat() call after an\n> ok lstat() call. Since we before the call to fstat() have already\n> test for the link case with S_ISLNK() the fstat() can be removed.\n\nGood eyes. Thanks.\n"},{"id":"102353","messageId":"7vy6wvkth1.fsf@gitster.siamese.dyndns.org","threadId":"17382","inReplyTo":"1233004637-15112-4-git-send-email-barvik@broadpark.no","subject":"Re: [PATCH/RFC v1 3/6] cleanup of write_entry() in entry.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T21:31:54Z","receivedAt":"2009-01-28T21:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kjetil Barvik writes:\n\n> The switch-cases for S_IFREG and S_IFLNK was so similar that it will\n> be better to do some cleanup and use the common parts of it.\n>\n> Also fold the longest lines such that no line is longer then 80 chars\n> or so.\n>\n> And the entry.c file should now be clean for 'gcc -Wextra' warnings.\n>\n> Signed-off-by: Kjetil Barvik \n> ---\n>\n> If people do not like this approach I can be willing to drop it from\n> this patch-series, but then I get some source code duplication from\n> the next patch (4/6).\n\nMerging of the two similar codepaths looked good from a cursory reading,\nbut if you have doubts about one patch, it usually is easier for other\npeople to work with you if the series is reordered to have it near the\nend, iow, undisputably good ones first.\n\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\t\treturn error(\"git checkout-index: \"\\\n> +\t\t\t\t \"unable to read sha1 file of %s (%s)\",\n> +\t\t\t\t path, sha1_to_hex(ce->sha1));\n\nYou lost greppability when somebody calls for help saying \"I am getting an\nerror message that says 'git checkout-index: unable to read sha1 file'\".\n"},{"id":"102377","messageId":"7vab9akj2p.fsf@gitster.siamese.dyndns.org","threadId":"17382","inReplyTo":"7v63jznp58.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-29T01:16:30Z","receivedAt":"2009-01-29T01:16:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Kjetil Barvik writes:\n>\n>> Currently inside show_patch_diff() we have and fstat() call after an\n>> ok lstat() call. Since we before the call to fstat() have already\n>> test for the link case with S_ISLNK() the fstat() can be removed.\n>\n> Good eyes. Thanks.\n\nHeh, I noticed you will update the commit log message, so I'll dequeue\nthis and wait for an update.\n"},{"id":"102417","messageId":"86y6wuh6am.fsf@broadpark.no","threadId":"17382","inReplyTo":"7vab9akj2p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-29T08:20:49Z","receivedAt":"2009-01-29T08:20:49Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Kjetil Barvik writes:\n>>\n>>> Currently inside show_patch_diff() we have and fstat() call after an\n>>> ok lstat() call. Since we before the call to fstat() have already\n>>> test for the link case with S_ISLNK() the fstat() can be removed.\n>>\n>> Good eyes. Thanks.\n>\n> Heh, I noticed you will update the commit log message, so I'll dequeue\n> this and wait for an update.\n\n Yes, I am planing a v2 in 1 or 3 days (it takes some time to run long-\n running tests). And then hopefully I have addressed all comments so\n far. Thanks for comments!\n\n -- kjetil\n"},{"id":"102455","messageId":"86iqnygppi.fsf@broadpark.no","threadId":"17382","inReplyTo":"7vskn3np6u.fsf@gitster.siamese.dyndns.org","subject":"Re: ??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation","fromName":"Kjetil Barvik","fromEmail":"barvik@broadpark.no","sentAt":"2009-01-29T14:19:05Z","receivedAt":"2009-01-29T14:19:05Z","isPatch":true,"sender":{"key":"barvik@broadpark.no","avatar":null},"body":"* Kjetil Barvik writes:\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 && ((i < len && name[i] == '/') ||\n> +\t\t\t (i < cache.len && cache.path[i] == '/') ||\n> +\t\t\t (len == cache.len))) {\n\n* Junio C Hamano writes:\n> As you described in your commit log message, what this really wants to\n> check is (i == max_len). By saying (i >= max_len) you are losing\n> readability, optimizing for compilers (that do not notice this) \n\n When the compiler see the source line:\n\n 'while (i < max_len && name[i] == cache.path[i])'\n\n it will, from what I know about compilers and machine instructions for\n intel cpu's, generate the following pseudo instructions for this line\n (before more compiler optimisation is done):\n\n 1 test !(i < max_len) /* which is (i >= max_len) */\n 2 jump-if-not-zero first_instruction_address_outside_the_loop\n 3 \"some instructions to retrieve some memory locations and test\n name[i] == cache.path[i]\"\n 4 jump-if-not-zero instruction-address-to-continue-to-loop\n\n So, I thought that if I could use the exact same test as the compiler\n have to generate for line 1, then I would maybe saved a test, and\n maybe also a jump instruction.\n\n Compiling with 'gcc -Os' (optimise for text segment size) I was able to\n save 4 bytes for this file, which I think shows that gcc is able to\n take advantage of this trick.\n\n> and pessimizing for human readers (like me, who had to spend a few\n> dozens of seconds to realize what you are doing, to speculate why you\n> might have thought this would be a good idea, and writing this\n> paragraph). I do not know if it is a good trade-off.\n\n But, it was not easy to come up with a real test which shows a\n noticeable time difference from this trick, so, I guess this is only a\n trick for the kernel people (at most), so I revert it and will use\n '==' for version 2, such that we keep a little better readability.\n\n -- kjetil\n\n ps! Compiling with '-march=core2 -O2 -g0 -s -fomit-frame-pointer', the\n difference in text segment was 64 bytes when using this trick.\n"}]}