{"thread":{"id":"53814","subject":"git grep --threads 12 --textconv is effectively single-threaded","startedAt":"2020-07-07T21:25:17Z","lastAt":"2020-07-10T16:43:21Z","messageCount":7,"participants":["Zach Riggle","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401155","messageId":"CAMP9c5nUteg_HouuYJZtq7g4MrSE638mns=HeKhNpNTYgQB4=w@mail.gmail.com","threadId":"53814","inReplyTo":null,"subject":"git grep --threads 12 --textconv is effectively single-threaded","fromName":"Zach Riggle","fromEmail":"zachriggle@gmail.com","sentAt":"2020-07-07T21:25:01Z","receivedAt":"2020-07-07T21:25:17Z","isPatch":false,"sender":{"key":"zachriggle@gmail.com","avatar":"https://gravatar.com/avatar/3eb62c0b142900b8c316fa0dcbf6f84079eb83e0d6fd6db79b44d6034c147604?d=mp&s=160"},"body":"It looks like the bit of code that is responsible for performing\ntextconv conversions is single-threaded, even if git-grep is provided\na number of threads to use.\n\nThis greatly limits the usability of textconv filters for grep\nexpressions over a large number of files.\n\nNote the difference in total CPU usage in the following expressions:\n\n$ git grep --threads 12 -e foobar --and -e fizzbuzz &> /dev/null\n0.24s user 0.28s system 710% cpu 0.073 total\n\n$ git grep --threads 12 -e foobar --and -e fizzbuzz --textconv &> /dev/null\n0.90s user 1.75s system 110% cpu 2.390 total\n\n$ git version\ngit version 2.27.0\n\nZach Riggle\n"},{"id":"401159","messageId":"20200707215951.GB2300296@coredump.intra.peff.net","threadId":"53814","inReplyTo":"CAMP9c5nUteg_HouuYJZtq7g4MrSE638mns=HeKhNpNTYgQB4=w@mail.gmail.com","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-07T21:59:51Z","receivedAt":"2020-07-07T21:59:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2020 at 04:25:01PM -0500, Zach Riggle wrote:\n\n> It looks like the bit of code that is responsible for performing\n> textconv conversions is single-threaded, even if git-grep is provided\n> a number of threads to use.\n\nYes, the locking is much coarser than it could be. The issue is in\ngrep.c's fill_textconv_grep():\n\n /*\n * fill_textconv is not remotely thread-safe; it modifies the global\n * diff tempfile structure, writes to the_repo's odb and might\n * internally call thread-unsafe functions such as the\n * prepare_packed_git() lazy-initializator. Because of the last two, we\n * must ensure mutual exclusion between this call and the object reading\n * API, thus we use obj_read_lock() here.\n *\n * TODO: allowing text conversion to run in parallel with object\n * reading operations might increase performance in the multithreaded\n * non-worktreee git-grep with --textconv.\n */\n obj_read_lock();\n size = fill_textconv(r, driver, df, &buf);\n obj_read_unlock();\n free_filespec(df);\n\nNote that this lock is used whether we're doing textconv's or not (i.e.,\nit also excludes reading two objects from the object database at the\nsame time, because none of that code is thread-safe). But the latency\nwhen we're doing a textconv is _much_ higher, because it's shelling out\nto a separate process and reading/writing the contents. Note the\nmuch-higher system CPU in your second timing:\n\n> Note the difference in total CPU usage in the following expressions:\n> \n> $ git grep --threads 12 -e foobar --and -e fizzbuzz &> /dev/null\n> 0.24s user 0.28s system 710% cpu 0.073 total\n> \n> $ git grep --threads 12 -e foobar --and -e fizzbuzz --textconv &> /dev/null\n> 0.90s user 1.75s system 110% cpu 2.390 total\n\nSo I think implementing that TODO would help a lot (because each\ntextconv could in theory proceed in parallel).\n\nAs workaround in the meantime, I suspect that enabling\ndiff..cachetextconv for your particular textconv config might\nhelp. It would be slow on the first run, but then we'd be able to skip\nthe external process entirely for subsequent runs (the results are\ncached in a git-notes tree, which are just raw object reads).\n\n-Peff\n"},{"id":"401192","messageId":"CAMP9c5mpJ9_HvEBTiEQj=vocTdH6N9uXkpLKiE8+hFbAt9p5Ow@mail.gmail.com","threadId":"53814","inReplyTo":"20200707215951.GB2300296@coredump.intra.peff.net","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Zach Riggle","fromEmail":"zachriggle@gmail.com","sentAt":"2020-07-08T17:40:49Z","receivedAt":"2020-07-08T17:41:03Z","isPatch":false,"sender":{"key":"zachriggle@gmail.com","avatar":"https://gravatar.com/avatar/3eb62c0b142900b8c316fa0dcbf6f84079eb83e0d6fd6db79b44d6034c147604?d=mp&s=160"},"body":"So I just need to\n\ngit -c \"diff.c.cachetextconv=true\" ...\n\nAnd the cache should automagically work?\n\nZach Riggle\n\nOn Tue, Jul 7, 2020 at 4:59 PM Jeff King wrote:\n>\n> On Tue, Jul 07, 2020 at 04:25:01PM -0500, Zach Riggle wrote:\n>\n> > It looks like the bit of code that is responsible for performing\n> > textconv conversions is single-threaded, even if git-grep is provided\n> > a number of threads to use.\n>\n> Yes, the locking is much coarser than it could be. The issue is in\n> grep.c's fill_textconv_grep():\n>\n> /*\n> * fill_textconv is not remotely thread-safe; it modifies the global\n> * diff tempfile structure, writes to the_repo's odb and might\n> * internally call thread-unsafe functions such as the\n> * prepare_packed_git() lazy-initializator. Because of the last two, we\n> * must ensure mutual exclusion between this call and the object reading\n> * API, thus we use obj_read_lock() here.\n> *\n> * TODO: allowing text conversion to run in parallel with object\n> * reading operations might increase performance in the multithreaded\n> * non-worktreee git-grep with --textconv.\n> */\n> obj_read_lock();\n> size = fill_textconv(r, driver, df, &buf);\n> obj_read_unlock();\n> free_filespec(df);\n>\n> Note that this lock is used whether we're doing textconv's or not (i.e.,\n> it also excludes reading two objects from the object database at the\n> same time, because none of that code is thread-safe). But the latency\n> when we're doing a textconv is _much_ higher, because it's shelling out\n> to a separate process and reading/writing the contents. Note the\n> much-higher system CPU in your second timing:\n>\n> > Note the difference in total CPU usage in the following expressions:\n> >\n> > $ git grep --threads 12 -e foobar --and -e fizzbuzz &> /dev/null\n> > 0.24s user 0.28s system 710% cpu 0.073 total\n> >\n> > $ git grep --threads 12 -e foobar --and -e fizzbuzz --textconv &> /dev/null\n> > 0.90s user 1.75s system 110% cpu 2.390 total\n>\n> So I think implementing that TODO would help a lot (because each\n> textconv could in theory proceed in parallel).\n>\n> As workaround in the meantime, I suspect that enabling\n> diff..cachetextconv for your particular textconv config might\n> help. It would be slow on the first run, but then we'd be able to skip\n> the external process entirely for subsequent runs (the results are\n> cached in a git-notes tree, which are just raw object reads).\n>\n> -Peff\n"},{"id":"401195","messageId":"20200708201353.GA2354599@coredump.intra.peff.net","threadId":"53814","inReplyTo":"CAMP9c5mpJ9_HvEBTiEQj=vocTdH6N9uXkpLKiE8+hFbAt9p5Ow@mail.gmail.com","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-08T20:13:53Z","receivedAt":"2020-07-08T20:13:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2020 at 12:40:49PM -0500, Zach Riggle wrote:\n\n> So I just need to\n> \n> git -c \"diff.c.cachetextconv=true\" ...\n> \n> And the cache should automagically work?\n\nYes, but there's a bit of subtlety with what you're grepping.\n\nTry this example setup:\n\n git init -q repo\n cd repo\n echo content >file\n echo 'file diff=foo' >.gitattributes\n git add .\n git commit -m base\n git config diff.foo.textconv 'echo >&2 converting...; sleep 1; tr a-z A-Z <'\n\nwhich should make it clear when the filter actually runs.\n\nNow try this:\n\n echo >&2 \"==> no textconv\"\n git grep . file\n echo >&2 \"==> with textconv\"\n git grep --textconv . file\n echo >&2 \"==> cached (one)\"\n git -c diff.foo.cachetextconv=true grep --textconv . file\n echo >&2 \"==> cached (two)\"\n git -c diff.foo.cachetextconv=true grep --textconv . file\n\nWe'd expect the final one to say \"file:CONTENT\" but _without_ a\n\"converting\" line. But it still runs the textconv script!\n\nThe problem is that the cache uses the blob id of the source as its key.\nBut since we're grepping files in the working directory, they don't have\nan object id at all. If we grep in the tree, it works as expected:\n\n $ git -c diff.foo.cachetextconv=true grep --textconv . HEAD -- file\n converting...\n HEAD:file:CONTENT\n $ git -c diff.foo.cachetextconv=true grep --textconv . HEAD -- file\n HEAD:file:CONTENT\n\nAll of this textconv stuff was originally designed for the diff\ninfrastructure. Even when it's diffing files, if they're tracked in the\nrepository the diff code will pull the oid from the index (assuming the\nfile is stat-clean). But the grep code doesn't do that (and I doubt that\nit matters for anything at all except this textconv caching feature).\n\nIt's probably possible to teach the grep code to do the same\ncheck-in-the-index trick, but I'm not sure how complicated it would be.\n\n-Peff\n"},{"id":"401198","messageId":"xmqqfta11zu0.fsf@gitster.c.googlers.com","threadId":"53814","inReplyTo":"20200708201353.GA2354599@coredump.intra.peff.net","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-08T21:06:31Z","receivedAt":"2020-07-08T21:06:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's probably possible to teach the grep code to do the same\n> check-in-the-index trick, but I'm not sure how complicated it would be.\n\nI am not sure if we should even depend on the \"check the object\ndatabase and use it instead of reading the working tree files\" done\nin diff code---somehow I thought we did the opposite for performance\n(i.e. when we ought to be comparing two objects, taken from tree and\nthe index, if we notice that the index side is stat clean, we can\nread/mmap the working tree file instead of going to the object layer\nand deflating a loose object, or, worse yet, construct the blob by\nrepeatedly applying deltas on a base object in a packfile).\n\nIs this one in the opposite direction done specifically for gaining\nperformance when textconv cache is in use? If so, kudos to whoever\ndid it---that sounds like a clever thing to do.\n\n\n"},{"id":"401276","messageId":"20200709231030.GD664420@coredump.intra.peff.net","threadId":"53814","inReplyTo":"xmqqfta11zu0.fsf@gitster.c.googlers.com","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-09T23:10:30Z","receivedAt":"2020-07-09T23:10:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2020 at 02:06:31PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It's probably possible to teach the grep code to do the same\n> > check-in-the-index trick, but I'm not sure how complicated it would be.\n> \n> I am not sure if we should even depend on the \"check the object\n> database and use it instead of reading the working tree files\" done\n> in diff code---somehow I thought we did the opposite for performance\n> (i.e. when we ought to be comparing two objects, taken from tree and\n> the index, if we notice that the index side is stat clean, we can\n> read/mmap the working tree file instead of going to the object layer\n> and deflating a loose object, or, worse yet, construct the blob by\n> repeatedly applying deltas on a base object in a packfile).\n> \n> Is this one in the opposite direction done specifically for gaining\n> performance when textconv cache is in use? If so, kudos to whoever\n> did it---that sounds like a clever thing to do.\n\nNo, it turns out that nobody was that clever (and I was simply\nmisremembering how it worked).\n\nFor a tree-to-tree or index-to-tree comparison, both sides will have an\noid and can use the textconv cache. Even for an index case where we\nmight choose to use a stat-fresh working tree file as an optimization,\nwe'll still consult the textconv cache before loading those contents.\n\nBut for diffing a file in the working tree, we'll never have an oid and\nwill always run the textconv command). So \"git diff\" against the index,\nfor example, would run _one_ textconv (using the cached value for the\nindex, and running one for the working tree version). And we know that\nisn't that interesting for optimizing, since by definition the file is\nstat-dirty in that case (or else we'd skip the content-level comparison\nentirely). So you'd have to compute the sha1 of the working tree file\nfrom scratch. Plus the lifetime of a working tree file's entry in the\ntextconv cache is probably smaller, since it hasn't even been committed\nyet.\n\nI don't think I ever noticed because the primary thing I was trying to\nspeed up with the textconv cache is \"git log -p\", etc, which always has\nan oid to work with.\n\nBut \"grep\" is a totally different story. It is frequently looking at all\nof the stat-fresh working tree files.\n\n-Peff\n"},{"id":"401349","messageId":"xmqq5zavuxr0.fsf@gitster.c.googlers.com","threadId":"53814","inReplyTo":"20200709231030.GD664420@coredump.intra.peff.net","subject":"Re: git grep --threads 12 --textconv is effectively single-threaded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-10T16:43:15Z","receivedAt":"2020-07-10T16:43:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For a tree-to-tree or index-to-tree comparison, both sides will have an\n> oid and can use the textconv cache. Even for an index case where we\n> might choose to use a stat-fresh working tree file as an optimization,\n> we'll still consult the textconv cache before loading those contents.\n\nOK, that \"we'll still consult\" part does sound like sort-of\n\"clever\", but after thinking about the whole sentence twice,\nI realize the actual cleverness lies in the reuse of stat-fresh\nworking tree file, not the use of textconv cache ;-) The cache\nis just doing its normal thing: if we know the oid, look up the\ncached one.\n\n> But for diffing a file in the working tree, we'll never have an oid and\n> will always run the textconv command).\n\nOK. In such a case, we need to run the clean filter on the working\ntree contents, and then finally we need to run the textconv on the\nresult. We could internally hash the result of applying the clean\nfilter to see if we have the blob in the object database and use it\nas the look-up key in the textconv cache, but we are talking about\nthe working tree files, which by definition is more fluid than what\nis in the index, which is likely more fluid than what is already\ncommitted, so the chance of finding a hit may be slim.\n\nWe could still see if the oid in the index is correct with the stat\ncheck, but by definition, diff-files won't compare between a cache\nentry and a working tree file if the path is stat-clean, so that\ndoes not help all that much. I wonder if diff-index comparing a\ntree and the working tree (i.e. without \"--cached\") can be improved,\nthough.\n\n> So \"git diff\" against the index,\n> for example, would run _one_ textconv (using the cached value for the\n> index, and running one for the working tree version). And we know that\n> isn't that interesting for optimizing, since by definition the file is\n> stat-dirty in that case (or else we'd skip the content-level comparison\n> entirely).\n\nYup, we reached the same conclusion here ;-)\n\n> So you'd have to compute the sha1 of the working tree file\n> from scratch. Plus the lifetime of a working tree file's entry in the\n> textconv cache is probably smaller, since it hasn't even been committed\n> yet.\n\nYes again.\n\n> I don't think I ever noticed because the primary thing I was trying to\n> speed up with the textconv cache is \"git log -p\", etc, which always has\n> an oid to work with.\n\nAbsolutely.\n\n> But \"grep\" is a totally different story. It is frequently looking at all\n> of the stat-fresh working tree files.\n\nYeah. Grepping in the working tree files could be optimized with\nthe same technique that would be used to optimize diff-index without\n\"--cached\". When we look at the working tree file, we consult the\nindex and possibly learn its object name if the path is stat-clean,\nprobably if and only if textconv is in use.\n\n\"git grep\" would divert to the \"grep in a blob object\" codepath from\nthere, and \"git diff-index\" would make clever use of oid_valid bit\nin diff_filespec when running textconv (we might need a separate\nbit, though; I haven't thought it through).\n\nOr something like that.\n\n"}]}