{"thread":{"id":"61401","subject":"Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","startedAt":"2024-05-01T05:27:10Z","lastAt":"2024-06-06T16:02:47Z","messageCount":20,"participants":["Dhruva Krishnamurthy","Jeff King","rsbecker@nexbridge.com","Junio C Hamano","Taylor Blau","Karthik Nayak","John Cai"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"493827","messageId":"CAKOHPAn1btewYTdLYWpW+fOaXMY+JQZsLCQxUSwoUqnnFN_ohA@mail.gmail.com","threadId":"61401","inReplyTo":null,"subject":"Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Dhruva Krishnamurthy","fromEmail":"dhruvakm@gmail.com","sentAt":"2024-05-01T05:26:32Z","receivedAt":"2024-05-01T05:27:10Z","isPatch":false,"sender":{"key":"dhruvakm@gmail.com","avatar":"https://gravatar.com/avatar/96fe022a95b60fd0de7f9f521364d964cdc7c46be5cfef279f8d4379e49e19a6?d=mp&s=160"},"body":"Hello,\nCloning by specifying depth exhibits performance regression in\npack-objects (~20x). The repository I am cloning is on NFS (mounted\nwith NFSv3 & positive lookup cache enabled).\n\nRan under 'perf' command to capture profiling information to see if\nsomething really stands out. There is a significant overhead in calls\nto file open/open64, fstat64 & mmap/munmap in git 2.44 compared to git\n2.42. Not sure if there is an increase in the number of calls or\nsomething more is done.\n\nCould someone please guide me on how to troubleshoot this better?\n\n--- Details of the test environment and the clone commands with output ---\n# There are 10 loose objects under objects/08\n$ git count-objects -vH\ncount: 3627\nsize: 25.84 MiB\nin-pack: 1108374\npacks: 2\nsize-pack: 303.38 MiB\nprune-packable: 0\ngarbage: 0\nsize-garbage: 0 bytes\n\n# Simple driver script to enable performance tracking for upload-pack only\n$ cat trace-git-upload-pack\n#!/usr/bin/env bash\nexport GIT_TRACE_PERFORMANCE=true\nexec git-upload-pack \"$@\"\n\n# git clone with 2.42: pack objects take 17s\n$ /opt/git/bin/git clone --no-checkout --no-local --depth=500\n--upload-pack=$(pwd)/trace-git-upload-pack .. prod\nCloning into 'prod'...\nremote: Enumerating objects: 669941, done.\nremote: Counting objects: 100% (669941/669941), done.\nremote: Compressing objects: 100% (154988/154988), done.\nReceiving objects: 100% (669941/669941), 144.54 MiB | 25.78 MiB/s, done.\nremote: Total 669941 (delta 533745), reused 645666 (delta 512193), pack-reused 0\nremote: 05:35:40.654828 trace.c:414 performance:\n17.098198597 s: git command: git --shallow-file '' pack-objects --revs\n--thin --stdout --shallow --progress --delta-base-offset --include-tag\n 05:35:40.708162 trace.c:414 performance:\n24.764812264 s: git command: git-upload-pack /large_repo/perf/..\nResolving deltas: 100% (533745/533745), done.\nChecking connectivity: 669940, done.\n\n# git clone with 2.44: pack objects take 325s\n$ /opt/gitn/bin/git clone --no-checkout --no-local --depth=500\n--upload-pack=$(pwd)/trace-git-upload-pack .. dev\nCloning into 'dev'...\nremote: Enumerating objects: 669941, done.\nremote: Counting objects: 100% (669941/669941), done.\nremote: Compressing objects: 100% (154988/154988), done.\nReceiving objects: 100% (669941/669941), 144.66 MiB | 29.08 MiB/s, done.\nremote: Total 669941 (delta 533742), reused 645666 (delta 512193),\npack-reused 0 (from 0)\nremote: 05:42:01.017156 trace.c:414 performance:\n325.552424902 s: git command: git --shallow-file '' pack-objects\n--revs --thin --stdout --shallow --progress --delta-base-offset\n--include-tag\n 05:42:01.063013 trace.c:414 performance:\n330.965731114 s: git command: git-upload-pack /large_repo/perf/..\nResolving deltas: 100% (533742/533742), done.\nChecking connectivity: 669940, done.\n\nBest regards,\nDhruva\n"},{"id":"493847","messageId":"20240501220030.GA1442509@coredump.intra.peff.net","threadId":"61401","inReplyTo":"CAKOHPAn1btewYTdLYWpW+fOaXMY+JQZsLCQxUSwoUqnnFN_ohA@mail.gmail.com","subject":"using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-01T22:00:30Z","receivedAt":"2024-05-01T22:00:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2024 at 10:26:32PM -0700, Dhruva Krishnamurthy wrote:\n\n> Cloning by specifying depth exhibits performance regression in\n> pack-objects (~20x). The repository I am cloning is on NFS (mounted\n> with NFSv3 & positive lookup cache enabled).\n> \n> Ran under 'perf' command to capture profiling information to see if\n> something really stands out. There is a significant overhead in calls\n> to file open/open64, fstat64 & mmap/munmap in git 2.44 compared to git\n> 2.42. Not sure if there is an increase in the number of calls or\n> something more is done.\n> \n> Could someone please guide me on how to troubleshoot this better?\n\nOof. I was able to reproduce this regression, and the impact can be\npretty severe. You can reproduce it without NFS and without shallow\nclones. Get a bare clone of something big like the kernel:\n\n git clone --bare https://github.com/torvalds/linux\n cd linux.git\n\nand then do something similar to the server side of a clone:\n\n time git pack-objects --all --stdout /dev/null\n\nMost of the time will be spent in the \"Enumerating objects\" phase,\nwalking the graph (since the repo is fully packed, delta searching and\nwriting are fairly quick).\n\nWith v2.42.0, I get:\n\n real\t1m12.409s\n user\t1m13.021s\n sys\t0m6.091s\n\nWith v2.44.0, I get:\n\n real\t4m12.075s\n user\t4m12.763s\n sys\t0m5.546s\n\nBisecting show the culprit is 2386535511 (attr: read attributes from\nHEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a\nbare repository would only look for attributes in the info/attributes\nfile. But after, we look at the HEAD tree-ish, too. And pack-objects\nwill check the \"delta\" attribute for every path of every object we are\npacking. And remember that in-tree lookups for foo/bar/baz require\nlooking not just for .gitattributes, but also foo/.gitattributes,\nfoo/bar/.gitattributes, and foo/bar/baz/.gitattributes.\n\nIn a non-bare repo, this isn't _too_ bad. We'll try an open() for each\npath, but the common case is that we don't have such a file, so the\ntotal cost is the one syscall per object. But when we're pulling\nattributes from HEAD, each lookup requires walking the whole chain of\ntrees (so for the final one, tree foo points to tree bar points to tree\nbaz, where we see there is no .gitattributes entry). And so all of that\nextra time goes to reading in trees over and over.\n\nYou can repeat the same test with git.git. It gets slower, but it's\nbarely perceptible. This is because we have a relatively shallow tree,\nso most lookups are hitting root-level .gitattributes or maybe one or\ntwo levels deep. The kernel has a much deeper tree, so those lookups are\nmore expensive (and of course there are a lot more of them).\n\nGetting back to shallow clones and NFS:\n\n - the effect is more pronounced on NFS because the cost to access\n objects is higher. With git.git I was even able to see some\n slowdown, probably just from the cost/latency of opening up all of\n those objects.\n\n - the problem doesn't show up if the repo has reachability bitmaps.\n This is because the bitmap result doesn't have the pathnames of each\n object (we do have the \"name hash\", but it's not enough for us to do\n an attr lookup), and so objects we get from a bitmap do not\n respect the delta attribute at all.\n\n But when doing a shallow clone, we have to disable bitmaps and do a\n regular traversal. So even if you have bitmaps, you still run into\n the problem.\n\n The example above should not have bitmaps (we do build them by\n default when repacking bare repos these days, but I don't think\n we'll do so right after cloning). If you have a local repo that\n already has bitmaps, you should be able to see the difference by\n using \"git -c pack.usebitmaps=false pack-objects\".\n\n So even if you are a server which generally enables bitmaps, you can\n still get bit by this for shallow clones, but also for other\n non-bitmap invocations, like say \"git repack -ad\". There I see the\n same 3-minute slowdown in the enumeration phase.\n\nSo what to do? It seems like some kind of caching would help here. We're\nlooking up the same paths over and over, for two reasons:\n\n 1. We'll have many objects with the same paths, one for each time the\n path was modified through history.\n\n 2. Adjacent objects share the higher-level lookups. Both \"dir/a\" and\n \"dir/b\" will need to look up \"dir/.gitattributes\" (and all the way\n up to \".gitattributes\").\n\nSo even something simple and stupid like this:\n\ndiff --git a/attr.c b/attr.c\nindex 679e42258c..b32af9a78b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -24,6 +24,7 @@\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n+#include \"strmap.h\"\n \n const char *git_attr_tree;\n \n@@ -795,25 +796,31 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n \t\t\t\t\t const struct object_id *tree_oid,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n-\tstruct object_id oid;\n-\tunsigned long sz;\n-\tenum object_type type;\n+\tstatic struct strmap cache = STRMAP_INIT;\n+\tvoid *CACHE_MISSING = (void *)1;\n \tvoid *buf;\n-\tunsigned short mode;\n \n \tif (!tree_oid)\n \t\treturn NULL;\n \n-\tif (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))\n-\t\treturn NULL;\n-\n-\tbuf = repo_read_object_file(istate->repo, &oid, &type, &sz);\n-\tif (!buf || type != OBJ_BLOB) {\n-\t\tfree(buf);\n-\t\treturn NULL;\n+\tbuf = strmap_get(&cache, path);\n+\tif (!buf) {\n+\t\tstruct object_id oid;\n+\t\tunsigned short mode;\n+\t\tif (!get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) {\n+\t\t\tunsigned long sz;\n+\t\t\tenum object_type type;\n+\t\t\tbuf = repo_read_object_file(istate->repo, &oid, &type, &sz);\n+\t\t\tif (!buf || type != OBJ_BLOB) {\n+\t\t\t\tfree(buf);\n+\t\t\t\tbuf = NULL;\n+\t\t\t}\n+\t\t}\n+\t\tstrmap_put(&cache, path, buf ? buf : CACHE_MISSING);\n \t}\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\treturn buf == CACHE_MISSING ? NULL :\n+\t\tread_attr_from_buf(buf, path, flags);\n }\n \n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n\nrestores the v2.42 performance. But there are probably better options:\n\n - this is caching whole .gitattributes buffers. In pack-objects we\n only care about a single bit for try_delta. For linux.git it doesn't\n really matter, as 99% of our entries are just CACHE_MISSING and the\n real value is avoiding the negative lookups. But the same problem\n exists to a lesser degree for \"git log -p\" in a bare repo. So I\n think it makes sense to try to solve it in the attr layer.\n\n - the string keys have a lot of duplication. You'll have\n \"foo/.gitattributes\", \"foo/bar/.gitattributes\", and so on. A trie\n structure split by path component would let you store each component\n just once. And perhaps have even faster lookups. I think this is\n roughly the same issue faced by the kernel VFS for doing path\n lookups, so something dentry/dcache-like would help.\n\n I don't know how much it matters in practice, though. The sum of all\n of the paths in HEAD for linux.git is ~3.5MB, which is a rounding\n error on the needs of the rest of the packing process.\n\n - Something dcache-like could also be pushed down lower, to the\n get_tree_entry() API (imagine it quietly caching uncompressed trees\n behind the scenes and then using them to traverse). That depends on\n high locality of requests, though. I don't know if we have that\n here, because we do our lookups in traversal order. So you'd look at\n entries in HEAD^{tree}, then HEAD~1^{tree}, then HEAD~2^{tree}, and\n so on.\n\n - Speaking of locality, the attr code tries to make use of request\n locality in its stack (so if we ask for attributes for \"foo/bar\",\n then \"foo/baz\", we should be able to keep the parsed data for\n \"foo/.gitattributes\" available between them). But our pattern here\n violates that. Again, not that big an issue if you don't have that\n many .gitattributes files in the first place, so it might not be\n worth worrying about. But if we did want to rearrange the lookups to\n exploit locality, it might change the overall caching strategy.\n\n - As noted above, most entries are just CACHE_MISSING. So rather than\n lazily looking up and caching entries, we could just prepopulate the\n cache. And then you know that if an entry isn't present in the\n cache, it does not exist in the tree. The downside is that you pay\n to walk the all of HEAD^{tree}, even if you only have a few lookups\n to do. That's a good tradeoff for pack-objects (which usually ends\n up looking for every path anyway), but not for \"git diff\" (where you\n only care about a few changed paths.\n\n - the cache here is static-local in the function. It should probably\n at least be predicated on the tree_oid, and maybe attached to the\n repository object? I think having one per repository at a time would\n be fine (generally the tree_oid is set once per process, so it's not\n like you're switching between multiple options).\n\nI've cc'd John as the author of 2386535511. But really, that was just\nenabling by default the attr-tree code added by 47cfc9bd7d (attr: add\nflag `--source` to work with tree-ish, 2023-01-14). Although in that\noriginal context (git check-attr) the lack of caching would be much less\nimportant.\n\n-Peff\n"},{"id":"493850","messageId":"0c2501da9c18$1de0c970$59a25c50$@nexbridge.com","threadId":"61401","inReplyTo":"20240501220030.GA1442509@coredump.intra.peff.net","subject":"RE: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-05-01T22:37:09Z","receivedAt":"2024-05-01T22:37:25Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, May 1, 2024 6:01 PM, Jeff King wrote:\n>On Tue, Apr 30, 2024 at 10:26:32PM -0700, Dhruva Krishnamurthy wrote:\n>\n>> Cloning by specifying depth exhibits performance regression in\n>> pack-objects (~20x). The repository I am cloning is on NFS (mounted\n>> with NFSv3 & positive lookup cache enabled).\n>>\n>> Ran under 'perf' command to capture profiling information to see if\n>> something really stands out. There is a significant overhead in calls\n>> to file open/open64, fstat64 & mmap/munmap in git 2.44 compared to git\n>> 2.42. Not sure if there is an increase in the number of calls or\n>> something more is done.\n>>\n>> Could someone please guide me on how to troubleshoot this better?\n>\n>Oof. I was able to reproduce this regression, and the impact can be pretty severe.\n>You can reproduce it without NFS and without shallow clones. Get a bare clone of\n>something big like the kernel:\n>\n> git clone --bare https://github.com/torvalds/linux\n> cd linux.git\n>\n>and then do something similar to the server side of a clone:\n>\n> time git pack-objects --all --stdout /dev/null\n>\n>Most of the time will be spent in the \"Enumerating objects\" phase, walking the\n>graph (since the repo is fully packed, delta searching and writing are fairly quick).\n>\n>With v2.42.0, I get:\n>\n> real\t1m12.409s\n> user\t1m13.021s\n> sys\t0m6.091s\n>\n>With v2.44.0, I get:\n>\n> real\t4m12.075s\n> user\t4m12.763s\n> sys\t0m5.546s\n>\n>Bisecting show the culprit is 2386535511 (attr: read attributes from HEAD when\n>bare repo, 2023-10-13), which is in v2.43.0. Before that, a bare repository would\n>only look for attributes in the info/attributes file. But after, we look at the HEAD\n>tree-ish, too. And pack-objects will check the \"delta\" attribute for every path of\n>every object we are packing. And remember that in-tree lookups for foo/bar/baz\n>require looking not just for .gitattributes, but also foo/.gitattributes,\n>foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.\n>\n>In a non-bare repo, this isn't _too_ bad. We'll try an open() for each path, but the\n>common case is that we don't have such a file, so the total cost is the one syscall per\n>object. But when we're pulling attributes from HEAD, each lookup requires walking\n>the whole chain of trees (so for the final one, tree foo points to tree bar points to\n>tree baz, where we see there is no .gitattributes entry). And so all of that extra time\n>goes to reading in trees over and over.\n>\n>You can repeat the same test with git.git. It gets slower, but it's barely perceptible.\n>This is because we have a relatively shallow tree, so most lookups are hitting root-\n>level .gitattributes or maybe one or two levels deep. The kernel has a much deeper\n>tree, so those lookups are more expensive (and of course there are a lot more of\n>them).\n>\n>Getting back to shallow clones and NFS:\n>\n> - the effect is more pronounced on NFS because the cost to access\n> objects is higher. With git.git I was even able to see some\n> slowdown, probably just from the cost/latency of opening up all of\n> those objects.\n>\n> - the problem doesn't show up if the repo has reachability bitmaps.\n> This is because the bitmap result doesn't have the pathnames of each\n> object (we do have the \"name hash\", but it's not enough for us to do\n> an attr lookup), and so objects we get from a bitmap do not\n> respect the delta attribute at all.\n>\n> But when doing a shallow clone, we have to disable bitmaps and do a\n> regular traversal. So even if you have bitmaps, you still run into\n> the problem.\n>\n> The example above should not have bitmaps (we do build them by\n> default when repacking bare repos these days, but I don't think\n> we'll do so right after cloning). If you have a local repo that\n> already has bitmaps, you should be able to see the difference by\n> using \"git -c pack.usebitmaps=false pack-objects\".\n>\n> So even if you are a server which generally enables bitmaps, you can\n> still get bit by this for shallow clones, but also for other\n> non-bitmap invocations, like say \"git repack -ad\". There I see the\n> same 3-minute slowdown in the enumeration phase.\n>\n>So what to do? It seems like some kind of caching would help here. We're looking\n>up the same paths over and over, for two reasons:\n>\n> 1. We'll have many objects with the same paths, one for each time the\n> path was modified through history.\n>\n> 2. Adjacent objects share the higher-level lookups. Both \"dir/a\" and\n> \"dir/b\" will need to look up \"dir/.gitattributes\" (and all the way\n> up to \".gitattributes\").\n>\n>So even something simple and stupid like this:\n>\n>diff --git a/attr.c b/attr.c\n>index 679e42258c..b32af9a78b 100644\n>--- a/attr.c\n>+++ b/attr.c\n>@@ -24,6 +24,7 @@\n> #include \"thread-utils.h\"\n> #include \"tree-walk.h\"\n> #include \"object-name.h\"\n>+#include \"strmap.h\"\n>\n> const char *git_attr_tree;\n>\n>@@ -795,25 +796,31 @@ static struct attr_stack *read_attr_from_blob(struct\n>index_state *istate,\n> \t\t\t\t\t const struct object_id *tree_oid,\n> \t\t\t\t\t const char *path, unsigned flags) {\n>-\tstruct object_id oid;\n>-\tunsigned long sz;\n>-\tenum object_type type;\n>+\tstatic struct strmap cache = STRMAP_INIT;\n>+\tvoid *CACHE_MISSING = (void *)1;\n> \tvoid *buf;\n>-\tunsigned short mode;\n>\n> \tif (!tree_oid)\n> \t\treturn NULL;\n>\n>-\tif (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))\n>-\t\treturn NULL;\n>-\n>-\tbuf = repo_read_object_file(istate->repo, &oid, &type, &sz);\n>-\tif (!buf || type != OBJ_BLOB) {\n>-\t\tfree(buf);\n>-\t\treturn NULL;\n>+\tbuf = strmap_get(&cache, path);\n>+\tif (!buf) {\n>+\t\tstruct object_id oid;\n>+\t\tunsigned short mode;\n>+\t\tif (!get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) {\n>+\t\t\tunsigned long sz;\n>+\t\t\tenum object_type type;\n>+\t\t\tbuf = repo_read_object_file(istate->repo, &oid, &type,\n>&sz);\n>+\t\t\tif (!buf || type != OBJ_BLOB) {\n>+\t\t\t\tfree(buf);\n>+\t\t\t\tbuf = NULL;\n>+\t\t\t}\n>+\t\t}\n>+\t\tstrmap_put(&cache, path, buf ? buf : CACHE_MISSING);\n> \t}\n>\n>-\treturn read_attr_from_buf(buf, path, flags);\n>+\treturn buf == CACHE_MISSING ? NULL :\n>+\t\tread_attr_from_buf(buf, path, flags);\n> }\n>\n> static struct attr_stack *read_attr_from_index(struct index_state *istate,\n>\n>restores the v2.42 performance. But there are probably better options:\n>\n> - this is caching whole .gitattributes buffers. In pack-objects we\n> only care about a single bit for try_delta. For linux.git it doesn't\n> really matter, as 99% of our entries are just CACHE_MISSING and the\n> real value is avoiding the negative lookups. But the same problem\n> exists to a lesser degree for \"git log -p\" in a bare repo. So I\n> think it makes sense to try to solve it in the attr layer.\n>\n> - the string keys have a lot of duplication. You'll have\n> \"foo/.gitattributes\", \"foo/bar/.gitattributes\", and so on. A trie\n> structure split by path component would let you store each component\n> just once. And perhaps have even faster lookups. I think this is\n> roughly the same issue faced by the kernel VFS for doing path\n> lookups, so something dentry/dcache-like would help.\n>\n> I don't know how much it matters in practice, though. The sum of all\n> of the paths in HEAD for linux.git is ~3.5MB, which is a rounding\n> error on the needs of the rest of the packing process.\n>\n> - Something dcache-like could also be pushed down lower, to the\n> get_tree_entry() API (imagine it quietly caching uncompressed trees\n> behind the scenes and then using them to traverse). That depends on\n> high locality of requests, though. I don't know if we have that\n> here, because we do our lookups in traversal order. So you'd look at\n> entries in HEAD^{tree}, then HEAD~1^{tree}, then HEAD~2^{tree}, and\n> so on.\n>\n> - Speaking of locality, the attr code tries to make use of request\n> locality in its stack (so if we ask for attributes for \"foo/bar\",\n> then \"foo/baz\", we should be able to keep the parsed data for\n> \"foo/.gitattributes\" available between them). But our pattern here\n> violates that. Again, not that big an issue if you don't have that\n> many .gitattributes files in the first place, so it might not be\n> worth worrying about. But if we did want to rearrange the lookups to\n> exploit locality, it might change the overall caching strategy.\n>\n> - As noted above, most entries are just CACHE_MISSING. So rather than\n> lazily looking up and caching entries, we could just prepopulate the\n> cache. And then you know that if an entry isn't present in the\n> cache, it does not exist in the tree. The downside is that you pay\n> to walk the all of HEAD^{tree}, even if you only have a few lookups\n> to do. That's a good tradeoff for pack-objects (which usually ends\n> up looking for every path anyway), but not for \"git diff\" (where you\n> only care about a few changed paths.\n>\n> - the cache here is static-local in the function. It should probably\n> at least be predicated on the tree_oid, and maybe attached to the\n> repository object? I think having one per repository at a time would\n> be fine (generally the tree_oid is set once per process, so it's not\n> like you're switching between multiple options).\n>\n>I've cc'd John as the author of 2386535511. But really, that was just enabling by\n>default the attr-tree code added by 47cfc9bd7d (attr: add flag `--source` to work\n>with tree-ish, 2023-01-14). Although in that original context (git check-attr) the\n>lack of caching would be much less important.\n\nAlthough this is an unbaked thought... \n\nI am not sure this will help, but I have been considering this since the trie data structure was introduced. Should we consider moving to a sparse trie with a lazy load approach? There is no current indication in the trie structure in path.c that a node is incomplete (not as far as I can tell anyway), so the assumption is the trie must be fully populated. Loading tries are generally the same performance as searching - Order(1) - given there are limited numbers of splits compared to a balanced or red-black tree; so a lazy load would not significantly change the trie load time. Each lookup might expand the node population but could cut down times where parts of the trie are ignored. Although this would be a non-trivial change, knowing that the trie is incomplete in a sparse situation might help here.\n\n--Randall\n\n"},{"id":"493851","messageId":"xmqqikzxi2aa.fsf@gitster.g","threadId":"61401","inReplyTo":"20240501220030.GA1442509@coredump.intra.peff.net","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-01T22:40:45Z","receivedAt":"2024-05-01T22:40:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - the cache here is static-local in the function. It should probably\n> at least be predicated on the tree_oid, and maybe attached to the\n> repository object? I think having one per repository at a time would\n> be fine (generally the tree_oid is set once per process, so it's not\n> like you're switching between multiple options).\n\nIt should be per tree_oid or you will get a stale and incorrect\nresult when you read paths from a different tree. But thanks for\nthat \"something simple and stupid\" code to clearly demonstrate\nthat repeated reading of the attributes data is the problem.\n\nGiven a tree with a name, the result of reading a path from that\ntree does not depend on the repository the tree appears in, so the\ncache does not have a reason to be tied to a particular repository.\nGenerally we work only inside a single repository, so attaching the\ncache to that single repository would be a good way to make it\navailable globally without adding another global variable, as\nthe_repository can serve as the starting point for the global state,\nbut other than that there is no reason.\n\nI agree that the attribute layer may be a better place to cache this\ndata. As you pointed out, it already has a caching behaviour in its\nattr_stack data structure that is optimized for local walk that\nvisits every path in a tree in depth first order, but it is likely\nthat a different caching scheme that is more suitable for random\naccess may need to be introduced. The cache eviction strategy may\nneed some thought (the attr_stack based caching has an obviously\noptimal eviction strategy---to evict the attribute data read from a\ndirectory when the traversal leaves that directory) in order to\navoid unbounded bloat of the cached data.\n\n> I've cc'd John as the author of 2386535511. But really, that was just\n> enabling by default the attr-tree code added by 47cfc9bd7d (attr: add\n> flag `--source` to work with tree-ish, 2023-01-14). Although in that\n> original context (git check-attr) the lack of caching would be much less\n> important.\n>\n> -Peff\n"},{"id":"493855","messageId":"ZjLfcCxjLq4o7hpw@nand.local","threadId":"61401","inReplyTo":"20240501220030.GA1442509@coredump.intra.peff.net","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-02T00:33:52Z","receivedAt":"2024-05-02T00:33:58Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 01, 2024 at 06:00:30PM -0400, Jeff King wrote:\n> Bisecting show the culprit is 2386535511 (attr: read attributes from\n> HEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a\n> bare repository would only look for attributes in the info/attributes\n> file. But after, we look at the HEAD tree-ish, too. And pack-objects\n> will check the \"delta\" attribute for every path of every object we are\n> packing. And remember that in-tree lookups for foo/bar/baz require\n> looking not just for .gitattributes, but also foo/.gitattributes,\n> foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.\n\nThanks for the explanation and bisection. I agree that 2386535511 makes\nsense as a likely culprit given what you wrote here.\n\n> - the problem doesn't show up if the repo has reachability bitmaps.\n> This is because the bitmap result doesn't have the pathnames of each\n> object (we do have the \"name hash\", but it's not enough for us to do\n> an attr lookup), and so objects we get from a bitmap do not\n> respect the delta attribute at all.\n>\n> But when doing a shallow clone, we have to disable bitmaps and do a\n> regular traversal. So even if you have bitmaps, you still run into\n> the problem.\n>\n> The example above should not have bitmaps (we do build them by\n> default when repacking bare repos these days, but I don't think\n> we'll do so right after cloning). If you have a local repo that\n> already has bitmaps, you should be able to see the difference by\n> using \"git -c pack.usebitmaps=false pack-objects\".\n\nYikes. I was hoping that bitmaps would be a saving grace here for setups\nthat have bitmap generation enabled, but it makes sense that it doesn't\nhelp if you are doing a shallow clone where you have to disable bitmaps.\n\n> So even if you are a server which generally enables bitmaps, you can\n> still get bit by this for shallow clones, but also for other\n> non-bitmap invocations, like say \"git repack -ad\". There I see the\n> same 3-minute slowdown in the enumeration phase.\n\nThat's also pretty scary, and a worthwhile callout.\n\n> So what to do? It seems like some kind of caching would help here. We're\n> looking up the same paths over and over, for two reasons:\n>\n> 1. We'll have many objects with the same paths, one for each time the\n> path was modified through history.\n>\n> 2. Adjacent objects share the higher-level lookups. Both \"dir/a\" and\n> \"dir/b\" will need to look up \"dir/.gitattributes\" (and all the way\n> up to \".gitattributes\").\n\nRight. I guess you need to cache something like on the order of the set\nof dirnames of all modified paths in the repository (and recursively the\ndirnames of those dirnames up until you get to the root).\n\n> So even something simple and stupid like this:\n\n...makes sense.\n\n> restores the v2.42 performance. But there are probably better options:\n>\n> - this is caching whole .gitattributes buffers. In pack-objects we\n> only care about a single bit for try_delta. For linux.git it doesn't\n> really matter, as 99% of our entries are just CACHE_MISSING and the\n> real value is avoiding the negative lookups. But the same problem\n> exists to a lesser degree for \"git log -p\" in a bare repo. So I\n> think it makes sense to try to solve it in the attr layer.\n>\n> - the string keys have a lot of duplication. You'll have\n> \"foo/.gitattributes\", \"foo/bar/.gitattributes\", and so on. A trie\n> structure split by path component would let you store each component\n> just once. And perhaps have even faster lookups. I think this is\n> roughly the same issue faced by the kernel VFS for doing path\n> lookups, so something dentry/dcache-like would help.\n>\n> I don't know how much it matters in practice, though. The sum of all\n> of the paths in HEAD for linux.git is ~3.5MB, which is a rounding\n> error on the needs of the rest of the packing process.\n\nThis was my gut reaction when I started reading this bullet point, too.\nI have a hard time imagining a repository that would be so large that it\nwould have a lot of unique paths, but not so large that it would be\notherwise cheap to run pack-objects.\n\n> - As noted above, most entries are just CACHE_MISSING. So rather than\n> lazily looking up and caching entries, we could just prepopulate the\n> cache. And then you know that if an entry isn't present in the\n> cache, it does not exist in the tree. The downside is that you pay\n> to walk the all of HEAD^{tree}, even if you only have a few lookups\n> to do. That's a good tradeoff for pack-objects (which usually ends\n> up looking for every path anyway), but not for \"git diff\" (where you\n> only care about a few changed paths.\n\nThat seems reasonable to do.\n\nThanks,\nTaylor\n"},{"id":"493856","messageId":"CAKOHPA=ow5-TVG6FZd8fZRCx_takHyBQxj18WmEzKaFW9-F8nA@mail.gmail.com","threadId":"61401","inReplyTo":"20240501220030.GA1442509@coredump.intra.peff.net","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Dhruva Krishnamurthy","fromEmail":"dhruvakm@gmail.com","sentAt":"2024-05-02T00:45:38Z","receivedAt":"2024-05-02T00:46:16Z","isPatch":false,"sender":{"key":"dhruvakm@gmail.com","avatar":"https://gravatar.com/avatar/96fe022a95b60fd0de7f9f521364d964cdc7c46be5cfef279f8d4379e49e19a6?d=mp&s=160"},"body":"On Wed, May 1, 2024 at 3:00 PM Jeff King wrote:\n> Oof. I was able to reproduce this regression, and the impact can be\n> pretty severe. You can reproduce it without NFS and without shallow\n> clones. Get a bare clone of something big like the kernel:\n\nWow, that was really fast! I spent quite some time narrowing down and\nstill was far off. I hope to develop such deep insights and\nunderstanding of git code some day. Thank you very much for sharing\nall the details and giving me leads to further explore.\n\n-dhruva\n"},{"id":"493929","messageId":"ZjPOd83r+tkmsv3o@nand.local","threadId":"61401","inReplyTo":"ZjLfcCxjLq4o7hpw@nand.local","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-02T17:33:43Z","receivedAt":"2024-05-02T17:33:50Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 01, 2024 at 08:33:52PM -0400, Taylor Blau wrote:\n> On Wed, May 01, 2024 at 06:00:30PM -0400, Jeff King wrote:\n> > Bisecting show the culprit is 2386535511 (attr: read attributes from\n> > HEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a\n> > bare repository would only look for attributes in the info/attributes\n> > file. But after, we look at the HEAD tree-ish, too. And pack-objects\n> > will check the \"delta\" attribute for every path of every object we are\n> > packing. And remember that in-tree lookups for foo/bar/baz require\n> > looking not just for .gitattributes, but also foo/.gitattributes,\n> > foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.\n>\n> Thanks for the explanation and bisection. I agree that 2386535511 makes\n> sense as a likely culprit given what you wrote here.\n\nHere is one possible approach, which is a partial revert of 2386535511.\nI thought about suggesting that we revert 2386535511 entirely, but I\nthink that may be too strong of an approach especially if there are\nplans to otherwise improve the performance of attr lookups with some\ncaching layer.\n\nInstead, this patch changes the behavior to only fallback to \"HEAD\" in\nbare repositories from check-attr, but leaves pack-objects, archive, and\nall other builtins alone.\n\nI should note, this is a pretty hacky approach to use the extern'd\ngit_attr_tree variable from within the check-attr builtin, but I think\nthat this does do the trick.\n\nAlternatively, if this is too hacky or magical that check-attr does one\nthing but every other command does something else, I would personally be\nfine with a full revert of 2386535511.\n\nAnyway, here is the patch:\n\n--- 8< ---\n\nSubject: [PATCH] attr.c: only read attributes from HEAD via check-attr\n\nThis patch is a partial revert of commit 2386535511d (attr: read\nattributes from HEAD when bare repo, 2023-10-13), which caused Git to\nstart reading from .gitattributes files from HEAD^{tree} when invoked in\na bare repository.\n\nThis patch has an unfortunate side-effect of significantly slowing down\npack-objects, for example, when invoked in a bare repository without\nusing reachability bitmaps.\n\nPrior to 2386535511d, pack-objects would only look at the\ninfo/attributes file when working in a bare repository. But after,\npack-objects ends up looking at every \"delta\" attribute not just in the\ninfo/attributes file, but for every .gitattributes file in each tree\nrecursively from the root down to the dirname of whatever path we're\ninspecting. In other words, gathering attributes for path foo/bar/baz\nrequires reading .gitattributes, foo/.gitattributes,\nfoo/bar/.gitattributes, and foo/bar/baz/.gitattributes.\n\nRestore the pre-2386535511d behavior for commands other than check-attr\n(which was the intended target of the change described in 2386535511d).\n\nIf we want to cause pack-objects to use HEAD^{tree} as an attributes\nsource in bare repositories by default again, it would likely come after\nsome caching layer to avoid the performance penalty.\n\nReported-by: Dhruva Krishnamurthy \nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n attr.c | 7 -------\n builtin/check-attr.c | 2 ++\n t/t5001-archive-attr.sh | 2 +-\n 3 files changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7c380c17317..33473bdce01 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1220,17 +1220,10 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name && git_attr_tree) {\n \t\tdefault_attr_source_tree_object_name = git_attr_tree;\n \t\tignore_bad_attr_tree = 1;\n \t}\n\n-\tif (!default_attr_source_tree_object_name &&\n-\t startup_info->have_repository &&\n-\t is_bare_repository()) {\n-\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n-\t\tignore_bad_attr_tree = 1;\n-\t}\n-\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n\n \tif (repo_get_oid_treeish(the_repository,\n \t\t\t\t default_attr_source_tree_object_name,\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex c1da1d184e9..9b445fe33c6 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -188,10 +188,12 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n\n \tif (source) {\n \t\tif (repo_get_oid_tree(the_repository, source, &initialized_oid))\n \t\t\tdie(\"%s: not a valid tree-ish source\", source);\n \t\tset_git_attr_source(source);\n+\t} else if (startup_info->have_repository && is_bare_repository()) {\n+\t\tgit_attr_tree = \"HEAD\";\n \t}\n\n \tif (stdin_paths)\n \t\tcheck_attr_stdin_paths(prefix, check, all_attrs);\n \telse {\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex eaf959d8f63..0ff47a239db 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -136,11 +136,11 @@ test_expect_success 'git archive with worktree attributes, bare' '\n \t(cd bare && git archive --worktree-attributes HEAD) >bare-worktree.tar &&\n \t(mkdir bare-worktree && cd bare-worktree && \"$TAR\" xf -) substfile1.expected &&\n \ttest_cmp nosubstfile archive/nosubstfile &&\n--\n2.45.0.1.g3e84e921a0a.dirty\n\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"493930","messageId":"xmqqfrv0ds7f.fsf@gitster.g","threadId":"61401","inReplyTo":"ZjPOd83r+tkmsv3o@nand.local","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-02T17:44:20Z","receivedAt":"2024-05-02T17:44:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Instead, this patch changes the behavior to only fallback to \"HEAD\" in\n> bare repositories from check-attr, but leaves pack-objects, archive, and\n> all other builtins alone.\n\nI thought the whole point of the exercise was to allow server-side\n(which typically is bare and cannot use anything from the working\ntree) to pay attention to the attributes. This patch rips that out\nand piles even more new and unproven code on top? I am not sure.\n\n"},{"id":"493932","messageId":"ZjPTlrMdpI+jXxyW@nand.local","threadId":"61401","inReplyTo":"xmqqfrv0ds7f.fsf@gitster.g","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-02T17:55:34Z","receivedAt":"2024-05-02T17:55:37Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 02, 2024 at 10:44:20AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Instead, this patch changes the behavior to only fallback to \"HEAD\" in\n> > bare repositories from check-attr, but leaves pack-objects, archive, and\n> > all other builtins alone.\n>\n> I thought the whole point of the exercise was to allow server-side\n> (which typically is bare and cannot use anything from the working\n> tree) to pay attention to the attributes. This patch rips that out\n> and piles even more new and unproven code on top? I am not sure.\n\nI thought the point of John's patch was to allow just check-attr to read\nfrom HEAD^{tree} in bare repositories, and not to touch other commands.\n\nI could be misunderstanding the original intent of John's patch (the\ncommit message there isn't clear whether the change was intended to\ntarget just check-attr or all of Git). But my hope is that it was the\nformer, which this patch preserves.\n\nI do not know whether servers should in general be trusting\nuser-provided attributes for things like \"delta\".\n\nThanks,\nTaylor\n"},{"id":"493934","messageId":"CAKOHPAnoER5nNrTK=7O6UP11ri3Cx_fxFP5PjMeYgOsYOQNXBg@mail.gmail.com","threadId":"61401","inReplyTo":"xmqqfrv0ds7f.fsf@gitster.g","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Dhruva Krishnamurthy","fromEmail":"dhruvakm@gmail.com","sentAt":"2024-05-02T18:34:08Z","receivedAt":"2024-05-02T18:34:47Z","isPatch":false,"sender":{"key":"dhruvakm@gmail.com","avatar":"https://gravatar.com/avatar/96fe022a95b60fd0de7f9f521364d964cdc7c46be5cfef279f8d4379e49e19a6?d=mp&s=160"},"body":"On Thu, May 2, 2024 at 10:44 AM Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> > Instead, this patch changes the behavior to only fallback to \"HEAD\" in\n> > bare repositories from check-attr, but leaves pack-objects, archive, and\n> > all other builtins alone.\n>\n> I thought the whole point of the exercise was to allow server-side\n> (which typically is bare and cannot use anything from the working\n> tree) to pay attention to the attributes. This patch rips that out\n> and piles even more new and unproven code on top? I am not sure.\n\nYes, I am particularly interested in bare repositories (server/hosting side).\n"},{"id":"493936","messageId":"CAOLa=ZRe6eWJ_ZyH+HRq=6Lh0-xZ=1X2Z2f3HW4+EVXNquaDTQ@mail.gmail.com","threadId":"61401","inReplyTo":"ZjPTlrMdpI+jXxyW@nand.local","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-02T19:01:15Z","receivedAt":"2024-05-02T19:01:17Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Taylor Blau writes:\n\n> On Thu, May 02, 2024 at 10:44:20AM -0700, Junio C Hamano wrote:\n>> Taylor Blau writes:\n>>\n>> > Instead, this patch changes the behavior to only fallback to \"HEAD\" in\n>> > bare repositories from check-attr, but leaves pack-objects, archive, and\n>> > all other builtins alone.\n>>\n>> I thought the whole point of the exercise was to allow server-side\n>> (which typically is bare and cannot use anything from the working\n>> tree) to pay attention to the attributes. This patch rips that out\n>> and piles even more new and unproven code on top? I am not sure.\n>\n> I thought the point of John's patch was to allow just check-attr to read\n> from HEAD^{tree} in bare repositories, and not to touch other commands.\n>\n> I could be misunderstanding the original intent of John's patch (the\n> commit message there isn't clear whether the change was intended to\n> target just check-attr or all of Git). But my hope is that it was the\n> former, which this patch preserves.\n>\n\nFrom the series [1] it becomes more clear that the intention was to\ntarget all commands.\n\n[1]: https://lore.kernel.org/git/pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com/\n\n> I do not know whether servers should in general be trusting\n> user-provided attributes for things like \"delta\".\n>\n> Thanks,\n> Taylor\n"},{"id":"493950","messageId":"xmqqbk5ndiqk.fsf@gitster.g","threadId":"61401","inReplyTo":"CAOLa=ZRe6eWJ_ZyH+HRq=6Lh0-xZ=1X2Z2f3HW4+EVXNquaDTQ@mail.gmail.com","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-02T21:08:51Z","receivedAt":"2024-05-02T21:08:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Taylor Blau writes:\n>\n>> I could be misunderstanding the original intent of John's patch (the\n>> commit message there isn't clear whether the change was intended to\n>> target just check-attr or all of Git). But my hope is that it was the\n>> former, which this patch preserves.\n>>\n>\n> From the series [1] it becomes more clear that the intention was to\n> target all commands.\n>\n> [1]: https://lore.kernel.org/git/pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com/\n\nTrue. \n\nWe could drop [1/2] from the series in the meantime to make it a\nGitLab installation specific issue where they explicitly use\nattr.tree to point at HEAD ;-) That is not solving anything for\nthose who set attr.tree (in a sense, they are buying the feature\nwith overhead of reading attributes from the named tree), but at\nleast for most people who are used to seeing the bare repository\nignoring the attributes, it would be an improvement to drop the\n\"bare repositories the tree of the HEAD commit is used to look up\nattributes files by default\" half from the series.\n\n"},{"id":"493956","messageId":"CAKOHPA==xgRBLXmyURkdZ9X4LqQoBHYy=XD0Q_KTQHbK54DOFg@mail.gmail.com","threadId":"61401","inReplyTo":"xmqqbk5ndiqk.fsf@gitster.g","subject":"Re: using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Dhruva Krishnamurthy","fromEmail":"dhruvakm@gmail.com","sentAt":"2024-05-03T05:37:59Z","receivedAt":"2024-05-03T05:38:37Z","isPatch":false,"sender":{"key":"dhruvakm@gmail.com","avatar":"https://gravatar.com/avatar/96fe022a95b60fd0de7f9f521364d964cdc7c46be5cfef279f8d4379e49e19a6?d=mp&s=160"},"body":"On Thu, May 2, 2024 at 2:08 PM Junio C Hamano wrote:\n> We could drop [1/2] from the series in the meantime to make it a\n> GitLab installation specific issue where they explicitly use\n> attr.tree to point at HEAD ;-) That is not solving anything for\n> those who set attr.tree (in a sense, they are buying the feature\n> with overhead of reading attributes from the named tree), but at\n> least for most people who are used to seeing the bare repository\n> ignoring the attributes, it would be an improvement to drop the\n> \"bare repositories the tree of the HEAD commit is used to look up\n> attributes files by default\" half from the series.\n>\n\nA hack (without knowing side effects if any) is to use an empty tree\nfor attr source:\n$ git config --add attr.tree $(git hash-object -t tree /dev/null)\n\nThis gives me performance comparable to git 2.42\n"},{"id":"494007","messageId":"xmqqzft6aozg.fsf_-_@gitster.g","threadId":"61401","inReplyTo":"CAKOHPA==xgRBLXmyURkdZ9X4LqQoBHYy=XD0Q_KTQHbK54DOFg@mail.gmail.com","subject":"Re* using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T15:34:27Z","receivedAt":"2024-05-03T15:34:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dhruva Krishnamurthy writes:\n\n> On Thu, May 2, 2024 at 2:08 PM Junio C Hamano wrote:\n>> We could drop [1/2] from the series in the meantime to make it a\n>> GitLab installation specific issue where they explicitly use\n>> attr.tree to point at HEAD ;-) That is not solving anything for\n>> those who set attr.tree (in a sense, they are buying the feature\n>> with overhead of reading attributes from the named tree), but at\n>> least for most people who are used to seeing the bare repository\n>> ignoring the attributes, it would be an improvement to drop the\n>> \"bare repositories the tree of the HEAD commit is used to look up\n>> attributes files by default\" half from the series.\n>>\n>\n> A hack (without knowing side effects if any) is to use an empty tree\n> for attr source:\n> $ git config --add attr.tree $(git hash-object -t tree /dev/null)\n>\n> This gives me performance comparable to git 2.42\n\nThat is clever. Instead of crawling a potentially large tree that\nis at the HEAD of the main project payload to find \".gitattributes\"\nfiles that may be relevant (and often not), folks can set an empty\ntree to attr.tree to the configuration until this gets corrected.\n\nAnd for folks who had been happy with the pre 2.42 behaviour,\nwe could do something like the attached as the first step to a real fix.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] stop using HEAD for attributes in bare repository by default\n\nWith 23865355 (attr: read attributes from HEAD when bare repo,\n2023-10-13), we started to use the HEAD tree as the default\nattribute source in a bare repository. One argument for such a\nbehaviour is that it would make things like \"git archive\" run in\nbare and non-bare repositories for the same commit consistent.\nThis changes was merged to Git 2.43 but without an explicit mention\nin its release notes.\n\nIt turns out that this change destroys performance of shallowly\ncloning from a bare repository. As the \"server\" installations are\nexpected to be mostly bare, and \"git pack-objects\", which is the\ncore of driving the other side of \"git clone\" and \"git fetch\" wants\nto see if a path is set not to delta with blobs from other paths via\nthe attribute system, the change forces the server side to traverse\nthe tree of the HEAD commit needlessly to find if each and every\npaths the objects it sends out has the attribute that controls the\ndeltification. Given that (1) most projects do not configure such\nan attribute, and (2) it is dubious for the server side to honor\nsuch an end-user supplied attribute anyway, this was a poor choice\nof the default.\n\nTo mitigate the current situation, let's revert the change that uses\nthe tree of HEAD in a bare repository by default as the attribute\nsource. This will help most people who have been happy with the\nbehaviour of Git 2.42 and before.\n\nTwo things to note:\n\n * If you are stuck with versions of Git 2.43 or newer, that is\n older than the release this fix appears in, you can explicitly\n set the attr.tree configuration variable to point at an empty\n tree object, i.e.\n\n\t$ git config attr.tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\n * If you like the behaviour we are reverting, you can explicitly\n set the attr.tree configuration variable to HEAD, i.e.\n\n\t$ git config attr.tree HEAD\n\nThe right fix for this is to optimize the code paths that allow\naccesses to attributes in tree objects, but that is a much more\ninvolved change and is left as a longer-term project, outside the\nscope of this \"first step\" fix.\n\nSigned-off-by: Junio C Hamano \n---\n attr.c | 7 -------\n t/t0003-attributes.sh | 10 ++++++++--\n t/t5001-archive-attr.sh | 3 ++-\n 3 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git c/attr.c w/attr.c\nindex 679e42258c..6af7151088 100644\n--- c/attr.c\n+++ w/attr.c\n@@ -1223,13 +1223,6 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \t\tignore_bad_attr_tree = 1;\n \t}\n \n-\tif (!default_attr_source_tree_object_name &&\n-\t startup_info->have_repository &&\n-\t is_bare_repository()) {\n-\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n-\t\tignore_bad_attr_tree = 1;\n-\t}\n-\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \ndiff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\nindex 774b52c298..d755cc3c29 100755\n--- c/t/t0003-attributes.sh\n+++ w/t/t0003-attributes.sh\n@@ -398,13 +398,19 @@ test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n \t)\n '\n \n-test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n+test_expect_success 'bare repo no longer defaults to reading .gitattributes from HEAD' '\n \ttest_when_finished rm -rf test bare_with_gitattribute &&\n \tgit init test &&\n \ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n \tgit clone --bare test bare_with_gitattribute &&\n-\techo \"f/path: test: val\" >expect &&\n+\n+\techo \"f/path: test: unspecified\" >expect &&\n \tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C bare_with_gitattribute -c attr.tree=HEAD \\\n+\t\tcheck-attr test -- f/path >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git c/t/t5001-archive-attr.sh w/t/t5001-archive-attr.sh\nindex eaf959d8f6..7310774af5 100755\n--- c/t/t5001-archive-attr.sh\n+++ w/t/t5001-archive-attr.sh\n@@ -133,7 +133,8 @@ test_expect_success 'git archive vs. bare' '\n '\n \n test_expect_success 'git archive with worktree attributes, bare' '\n-\t(cd bare && git archive --worktree-attributes HEAD) >bare-worktree.tar &&\n+\t(cd bare &&\n+\tgit -c attr.tree=HEAD archive --worktree-attributes HEAD) >bare-worktree.tar &&\n \t(mkdir bare-worktree && cd bare-worktree && \"$TAR\" xf -) And for folks who had been happy with the pre 2.42 behaviour,\n> we could do something like the attached as the first step to a real fix.\n\nIt looks like lots of discussion happened with out me, and everybody\nalready posted all of the responses I was going to. Good. :)\n\nIn particular...\n\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] stop using HEAD for attributes in bare repository by default\n> [...]\n> The right fix for this is to optimize the code paths that allow\n> accesses to attributes in tree objects, but that is a much more\n> involved change and is left as a longer-term project, outside the\n> scope of this \"first step\" fix.\n\n...this was the exact first step I was going to suggest. And your patch\nlooks correct to me. I assume you'd target this for 'maint'. The\nregression goes back to v2.43.0, so it's not exactly new, but given the\nseverity in some cases it seems like it's worth getting it into a\nrelease sooner rather than later.\n\nI am mildly surprised nobody noticed the issue until now. I wonder if\nt/perf would notice it and nobody is running it, or if this is a gap in\nour coverage there. If the latter, it might be worth adding such a\nscript, which should be able to show off that your change here takes us\nback to the v2.42 state.\n\nRunning the perf suite against linux.git between 2.42 and 2.43 would\nanswer the \"is this a gap\" question, but I haven't had a chance to do\nso, and it takes a while.\n\n-Peff\n"},{"id":"494202","messageId":"Zjk9eH9e4fByGG9Z@nand.local","threadId":"61401","inReplyTo":"20240503174653.GD3631237@coredump.intra.peff.net","subject":"Re: Re* using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-06T20:28:40Z","receivedAt":"2024-05-06T20:28:42Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 03, 2024 at 01:46:53PM -0400, Jeff King wrote:\n> On Fri, May 03, 2024 at 08:34:27AM -0700, Junio C Hamano wrote:\n>\n> > And for folks who had been happy with the pre 2.42 behaviour,\n> > we could do something like the attached as the first step to a real fix.\n>\n> It looks like lots of discussion happened with out me, and everybody\n> already posted all of the responses I was going to. Good. :)\n>\n> In particular...\n>\n> > ----- >8 --------- >8 --------- >8 --------- >8 -----\n> > Subject: [PATCH] stop using HEAD for attributes in bare repository by default\n> > [...]\n> > The right fix for this is to optimize the code paths that allow\n> > accesses to attributes in tree objects, but that is a much more\n> > involved change and is left as a longer-term project, outside the\n> > scope of this \"first step\" fix.\n>\n> ...this was the exact first step I was going to suggest. And your patch\n> looks correct to me. I assume you'd target this for 'maint'. The\n> regression goes back to v2.43.0, so it's not exactly new, but given the\n> severity in some cases it seems like it's worth getting it into a\n> release sooner rather than later.\n\nFor what it's worth, I am in favor of the patch that Junio proposed\nhere.\n\nThanks,\nTaylor\n"},{"id":"494661","messageId":"1BDD4F51-4B62-463D-A876-FB16E38E86C2@gmail.com","threadId":"61401","inReplyTo":"xmqqzft6aozg.fsf_-_@gitster.g","subject":"Re: Re* using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-05-13T20:16:53Z","receivedAt":"2024-05-13T20:16:55Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\nOn 3 May 2024, at 11:34, Junio C Hamano wrote:\n\n> Dhruva Krishnamurthy writes:\n>\n>> On Thu, May 2, 2024 at 2:08 PM Junio C Hamano wrote:\n>>> We could drop [1/2] from the series in the meantime to make it a\n>>> GitLab installation specific issue where they explicitly use\n>>> attr.tree to point at HEAD ;-) That is not solving anything for\n>>> those who set attr.tree (in a sense, they are buying the feature\n>>> with overhead of reading attributes from the named tree), but at\n>>> least for most people who are used to seeing the bare repository\n>>> ignoring the attributes, it would be an improvement to drop the\n>>> \"bare repositories the tree of the HEAD commit is used to look up\n>>> attributes files by default\" half from the series.\n>>>\n>>\n>> A hack (without knowing side effects if any) is to use an empty tree\n>> for attr source:\n>> $ git config --add attr.tree $(git hash-object -t tree /dev/null)\n>>\n>> This gives me performance comparable to git 2.42\n>\n> That is clever. Instead of crawling a potentially large tree that\n> is at the HEAD of the main project payload to find \".gitattributes\"\n> files that may be relevant (and often not), folks can set an empty\n> tree to attr.tree to the configuration until this gets corrected.\n>\n> And for folks who had been happy with the pre 2.42 behaviour,\n> we could do something like the attached as the first step to a real fix.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] stop using HEAD for attributes in bare repository by default\n>\n> With 23865355 (attr: read attributes from HEAD when bare repo,\n> 2023-10-13), we started to use the HEAD tree as the default\n> attribute source in a bare repository. One argument for such a\n> behaviour is that it would make things like \"git archive\" run in\n> bare and non-bare repositories for the same commit consistent.\n> This changes was merged to Git 2.43 but without an explicit mention\n> in its release notes.\n>\n> It turns out that this change destroys performance of shallowly\n> cloning from a bare repository. As the \"server\" installations are\n> expected to be mostly bare, and \"git pack-objects\", which is the\n> core of driving the other side of \"git clone\" and \"git fetch\" wants\n> to see if a path is set not to delta with blobs from other paths via\n> the attribute system, the change forces the server side to traverse\n> the tree of the HEAD commit needlessly to find if each and every\n> paths the objects it sends out has the attribute that controls the\n> deltification. Given that (1) most projects do not configure such\n> an attribute, and (2) it is dubious for the server side to honor\n> such an end-user supplied attribute anyway, this was a poor choice\n> of the default.\n>\n> To mitigate the current situation, let's revert the change that uses\n> the tree of HEAD in a bare repository by default as the attribute\n> source. This will help most people who have been happy with the\n> behaviour of Git 2.42 and before.\n\nThis change makes sense to me, and glad it got uncovered. Thanks to all who\nchimed in with this root cause analysis and the proposed patches. Sorry I\nhaven't replied until now-I was traveling the past two weeks.\n\nthanks\nJohn\n\n>\n> Two things to note:\n>\n> * If you are stuck with versions of Git 2.43 or newer, that is\n> older than the release this fix appears in, you can explicitly\n> set the attr.tree configuration variable to point at an empty\n> tree object, i.e.\n>\n> \t$ git config attr.tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>\n> * If you like the behaviour we are reverting, you can explicitly\n> set the attr.tree configuration variable to HEAD, i.e.\n>\n> \t$ git config attr.tree HEAD\n>\n> The right fix for this is to optimize the code paths that allow\n> accesses to attributes in tree objects, but that is a much more\n> involved change and is left as a longer-term project, outside the\n> scope of this \"first step\" fix.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> attr.c | 7 -------\n> t/t0003-attributes.sh | 10 ++++++++--\n> t/t5001-archive-attr.sh | 3 ++-\n> 3 files changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git c/attr.c w/attr.c\n> index 679e42258c..6af7151088 100644\n> --- c/attr.c\n> +++ w/attr.c\n> @@ -1223,13 +1223,6 @@ static void compute_default_attr_source(struct object_id *attr_source)\n> \t\tignore_bad_attr_tree = 1;\n> \t}\n>\n> -\tif (!default_attr_source_tree_object_name &&\n> -\t startup_info->have_repository &&\n> -\t is_bare_repository()) {\n> -\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n> -\t\tignore_bad_attr_tree = 1;\n> -\t}\n> -\n> \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n> \t\treturn;\n>\n> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\n> index 774b52c298..d755cc3c29 100755\n> --- c/t/t0003-attributes.sh\n> +++ w/t/t0003-attributes.sh\n> @@ -398,13 +398,19 @@ test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n> \t)\n> '\n>\n> -test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n> +test_expect_success 'bare repo no longer defaults to reading .gitattributes from HEAD' '\n> \ttest_when_finished rm -rf test bare_with_gitattribute &&\n> \tgit init test &&\n> \ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n> \tgit clone --bare test bare_with_gitattribute &&\n> -\techo \"f/path: test: val\" >expect &&\n> +\n> +\techo \"f/path: test: unspecified\" >expect &&\n> \tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\techo \"f/path: test: val\" >expect &&\n> +\tgit -C bare_with_gitattribute -c attr.tree=HEAD \\\n> +\t\tcheck-attr test -- f/path >actual &&\n> \ttest_cmp expect actual\n> '\n>\n> diff --git c/t/t5001-archive-attr.sh w/t/t5001-archive-attr.sh\n> index eaf959d8f6..7310774af5 100755\n> --- c/t/t5001-archive-attr.sh\n> +++ w/t/t5001-archive-attr.sh\n> @@ -133,7 +133,8 @@ test_expect_success 'git archive vs. bare' '\n> '\n>\n> test_expect_success 'git archive with worktree attributes, bare' '\n> -\t(cd bare && git archive --worktree-attributes HEAD) >bare-worktree.tar &&\n> +\t(cd bare &&\n> +\tgit -c attr.tree=HEAD archive --worktree-attributes HEAD) >bare-worktree.tar &&\n> \t(mkdir bare-worktree && cd bare-worktree && \"$TAR\" xf -) '\n"},{"id":"496401","messageId":"xmqqa5jzqd5k.fsf_-_@gitster.g","threadId":"61401","inReplyTo":"xmqqzft6aozg.fsf_-_@gitster.g","subject":"[PATCH] attr.tree: HEAD:.gitattributes is no longer the default in a bare repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T21:43:03Z","receivedAt":"2024-06-05T21:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"51441e64 (stop using HEAD for attributes in bare repository by\ndefault, 2024-05-03) has addressed a recent performance regression\nby partially reverting a topic that was merged at 26dd307c (Merge\nbranch 'jc/attr-tree-config', 2023-10-30). But it forgot to update\nthe documentation to remove the mention of a special case in bare\nrepositories.\n\nLet's update the document before the update hits the next release.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config/attr.txt | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/config/attr.txt w/Documentation/config/attr.txt\nindex 1a482d6af2..c4a5857993 100644\n--- c/Documentation/config/attr.txt\n+++ w/Documentation/config/attr.txt\n@@ -1,7 +1,6 @@\n attr.tree::\n \tA reference to a tree in the repository from which to read attributes,\n-\tinstead of the `.gitattributes` file in the working tree. In a bare\n-\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n-\tnot resolve to a valid tree object, an empty tree is used instead.\n+\tinstead of the `.gitattributes` file in the working tree. If the value\n+\tdoes not resolve to a valid tree object, an empty tree is used instead.\n \tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n \tcommand line option are used, this configuration variable has no effect.\n"},{"id":"496467","messageId":"20240606083216.GE658959@coredump.intra.peff.net","threadId":"61401","inReplyTo":"xmqqa5jzqd5k.fsf_-_@gitster.g","subject":"Re: [PATCH] attr.tree: HEAD:.gitattributes is no longer the default in a bare repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:32:16Z","receivedAt":"2024-06-06T08:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2024 at 02:43:03PM -0700, Junio C Hamano wrote:\n\n> 51441e64 (stop using HEAD for attributes in bare repository by\n> default, 2024-05-03) has addressed a recent performance regression\n> by partially reverting a topic that was merged at 26dd307c (Merge\n> branch 'jc/attr-tree-config', 2023-10-30). But it forgot to update\n> the documentation to remove the mention of a special case in bare\n> repositories.\n> \n> Let's update the document before the update hits the next release.\n\nGood catch, and the patch looks good.\n\nI think 51441e64 is essentially a revert of 2386535511 (attr: read\nattributes from HEAD when bare repo, 2023-10-13). I don't know how you\nprepared it, but I'd probably have started with \"cherry-pick -n\". But\nthat wouldn't help, because the documentation didn't come until after\nthat in 9f9c40cf34 (attr: add attr.tree for setting the treeish to read\nattributes from, 2023-10-13).\n\nNot that it really matters much now, but always just curious about how\nwe can avoid missing stuff like this next time.\n\n-Peff\n"},{"id":"496537","messageId":"xmqqle3injob.fsf@gitster.g","threadId":"61401","inReplyTo":"20240606083216.GE658959@coredump.intra.peff.net","subject":"Re: [PATCH] attr.tree: HEAD:.gitattributes is no longer the default in a bare repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T16:02:44Z","receivedAt":"2024-06-06T16:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think 51441e64 is essentially a revert of 2386535511 (attr: read\n> attributes from HEAD when bare repo, 2023-10-13). I don't know how you\n> prepared it, but I'd probably have started with \"cherry-pick -n\". But\n> that wouldn't help, because the documentation didn't come until after\n> that in 9f9c40cf34 (attr: add attr.tree for setting the treeish to read\n> attributes from, 2023-10-13).\n\n\"revert -m 1\" followed by \"commit --amend\" might have worked well in\nthis case to get rid of the code that came from one and doc update\nthat came from the other in a two patch series, but in general, that\nwould be too much noise to wade through in general.\n\n> Not that it really matters much now, but always just curious about how\n> we can avoid missing stuff like this next time.\n\nThe series first did \"HEAD tree is used in bare\" without doc, and\nfollowed up with \"configuration can be used to name any tree\" with\ndoc that mentions the behaviour of the first step as a special case\nof default value for the configuration variable. The only way it\ncould have been made easier to spot is to introduce the variable\nwith documentation first, and then do the \"bare repo uses HEAD as\nthe default\" thing on top.\n\n"}]}