{"thread":{"id":"23214","subject":"[PATCH 0/3] fast textconv","startedAt":"2010-03-28T14:53:02Z","lastAt":"2010-03-30T22:56:29Z","messageCount":19,"participants":["Jeff King","Michael J Gruber","Sverre Rabbelier","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"138004","messageId":"20100328145301.GA26213@coredump.intra.peff.net","threadId":"23214","inReplyTo":null,"subject":"[PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T14:53:02Z","receivedAt":"2010-03-28T14:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The normal textconv procedure is to dump the binary file to a tempfile\n(optionally using a working tree file if available), then run the\ntextconv helper to produce a textual version on stdout. This is a very\nconvenient interface, as helpers don't need to be aware of git at all\nand many standard commands can be used without wrappers.\n\nUnfortunately, it can be slow for large binary files. We spool the file\nto disk before invoking the textconv helper, so the helper has no way to\ndo any optimizations. For example, the helper may need only part of the\nfile (e.g., when showing metadata at the beginning of a media file), or\nit may implement a caching scheme to avoid repeating expensive\nconversions.\n\nThis series introduces a \"fast textconv\", which does not automatically\nspool a tempfile, but instead gives the helper program the sha1 of the\nblob to be converted.\n\nHere are some timings from my photo repository, on a commit with 37\nJPEGs and 8 AVIs. Each file had two lines added to its exif metadata.\nMy textconv helper is a perl script that dumps the exif tags, and\nimplements its own caching scheme.\n\n  $ time git show >/dev/null ;# before patch\n  real    0m13.818s\n  user    0m12.137s\n  sys     0m1.552s\n\n  $ time git show >/dev/null ;# after patch, first run\n  real    0m15.076s\n  user    0m13.321s\n  sys     0m1.772s\n\n  $ time git show >/dev/null ;# after patch, subsequent runs\n  real    0m2.502s\n  user    0m1.820s\n  sys     0m0.592s\n\nSo you can see a 5.5x speedup. The first run is a little bit slower,\npresumably due to the extra git-cat-file calls by the helper.\n\nThe speedup is purely from caching; I am not using the \"we only need to\nread the first part of the file\" optimization. My files are only a few\nmegabytes. Probably that would be more useful for people storing files\nin the hundreds of megabytes, where a full cat-file will cause a lot of\nunwanted I/O.\n\nThere are two things I'm still not 100% happy with:\n\n 1. 2.5 seconds is still a little slower than I would like. The slowness\n    comes from the fact that my helper is written in perl, and therefore\n    perl gets invoked for each diff. I could try collecting all of the\n    to-be-textconv'd files at the beginning of the diff process and just\n    invoking the helper once. But that means we need to store the\n    results in core, and they could potentially be long (in my case,\n    they are only a few hundred bytes, but somebody could potentially be\n    textconv'ing a large documents).\n\n 2. It is up to the helper to implement a caching layer. This offers a\n    lot of flexibility, but it means each helper must implement its own.\n    It also means we have to run the helper even for a cache hit, which\n    causes slowness.\n\n    An alternative would be for git to support textconv caching\n    natively, probably by using the notes mechanism to map blob sha1's\n    to their textconv'd contents. But that opens a whole can of worms\n    with how the cache is managed. If I change my textconv helper to\n    produce different results, how do I invalidate the cache? Should it\n    happen automatically if I change the contents of\n    diff.$method.textconv? Or do I need to do it manually (you will\n    still need to do it manually if, e.g., you upgrade your textconv\n    helper. Git can't know about that). How do I evict entries if the\n    cache gets too large when notes are stored as a history?\n\nSo I'm not sure. This series works and is simple from git's perspective.\nBut caching textconv results in notes would be faster, and easier for\npeople to write helper scripts.\n\nThe patches are:\n\n  [1/3]: textconv: refactor calls to run_textconv\n  [2/3]: textconv: refactor to handle multiple textconv types\n  [3/3]: diff: add \"fasttextconv\" config option\n\n-Peff\n"},{"id":"138005","messageId":"20100328145342.GA27970@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328145301.GA26213@coredump.intra.peff.net","subject":"[PATCH 1/3] textconv: refactor calls to run_textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T14:53:42Z","receivedAt":"2010-03-28T14:53:42Z","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---\n diff.c |   58 ++++++++++++++++++++++++----------------------------------\n 1 files changed, 24 insertions(+), 34 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f5d93e9..3ddc05e 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\tstruct 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@@ -501,24 +502,8 @@ static void emit_rewrite_diff(const char *name_a,\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@@ -1713,20 +1698,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@@ -3907,3 +3880,20 @@ 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\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.3.294.g8e730.dirty\n"},{"id":"138006","messageId":"20100328145352.GB27970@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328145301.GA26213@coredump.intra.peff.net","subject":"[PATCH 2/3] textconv: refactor to handle multiple textconv types","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T14:53:52Z","receivedAt":"2010-03-28T14:53:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass around textconv information as a string which is\neither NULL (no textconv) or not (and contains the textconv\ncommand). In preparation for handling multiple types of\ntextconv interface, this patch converts it into a struct\nwith an \"interface\" field.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c     |   26 ++++++++++++++++----------\n userdiff.c |    9 ++++++++-\n userdiff.h |   12 +++++++++++-\n 3 files changed, 35 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3ddc05e..0d31320 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_extcmd *cmd,\n \t\tstruct 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_extcmd *textconv_one,\n+\t\t\t      struct userdiff_extcmd *textconv_two,\n \t\t\t      struct diff_options *o)\n {\n \tint lc_a, lc_b;\n@@ -1567,14 +1567,16 @@ 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_extcmd *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.interface)\n+\t\treturn NULL;\n+\treturn &one->driver->textconv;\n }\n \n static void builtin_diff(const char *name_a,\n@@ -1591,7 +1593,7 @@ 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_extcmd *textconv_one = NULL, *textconv_two = NULL;\n \tstruct strbuf header = STRBUF_INIT;\n \n \tif (DIFF_OPT_TST(o, SUBMODULE_LOG) &&\n@@ -3848,7 +3850,7 @@ void diff_unmerge(struct diff_options *options,\n \tdiff_queue(&diff_queued_diff, one, two)->is_unmerged = 1;\n }\n \n-static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n+static char *run_textconv_simple(const char *pgm, struct diff_filespec *spec,\n \t\tsize_t *outsize)\n {\n \tstruct diff_tempfile *temp;\n@@ -3881,18 +3883,22 @@ 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_extcmd *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+\tif (!cmd || !cmd->interface) {\n \t\t*outbuf = df->data;\n \t\treturn df->size;\n \t}\n \n-\t*outbuf = run_textconv(cmd, df, &size);\n+\tif (cmd->interface == USERDIFF_EXTCMD_SIMPLE)\n+\t\t*outbuf = run_textconv_simple(cmd->cmd, df, &size);\n+\telse\n+\t\tdie(\"BUG: invalid internal textconv type\");\n+\n \tif (!*outbuf)\n \t\tdie(\"unable to read files to diff\");\n \treturn size;\ndiff --git a/userdiff.c b/userdiff.c\nindex df99249..6850f21 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -158,6 +158,13 @@ static int parse_string(const char **d, const char *k, const char *v)\n \treturn 1;\n }\n \n+static int parse_extcmd(struct userdiff_extcmd *e, const char *k,\n+\t\tconst char *v)\n+{\n+\te->interface = USERDIFF_EXTCMD_SIMPLE;\n+\treturn parse_string(&e->cmd, k, v);\n+}\n+\n static int parse_tristate(int *b, const char *k, const char *v)\n {\n \tif (v && !strcasecmp(v, \"auto\"))\n@@ -180,7 +187,7 @@ int userdiff_config(const char *k, const char *v)\n \tif ((drv = parse_driver(k, v, \"command\")))\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+\t\treturn parse_extcmd(&drv->textconv, 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..0c1ac06 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -6,13 +6,23 @@ struct userdiff_funcname {\n \tint cflags;\n };\n \n+enum userdiff_extcmd_interface {\n+\tUSERDIFF_EXTCMD_NONE = 0,\n+\tUSERDIFF_EXTCMD_SIMPLE,\n+};\n+\n+struct userdiff_extcmd {\n+\tconst char *cmd;\n+\tenum userdiff_extcmd_interface interface;\n+};\n+\n struct userdiff_driver {\n \tconst char *name;\n \tconst char *external;\n \tint binary;\n \tstruct userdiff_funcname funcname;\n \tconst char *word_regex;\n-\tconst char *textconv;\n+\tstruct userdiff_extcmd textconv;\n };\n \n int userdiff_config(const char *k, const char *v);\n-- \n1.7.0.3.294.g8e730.dirty\n"},{"id":"138007","messageId":"20100328145421.GC27970@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328145301.GA26213@coredump.intra.peff.net","subject":"[PATCH 3/3] diff: add \"fasttextconv\" config option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T14:54:21Z","receivedAt":"2010-03-28T14:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The normal textconv procedure is to dump the binary file to\na tempfile (optionally using a working tree file if\navailable), then run the textconv helper to produce a\ntextual version on stdout. This is a very convenient\ninterface, as helpers don't need to be aware of git at all\nand many standard commands can be used without wrappers.\n\nUnfortunately, it can be slow for large binary files. We\nspool the file to disk before invoking the textconv helper,\nso the helper has no way to do any optimizations. For\nexample, the helper may need only part of the file (e.g.,\nwhen showing metadata at the beginning of a media file), or\nit may implement a caching scheme to avoid repeating\nexpensive conversions.\n\nThis patch introduces diff.*.fasttextconv, which invokes the\ntextconv helper with the sha1 of the blob to be converted.\nThe helper can then retrieve the file with git-cat-file if\nnecessary.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/gitattributes.txt |   11 +++++++++++\n diff.c                          |   32 ++++++++++++++++++++++++++++++++\n t/t4030-diff-textconv.sh        |   15 +++++++++++++++\n userdiff.c                      |    8 +++++---\n userdiff.h                      |    1 +\n 5 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex d892e64..6066256 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -414,6 +414,17 @@ 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+The `fasttextconv` config option is similar to `textconv`, but\n+uses a different interface that provides more opporunity for\n+optimization at the cost of increased complexity. The program\n+specified by `fasttextconv` receives on the command line the sha1\n+of the blob to be converted and the pathname, if any, from which\n+the blob was accessed. It is the responsibility of the invoked\n+program to retrieve the blob's contents (e.g., using `git\n+cat-file blob $sha1`). This allows the helper program to avoid\n+looking at the blob contents in some situations (e.g., if it\n+implements a caching layer keyed by sha1).\n+\n \n Performing a three-way merge\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\ndiff --git a/diff.c b/diff.c\nindex 0d31320..5208d4d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3883,6 +3883,36 @@ static char *run_textconv_simple(const char *pgm, struct diff_filespec *spec,\n \treturn strbuf_detach(&buf, outsize);\n }\n \n+static char *run_textconv_fast(const char *pgm, struct diff_filespec *spec,\n+\t\tsize_t *outsize)\n+{\n+\tconst char *argv[4];\n+\tconst char **arg = argv;\n+\tstruct child_process child;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t*arg++ = pgm;\n+\t*arg++ = sha1_to_hex(spec->sha1);\n+\t*arg++ = spec->path;\n+\t*arg = NULL;\n+\n+\tmemset(&child, 0, sizeof(child));\n+\tchild.use_shell = 1;\n+\tchild.argv = argv;\n+\tchild.out = -1;\n+\tif (start_command(&child) != 0 ||\n+\t    strbuf_read(&buf, child.out, 0) < 0 ||\n+\t    finish_command(&child) != 0) {\n+\t\tclose(child.out);\n+\t\tstrbuf_release(&buf);\n+\t\terror(\"error running textconv command '%s'\", pgm);\n+\t\treturn NULL;\n+\t}\n+\tclose(child.out);\n+\n+\treturn strbuf_detach(&buf, outsize);\n+}\n+\n static size_t fill_textconv(struct userdiff_extcmd *cmd,\n \t\t\t    struct diff_filespec *df,\n \t\t\t    char **outbuf)\n@@ -3896,6 +3926,8 @@ static size_t fill_textconv(struct userdiff_extcmd *cmd,\n \n \tif (cmd->interface == USERDIFF_EXTCMD_SIMPLE)\n \t\t*outbuf = run_textconv_simple(cmd->cmd, df, &size);\n+\telse if (cmd->interface == USERDIFF_EXTCMD_FAST)\n+\t\t*outbuf = run_textconv_fast(cmd->cmd, df, &size);\n \telse\n \t\tdie(\"BUG: invalid internal textconv type\");\n \ndiff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh\nindex 88c5619..4b62434 100755\n--- a/t/t4030-diff-textconv.sh\n+++ b/t/t4030-diff-textconv.sh\n@@ -25,6 +25,13 @@ perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < \"$1\"\n EOF\n chmod +x hexdump\n \n+cat >hexdump.fast <<'EOF'\n+#!/bin/sh\n+git cat-file blob \"$1\" |\n+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_'\n+EOF\n+chmod +x hexdump.fast\n+\n test_expect_success 'setup binary file with history' '\n \tprintf \"\\\\0\\\\n\" >file &&\n \tgit add file &&\n@@ -49,6 +56,7 @@ test_expect_success 'file is considered binary by plumbing' '\n test_expect_success 'setup textconv filters' '\n \techo file diff=foo >.gitattributes &&\n \tgit config diff.foo.textconv \"\\\"$(pwd)\\\"\"/hexdump &&\n+\tgit config diff.fast.fasttextconv \"\\\"$(pwd)\\\"\"/hexdump.fast &&\n \tgit config diff.fail.textconv false\n '\n \n@@ -84,6 +92,13 @@ test_expect_success 'status -v produces text' '\n \tgit reset --soft HEAD@{1}\n '\n \n+test_expect_success 'fast textconv works' '\n+\techo file diff=fast >.gitattributes &&\n+\tgit diff HEAD^ HEAD >diff &&\n+\tfind_diff <diff >actual &&\n+\ttest_cmp expect.text actual\n+'\n+\n cat >expect.stat <<'EOF'\n  file |  Bin 2 -> 4 bytes\n  1 files changed, 0 insertions(+), 0 deletions(-)\ndiff --git a/userdiff.c b/userdiff.c\nindex 6850f21..c913dc2 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -159,9 +159,9 @@ static int parse_string(const char **d, const char *k, const char *v)\n }\n \n static int parse_extcmd(struct userdiff_extcmd *e, const char *k,\n-\t\tconst char *v)\n+\t\tconst char *v, enum userdiff_extcmd_interface type)\n {\n-\te->interface = USERDIFF_EXTCMD_SIMPLE;\n+\te->interface = type;\n \treturn parse_string(&e->cmd, k, v);\n }\n \n@@ -187,7 +187,9 @@ int userdiff_config(const char *k, const char *v)\n \tif ((drv = parse_driver(k, v, \"command\")))\n \t\treturn parse_string(&drv->external, k, v);\n \tif ((drv = parse_driver(k, v, \"textconv\")))\n-\t\treturn parse_extcmd(&drv->textconv, k, v);\n+\t\treturn parse_extcmd(&drv->textconv, k, v, USERDIFF_EXTCMD_SIMPLE);\n+\tif ((drv = parse_driver(k, v, \"fasttextconv\")))\n+\t\treturn parse_extcmd(&drv->textconv, k, v, USERDIFF_EXTCMD_FAST);\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 0c1ac06..84fd3df 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -9,6 +9,7 @@ struct userdiff_funcname {\n enum userdiff_extcmd_interface {\n \tUSERDIFF_EXTCMD_NONE = 0,\n \tUSERDIFF_EXTCMD_SIMPLE,\n+\tUSERDIFF_EXTCMD_FAST,\n };\n \n struct userdiff_extcmd {\n-- \n1.7.0.3.294.g8e730.dirty\n"},{"id":"138008","messageId":"4BAF7F3F.5020604@drmicha.warpmail.net","threadId":"23214","inReplyTo":"20100328145301.GA26213@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-03-28T16:09:35Z","receivedAt":"2010-03-28T16:09:35Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 28.03.2010 16:53:\n> The normal textconv procedure is to dump the binary file to a tempfile\n> (optionally using a working tree file if available), then run the\n> textconv helper to produce a textual version on stdout. This is a very\n> convenient interface, as helpers don't need to be aware of git at all\n> and many standard commands can be used without wrappers.\n> \n> Unfortunately, it can be slow for large binary files. We spool the file\n> to disk before invoking the textconv helper, so the helper has no way to\n> do any optimizations. For example, the helper may need only part of the\n> file (e.g., when showing metadata at the beginning of a media file), or\n> it may implement a caching scheme to avoid repeating expensive\n> conversions.\n> \n> This series introduces a \"fast textconv\", which does not automatically\n> spool a tempfile, but instead gives the helper program the sha1 of the\n> blob to be converted.\n> \n> Here are some timings from my photo repository, on a commit with 37\n> JPEGs and 8 AVIs. Each file had two lines added to its exif metadata.\n> My textconv helper is a perl script that dumps the exif tags, and\n> implements its own caching scheme.\n> \n>   $ time git show >/dev/null ;# before patch\n>   real    0m13.818s\n>   user    0m12.137s\n>   sys     0m1.552s\n> \n>   $ time git show >/dev/null ;# after patch, first run\n>   real    0m15.076s\n>   user    0m13.321s\n>   sys     0m1.772s\n> \n>   $ time git show >/dev/null ;# after patch, subsequent runs\n>   real    0m2.502s\n>   user    0m1.820s\n>   sys     0m0.592s\n> \n> So you can see a 5.5x speedup. The first run is a little bit slower,\n> presumably due to the extra git-cat-file calls by the helper.\n> \n> The speedup is purely from caching; I am not using the \"we only need to\n> read the first part of the file\" optimization. My files are only a few\n> megabytes. Probably that would be more useful for people storing files\n> in the hundreds of megabytes, where a full cat-file will cause a lot of\n> unwanted I/O.\n> \n> There are two things I'm still not 100% happy with:\n> \n>  1. 2.5 seconds is still a little slower than I would like. The slowness\n>     comes from the fact that my helper is written in perl, and therefore\n>     perl gets invoked for each diff. I could try collecting all of the\n>     to-be-textconv'd files at the beginning of the diff process and just\n>     invoking the helper once. But that means we need to store the\n>     results in core, and they could potentially be long (in my case,\n>     they are only a few hundred bytes, but somebody could potentially be\n>     textconv'ing a large documents).\n> \n>  2. It is up to the helper to implement a caching layer. This offers a\n>     lot of flexibility, but it means each helper must implement its own.\n>     It also means we have to run the helper even for a cache hit, which\n>     causes slowness.\n> \n>     An alternative would be for git to support textconv caching\n>     natively, probably by using the notes mechanism to map blob sha1's\n>     to their textconv'd contents. But that opens a whole can of worms\n>     with how the cache is managed. If I change my textconv helper to\n>     produce different results, how do I invalidate the cache? Should it\n>     happen automatically if I change the contents of\n>     diff.$method.textconv? Or do I need to do it manually (you will\n>     still need to do it manually if, e.g., you upgrade your textconv\n>     helper. Git can't know about that). How do I evict entries if the\n>     cache gets too large when notes are stored as a history?\n\nReally, \"Notes!\" was my first thought even before reading 2. Happy to\nhave found a like mind :)\n\nThis would still need a mechanism where the conv helper gets the blob's\nSHA1 - hey, it's there in your patch...\n\nHow about:\n\nSet fasttextconv=notestextconv\n\nnotestextconv does the following:\n\n- If $sha1 has a note in refs/notes/bikeshed display it.\n- If not create one and then display it.\n\nIn fact, the creation could be done using the textconv setting!\n\nPruning the cache is done be deleting the refs/notes/bikeshed ref,\ntruncating it by truncating it's DAG (filter-branch...).\n\nCheers,\nMichael\n"},{"id":"138009","messageId":"20100328161728.GA2828@coredump.intra.peff.net","threadId":"23214","inReplyTo":"4BAF7F3F.5020604@drmicha.warpmail.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T16:17:28Z","receivedAt":"2010-03-28T16:17:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 06:09:35PM +0200, Michael J Gruber wrote:\n\n> Really, \"Notes!\" was my first thought even before reading 2. Happy to\n> have found a like mind :)\n> \n> This would still need a mechanism where the conv helper gets the blob's\n> SHA1 - hey, it's there in your patch...\n> \n> How about:\n> \n> Set fasttextconv=notestextconv\n> \n> notestextconv does the following:\n> \n> - If $sha1 has a note in refs/notes/bikeshed display it.\n> - If not create one and then display it.\n> \n> In fact, the creation could be done using the textconv setting!\n\nIf I understand you right, you are proposing a separate program\nthat you would pass as a fasttextconv helper, and that would look in a\nnotes tree. So you would still have a per-diff fork/exec, and pipe all\nthe data.\n\nI was thinking of actually doing it in-core, so cache hits would be as\nlightweight as a notes lookup (and cache misses obviously would still\nfork/exec a helper, but we don't care too much since the helper's time\nto convert will dominate in that path).\n\n> Pruning the cache is done be deleting the refs/notes/bikeshed ref,\n> truncating it by truncating it's DAG (filter-branch...).\n\nYeah, that would work. It just means it's one more thing the user has to\ndeal with. I didn't want to have to introduce a \"git textconv\"\nmanagement helper. ;)\n\n-Peff\n"},{"id":"138010","messageId":"20100328161921.GA3435@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328161728.GA2828@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T16:19:21Z","receivedAt":"2010-03-28T16:19:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 12:17:28PM -0400, Jeff King wrote:\n\n> If I understand you right, you are proposing a separate program\n> that you would pass as a fasttextconv helper, and that would look in a\n> notes tree. So you would still have a per-diff fork/exec, and pipe all\n> the data.\n> \n> I was thinking of actually doing it in-core, so cache hits would be as\n> lightweight as a notes lookup (and cache misses obviously would still\n> fork/exec a helper, but we don't care too much since the helper's time\n> to convert will dominate in that path).\n\nSide note: I think I might prototype it as a separate program and see\nwhat kind of speed I can get.\n\n> > Pruning the cache is done be deleting the refs/notes/bikeshed ref,\n> > truncating it by truncating it's DAG (filter-branch...).\n> \n> Yeah, that would work. It just means it's one more thing the user has to\n> deal with. I didn't want to have to introduce a \"git textconv\"\n> management helper. ;)\n\nOther side note: it's not even that it's that hard to prune. It's that\nyou might tweak your config, and then you have to _remember_ to prune.\n\n-Peff\n"},{"id":"138017","messageId":"20100328165646.GA10293@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328161921.GA3435@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T16:56:47Z","receivedAt":"2010-03-28T16:56:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 12:19:21PM -0400, Jeff King wrote:\n\n> On Sun, Mar 28, 2010 at 12:17:28PM -0400, Jeff King wrote:\n> \n> > If I understand you right, you are proposing a separate program\n> > that you would pass as a fasttextconv helper, and that would look in a\n> > notes tree. So you would still have a per-diff fork/exec, and pipe all\n> > the data.\n> > \n> > I was thinking of actually doing it in-core, so cache hits would be as\n> > lightweight as a notes lookup (and cache misses obviously would still\n> > fork/exec a helper, but we don't care too much since the helper's time\n> > to convert will dominate in that path).\n> \n> Side note: I think I might prototype it as a separate program and see\n> what kind of speed I can get.\n\nBetter, but not perfect. My script is below. I get:\n\n  $ time git show >/dev/null\n  real    0m1.036s\n  user    0m0.412s\n  sys     0m0.672s\n\nwhich is still a 2.5x speedup (versus my other fast-textconv solution\nearlier in the thread), but I suspect we can do better. The notes\nmechanism does some up-front work to get very fast lookups, but because\nwe invoke git-notes repeatedly, we never get the amortized benefit of\nthat up-front work.  Doing it in-core would fix that.\n\nMy script was:\n\n-- >8 --\n#!/bin/sh\n\ntype=$1; shift\nprogram=$1; shift\nsha1=$1; shift\nfilename=$1; shift\n\nGIT_NOTES_REF=refs/notes/textconv/$type; export GIT_NOTES_REF\n\n# try the cache\ngit notes show $sha1 2>/dev/null && exit 0\n\n# otherwise, insert the cache entry.\n# We can be as slow as we like.\next=`echo \"$filename\" | sed 's/.*\\.//'`\ntmp=`mktemp notes-textconv-XXXXXX.$ext`\ngit cat-file blob $sha1 >$tmp\n$program $tmp | git notes add -f -F - $sha1\ngit notes show $sha1\nrm -f $tmp\n-- 8< --\n\nand my config is:\n\n  $ git config diff.mfo.textconv /home/peff/fast mfo mfo-tags\n\nwhere \"mfo-tags\" is the program to display metadata and \"mfo\" is a\nuser-selected shorthand name for it.\n\n-Peff\n"},{"id":"138020","messageId":"20100328173420.GA18047@coredump.intra.peff.net","threadId":"23214","inReplyTo":"20100328165646.GA10293@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-28T17:34:20Z","receivedAt":"2010-03-28T17:34:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 12:56:46PM -0400, Jeff King wrote:\n\n> Better, but not perfect. My script is below. I get:\n> \n>   $ time git show >/dev/null\n>   real    0m1.036s\n>   user    0m0.412s\n>   sys     0m0.672s\n> \n> which is still a 2.5x speedup (versus my other fast-textconv solution\n> earlier in the thread), but I suspect we can do better. The notes\n> mechanism does some up-front work to get very fast lookups, but because\n> we invoke git-notes repeatedly, we never get the amortized benefit of\n> that up-front work.  Doing it in-core would fix that.\n\nHere is a quick and dirty in-core implementation. The most notable\ndefect is that all textconvs store under refs/notes/textconv, which is\nobviously bogus if you might textconv the same blob in two different\nways.\n\nIt was relatively easy to code thanks to a very nice notes interface\nfrom Johan Herland. That same commit is now:\n\n  $ time git show >/dev/null\n  real    0m0.350s\n  user    0m0.172s\n  sys     0m0.176s\n\nwhich is starting to feel sufficiently snappy.\n\nPatch is on top of 1/3 from my earlier series (it uses some of the\ncentralizing cleanup).\n\ndiff --git a/diff.c b/diff.c\nindex 3ddc05e..e1ef25e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -14,6 +14,8 @@\n #include \"userdiff.h\"\n #include \"sigchain.h\"\n #include \"submodule.h\"\n+#include \"notes.h\"\n+#include \"refs.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -3881,19 +3883,57 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \treturn strbuf_detach(&buf, outsize);\n }\n \n+static struct notes_tree textconv_notes;\n+static int textconv_notes_initialized;\n+\n+static const unsigned char *run_and_cache_textconv(const char *cmd,\n+\t\tstruct diff_filespec *df)\n+{\n+\tchar *data;\n+\tsize_t size;\n+\tunsigned char blob_sha1[20];\n+\tunsigned char tree_sha1[20];\n+\n+\tdata = run_textconv(cmd, df, &size);\n+\tif (!data)\n+\t\tdie(\"unable to read files to diff\");\n+\tif (write_sha1_file(data, size, \"blob\", blob_sha1) < 0)\n+\t\tdie(\"unable to write textconv cache blob\");\n+\tfree(data);\n+\n+\tadd_note(&textconv_notes, df->sha1, blob_sha1, combine_notes_overwrite);\n+\tif (write_notes_tree(&textconv_notes, tree_sha1) < 0)\n+\t\tdie(\"unable to write textconv cache notes tree\");\n+\tupdate_ref(\"textconv cache update\", \"refs/notes/textconv\",\n+\t\t\ttree_sha1, NULL, 0, DIE_ON_ERR);\n+\n+\treturn get_note(&textconv_notes, df->sha1);\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+\tconst unsigned char *sha1;\n+\tunsigned long size;\n+\tenum object_type type;\n \n \tif (!cmd) {\n \t\t*outbuf = df->data;\n \t\treturn df->size;\n \t}\n \n-\t*outbuf = run_textconv(cmd, df, &size);\n+\tif (!textconv_notes_initialized) {\n+\t\tinit_notes(&textconv_notes, \"refs/notes/textconv\", 0, 0);\n+\t\ttextconv_notes_initialized = 1;\n+\t}\n+\n+\tsha1 = get_note(&textconv_notes, df->sha1);\n+\tif (!sha1)\n+\t\tsha1 = run_and_cache_textconv(cmd, df);\n+\n+\t*outbuf = read_sha1_file(sha1, &type, &size);\n \tif (!*outbuf)\n-\t\tdie(\"unable to read files to diff\");\n+\t\tdie(\"unable to read textconv cache object\");\n \treturn size;\n }\n"},{"id":"138030","messageId":"fabb9a1e1003281113o4cb1e723p4ed39521b645fffe@mail.gmail.com","threadId":"23214","inReplyTo":"20100328173420.GA18047@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-03-28T18:13:10Z","receivedAt":"2010-03-28T18:13:10Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Mar 28, 2010 at 11:34, Jeff King <peff@peff.net> wrote:\n> Here is a quick and dirty in-core implementation. The most notable\n> defect is that all textconvs store under refs/notes/textconv, which is\n> obviously bogus if you might textconv the same blob in two different\n> ways.\n\nWhat I did for the implementation of remote helpers is hash the url of\nthe remote when storing it locally, that way you don't have to worry\nabout escaping urls etc. You could do the same with the textconvs,\nstore it under  refs/notes/textconv/<hash of textconv filter>?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"138032","messageId":"201003282023.00913.j6t@kdbg.org","threadId":"23214","inReplyTo":"20100328145421.GC27970@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] diff: add \"fasttextconv\" config option","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-28T18:23:00Z","receivedAt":"2010-03-28T18:23:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 28. März 2010, Jeff King wrote:\n> +\tif (start_command(&child) != 0 ||\n> +\t    strbuf_read(&buf, child.out, 0) < 0 ||\n> +\t    finish_command(&child) != 0) {\n\nThis conditional is somewhat dubious. If strbuf_read fails, you do not wait \nfor the child, and a zombie remains.\n\nThe have this sequence already in run_textconv().\n\n-- Hannes\n"},{"id":"138155","messageId":"7vtyryxy8r.fsf@alter.siamese.dyndns.org","threadId":"23214","inReplyTo":"20100328145301.GA26213@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-30T03:52:04Z","receivedAt":"2010-03-30T03:52:04Z","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> The speedup is purely from caching; I am not using the \"we only need to\n> read the first part of the file\" optimization.\n\nThis made me wonder if the end result might be easier to use if the\ninterface does not change \"textconv\", but adds some property to the\nfilter, i.e. \"the output from this filter is stable and it is safe to\nreuse a cached conversion result for a given blob object\", boolean.  E.g.\n\n        [diff \"jpg\"]\n                textconv = exif\n                textconv_stable = true\n\nand let the calling side handle the caching.  I further suspect that\nan unstable textconv filter would be an anomaly, so this could even be on\nby default.\n\nIf we do so, stock conversion filters people have accumulated in the past\ncould be sped up without any additional change from the end user's side.\n\nI guess that I am suggesting to postpone the potential speed-up that could\ncome from being able to inspect the header information as a separate\ntopic.  Besides, some file format has metadata at the end, which won't\nhelp you.\n\nAbout the caching scheme; to help invalidating the cache, it probably is a\ngood idea to use not just the blob object name but also at least the name\n(command line) of the textconv filter as the key for the caching layer.\n\nInstead of the \"textconv_stable\" boolean depicted above, you could add a\n\"textconv_filter_version\" variable there, compute a hash over blob object\nname, textconv filter name and textconv filter version, and use that as\nthe key to look into the cache (filters lacking textconv_filter_version\nwould then get no caching, and if you update your \"exif\" program you bump\nthe \"textconv_filter_version\" variable).\n"},{"id":"138188","messageId":"20100330160449.GB17763@coredump.intra.peff.net","threadId":"23214","inReplyTo":"fabb9a1e1003281113o4cb1e723p4ed39521b645fffe@mail.gmail.com","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-30T16:04:49Z","receivedAt":"2010-03-30T16:04:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 12:13:10PM -0600, Sverre Rabbelier wrote:\n\n> On Sun, Mar 28, 2010 at 11:34, Jeff King <peff@peff.net> wrote:\n> > Here is a quick and dirty in-core implementation. The most notable\n> > defect is that all textconvs store under refs/notes/textconv, which is\n> > obviously bogus if you might textconv the same blob in two different\n> > ways.\n> \n> What I did for the implementation of remote helpers is hash the url of\n> the remote when storing it locally, that way you don't have to worry\n> about escaping urls etc. You could do the same with the textconvs,\n> store it under  refs/notes/textconv/<hash of textconv filter>?\n\nI thought about something like that, but we want to keep something that\nis comprehensible to the user. If my install of \"exif\" changes, I need\nto be able to say \"invalidate the cache for exif\". If it's\nhuman-readable, I know I need to clear refs/notes/textconv/exif. But if\nit's a hash, then I need a special tool to figure out which ref is\nrelevant.\n\nInstead, my plan is to use refs/notes/textconv/$TYPE, where $TYPE is the\nsame string used by the config (e.g., \"diff.$TYPE.textconv\") and the\ngitattributes file (e.g., \"*.jpg diff=$TYPE\"). The user has already\nprovided us a with a short, unique, meaningful name.\n\n-Peff\n"},{"id":"138190","messageId":"20100330163004.GC17763@coredump.intra.peff.net","threadId":"23214","inReplyTo":"201003282023.00913.j6t@kdbg.org","subject":"Re: [PATCH 3/3] diff: add \"fasttextconv\" config option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-30T16:30:04Z","receivedAt":"2010-03-30T16:30:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 28, 2010 at 08:23:00PM +0200, Johannes Sixt wrote:\n\n> On Sonntag, 28. März 2010, Jeff King wrote:\n> > +\tif (start_command(&child) != 0 ||\n> > +\t    strbuf_read(&buf, child.out, 0) < 0 ||\n> > +\t    finish_command(&child) != 0) {\n> \n> This conditional is somewhat dubious. If strbuf_read fails, you do not wait \n> for the child, and a zombie remains.\n> \n> The have this sequence already in run_textconv().\n\nUgh. I would blame the author of run_textconv, but that is also me. :)\n\nI doubt it is a big deal in practice, as the read() call is the least\nlikely to produce an error, and even if we do get a zombie, it will die\nalong with the diffing process. But it is still worth fixing. Thanks for\nnoticing.\n\nJunio, can you apply the patch below to maint?\n\n-- >8 --\nSubject: [PATCH] diff: fix textconv error zombies\n\nTo make the code simpler, run_textconv lumps all of its\nerror checking into one conditional. However, the\nshort-circuit means that an error in reading will prevent us\nfrom calling finish_command, leaving a zombie child.\n\nThe cleanup requirements are actually different for each of\nthe three error checks, so let's just write them out\nlonghand.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nYes, there are clever ways to make this shorter, but I think it is\nclearer just written out.\n\n diff.c |   16 +++++++++++++---\n 1 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0d465fa..c268cfc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3875,9 +3875,19 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \tchild.use_shell = 1;\n \tchild.argv = argv;\n \tchild.out = -1;\n-\tif (start_command(&child) != 0 ||\n-\t    strbuf_read(&buf, child.out, 0) < 0 ||\n-\t    finish_command(&child) != 0) {\n+\tif (start_command(&child) != 0) {\n+\t\tremove_tempfile();\n+\t\terror(\"error running textconv command '%s'\", pgm);\n+\t\treturn NULL;\n+\t}\n+\tif (strbuf_read(&buf, child.out, 0) < 0) {\n+\t\tclose(child.out);\n+\t\tfinish_command(&child);\n+\t\tremove_tempfile();\n+\t\terror(\"error reading from textconv command '%s'\", pgm);\n+\t\treturn NULL;\n+\t}\n+\tif (finish_command(&child) != 0) {\n \t\tclose(child.out);\n \t\tstrbuf_release(&buf);\n \t\tremove_tempfile();\n-- \n1.7.0.3.460.g6f052\n"},{"id":"138192","messageId":"20100330170748.GD17763@coredump.intra.peff.net","threadId":"23214","inReplyTo":"7vtyryxy8r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] fast textconv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-30T17:07:48Z","receivedAt":"2010-03-30T17:07:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 29, 2010 at 08:52:04PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The speedup is purely from caching; I am not using the \"we only need to\n> > read the first part of the file\" optimization.\n> \n> This made me wonder if the end result might be easier to use if the\n> interface does not change \"textconv\", but adds some property to the\n> filter, i.e. \"the output from this filter is stable and it is safe to\n> reuse a cached conversion result for a given blob object\", boolean.  E.g.\n> \n>         [diff \"jpg\"]\n>                 textconv = exif\n>                 textconv_stable = true\n> \n> and let the calling side handle the caching.  I further suspect that\n> an unstable textconv filter would be an anomaly, so this could even be on\n> by default.\n\nYep, that is the conclusion I came to later in the thread. I am tempted\nnot to even make it configurable, but it is a space-time tradeoff, so I\ncan see how certain corner cases would want to disable.\n\nSo consider \"fasttextconv\" (and this series) scrapped, and I'll work on\na new series that does internal caching via notes.\n\n> If we do so, stock conversion filters people have accumulated in the past\n> could be sped up without any additional change from the end user's side.\n> \n> I guess that I am suggesting to postpone the potential speed-up that could\n> come from being able to inspect the header information as a separate\n> topic.  Besides, some file format has metadata at the end, which won't\n> help you.\n\nYes, I agree that the \"header only\" speed-up is not worth pursuing at\nthis point. With caching, actually looking at the file at all becomes\nthe slow path, and you just don't care as much. It might help if you\nhave 2G files or something where even just spooling the file the first\ntime is awful, but I will leave that as a later itch to be scratched if\npeople want.\n\nFor metadata at the end (or other spots), probably it would need to be\naccompanied by an extension to cat-file to peek at random positions\nwithin the file.\n\n> About the caching scheme; to help invalidating the cache, it probably is a\n> good idea to use not just the blob object name but also at least the name\n> (command line) of the textconv filter as the key for the caching layer.\n> \n> Instead of the \"textconv_stable\" boolean depicted above, you could add a\n> \"textconv_filter_version\" variable there, compute a hash over blob object\n> name, textconv filter name and textconv filter version, and use that as\n> the key to look into the cache (filters lacking textconv_filter_version\n> would then get no caching, and if you update your \"exif\" program you bump\n> the \"textconv_filter_version\" variable).\n\nI was thinking of having a separate notes tree for each textconv helper.\nMost projects will only have a small number, so the extra tree setup\ncost is negligible. And because we know which helper we are using, we\ncan look in each tree separately, which means you may not even need to\nload cache entries at all for filetypes which are not being diffed.\n\nFor cache validity, I'm planning to store a special \"validity\" entry\n(either keyed by the null sha1 in a cache tree, or to wrap the tree in a\nparentless commit with that information in the comment field). The entry\nwould contain the command line used to run the helper. If it matches,\nthe cache is valid. If not, we clear the notes tree.\n\nI think this ends up being slightly simpler for the user to manage the\ncache, and it has better cache growth characteristics.\n\nIn both schemes, git will automatically correctly handle:\n\n  $ git config diff.foo.textconv foo-helper\n  $ git show\n  $ git config diff.foo.textconv \"foo-helper --options\"\n  $ git show\n\nIn my case, by clearing refs/notes/textconv/foo, and in your case by\nstoring under a different key sha1. I would argue my scheme is better\nhere, because it actually _throws away_ the now useless cache entries.\nSo the cache is automatically pruned (and if you really want to go back\nin time to salvage an old cache, you can do it via the reflog).\n\nThey will also both automatically handle:\n\n  $ echo '*.jpg diff=foo' >.gitattributes\n  $ git show\n  $ echo '*.jpg diff=bar' >.gitattributes\n  $ git show\n\nIn my case, by using a totally different tree. In your case, again a\ndifferent key sha1. Which means I'm now leaving the old \"foo\" tree in\nplace with stale entries for '*.jpg' files. But this is the same data\nwastage as with your scheme. In both cases, you can clean things out by\nblowing away the cache. But in my scheme, you are allowed to blow away\nonly the \"foo\" cache. The cache data for other helpers isn't precious,\nobviously, but it does make things slower.\n\nNeither scheme will automatically handle the semantics of a helper\nchanging (e.g., upgrade of the \"foo\" helper). In my scheme, the solution\nis to clear the \"foo\" cache (git update-ref -d refs/notes/textconv/foo).\nIn yours, it would be to update the textconv_filter_version field in the\nconfig. I would argue that mine is conceptually simpler for the user.\nIt's a cache. When a cache is in a broken or invalid state, you clear it\nand start again.\n\nYou could try to get very fancy and include the stat() data for the\nhelper script in the cache validity information to detect version\nchanges, but I doubt that it's worth it. It's much more complex (we run\nhelpers with the shell, so it may not be a single program), and it still\nisn't foolproof (your environment, dynamic libraries, or other factors\nmay be what's changing the answer).\n\nI'll see if I can work up some patches in the next day or two.\n\n-Peff\n"},{"id":"138197","messageId":"201003301936.03394.j6t@kdbg.org","threadId":"23214","inReplyTo":"20100330163004.GC17763@coredump.intra.peff.net","subject":"[PATCH] diff: fix textconv error zombies","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-30T17:36:03Z","receivedAt":"2010-03-30T17:36:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"To make the code simpler, run_textconv lumps all of its\nerror checking into one conditional. However, the\nshort-circuit means that an error in reading will prevent us\nfrom calling finish_command, leaving a zombie child.\nClean up properly after errors.\n\nBased-on-work-by: Jeff King <peff@peff.net>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nJeff,\n\n> Yes, there are clever ways to make this shorter, but I think it is\n> clearer just written out.\n\nThanks for your work, but I'm worried that in your version the close()\ncall is not before the finish_command (but that's really not _that_\nimportant in this case). My version is perhaps not as easy to read, but\nhas a slightly better diff-stat.\n\nOh, I removed the error messages after start_command and finish_command,\nbecause these two print one themselves; Junio, if you disagree with\nthis change, ditch my version and take Jeff's.\n\n-- Hannes\n\n diff.c |   17 +++++++++++------\n 1 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex dfdfa1a..b96faea 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3874,6 +3874,7 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \tconst char **arg = argv;\n \tstruct child_process child;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tint err = 0;\n \n \ttemp = prepare_temp_file(spec->path, spec);\n \t*arg++ = pgm;\n@@ -3884,16 +3885,20 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \tchild.use_shell = 1;\n \tchild.argv = argv;\n \tchild.out = -1;\n-\tif (start_command(&child) != 0 ||\n-\t    strbuf_read(&buf, child.out, 0) < 0 ||\n-\t    finish_command(&child) != 0) {\n-\t\tclose(child.out);\n-\t\tstrbuf_release(&buf);\n+\tif (start_command(&child)) {\n \t\tremove_tempfile();\n-\t\terror(\"error running textconv command '%s'\", pgm);\n \t\treturn NULL;\n \t}\n+\n+\tif (strbuf_read(&buf, child.out, 0) < 0)\n+\t\terr = error(\"error reading from textconv command '%s'\", pgm);\n \tclose(child.out);\n+\n+\tif (finish_command(&child) || err) {\n+\t\tstrbuf_release(&buf);\n+\t\tremove_tempfile();\n+\t\treturn NULL;\n+\t}\n \tremove_tempfile();\n \n \treturn strbuf_detach(&buf, outsize);\n-- \n1.7.0.2.250.ge5578\n"},{"id":"138220","messageId":"7vk4stv5y5.fsf@alter.siamese.dyndns.org","threadId":"23214","inReplyTo":"201003301936.03394.j6t@kdbg.org","subject":"Re: [PATCH] diff: fix textconv error zombies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-30T21:46:10Z","receivedAt":"2010-03-30T21:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Thanks for your work, but I'm worried that in your version the close()\n> call is not before the finish_command (but that's really not _that_\n> important in this case).\n\nAre you referring to this part in api-run-command.txt?\n\n    This describes the arguments, redirections, and environment of a\n    command to run in a sub-process.\n\n    The caller:\n\n    1. allocates and clears (memset(&chld, 0, sizeof(chld));) a\n       struct child_process variable;\n    2. initializes the members;\n    3. calls start_command();\n    4. processes the data;\n    5. closes file descriptors (if necessary; see below);\n    6. calls finish_command().\n\nIn \"if necessary; see below\", I tried to find something to help me judge\nwhy you said \"That's really not _that_ important in this case\", but I\nthink I failed.  Perhaps we would want a bit more detailed discussion in\nthe document there?\n\nThe patch looks fine.  Thanks; will queue this one.\n"},{"id":"138223","messageId":"201003310017.24979.j6t@kdbg.org","threadId":"23214","inReplyTo":"7vk4stv5y5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: fix textconv error zombies","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-03-30T22:17:24Z","receivedAt":"2010-03-30T22:17:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 30. März 2010, Junio C Hamano wrote:\n> In \"if necessary; see below\", I tried to find something to help me judge\n> why you said \"That's really not _that_ important in this case\", but I\n> think I failed.  Perhaps we would want a bit more detailed discussion in\n> the document there?\n\nThe \"if necessary; see below\" refers to the cases where the caller requests a \nfile descriptor from start_command(). It is safe to follow the recipe. But I \nactually would not want to expand on the special case like we have in \nrun_textconv, where it is not strictly necessary to close the fd before \nfinish_command(), namely when we know that the writer will exit after it has \nclosed its end of the pipe and the reader reads until EOF.\n\nThis could deadlock in the theoretical case that the read() in the parent \nreturns an error even though the child is still writing data; the child would \nhang while waiting for the pipe being drained by the parent; but the parent \nwould wait for the child to exit, which never happens. Doing the close() \nbefore finish_command() avoids this situation because the child dies of \nSIGPIPE.\n\nBut since the communication is via a pipe, and read failures are highly \nunlikely (and a read failure in a way that the writer does not fail, too, is \neven more unlikely, I think), the close() before finish_command() is \"not \nimportant\".\n\n-- Hannes\n"},{"id":"138229","messageId":"20100330225628.GB608@coredump.intra.peff.net","threadId":"23214","inReplyTo":"201003310017.24979.j6t@kdbg.org","subject":"Re: [PATCH] diff: fix textconv error zombies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-03-30T22:56:29Z","receivedAt":"2010-03-30T22:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 31, 2010 at 12:17:24AM +0200, Johannes Sixt wrote:\n\n> This could deadlock in the theoretical case that the read() in the parent \n> returns an error even though the child is still writing data; the child would \n> hang while waiting for the pipe being drained by the parent; but the parent \n> would wait for the child to exit, which never happens. Doing the close() \n> before finish_command() avoids this situation because the child dies of \n> SIGPIPE.\n\nYeah, I actually thought about that while writing the error case for\nstrbuf_read (I do close(child.out) before finish_command). But I didn't\nconsider it in the success case. I don't think it matters here because\nwe will already have gotten EOF from child.out, which should mean the\nchild is no longer writing and will not block.\n\nSo I am not sure there is any deadlock, but I am fine with your version\nof the patch.\n\n-Peff\n"}]}