{"thread":{"id":"42903","subject":"[PATCH v1] convert: add test to demonstrate clean invocations","startedAt":"2016-07-21T20:59:19Z","lastAt":"2016-07-22T19:30:43Z","messageCount":6,"participants":["larsxschneider@gmail.com","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291935","messageId":"1469134747-26785-1-git-send-email-larsxschneider@gmail.com","threadId":"42903","inReplyTo":null,"subject":"[PATCH v1] convert: add test to demonstrate clean invocations","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-21T20:59:07Z","receivedAt":"2016-07-21T20:59:19Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nIf three files processed by a filter are added and committed to a\nrepository then I expect three clean invocations per Git operation.\nApparently Git invokes the clean process 12 times.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n\nIt's pretty late here and I hope do not miss something obvious... but\ncan anyone explain to me why the clean operation is executed 12 times\nfor 3 files?\n\nThanks,\nLars\n\n\n t/t0021-conversion.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7bac2bc..ab3d299 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -12,6 +12,12 @@ tr \\\n EOF\n chmod +x rot13.sh\n\n+cat <<EOF >run.sh\n+#!$SHELL_PATH\n+\techo \".\" >> run.count\n+EOF\n+chmod +x run.sh\n+\n test_expect_success setup '\n \tgit config filter.rot13.smudge ./rot13.sh &&\n \tgit config filter.rot13.clean ./rot13.sh &&\n@@ -268,4 +274,28 @@ test_expect_success 'disable filter with empty override' '\n \ttest_must_be_empty err\n '\n\n+test_expect_failure 'required clean filter count' '\n+\ttest_config_global filter.run_count.clean ./../run.sh &&\n+\ttest_config_global filter.run_count.required true &&\n+\trm -rf repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\n+\t\techo \"*.r filter=run_count\" >.gitattributes &&\n+\n+\t\tcat ../test.o >test.r &&\n+\t\techo \"test22\" >test2.r &&\n+\t\techo \"test333\" >test3.r &&\n+\n+\t\trm -f run.count &&\n+\t\tgit add . &&\n+\t\ttest_line_count = 3 run.count\n+\n+\t\trm -f run.count &&\n+\t\tgit commit . -m \"test commit\" &&\n+\t\ttest_line_count = 3 run.count\n+\t)\n+'\n test_done\n--\n2.5.1\n\n"},{"id":"291938","messageId":"20160721213740.GB4604@sigill.intra.peff.net","threadId":"42903","inReplyTo":"1469134747-26785-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] convert: add test to demonstrate clean invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-21T21:37:40Z","receivedAt":"2016-07-21T21:37:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 21, 2016 at 10:59:07PM +0200, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n> \n> If three files processed by a filter are added and committed to a\n> repository then I expect three clean invocations per Git operation.\n> Apparently Git invokes the clean process 12 times.\n> \n> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n> ---\n> \n> It's pretty late here and I hope do not miss something obvious... but\n> can anyone explain to me why the clean operation is executed 12 times\n> for 3 files?\n\nHmm. If I run this under gdb and backtrace when we hit apply_filter(), I\nsee:\n\n> +\t\trm -f run.count &&\n> +\t\tgit add . &&\n> +\t\ttest_line_count = 3 run.count\n\nThis \"git add\" does invoke apply_filter() 3 times, which I'd expect.\n\n> +\t\trm -f run.count &&\n> +\t\tgit commit . -m \"test commit\" &&\n> +\t\ttest_line_count = 3 run.count\n\nThis invokes apply_filter() six times. So something funny is going on\nalready (I do get 12 dots, so checking apply_filter() seems to only\ncover half the invocations).\n\nThree of them are for re-adding the three files to the index again,\nsince \"git commit .\" is asking us to do so. I'm surprised, though; I\nwould have thought we could avoid doing so just based on the stat\ninformation. Maybe it's a racy-index thing because the files' mtimes are\nlikely to be the same as the index?\n\nIndeed, if I stick a \"sleep 1\" between modifying the files and calling\n\"git add\", then the \"git commit\" invocation drops to only 6 invocations\nof the filter. So that explains half of it (though I'm still confused\nwhy index refreshing requires 6 and not 3; I guess it may be because\n\"git commit .\" works in a separate index, and we first refresh that\nindex, and then refresh the \"real\" index again afterwards, when we could\ntheoretically just copy the entries).\n\nThe next three are to show the final diffstat after the commit\ncompletes. It looks like the \"should we reuse the worktree file?\"\noptimization in diff_populate_filespec() does not take into account\nwhether we will have to convert the contents, and it probably should.\nThe point is that sometimes mmap-ing the file contents is more efficient\nthan zlib inflating the object from disk. But if we have to exec an\nextra process and read the whole object contents into a strbuf, that is\nprobably not a win.\n\nAdding a \"return 0\" at the top of reuse_worktree_file() drops our 6 to\n3 (but obviously it should actually check the attributes).\n\nSo of the 12 invocations:\n\n  - 3 must be for refreshing the index again, because the way the test\n    is written causes us to err on the side of caution when the mtimes\n    are the same (and also means that even if your test is fixed to\n    pass, it would racily fail when the system is under load)\n\n  - 3 are for the double-refresh when \"git commit ---only\" is used (and\n    \"git commit .\" implies \"--only\". Seems like there is room for\n    optimization there.\n\n  - 3 are for the tree-diff reusing the worktree files. This should be\n    dropped.\n\n  - The other 3, I'm not sure.\n\n-Peff\n"},{"id":"291962","messageId":"20160722152753.GA6859@sigill.intra.peff.net","threadId":"42903","inReplyTo":"20160721213740.GB4604@sigill.intra.peff.net","subject":"[PATCH] diff: do not reuse worktree files that need \"clean\" conversion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-22T15:27:53Z","receivedAt":"2016-07-22T15:28:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 21, 2016 at 05:37:40PM -0400, Jeff King wrote:\n\n> The next three are to show the final diffstat after the commit\n> completes. It looks like the \"should we reuse the worktree file?\"\n> optimization in diff_populate_filespec() does not take into account\n> whether we will have to convert the contents, and it probably should.\n> The point is that sometimes mmap-ing the file contents is more efficient\n> than zlib inflating the object from disk. But if we have to exec an\n> extra process and read the whole object contents into a strbuf, that is\n> probably not a win.\n> \n> Adding a \"return 0\" at the top of reuse_worktree_file() drops our 6 to\n> 3 (but obviously it should actually check the attributes).\n\nHere's a patch for that. I put it in t0021, as well, since I couldn't\nfind a more appropriate place. This means it conflicts with your earlier\npatch, but I think it does a better job of addressing the specific area\nit fixes.\n\nWe may want further tests on top as we fix more areas (though as I said\nearlier, I think by avoiding the racy timestamps, your \"commit\" call\ndrops to just a single invocation per file. That still seems like one\nmore than we should need, but it at least matches your original\nexpectation :) ).\n\n-- >8 --\nSubject: diff: do not reuse worktree files that need \"clean\" conversion\n\nWhen accessing a blob for a diff, we may try to reuse file\ncontents in the working tree, under the theory that it is\nfaster to mmap those file contents than it would be to\nextract the content from the object database.\n\nWhen we have to filter those contents, though, that\nassumption does not hold. Even for our internal conversions\nlike CRLF, we have to allocate and fill a new buffer anyway.\nBut much worse, for external clean filters we have to exec\nan arbitrary script, and we have no idea how expensive it\nmay be to run.\n\nSo let's skip this optimization when conversion into git's\n\"clean\" form is required. This applies whenever the\n\"want_file\" flag is false. When it's true, the caller\nactually wants the smudged worktree contents, which the\nreused file by definition already has (in fact, this is a\nkey optimization going the other direction, since reusing\nthe worktree file there lets us skip smudge filters).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                |  7 +++++++\n t/t0021-conversion.sh | 11 +++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 7d03419..b43d3dd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2683,6 +2683,13 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \tif (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))\n \t\treturn 0;\n \n+\t/*\n+\t * Similarly, if we'd have to convert the file contents anyway, that\n+\t * makes the optimization not worthwhile.\n+\t */\n+\tif (!want_file && would_convert_to_git(name))\n+\t\treturn 0;\n+\n \tlen = strlen(name);\n \tpos = cache_name_pos(name, len);\n \tif (pos < 0)\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 7bac2bc..e799e59 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -268,4 +268,15 @@ test_expect_success 'disable filter with empty override' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'diff does not reuse worktree files that need cleaning' '\n+\ttest_config filter.counter.clean \"echo . >>count; sed s/^/clean:/\" &&\n+\techo \"file filter=counter\" >.gitattributes &&\n+\ttest_commit one file &&\n+\ttest_commit two file &&\n+\n+\t>count &&\n+\tgit diff-tree -p HEAD &&\n+\ttest_line_count = 0 count\n+'\n+\n test_done\n-- \n2.9.2.506.g8452fe7\n\n"},{"id":"291986","messageId":"xmqq60rxbmaf.fsf@gitster.mtv.corp.google.com","threadId":"42903","inReplyTo":"20160722152753.GA6859@sigill.intra.peff.net","subject":"Re: [PATCH] diff: do not reuse worktree files that need \"clean\" conversion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T17:44:08Z","receivedAt":"2016-07-22T17:44:16Z","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> Subject: diff: do not reuse worktree files that need \"clean\" conversion\n>\n> When accessing a blob for a diff, we may try to reuse file\n> contents in the working tree, under the theory that it is\n> faster to mmap those file contents than it would be to\n> extract the content from the object database.\n>\n> When we have to filter those contents, though, that\n> assumption does not hold. Even for our internal conversions\n> like CRLF, we have to allocate and fill a new buffer anyway.\n> But much worse, for external clean filters we have to exec\n> an arbitrary script, and we have no idea how expensive it\n> may be to run.\n>\n> So let's skip this optimization when conversion into git's\n> \"clean\" form is required. This applies whenever the\n> \"want_file\" flag is false. When it's true, the caller\n> actually wants the smudged worktree contents, which the\n> reused file by definition already has (in fact, this is a\n> key optimization going the other direction, since reusing\n> the worktree file there lets us skip smudge filters).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  diff.c                |  7 +++++++\n>  t/t0021-conversion.sh | 11 +++++++++++\n>  2 files changed, 18 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 7d03419..b43d3dd 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2683,6 +2683,13 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n>  \tif (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))\n>  \t\treturn 0;\n>  \n> +\t/*\n> +\t * Similarly, if we'd have to convert the file contents anyway, that\n> +\t * makes the optimization not worthwhile.\n> +\t */\n> +\tif (!want_file && would_convert_to_git(name))\n> +\t\treturn 0;\n\nThe would_convert_to_git() function is not a free operation.  It\nneeds to prime the attribute stack, so it needs to open/read/parse a\nfew files ($GIT_DIR/info/attributes and .gitattributes at least, and\nmore if your directory hierarchy is deep) on the filesystem.  The\ncost is amortized across paths, but we do not even enable the\noptimization if we have to pay the cost of reading the index\nourselves.\n\nI suspect that we may be better off disabling this optimization if\nwe need to always call the helper.\n\n>  \tlen = strlen(name);\n>  \tpos = cache_name_pos(name, len);\n>  \tif (pos < 0)\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index 7bac2bc..e799e59 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -268,4 +268,15 @@ test_expect_success 'disable filter with empty override' '\n>  \ttest_must_be_empty err\n>  '\n>  \n> +test_expect_success 'diff does not reuse worktree files that need cleaning' '\n> +\ttest_config filter.counter.clean \"echo . >>count; sed s/^/clean:/\" &&\n> +\techo \"file filter=counter\" >.gitattributes &&\n> +\ttest_commit one file &&\n> +\ttest_commit two file &&\n> +\n> +\t>count &&\n> +\tgit diff-tree -p HEAD &&\n> +\ttest_line_count = 0 count\n> +'\n> +\n>  test_done\n"},{"id":"291989","messageId":"20160722181024.GA16595@sigill.intra.peff.net","threadId":"42903","inReplyTo":"xmqq60rxbmaf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff: do not reuse worktree files that need \"clean\" conversion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-22T18:10:24Z","receivedAt":"2016-07-22T18:10:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 22, 2016 at 10:44:08AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/diff.c b/diff.c\n> > index 7d03419..b43d3dd 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -2683,6 +2683,13 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n> >  \tif (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))\n> >  \t\treturn 0;\n> >  \n> > +\t/*\n> > +\t * Similarly, if we'd have to convert the file contents anyway, that\n> > +\t * makes the optimization not worthwhile.\n> > +\t */\n> > +\tif (!want_file && would_convert_to_git(name))\n> > +\t\treturn 0;\n> \n> The would_convert_to_git() function is not a free operation.  It\n> needs to prime the attribute stack, so it needs to open/read/parse a\n> few files ($GIT_DIR/info/attributes and .gitattributes at least, and\n> more if your directory hierarchy is deep) on the filesystem.  The\n> cost is amortized across paths, but we do not even enable the\n> optimization if we have to pay the cost of reading the index\n> ourselves.\n\nYeah, I almost commented on that, and its position in the function, but\nforgot to.\n\nThe only code path which will trigger this is diff_populate_filespec.\nAfter reuse_worktree_file() says \"yes, we can reuse\", we drop into a\nconditional that will end in us calling convert_to_git() anyway, which\nwill do the same lookup. We don't need to cache, because the expensive\nparts of the attribute-lookup are already cached for us by the attribute\ncode.\n\nSo my initial thought was to put it at the end of reuse_worktree_file(),\nwhere it would have the least impact. If we find we cannot reuse the\nfile, then we would skip both this new attr lookup and the one in\ndiff_populate_filespec().\n\nBut in practice, I think we'll already have cached those attrs before we\neven hit this function, because we'll hit the userdiff_find_by_path()\ncode earlier in the diff process (e.g., to see if it's binary, if we\nneed to textconv, etc). Those look for different attributes, but I think\nthe expensive bits (finding, opening, reading attribute files) are\ncached across all lookups.\n\nSo I think we actually _can_ think of would_convert_to_git() as\nbasically free. Or as free as other efficient-lookup functions we call\nlike cache_name_pos(). And so I moved it further up in the function,\nwhere it lets us avoid doing more out-of-process work (like calling\nlstat() so we can ce_match_stat() on the result).\n\nPossibly it should go after the cache_name_pos() call, though. That's\nlikely to be less expensive than the actual walk of the attr tree.\n\n> I suspect that we may be better off disabling this optimization if\n> we need to always call the helper.\n\nThe thought \"does this tree reuse even speed things up enough to justify\nits complexity\" definitely crossed my mind. It's basically swapping\nopen/mmap for zlib inflating the content.\n\nBut I do think it helps in the \"want_file\" case (i.e., when we are\nwriting out a tempfile for an external command via prepare_temp_file()).\nThere it helps us omit writing a tempfile to disk entirely, including\nany possible conversion.\n\n-Peff\n"},{"id":"291997","messageId":"xmqqinvxa2sk.fsf@gitster.mtv.corp.google.com","threadId":"42903","inReplyTo":"20160722181024.GA16595@sigill.intra.peff.net","subject":"Re: [PATCH] diff: do not reuse worktree files that need \"clean\" conversion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T19:30:35Z","receivedAt":"2016-07-22T19:30:43Z","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> So I think we actually _can_ think of would_convert_to_git() as\n> basically free. Or as free as other efficient-lookup functions we call\n> like cache_name_pos().\n\nOK.\n\n> The thought \"does this tree reuse even speed things up enough to justify\n> its complexity\" definitely crossed my mind. It's basically swapping\n> open/mmap for zlib inflating the content.\n>\n> But I do think it helps in the \"want_file\" case (i.e., when we are\n> writing out a tempfile for an external command via prepare_temp_file()).\n> There it helps us omit writing a tempfile to disk entirely, including\n> any possible conversion.\n\nOK.\n"}]}