{"thread":{"id":"23295","subject":"[PATCH 0/7] textconv caching","startedAt":"2010-04-02T00:01:59Z","lastAt":"2010-04-02T07:38:32Z","messageCount":13,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"138372","messageId":"20100402000159.GA15101@coredump.intra.peff.net","threadId":"23295","inReplyTo":null,"subject":"[PATCH 0/7] textconv caching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:01:59Z","receivedAt":"2010-04-02T00:01:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is replacement for my fast-textconv series from a week ago. It's\nmuch faster, and doesn't require the user to do any additional work when\nwriting or configuring a textconv helper. The cache is stored as a notes\ntree.\n\nOn my sample commit, 45 jpegs and avi files with their metadata changed,\nand a textconv helper that extracts the metadata, the speedup is:\n\n  [before]\n  $ time git show >/dev/null\n  real    0m13.724s\n  user    0m12.057s\n  sys     0m1.624s\n\n  [after (with cache primed)]\n  $ time git show >/dev/null\n  real    0m0.009s\n  user    0m0.004s\n  sys     0m0.004s\n\nIf you just blinked at those numbers, yes, it really is that much\nfaster.  The caching dropped it to about .35 seconds, which I showed in\nan earlier \"how about this\" patch. The rest of it comes from patch 7/7,\nwhere we can avoid even opening the binary blobs at all (in my sample,\nthey total about 180M).\n\nThe patches are:\n\n  [1/7]: fix const-correctness of write_sha1_file\n  [2/7]: fix textconv leak in emit_rewrite_diff\n  [3/7]: make commit_tree a library function\n  [4/7]: introduce notes-cache interface\n  [5/7]: textconv: refactor calls to run_textconv\n  [6/7]: diff: cache textconv output\n  [7/7]: diff: avoid useless filespec population\n\n-Peff\n"},{"id":"138373","messageId":"20100402000318.GA16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 1/7] fix const-correctness of write_sha1_file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:03:18Z","receivedAt":"2010-04-02T00:03:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These should take const buffers as input data, but zlib's\nnext_in pointer is not const-correct. Let's fix it at the\nzlib level, though, so the cast happens in one obvious\nplace. This should be safe, as a similar cast is used in\nzlib's example code for a const array.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis helps me avoid casting at a higher level in the code, but I also\nthink it's the right thing to do.\n\n cache.h     |    2 +-\n sha1_file.c |    6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6dcb100..5eb0573 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -701,7 +701,7 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *\n \treturn read_sha1_file_repl(sha1, type, size, NULL);\n }\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n-extern int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n+extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex a08a9d0..ff65328 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2271,7 +2271,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n }\n \n static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n-\t\t\t      void *buf, unsigned long len, time_t mtime)\n+\t\t\t      const void *buf, unsigned long len, time_t mtime)\n {\n \tint fd, ret;\n \tunsigned char compressed[4096];\n@@ -2307,7 +2307,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tgit_SHA1_Update(&c, hdr, hdrlen);\n \n \t/* Then the data itself.. */\n-\tstream.next_in = buf;\n+\tstream.next_in = (void *)buf;\n \tstream.avail_in = len;\n \tdo {\n \t\tunsigned char *in0 = stream.next_in;\n@@ -2342,7 +2342,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmpfile, filename);\n }\n \n-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n \tunsigned char sha1[20];\n \tchar hdr[32];\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138374","messageId":"20100402000414.GB16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 2/7] fix textconv leak in emit_rewrite_diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:04:14Z","receivedAt":"2010-04-02T00:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We correctly free() for the normal diff case, but leak for\nrewrite diffs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suppose we never noticed this because rewrite diffs and textconv are\nboth uncommonly used codepaths, and leaking a little bit of textconv\nprobably just isn't that big a deal in a short-running process. But\nstopping leaks is good.\n\n diff.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2daa732..db2cd5d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -550,6 +550,10 @@ static void emit_rewrite_diff(const char *name_a,\n \t\temit_rewrite_lines(&ecbdata, '-', data_one, size_one);\n \tif (lc_b)\n \t\temit_rewrite_lines(&ecbdata, '+', data_two, size_two);\n+\tif (textconv_one)\n+\t\tfree(data_one);\n+\tif (textconv_two)\n+\t\tfree(data_two);\n }\n \n struct diff_words_buffer {\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138375","messageId":"20100402000523.GC16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 3/7] make commit_tree a library function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:05:23Z","receivedAt":"2010-04-02T00:05:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Until now, this has been part of the commit-tree builtin.\nHowever, it is already used by other builtins (like commit,\nmerge, and notes), and it would be useful to access it from\nlibrary code.\n\nThe check_valid helper has to come along, too, but is given\na more library-ish name of \"assert_sha1_type\".\n\nOtherwise, the code is unchanged. There are still a few\nrough edges for a library function, like printing the utf8\nwarning to stderr, but we can address those if and when they\ncome up as inappropriate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not inclined to try lib-ifying it any more, as it would just feel\nlike code churn. It's not as if we aren't calling it from all over\nalready, so this is really just about moving it out of builtin/.\n\n builtin.h             |    3 --\n builtin/commit-tree.c |   70 +------------------------------------------------\n cache.h               |    2 +\n commit.c              |   55 ++++++++++++++++++++++++++++++++++++++\n commit.h              |    4 +++\n sha1_file.c           |   10 +++++++\n 6 files changed, 72 insertions(+), 72 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 464588b..5c887ef 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -16,9 +16,6 @@ extern const char *help_unknown_cmd(const char *cmd);\n extern void prune_packed_objects(int);\n extern int fmt_merge_msg(int merge_summary, struct strbuf *in,\n \tstruct strbuf *out);\n-extern int commit_tree(const char *msg, unsigned char *tree,\n-\t\tstruct commit_list *parents, unsigned char *ret,\n-\t\tconst char *author);\n extern int commit_notes(struct notes_tree *t, const char *msg);\n \n struct notes_rewrite_cfg {\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 90dac34..87f0591 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -9,19 +9,6 @@\n #include \"builtin.h\"\n #include \"utf8.h\"\n \n-/*\n- * FIXME! Share the code with \"write-tree.c\"\n- */\n-static void check_valid(unsigned char *sha1, enum object_type expect)\n-{\n-\tenum object_type type = sha1_object_info(sha1, NULL);\n-\tif (type < 0)\n-\t\tdie(\"%s is not a valid object\", sha1_to_hex(sha1));\n-\tif (type != expect)\n-\t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1),\n-\t\t    typename(expect));\n-}\n-\n static const char commit_tree_usage[] = \"git commit-tree <sha1> [-p <sha1>]* < changelog\";\n \n static void new_parent(struct commit *parent, struct commit_list **parents_p)\n@@ -38,61 +25,6 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)\n \tcommit_list_insert(parent, parents_p);\n }\n \n-static const char commit_utf8_warn[] =\n-\"Warning: commit message does not conform to UTF-8.\\n\"\n-\"You may want to amend it after fixing the message, or set the config\\n\"\n-\"variable i18n.commitencoding to the encoding your project uses.\\n\";\n-\n-int commit_tree(const char *msg, unsigned char *tree,\n-\t\tstruct commit_list *parents, unsigned char *ret,\n-\t\tconst char *author)\n-{\n-\tint result;\n-\tint encoding_is_utf8;\n-\tstruct strbuf buffer;\n-\n-\tcheck_valid(tree, OBJ_TREE);\n-\n-\t/* Not having i18n.commitencoding is the same as having utf-8 */\n-\tencoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n-\n-\tstrbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */\n-\tstrbuf_addf(&buffer, \"tree %s\\n\", sha1_to_hex(tree));\n-\n-\t/*\n-\t * NOTE! This ordering means that the same exact tree merged with a\n-\t * different order of parents will be a _different_ changeset even\n-\t * if everything else stays the same.\n-\t */\n-\twhile (parents) {\n-\t\tstruct commit_list *next = parents->next;\n-\t\tstrbuf_addf(&buffer, \"parent %s\\n\",\n-\t\t\tsha1_to_hex(parents->item->object.sha1));\n-\t\tfree(parents);\n-\t\tparents = next;\n-\t}\n-\n-\t/* Person/date information */\n-\tif (!author)\n-\t\tauthor = git_author_info(IDENT_ERROR_ON_NO_NAME);\n-\tstrbuf_addf(&buffer, \"author %s\\n\", author);\n-\tstrbuf_addf(&buffer, \"committer %s\\n\", git_committer_info(IDENT_ERROR_ON_NO_NAME));\n-\tif (!encoding_is_utf8)\n-\t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n-\tstrbuf_addch(&buffer, '\\n');\n-\n-\t/* And add the comment */\n-\tstrbuf_addstr(&buffer, msg);\n-\n-\t/* And check the encoding */\n-\tif (encoding_is_utf8 && !is_utf8(buffer.buf))\n-\t\tfprintf(stderr, commit_utf8_warn);\n-\n-\tresult = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);\n-\tstrbuf_release(&buffer);\n-\treturn result;\n-}\n-\n int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -117,7 +49,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \n \t\tif (get_sha1(b, sha1))\n \t\t\tdie(\"Not a valid object name %s\", b);\n-\t\tcheck_valid(sha1, OBJ_COMMIT);\n+\t\tassert_sha1_type(sha1, OBJ_COMMIT);\n \t\tnew_parent(lookup_commit(sha1), &parents);\n \t}\n \ndiff --git a/cache.h b/cache.h\nindex 5eb0573..bfc4d82 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -718,6 +718,8 @@ extern int has_loose_object_nonlocal(const unsigned char *sha1);\n \n extern int has_pack_index(const unsigned char *sha1);\n \n+extern void assert_sha1_type(const unsigned char *sha1, enum object_type expect);\n+\n extern const signed char hexval_table[256];\n static inline unsigned int hexval(unsigned char c)\n {\ndiff --git a/commit.c b/commit.c\nindex 731191e..e9b0750 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -790,3 +790,58 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \tfree(other);\n \treturn result;\n }\n+\n+static const char commit_utf8_warn[] =\n+\"Warning: commit message does not conform to UTF-8.\\n\"\n+\"You may want to amend it after fixing the message, or set the config\\n\"\n+\"variable i18n.commitencoding to the encoding your project uses.\\n\";\n+\n+int commit_tree(const char *msg, unsigned char *tree,\n+\t\tstruct commit_list *parents, unsigned char *ret,\n+\t\tconst char *author)\n+{\n+\tint result;\n+\tint encoding_is_utf8;\n+\tstruct strbuf buffer;\n+\n+\tassert_sha1_type(tree, OBJ_TREE);\n+\n+\t/* Not having i18n.commitencoding is the same as having utf-8 */\n+\tencoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n+\n+\tstrbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */\n+\tstrbuf_addf(&buffer, \"tree %s\\n\", sha1_to_hex(tree));\n+\n+\t/*\n+\t * NOTE! This ordering means that the same exact tree merged with a\n+\t * different order of parents will be a _different_ changeset even\n+\t * if everything else stays the same.\n+\t */\n+\twhile (parents) {\n+\t\tstruct commit_list *next = parents->next;\n+\t\tstrbuf_addf(&buffer, \"parent %s\\n\",\n+\t\t\tsha1_to_hex(parents->item->object.sha1));\n+\t\tfree(parents);\n+\t\tparents = next;\n+\t}\n+\n+\t/* Person/date information */\n+\tif (!author)\n+\t\tauthor = git_author_info(IDENT_ERROR_ON_NO_NAME);\n+\tstrbuf_addf(&buffer, \"author %s\\n\", author);\n+\tstrbuf_addf(&buffer, \"committer %s\\n\", git_committer_info(IDENT_ERROR_ON_NO_NAME));\n+\tif (!encoding_is_utf8)\n+\t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n+\tstrbuf_addch(&buffer, '\\n');\n+\n+\t/* And add the comment */\n+\tstrbuf_addstr(&buffer, msg);\n+\n+\t/* And check the encoding */\n+\tif (encoding_is_utf8 && !is_utf8(buffer.buf))\n+\t\tfprintf(stderr, commit_utf8_warn);\n+\n+\tresult = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);\n+\tstrbuf_release(&buffer);\n+\treturn result;\n+}\ndiff --git a/commit.h b/commit.h\nindex 3cf5166..2b7fd89 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -158,4 +158,8 @@ static inline int single_parent(struct commit *commit)\n \n struct commit_list *reduce_heads(struct commit_list *heads);\n \n+extern int commit_tree(const char *msg, unsigned char *tree,\n+\t\tstruct commit_list *parents, unsigned char *ret,\n+\t\tconst char *author);\n+\n #endif /* COMMIT_H */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ff65328..28c056e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2516,3 +2516,13 @@ int read_pack_header(int fd, struct pack_header *header)\n \t\treturn PH_ERROR_PROTOCOL;\n \treturn 0;\n }\n+\n+void assert_sha1_type(const unsigned char *sha1, enum object_type expect)\n+{\n+\tenum object_type type = sha1_object_info(sha1, NULL);\n+\tif (type < 0)\n+\t\tdie(\"%s is not a valid object\", sha1_to_hex(sha1));\n+\tif (type != expect)\n+\t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1),\n+\t\t    typename(expect));\n+}\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138376","messageId":"20100402000739.GD16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 4/7] introduce notes-cache interface","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:07:40Z","receivedAt":"2010-04-02T00:07:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Notes provide a fast lookup mechanism for data keyed by\nsha1. This is ideal for caching certain operations, like\ntextconv filters.\n\nThis patch builds some infrastructure to make it simpler to\nuse notes trees as caches. In particular, caches:\n\n  1. don't have arbitrary commit messages. They store a\n     cache validity string in the commit, and clear the tree\n     when the cache validity string changes.\n\n  2. don't keep any commit history. The accumulated history\n     of a a cache is just useless cruft.\n\n  3. use a looser form of locking for ref updates. If two\n     processes try to write to the cache simultaneously, it\n     is OK if one overwrites the other, losing some changes.\n     It's just a cache, so we will just end up with an extra\n     miss.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI tried lib-ifying commit_notes from builtin/notes.c, but for the\nreasons mentioned above, plus the totally different error handling (we\nneed silent error returns because not writing to the cache should not be\nfatal), the code ended up having more \"if (quiet)\" and \"if (nohistory)\"\nthan there was actual code. The notes_cache_write below ended up pretty\nterse and readable, I think.\n\n Makefile      |    2 +\n notes-cache.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n notes-cache.h |   20 ++++++++++++\n 3 files changed, 116 insertions(+), 0 deletions(-)\n create mode 100644 notes-cache.c\n create mode 100644 notes-cache.h\n\ndiff --git a/Makefile b/Makefile\nindex 8a0f5c4..24e92ab 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -486,6 +486,7 @@ LIB_H += log-tree.h\n LIB_H += mailmap.h\n LIB_H += merge-recursive.h\n LIB_H += notes.h\n+LIB_H += notes-cache.h\n LIB_H += object.h\n LIB_H += pack.h\n LIB_H += pack-refs.h\n@@ -575,6 +576,7 @@ LIB_OBJS += merge-file.o\n LIB_OBJS += merge-recursive.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += notes.o\n+LIB_OBJS += notes-cache.o\n LIB_OBJS += object.o\n LIB_OBJS += pack-check.o\n LIB_OBJS += pack-refs.o\ndiff --git a/notes-cache.c b/notes-cache.c\nnew file mode 100644\nindex 0000000..dee6d62\n--- /dev/null\n+++ b/notes-cache.c\n@@ -0,0 +1,94 @@\n+#include \"cache.h\"\n+#include \"notes-cache.h\"\n+#include \"commit.h\"\n+#include \"refs.h\"\n+\n+static int notes_cache_match_validity(const char *ref, const char *validity)\n+{\n+\tunsigned char sha1[20];\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pretty_ctx;\n+\tstruct strbuf msg = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (read_ref(ref, sha1) < 0)\n+\t\treturn 0;\n+\n+\tcommit = lookup_commit_reference_gently(sha1, 1);\n+\tif (!commit)\n+\t\treturn 0;\n+\n+\tmemset(&pretty_ctx, 0, sizeof(pretty_ctx));\n+\tformat_commit_message(commit, \"%s\", &msg, &pretty_ctx);\n+\tstrbuf_trim(&msg);\n+\n+\tret = !strcmp(msg.buf, validity);\n+\tstrbuf_release(&msg);\n+\n+\treturn ret;\n+}\n+\n+void notes_cache_init(struct notes_cache *c, const char *name,\n+\t\t     const char *validity)\n+{\n+\tstruct strbuf ref = STRBUF_INIT;\n+\tint flags = 0;\n+\n+\tmemset(c, 0, sizeof(*c));\n+\tc->validity = xstrdup(validity);\n+\n+\tstrbuf_addf(&ref, \"refs/notes/%s\", name);\n+\tif (!notes_cache_match_validity(ref.buf, validity))\n+\t\tflags = NOTES_INIT_EMPTY;\n+\tinit_notes(&c->tree, ref.buf, combine_notes_overwrite, flags);\n+\tstrbuf_release(&ref);\n+}\n+\n+int notes_cache_write(struct notes_cache *c)\n+{\n+\tunsigned char tree_sha1[20];\n+\tunsigned char commit_sha1[20];\n+\n+\tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n+\t\treturn -1;\n+\tif (!c->tree.dirty)\n+\t\treturn 0;\n+\n+\tif (write_notes_tree(&c->tree, tree_sha1))\n+\t\treturn -1;\n+\tif (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)\n+\t\treturn -1;\n+\tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n+\t\t       0, QUIET_ON_ERR) < 0)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n+char *notes_cache_get(struct notes_cache *c, unsigned char key_sha1[20],\n+\t\t      size_t *outsize)\n+{\n+\tconst unsigned char *value_sha1;\n+\tenum object_type type;\n+\tchar *value;\n+\tunsigned long size;\n+\n+\tvalue_sha1 = get_note(&c->tree, key_sha1);\n+\tif (!value_sha1)\n+\t\treturn NULL;\n+\tvalue = read_sha1_file(value_sha1, &type, &size);\n+\n+\t*outsize = size;\n+\treturn value;\n+}\n+\n+int notes_cache_put(struct notes_cache *c, unsigned char key_sha1[20],\n+\t\t    const char *data, size_t size)\n+{\n+\tunsigned char value_sha1[20];\n+\n+\tif (write_sha1_file(data, size, \"blob\", value_sha1) < 0)\n+\t\treturn -1;\n+\tadd_note(&c->tree, key_sha1, value_sha1, NULL);\n+\treturn 0;\n+}\ndiff --git a/notes-cache.h b/notes-cache.h\nnew file mode 100644\nindex 0000000..356f88f\n--- /dev/null\n+++ b/notes-cache.h\n@@ -0,0 +1,20 @@\n+#ifndef NOTES_CACHE_H\n+#define NOTES_CACHE_H\n+\n+#include \"notes.h\"\n+\n+struct notes_cache {\n+\tstruct notes_tree tree;\n+\tchar *validity;\n+};\n+\n+void notes_cache_init(struct notes_cache *c, const char *name,\n+\t\t     const char *validity);\n+int notes_cache_write(struct notes_cache *c);\n+\n+char *notes_cache_get(struct notes_cache *c, unsigned char sha1[20], size_t\n+\t\t      *outsize);\n+int notes_cache_put(struct notes_cache *c, unsigned char sha1[20],\n+\t\t    const char *data, size_t size);\n+\n+#endif /* NOTES_CACHE_H */\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138377","messageId":"20100402000926.GE16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 5/7] textconv: refactor calls to run_textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:09:26Z","receivedAt":"2010-04-02T00:09:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch adds a fill_textconv wrapper, which centralizes\nsome minor logic like error checking and handling the case\nof no-textconv.\n\nIn addition to dropping the number of lines, this will make\nit easier in future patches to handle multiple types of\ntextconv.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSimilar to 1/3 from the previous series, but fixes some deficiencies\nthere. Specifically, it handles !DIFF_FILE_VALID and filespec population\nbetter, which makes fill_textconv a full replacement for fill_mmfile,\njust with optional textconv-ing.\n\n diff.c |   66 +++++++++++++++++++++++++++++----------------------------------\n 1 files changed, 30 insertions(+), 36 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex db2cd5d..9665d6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -43,7 +43,8 @@ static char diff_colors[][COLOR_MAXLEN] = {\n };\n \n static void diff_filespec_load_driver(struct diff_filespec *one);\n-static char *run_textconv(const char *, struct diff_filespec *, size_t *);\n+static size_t fill_textconv(const char *cmd,\n+\t\t\t    struct diff_filespec *df, char **outbuf);\n \n static int parse_diff_color_slot(const char *var, int ofs)\n {\n@@ -477,7 +478,7 @@ static void emit_rewrite_diff(const char *name_a,\n \tconst char *reset = diff_get_color(color_diff, DIFF_RESET);\n \tstatic struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;\n \tconst char *a_prefix, *b_prefix;\n-\tconst char *data_one, *data_two;\n+\tchar *data_one, *data_two;\n \tsize_t size_one, size_two;\n \tstruct emit_callback ecbdata;\n \n@@ -499,26 +500,8 @@ static void emit_rewrite_diff(const char *name_a,\n \tquote_two_c_style(&a_name, a_prefix, name_a, 0);\n \tquote_two_c_style(&b_name, b_prefix, name_b, 0);\n \n-\tdiff_populate_filespec(one, 0);\n-\tdiff_populate_filespec(two, 0);\n-\tif (textconv_one) {\n-\t\tdata_one = run_textconv(textconv_one, one, &size_one);\n-\t\tif (!data_one)\n-\t\t\tdie(\"unable to read files to diff\");\n-\t}\n-\telse {\n-\t\tdata_one = one->data;\n-\t\tsize_one = one->size;\n-\t}\n-\tif (textconv_two) {\n-\t\tdata_two = run_textconv(textconv_two, two, &size_two);\n-\t\tif (!data_two)\n-\t\t\tdie(\"unable to read files to diff\");\n-\t}\n-\telse {\n-\t\tdata_two = two->data;\n-\t\tsize_two = two->size;\n-\t}\n+\tsize_one = fill_textconv(textconv_one, one, &data_one);\n+\tsize_two = fill_textconv(textconv_two, two, &data_two);\n \n \tmemset(&ecbdata, 0, sizeof(ecbdata));\n \tecbdata.color_diff = color_diff;\n@@ -1717,20 +1700,8 @@ static void builtin_diff(const char *name_a,\n \t\t\tstrbuf_reset(&header);\n \t\t}\n \n-\t\tif (textconv_one) {\n-\t\t\tsize_t size;\n-\t\t\tmf1.ptr = run_textconv(textconv_one, one, &size);\n-\t\t\tif (!mf1.ptr)\n-\t\t\t\tdie(\"unable to read files to diff\");\n-\t\t\tmf1.size = size;\n-\t\t}\n-\t\tif (textconv_two) {\n-\t\t\tsize_t size;\n-\t\t\tmf2.ptr = run_textconv(textconv_two, two, &size);\n-\t\t\tif (!mf2.ptr)\n-\t\t\t\tdie(\"unable to read files to diff\");\n-\t\t\tmf2.size = size;\n-\t\t}\n+\t\tmf1.size = fill_textconv(textconv_one, one, &mf1.ptr);\n+\t\tmf2.size = fill_textconv(textconv_two, two, &mf2.ptr);\n \n \t\tpe = diff_funcname_pattern(one);\n \t\tif (!pe)\n@@ -3916,3 +3887,26 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \n \treturn strbuf_detach(&buf, outsize);\n }\n+\n+static size_t fill_textconv(const char *cmd,\n+\t\t\t    struct diff_filespec *df,\n+\t\t\t    char **outbuf)\n+{\n+\tsize_t size;\n+\n+\tif (!cmd) {\n+\t\tif (!DIFF_FILE_VALID(df)) {\n+\t\t\t*outbuf = \"\";\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (diff_populate_filespec(df, 0))\n+\t\t\tdie(\"unable to read files to diff\");\n+\t\t*outbuf = df->data;\n+\t\treturn df->size;\n+\t}\n+\n+\t*outbuf = run_textconv(cmd, df, &size);\n+\tif (!*outbuf)\n+\t\tdie(\"unable to read files to diff\");\n+\treturn size;\n+}\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138378","messageId":"20100402001215.GF16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 6/7] diff: cache textconv output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:12:15Z","receivedAt":"2010-04-02T00:12:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running a textconv filter can take a long time. It's\nparticularly bad for a large file which needs to be spooled\nto disk, but even for small files, the fork+exec overhead\ncan add up for something like \"git log -p\".\n\nThis patch uses the notes-cache mechanism to keep a fast\ncache of textconv output. Caches are stored in\nrefs/notes/textconv/$x, where $x is the userdiff driver\ndefined in gitattributes.\n\nCaching is enabled only if diff.$x.cachetextconv is true.\n\nIn my test repo, on a commit with 45 jpg and avi files\nchanged and a textconv to show their exif tags:\n\n  [before]\n  $ time git show >/dev/null\n  real    0m13.724s\n  user    0m12.057s\n  sys     0m1.624s\n\n  [after, first run]\n  $ git config diff.mfo.cachetextconv true\n  $ time git show >/dev/null\n  real    0m14.252s\n  user    0m12.197s\n  sys     0m1.800s\n\n  [after, subsequent runs]\n  $ time git show >/dev/null\n  real    0m0.352s\n  user    0m0.148s\n  sys     0m0.200s\n\nSo for a slight (3.8%) cost on the first run, we achieve an\nalmost 40x speed up on subsequent runs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe moral equivalent of what I posted earlier, but most of the heavy\nlifting is in notes-cache.[ch] from earlier in the series.\n\nIt's off by default. I think it should probably swithc to on by default,\nbut I figured we could let the feature mature a bit first.\n\n Documentation/gitattributes.txt  |   20 +++++++\n diff.c                           |   52 +++++++++++++++---\n t/t4042-diff-textconv-caching.sh |  109 ++++++++++++++++++++++++++++++++++++++\n userdiff.c                       |    9 +++\n userdiff.h                       |    4 ++\n 5 files changed, 185 insertions(+), 9 deletions(-)\n create mode 100755 t/t4042-diff-textconv-caching.sh\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex d892e64..a8500d1 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -414,6 +414,26 @@ because it quickly conveys the changes you have made), you\n should generate it separately and send it as a comment _in\n addition to_ the usual binary diff that you might send.\n \n+Because text conversion can be slow, especially when doing a\n+large number of them with `git log -p`, git provides a mechanism\n+to cache the output and use it in future diffs.  To enable\n+caching, set the \"cachetextconv\" variable in your diff driver's\n+config. For example:\n+\n+------------------------\n+[diff \"jpg\"]\n+\ttextconv = exif\n+\tcachetextconv = true\n+------------------------\n+\n+This will cache the result of running \"exif\" on each blob\n+indefinitely. If you change the textconv config variable for a\n+diff driver, git will automatically invalidate the cache entries\n+and re-run the textconv filter. If you want to invalidate the\n+cache manually (e.g., because your version of \"exif\" was updated\n+and now produces better output), you can remove the cache\n+manually with `git update-ref -d refs/notes/textconv/jpg` (where\n+\"jpg\" is the name of the diff driver, as in the example above).\n \n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\ndiff --git a/diff.c b/diff.c\nindex 9665d6d..72d8503 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -43,7 +43,7 @@ static char diff_colors[][COLOR_MAXLEN] = {\n };\n \n static void diff_filespec_load_driver(struct diff_filespec *one);\n-static size_t fill_textconv(const char *cmd,\n+static size_t fill_textconv(struct userdiff_driver *driver,\n \t\t\t    struct diff_filespec *df, char **outbuf);\n \n static int parse_diff_color_slot(const char *var, int ofs)\n@@ -466,8 +466,8 @@ static void emit_rewrite_diff(const char *name_a,\n \t\t\t      const char *name_b,\n \t\t\t      struct diff_filespec *one,\n \t\t\t      struct diff_filespec *two,\n-\t\t\t      const char *textconv_one,\n-\t\t\t      const char *textconv_two,\n+\t\t\t      struct userdiff_driver *textconv_one,\n+\t\t\t      struct userdiff_driver *textconv_two,\n \t\t\t      struct diff_options *o)\n {\n \tint lc_a, lc_b;\n@@ -1569,14 +1569,26 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const\n \t\toptions->b_prefix = b;\n }\n \n-static const char *get_textconv(struct diff_filespec *one)\n+static struct userdiff_driver *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n \tif (!S_ISREG(one->mode))\n \t\treturn NULL;\n \tdiff_filespec_load_driver(one);\n-\treturn one->driver->textconv;\n+\tif (!one->driver->textconv)\n+\t\treturn NULL;\n+\n+\tif (one->driver->textconv_want_cache && !one->driver->textconv_cache) {\n+\t\tstruct notes_cache *c = xmalloc(sizeof(*c));\n+\t\tstruct strbuf name = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&name, \"textconv/%s\", one->driver->name);\n+\t\tnotes_cache_init(c, name.buf, one->driver->textconv);\n+\t\tone->driver->textconv_cache = c;\n+\t}\n+\n+\treturn one->driver;\n }\n \n static void builtin_diff(const char *name_a,\n@@ -1593,7 +1605,8 @@ static void builtin_diff(const char *name_a,\n \tconst char *set = diff_get_color_opt(o, DIFF_METAINFO);\n \tconst char *reset = diff_get_color_opt(o, DIFF_RESET);\n \tconst char *a_prefix, *b_prefix;\n-\tconst char *textconv_one = NULL, *textconv_two = NULL;\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n \tstruct strbuf header = STRBUF_INIT;\n \n \tif (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n@@ -3888,13 +3901,13 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \treturn strbuf_detach(&buf, outsize);\n }\n \n-static size_t fill_textconv(const char *cmd,\n+static size_t fill_textconv(struct userdiff_driver *driver,\n \t\t\t    struct diff_filespec *df,\n \t\t\t    char **outbuf)\n {\n \tsize_t size;\n \n-\tif (!cmd) {\n+\tif (!driver || !driver->textconv) {\n \t\tif (!DIFF_FILE_VALID(df)) {\n \t\t\t*outbuf = \"\";\n \t\t\treturn 0;\n@@ -3905,8 +3918,29 @@ static size_t fill_textconv(const char *cmd,\n \t\treturn df->size;\n \t}\n \n-\t*outbuf = run_textconv(cmd, df, &size);\n+\tif (driver->textconv_cache) {\n+\t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n+\t\t\t\t\t  &size);\n+\t\tif (*outbuf)\n+\t\t\treturn size;\n+\t}\n+\n+\t*outbuf = run_textconv(driver->textconv, df, &size);\n \tif (!*outbuf)\n \t\tdie(\"unable to read files to diff\");\n+\n+\tif (driver->textconv_cache) {\n+\t\t/* ignore errors, as we might be in a readonly repository */\n+\t\tnotes_cache_put(driver->textconv_cache, df->sha1, *outbuf,\n+\t\t\t\tsize);\n+\t\t/*\n+\t\t * we could save up changes and flush them all at the end,\n+\t\t * but we would need an extra call after all diffing is done.\n+\t\t * Since generating a cache entry is the slow path anyway,\n+\t\t * this extra overhead probably isn't a big deal.\n+\t\t */\n+\t\tnotes_cache_write(driver->textconv_cache);\n+\t}\n+\n \treturn size;\n }\ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nnew file mode 100755\nindex 0000000..91f8198\n--- /dev/null\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -0,0 +1,109 @@\n+#!/bin/sh\n+\n+test_description='test textconv caching'\n+. ./test-lib.sh\n+\n+cat >helper <<'EOF'\n+#!/bin/sh\n+sed 's/^/converted: /' \"$@\" >helper.out\n+cat helper.out\n+EOF\n+chmod +x helper\n+\n+test_expect_success 'setup' '\n+\techo foo content 1 >foo.bin &&\n+\techo bar content 1 >bar.bin &&\n+\tgit add . &&\n+\tgit commit -m one &&\n+\techo foo content 2 >foo.bin &&\n+\techo bar content 2 >bar.bin &&\n+\tgit commit -a -m two &&\n+\techo \"*.bin diff=magic\" >.gitattributes &&\n+\tgit config diff.magic.textconv ./helper &&\n+\tgit config diff.magic.cachetextconv true\n+'\n+\n+cat >expect <<EOF\n+diff --git a/bar.bin b/bar.bin\n+index fcf9166..28283d5 100644\n+--- a/bar.bin\n++++ b/bar.bin\n+@@ -1 +1 @@\n+-converted: bar content 1\n++converted: bar content 2\n+diff --git a/foo.bin b/foo.bin\n+index d5b9fe3..1345db2 100644\n+--- a/foo.bin\n++++ b/foo.bin\n+@@ -1 +1 @@\n+-converted: foo content 1\n++converted: foo content 2\n+EOF\n+\n+test_expect_success 'first textconv works' '\n+\tgit diff HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cached textconv produces same output' '\n+\tgit diff HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cached textconv does not run helper' '\n+\trm -f helper.out &&\n+\tgit diff HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\t! test -r helper.out\n+'\n+\n+cat >expect <<EOF\n+diff --git a/bar.bin b/bar.bin\n+index fcf9166..28283d5 100644\n+--- a/bar.bin\n++++ b/bar.bin\n+@@ -1,2 +1,2 @@\n+ converted: other\n+-converted: bar content 1\n++converted: bar content 2\n+diff --git a/foo.bin b/foo.bin\n+index d5b9fe3..1345db2 100644\n+--- a/foo.bin\n++++ b/foo.bin\n+@@ -1,2 +1,2 @@\n+ converted: other\n+-converted: foo content 1\n++converted: foo content 2\n+EOF\n+test_expect_success 'changing textconv invalidates cache' '\n+\techo other >other &&\n+\tgit config diff.magic.textconv \"./helper other\" &&\n+\tgit diff HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<EOF\n+diff --git a/bar.bin b/bar.bin\n+index fcf9166..28283d5 100644\n+--- a/bar.bin\n++++ b/bar.bin\n+@@ -1,2 +1,2 @@\n+ converted: other\n+-converted: bar content 1\n++converted: bar content 2\n+diff --git a/foo.bin b/foo.bin\n+index d5b9fe3..1345db2 100644\n+--- a/foo.bin\n++++ b/foo.bin\n+@@ -1 +1 @@\n+-converted: foo content 1\n++converted: foo content 2\n+EOF\n+test_expect_success 'switching diff driver produces correct results' '\n+\tgit config diff.moremagic.textconv ./helper &&\n+\techo foo.bin diff=moremagic >>.gitattributes &&\n+\tgit diff HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/userdiff.c b/userdiff.c\nindex df99249..67003fb 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -1,3 +1,4 @@\n+#include \"cache.h\"\n #include \"userdiff.h\"\n #include \"cache.h\"\n #include \"attr.h\"\n@@ -167,6 +168,12 @@ static int parse_tristate(int *b, const char *k, const char *v)\n \treturn 1;\n }\n \n+static int parse_bool(int *b, const char *k, const char *v)\n+{\n+\t*b = git_config_bool(k, v);\n+\treturn 1;\n+}\n+\n int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n@@ -181,6 +188,8 @@ int userdiff_config(const char *k, const char *v)\n \t\treturn parse_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\n \t\treturn parse_string(&drv->textconv, k, v);\n+\tif ((drv = parse_driver(k, v, \"cachetextconv\")))\n+\t\treturn parse_bool(&drv->textconv_want_cache, k, v);\n \tif ((drv = parse_driver(k, v, \"wordregex\")))\n \t\treturn parse_string(&drv->word_regex, k, v);\n \ndiff --git a/userdiff.h b/userdiff.h\nindex c315159..942d594 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -1,6 +1,8 @@\n #ifndef USERDIFF_H\n #define USERDIFF_H\n \n+#include \"notes-cache.h\"\n+\n struct userdiff_funcname {\n \tconst char *pattern;\n \tint cflags;\n@@ -13,6 +15,8 @@ struct userdiff_driver {\n \tstruct userdiff_funcname funcname;\n \tconst char *word_regex;\n \tconst char *textconv;\n+\tstruct notes_cache *textconv_cache;\n+\tint textconv_want_cache;\n };\n \n int userdiff_config(const char *k, const char *v);\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138379","messageId":"20100402001424.GG16462@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"[PATCH 7/7] diff: avoid useless filespec population","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T00:14:24Z","receivedAt":"2010-04-02T00:14:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"builtin_diff calls fill_mmfile fairly early, which in turn\ncalls diff_populate_filespec, which actually retrieves the\nfile's blob contents into a buffer. Long ago, this was\nsensible as we would need to look at the blobs eventually.\n\nThese days, however, we may not ever want those blobs if we\nend up using a textconv cache, and for large binary files\n(exactly the sort for which you might have a textconv\ncache), just retrieving the objects can be costly.\n\nThis patch just pushes the fill_mmfile call a bit later, so\nwe can avoid populating the filespec in some cases.  There\nis one thing to note that looks like a bug but isn't. We\npush the fill_mmfile down into the first branch of a\nconditional. It seems like we would need it on the other\nbranch, too, but we don't; fill_textconv does it for us (in\nfact, before this, we were just writing over the results of\nthe fill_mmfile on that branch).\n\nHere's a timing sample on a commit with 45 changed jpgs and\navis. The result is fully textconv cached, but we still\nwasted a lot of time just pulling the blobs from storage.\nThe total size of the blobs (source and dest) is about\n180M.\n\n  [before]\n  $ time git show >/dev/null\n  real    0m0.352s\n  user    0m0.148s\n  sys     0m0.200s\n\n  [after]\n  $ time git show >/dev/null\n  real    0m0.009s\n  user    0m0.004s\n  sys     0m0.004s\n\nAnd that's on a warm cache. On a cold cache, the \"after\"\ncase is not much worse, but the \"before\" case has to do an\nextra 180M of I/O.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI remember when I first started using git having it go so fast that I\nhad to double-check that it had actually worked correctly. I had another\nof those moments when I ran this. But it correctly processed everything\nI threw at it. More stress-testing appreciated. :)\n\n diff.c |    9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 72d8503..4cb6d9a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1680,12 +1680,11 @@ static void builtin_diff(const char *name_a,\n \t\t}\n \t}\n \n-\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n-\t\tdie(\"unable to read files to diff\");\n-\n \tif (!DIFF_OPT_TST(o, TEXT) &&\n-\t    ( (diff_filespec_is_binary(one) && !textconv_one) ||\n-\t      (diff_filespec_is_binary(two) && !textconv_two) )) {\n+\t    ( (!textconv_one && diff_filespec_is_binary(one)) ||\n+\t      (!textconv_two && diff_filespec_is_binary(two)) )) {\n+\t\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n+\t\t\tdie(\"unable to read files to diff\");\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n \t\t    !memcmp(mf1.ptr, mf2.ptr, mf1.size))\n-- \n1.7.0.4.299.gba9d4\n"},{"id":"138389","messageId":"20100402061420.GA5551@coredump.intra.peff.net","threadId":"23295","inReplyTo":"20100402000159.GA15101@coredump.intra.peff.net","subject":"Re: [PATCH 0/7] textconv caching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T06:14:21Z","receivedAt":"2010-04-02T06:14:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 01, 2010 at 08:01:59PM -0400, Jeff King wrote:\n\n>   [before]\n>   $ time git show >/dev/null\n>   real    0m13.724s\n>   user    0m12.057s\n>   sys     0m1.624s\n> \n>   [after (with cache primed)]\n>   $ time git show >/dev/null\n>   real    0m0.009s\n>   user    0m0.004s\n>   sys     0m0.004s\n\nSince this is a space-time tradeoff, I thought it would make sense to\nshow some size numbers as a followup.\n\nTo get a sense of the size of the repo (it's almost all photos and\nvideos):\n\n  [size of the repo, already fully packed]\n  $ du -sh .git/objects\n  4.0G    .git/objects\n\n  [the number of unique blobs through all history; most are binary media]\n  $ git log --raw --no-abbrev | awk '/^:/ {print $3 \"\\n\" $4}' | sort -u | wc -l\n  10605\n\nIn comparison, the metadata for a given file (produced by the textconv)\nis about 200 bytes of text.\n\nSo I did a big cache priming:\n\n  $ time git log -p >/dev/null\n  real    39m29.748s\n  user    23m1.090s\n  sys     3m46.642s\n\nSlow, and unsurprisingly spends quite a bit of time waiting on I/O. The\nresult is a notes tree with almost one textconv per blob:\n\n  $ git ls-tree -r notes/textconv/mfo | wc -l\n  10317\n\nWe're now using almost 200M:\n\n  $ git count-objects\n  39513 objects, 198604 kilobytes\n\nBut wait. Many of those objects are trees for stale versions of the\ncache.\n\n  $ git repack -d\n  $ (cd .git/objects/pack && du -k *.pack)\n  2056    pack-34170e72ec40a07e98aae044479abccc9e02751b.pack\n  4089224 pack-81797628f3aebf6a0bdc082fa05ec14932910534.pack\n  $ git count-objects\n  30685 objects, 163288 kilobytes\n\nIn actuality, a fully packed cache is only about 2M (from 35M of\nloose objects; it deltas quite well because there is a lot of overlap\nin my metadata). And we can prune away the other 160M of cruft:\n\n  $ git prune\n  $ git count-objects\n  0 objects, 0 kilobytes\n\nAnd of course, the final speed result:\n\n  $ time git log -p >/dev/null\n  real    0m7.606s\n  user    0m6.084s\n  sys     0m0.788s\n\nSo what I take away from this is two things:\n\n  1. The size tradeoff is definitely worthwhile for some workloads. In\n     this case, the textconv version is orders of magnitude smaller than\n     the original. I'd be interested to see numbers for something like a\n     repository of documents that get textconv'd to pure ascii.\n\n  2. We had 460% loose object overhead just from tree objects in\n     intermediate versions of the cache. While it was not too hard to\n     get rid of with a repack and a prune, we are probably better off\n     not generating it in the first place. In theory we could have\n     written only one notes tree, and kept the intermediate state in\n     memory. In practice, flushing once per commit-diff (instead of once\n     per file) would probably be fine, and would be simpler to\n     implement.\n\nAnd of course, now that I have a completely primed cache, I can push it\naround with \"git push $dest notes/textconv/mfo\". Yay for storing notes\nas git objects.\n\n-Peff\n"},{"id":"138394","messageId":"7vvdcal44p.fsf@alter.siamese.dyndns.org","threadId":"23295","inReplyTo":"20100402001424.GG16462@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] diff: avoid useless filespec population","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-02T07:12:22Z","receivedAt":"2010-04-02T07:12:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This patch just pushes the fill_mmfile call a bit later, so we can avoid\n> populating the filespec in some cases.  There is one thing to note that\n> looks like a bug but isn't. We push the fill_mmfile down into the first\n> branch of a conditional. It seems like we would need it on the other\n> branch, too, but we don't; fill_textconv does it for us (in fact, before\n> this, we were just writing over the results of the fill_mmfile on that\n> branch).\n\n... and leaking the memory in the \"cached\" codepath, perhaps?\n\n> I remember when I first started using git having it go so fast that I\n> had to double-check that it had actually worked correctly. I had another\n> of those moments when I ran this. But it correctly processed everything\n> I threw at it. More stress-testing appreciated. :)\n\nHeh.  Will queue.\n\nThanks.\n"},{"id":"138395","messageId":"7vpr2il3mt.fsf@alter.siamese.dyndns.org","threadId":"23295","inReplyTo":"20100402001215.GF16462@coredump.intra.peff.net","subject":"Re: [PATCH 6/7] diff: cache textconv output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-02T07:23:06Z","receivedAt":"2010-04-02T07:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Running a textconv filter can take a long time. It's\n> particularly bad for a large file which needs to be spooled\n> to disk, but even for small files, the fork+exec overhead\n> can add up for something like \"git log -p\".\n\nAnother reason that \"log -p\" gets benefit from caching is that you would\ntypically end up running textconv twice on the same blob, once when you\ncompare $commit:$path with $commit~1:$path, and again when you compare\n$commit~$n-1:$path with $commit~$n:$path (assuming that the $path didn't\nchange between $commit~$n-1 and $commit~1).\n\nIt _might_ give you even better performance characteristics if you noice\nthat you are walking history running many textconv, and cache the textconv\nresult from the \"older\" (i.e. \"one\" side) tree in-core, until it is used\nin a \"newer\" (i.e. \"two\" side) tree, at which time you would evict it.\n"},{"id":"138396","messageId":"20100402072455.GA2111@sigill.intra.peff.net","threadId":"23295","inReplyTo":"7vvdcal44p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] diff: avoid useless filespec population","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T07:24:55Z","receivedAt":"2010-04-02T07:24:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 02, 2010 at 12:12:22AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This patch just pushes the fill_mmfile call a bit later, so we can avoid\n> > populating the filespec in some cases.  There is one thing to note that\n> > looks like a bug but isn't. We push the fill_mmfile down into the first\n> > branch of a conditional. It seems like we would need it on the other\n> > branch, too, but we don't; fill_textconv does it for us (in fact, before\n> > this, we were just writing over the results of the fill_mmfile on that\n> > branch).\n> \n> ... and leaking the memory in the \"cached\" codepath, perhaps?\n\nI don't think so. fill_mmfile never allocates, but rather points to\nwhat's in the diff_filespec. So though we overwrote what was in the\nmmfile, that data was still pointed to and eventually freed by the\ndiff_filespec struct.\n\nfill_textconv will allocate if and only if we are doing a textconv\n(either a buffer we get from read_sha1_file in the case of a cached\nentry, or the detached strbuf from run-textconv otherwise). If we're not\ndoing a textconv, it falls back to basically doing the same as\nfill_mmfile. And that's the reason for the\n\n  if (textconv_one)\n          free(data_one);\n\nseen in patch 2/7 (and the matching one in builtin_diff from before).\n\n-Peff\n"},{"id":"138399","messageId":"20100402073832.GB2111@sigill.intra.peff.net","threadId":"23295","inReplyTo":"7vpr2il3mt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/7] diff: cache textconv output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-04-02T07:38:32Z","receivedAt":"2010-04-02T07:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 02, 2010 at 12:23:06AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Running a textconv filter can take a long time. It's\n> > particularly bad for a large file which needs to be spooled\n> > to disk, but even for small files, the fork+exec overhead\n> > can add up for something like \"git log -p\".\n> \n> Another reason that \"log -p\" gets benefit from caching is that you would\n> typically end up running textconv twice on the same blob, once when you\n> compare $commit:$path with $commit~1:$path, and again when you compare\n> $commit~$n-1:$path with $commit~$n:$path (assuming that the $path didn't\n> change between $commit~$n-1 and $commit~1).\n\nYep. I pointed out in one of my timing tests a slight slowdown in \"git\nshow\" when generating the cache. But for revision walking, it should\nactually be faster, since you see each blob twice but cache after the\nfirst time.\n\n> It _might_ give you even better performance characteristics if you noice\n> that you are walking history running many textconv, and cache the textconv\n> result from the \"older\" (i.e. \"one\" side) tree in-core, until it is used\n> in a \"newer\" (i.e. \"two\" side) tree, at which time you would evict it.\n\nI doubt it is worth the effort. We are already caching the sha1 in-core\ndue to the notes mechanism. So we could really only save one object\nretrieval. Which is already what a non-textconv diff will need to do, so\nwe should have performance on par with regular diffs at this point.\n\nIn fact, your optimization could be applied to all diff revision\nwalking, not just textconv, and you can halve the number of object\nretrievals. The problem is that you may have blobs sitting in the\nin-core cache as you walk many revisions, waiting for them to be changed\nagain. Depending on the locality of changes and the size of your\nproject, you won't be able to fit it all comfortably in memory, and will\nend up discarding entries.\n\nAnd all of that to save a few object retrievals, which are something\nthat git does very quickly already. Not to mention the ugly code that\nwould be involved.\n\n-Peff\n"}]}