diff mbox series

using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42

Message ID 20240501220030.GA1442509@coredump.intra.peff.net (mailing list archive)
State New, archived
Headers show
Series using tree as attribute source is slow, was Re: Help troubleshoot performance regression cloning with depth: git 2.44 vs git 2.42 | expand

Commit Message

Jeff King May 1, 2024, 10 p.m. UTC
On Tue, Apr 30, 2024 at 10:26:32PM -0700, Dhruva Krishnamurthy wrote:

> Cloning by specifying depth exhibits performance regression in
> pack-objects (~20x). The repository I am cloning is on NFS (mounted
> with NFSv3 & positive lookup cache enabled).
> 
> Ran under 'perf' command to capture profiling information to see if
> something really stands out. There is a significant overhead in calls
> to file open/open64, fstat64 & mmap/munmap in git 2.44 compared to git
> 2.42. Not sure if there is an increase in the number of calls or
> something more is done.
> 
> Could someone please guide me on how to troubleshoot this better?

Oof. I was able to reproduce this regression, and the impact can be
pretty severe. You can reproduce it without NFS and without shallow
clones. Get a bare clone of something big like the kernel:

  git clone --bare https://github.com/torvalds/linux
  cd linux.git

and then do something similar to the server side of a clone:

  time git pack-objects --all --stdout </dev/null >/dev/null

Most of the time will be spent in the "Enumerating objects" phase,
walking the graph (since the repo is fully packed, delta searching and
writing are fairly quick).

With v2.42.0, I get:

  real	1m12.409s
  user	1m13.021s
  sys	0m6.091s

With v2.44.0, I get:

  real	4m12.075s
  user	4m12.763s
  sys	0m5.546s

Bisecting show the culprit is 2386535511 (attr: read attributes from
HEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a
bare repository would only look for attributes in the info/attributes
file. But after, we look at the HEAD tree-ish, too. And pack-objects
will check the "delta" attribute for every path of every object we are
packing. And remember that in-tree lookups for foo/bar/baz require
looking not just for .gitattributes, but also foo/.gitattributes,
foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.

In a non-bare repo, this isn't _too_ bad. We'll try an open() for each
path, but the common case is that we don't have such a file, so the
total cost is the one syscall per object. But when we're pulling
attributes from HEAD, each lookup requires walking the whole chain of
trees (so for the final one, tree foo points to tree bar points to tree
baz, where we see there is no .gitattributes entry). And so all of that
extra time goes to reading in trees over and over.

You can repeat the same test with git.git. It gets slower, but it's
barely perceptible. This is because we have a relatively shallow tree,
so most lookups are hitting root-level .gitattributes or maybe one or
two levels deep. The kernel has a much deeper tree, so those lookups are
more expensive (and of course there are a lot more of them).

Getting back to shallow clones and NFS:

  - the effect is more pronounced on NFS because the cost to access
    objects is higher. With git.git I was even able to see some
    slowdown, probably just from the cost/latency of opening up all of
    those objects.

  - the problem doesn't show up if the repo has reachability bitmaps.
    This is because the bitmap result doesn't have the pathnames of each
    object (we do have the "name hash", but it's not enough for us to do
    an attr lookup), and so objects we get from a bitmap do not
    respect the delta attribute at all.

    But when doing a shallow clone, we have to disable bitmaps and do a
    regular traversal. So even if you have bitmaps, you still run into
    the problem.

    The example above should not have bitmaps (we do build them by
    default when repacking bare repos these days, but I don't think
    we'll do so right after cloning). If you have a local repo that
    already has bitmaps, you should be able to see the difference by
    using "git -c pack.usebitmaps=false pack-objects".

    So even if you are a server which generally enables bitmaps, you can
    still get bit by this for shallow clones, but also for other
    non-bitmap invocations, like say "git repack -ad". There I see the
    same 3-minute slowdown in the enumeration phase.

So what to do? It seems like some kind of caching would help here. We're
looking up the same paths over and over, for two reasons:

  1. We'll have many objects with the same paths, one for each time the
     path was modified through history.

  2. Adjacent objects share the higher-level lookups. Both "dir/a" and
     "dir/b" will need to look up "dir/.gitattributes" (and all the way
     up to ".gitattributes").

So even something simple and stupid like this:


restores the v2.42 performance. But there are probably better options:

  - this is caching whole .gitattributes buffers. In pack-objects we
    only care about a single bit for try_delta. For linux.git it doesn't
    really matter, as 99% of our entries are just CACHE_MISSING and the
    real value is avoiding the negative lookups. But the same problem
    exists to a lesser degree for "git log -p" in a bare repo. So I
    think it makes sense to try to solve it in the attr layer.

  - the string keys have a lot of duplication. You'll have
    "foo/.gitattributes", "foo/bar/.gitattributes", and so on. A trie
    structure split by path component would let you store each component
    just once. And perhaps have even faster lookups. I think this is
    roughly the same issue faced by the kernel VFS for doing path
    lookups, so something dentry/dcache-like would help.

    I don't know how much it matters in practice, though. The sum of all
    of the paths in HEAD for linux.git is ~3.5MB, which is a rounding
    error on the needs of the rest of the packing process.

  - Something dcache-like could also be pushed down lower, to the
    get_tree_entry() API (imagine it quietly caching uncompressed trees
    behind the scenes and then using them to traverse). That depends on
    high locality of requests, though. I don't know if we have that
    here, because we do our lookups in traversal order. So you'd look at
    entries in HEAD^{tree}, then HEAD~1^{tree}, then HEAD~2^{tree}, and
    so on.

  - Speaking of locality, the attr code tries to make use of request
    locality in its stack (so if we ask for attributes for "foo/bar",
    then "foo/baz", we should be able to keep the parsed data for
    "foo/.gitattributes" available between them). But our pattern here
    violates that. Again, not that big an issue if you don't have that
    many .gitattributes files in the first place, so it might not be
    worth worrying about. But if we did want to rearrange the lookups to
    exploit locality, it might change the overall caching strategy.

  - As noted above, most entries are just CACHE_MISSING. So rather than
    lazily looking up and caching entries, we could just prepopulate the
    cache. And then you know that if an entry isn't present in the
    cache, it does not exist in the tree. The downside is that you pay
    to walk the all of HEAD^{tree}, even if you only have a few lookups
    to do. That's a good tradeoff for pack-objects (which usually ends
    up looking for every path anyway), but not for "git diff" (where you
    only care about a few changed paths.

  - the cache here is static-local in the function. It should probably
    at least be predicated on the tree_oid, and maybe attached to the
    repository object? I think having one per repository at a time would
    be fine (generally the tree_oid is set once per process, so it's not
    like you're switching between multiple options).

I've cc'd John as the author of 2386535511. But really, that was just
enabling by default the attr-tree code added by 47cfc9bd7d (attr: add
flag `--source` to work with tree-ish, 2023-01-14). Although in that
original context (git check-attr) the lack of caching would be much less
important.

-Peff

Comments

Randall S. Becker May 1, 2024, 10:37 p.m. UTC | #1
On Wednesday, May 1, 2024 6:01 PM, Jeff King wrote:
>On Tue, Apr 30, 2024 at 10:26:32PM -0700, Dhruva Krishnamurthy wrote:
>
>> Cloning by specifying depth exhibits performance regression in
>> pack-objects (~20x). The repository I am cloning is on NFS (mounted
>> with NFSv3 & positive lookup cache enabled).
>>
>> Ran under 'perf' command to capture profiling information to see if
>> something really stands out. There is a significant overhead in calls
>> to file open/open64, fstat64 & mmap/munmap in git 2.44 compared to git
>> 2.42. Not sure if there is an increase in the number of calls or
>> something more is done.
>>
>> Could someone please guide me on how to troubleshoot this better?
>
>Oof. I was able to reproduce this regression, and the impact can be pretty severe.
>You can reproduce it without NFS and without shallow clones. Get a bare clone of
>something big like the kernel:
>
>  git clone --bare https://github.com/torvalds/linux
>  cd linux.git
>
>and then do something similar to the server side of a clone:
>
>  time git pack-objects --all --stdout </dev/null >/dev/null
>
>Most of the time will be spent in the "Enumerating objects" phase, walking the
>graph (since the repo is fully packed, delta searching and writing are fairly quick).
>
>With v2.42.0, I get:
>
>  real	1m12.409s
>  user	1m13.021s
>  sys	0m6.091s
>
>With v2.44.0, I get:
>
>  real	4m12.075s
>  user	4m12.763s
>  sys	0m5.546s
>
>Bisecting show the culprit is 2386535511 (attr: read attributes from HEAD when
>bare repo, 2023-10-13), which is in v2.43.0. Before that, a bare repository would
>only look for attributes in the info/attributes file. But after, we look at the HEAD
>tree-ish, too. And pack-objects will check the "delta" attribute for every path of
>every object we are packing. And remember that in-tree lookups for foo/bar/baz
>require looking not just for .gitattributes, but also foo/.gitattributes,
>foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.
>
>In a non-bare repo, this isn't _too_ bad. We'll try an open() for each path, but the
>common case is that we don't have such a file, so the total cost is the one syscall per
>object. But when we're pulling attributes from HEAD, each lookup requires walking
>the whole chain of trees (so for the final one, tree foo points to tree bar points to
>tree baz, where we see there is no .gitattributes entry). And so all of that extra time
>goes to reading in trees over and over.
>
>You can repeat the same test with git.git. It gets slower, but it's barely perceptible.
>This is because we have a relatively shallow tree, so most lookups are hitting root-
>level .gitattributes or maybe one or two levels deep. The kernel has a much deeper
>tree, so those lookups are more expensive (and of course there are a lot more of
>them).
>
>Getting back to shallow clones and NFS:
>
>  - the effect is more pronounced on NFS because the cost to access
>    objects is higher. With git.git I was even able to see some
>    slowdown, probably just from the cost/latency of opening up all of
>    those objects.
>
>  - the problem doesn't show up if the repo has reachability bitmaps.
>    This is because the bitmap result doesn't have the pathnames of each
>    object (we do have the "name hash", but it's not enough for us to do
>    an attr lookup), and so objects we get from a bitmap do not
>    respect the delta attribute at all.
>
>    But when doing a shallow clone, we have to disable bitmaps and do a
>    regular traversal. So even if you have bitmaps, you still run into
>    the problem.
>
>    The example above should not have bitmaps (we do build them by
>    default when repacking bare repos these days, but I don't think
>    we'll do so right after cloning). If you have a local repo that
>    already has bitmaps, you should be able to see the difference by
>    using "git -c pack.usebitmaps=false pack-objects".
>
>    So even if you are a server which generally enables bitmaps, you can
>    still get bit by this for shallow clones, but also for other
>    non-bitmap invocations, like say "git repack -ad". There I see the
>    same 3-minute slowdown in the enumeration phase.
>
>So what to do? It seems like some kind of caching would help here. We're looking
>up the same paths over and over, for two reasons:
>
>  1. We'll have many objects with the same paths, one for each time the
>     path was modified through history.
>
>  2. Adjacent objects share the higher-level lookups. Both "dir/a" and
>     "dir/b" will need to look up "dir/.gitattributes" (and all the way
>     up to ".gitattributes").
>
>So even something simple and stupid like this:
>
>diff --git a/attr.c b/attr.c
>index 679e42258c..b32af9a78b 100644
>--- a/attr.c
>+++ b/attr.c
>@@ -24,6 +24,7 @@
> #include "thread-utils.h"
> #include "tree-walk.h"
> #include "object-name.h"
>+#include "strmap.h"
>
> const char *git_attr_tree;
>
>@@ -795,25 +796,31 @@ static struct attr_stack *read_attr_from_blob(struct
>index_state *istate,
> 					      const struct object_id *tree_oid,
> 					      const char *path, unsigned flags)  {
>-	struct object_id oid;
>-	unsigned long sz;
>-	enum object_type type;
>+	static struct strmap cache = STRMAP_INIT;
>+	void *CACHE_MISSING = (void *)1;
> 	void *buf;
>-	unsigned short mode;
>
> 	if (!tree_oid)
> 		return NULL;
>
>-	if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
>-		return NULL;
>-
>-	buf = repo_read_object_file(istate->repo, &oid, &type, &sz);
>-	if (!buf || type != OBJ_BLOB) {
>-		free(buf);
>-		return NULL;
>+	buf = strmap_get(&cache, path);
>+	if (!buf) {
>+		struct object_id oid;
>+		unsigned short mode;
>+		if (!get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) {
>+			unsigned long sz;
>+			enum object_type type;
>+			buf = repo_read_object_file(istate->repo, &oid, &type,
>&sz);
>+			if (!buf || type != OBJ_BLOB) {
>+				free(buf);
>+				buf = NULL;
>+			}
>+		}
>+		strmap_put(&cache, path, buf ? buf : CACHE_MISSING);
> 	}
>
>-	return read_attr_from_buf(buf, path, flags);
>+	return buf == CACHE_MISSING ? NULL :
>+		read_attr_from_buf(buf, path, flags);
> }
>
> static struct attr_stack *read_attr_from_index(struct index_state *istate,
>
>restores the v2.42 performance. But there are probably better options:
>
>  - this is caching whole .gitattributes buffers. In pack-objects we
>    only care about a single bit for try_delta. For linux.git it doesn't
>    really matter, as 99% of our entries are just CACHE_MISSING and the
>    real value is avoiding the negative lookups. But the same problem
>    exists to a lesser degree for "git log -p" in a bare repo. So I
>    think it makes sense to try to solve it in the attr layer.
>
>  - the string keys have a lot of duplication. You'll have
>    "foo/.gitattributes", "foo/bar/.gitattributes", and so on. A trie
>    structure split by path component would let you store each component
>    just once. And perhaps have even faster lookups. I think this is
>    roughly the same issue faced by the kernel VFS for doing path
>    lookups, so something dentry/dcache-like would help.
>
>    I don't know how much it matters in practice, though. The sum of all
>    of the paths in HEAD for linux.git is ~3.5MB, which is a rounding
>    error on the needs of the rest of the packing process.
>
>  - Something dcache-like could also be pushed down lower, to the
>    get_tree_entry() API (imagine it quietly caching uncompressed trees
>    behind the scenes and then using them to traverse). That depends on
>    high locality of requests, though. I don't know if we have that
>    here, because we do our lookups in traversal order. So you'd look at
>    entries in HEAD^{tree}, then HEAD~1^{tree}, then HEAD~2^{tree}, and
>    so on.
>
>  - Speaking of locality, the attr code tries to make use of request
>    locality in its stack (so if we ask for attributes for "foo/bar",
>    then "foo/baz", we should be able to keep the parsed data for
>    "foo/.gitattributes" available between them). But our pattern here
>    violates that. Again, not that big an issue if you don't have that
>    many .gitattributes files in the first place, so it might not be
>    worth worrying about. But if we did want to rearrange the lookups to
>    exploit locality, it might change the overall caching strategy.
>
>  - As noted above, most entries are just CACHE_MISSING. So rather than
>    lazily looking up and caching entries, we could just prepopulate the
>    cache. And then you know that if an entry isn't present in the
>    cache, it does not exist in the tree. The downside is that you pay
>    to walk the all of HEAD^{tree}, even if you only have a few lookups
>    to do. That's a good tradeoff for pack-objects (which usually ends
>    up looking for every path anyway), but not for "git diff" (where you
>    only care about a few changed paths.
>
>  - the cache here is static-local in the function. It should probably
>    at least be predicated on the tree_oid, and maybe attached to the
>    repository object? I think having one per repository at a time would
>    be fine (generally the tree_oid is set once per process, so it's not
>    like you're switching between multiple options).
>
>I've cc'd John as the author of 2386535511. But really, that was just enabling by
>default the attr-tree code added by 47cfc9bd7d (attr: add flag `--source` to work
>with tree-ish, 2023-01-14). Although in that original context (git check-attr) the
>lack of caching would be much less important.

Although this is an unbaked thought... 

I 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.

--Randall
Junio C Hamano May 1, 2024, 10:40 p.m. UTC | #2
Jeff King <peff@peff.net> writes:

>   - the cache here is static-local in the function. It should probably
>     at least be predicated on the tree_oid, and maybe attached to the
>     repository object? I think having one per repository at a time would
>     be fine (generally the tree_oid is set once per process, so it's not
>     like you're switching between multiple options).

It should be per tree_oid or you will get a stale and incorrect
result when you read paths from a different tree.  But thanks for
that "something simple and stupid" code to clearly demonstrate
that repeated reading of the attributes data is the problem.

Given a tree with a name, the result of reading a path from that
tree does not depend on the repository the tree appears in, so the
cache does not have a reason to be tied to a particular repository.
Generally we work only inside a single repository, so attaching the
cache to that single repository would be a good way to make it
available globally without adding another global variable, as
the_repository can serve as the starting point for the global state,
but other than that there is no reason.

I agree that the attribute layer may be a better place to cache this
data.  As you pointed out, it already has a caching behaviour in its
attr_stack data structure that is optimized for local walk that
visits every path in a tree in depth first order, but it is likely
that a different caching scheme that is more suitable for random
access may need to be introduced.  The cache eviction strategy may
need some thought (the attr_stack based caching has an obviously
optimal eviction strategy---to evict the attribute data read from a
directory when the traversal leaves that directory) in order to
avoid unbounded bloat of the cached data.

> I've cc'd John as the author of 2386535511. But really, that was just
> enabling by default the attr-tree code added by 47cfc9bd7d (attr: add
> flag `--source` to work with tree-ish, 2023-01-14). Although in that
> original context (git check-attr) the lack of caching would be much less
> important.
>
> -Peff
Taylor Blau May 2, 2024, 12:33 a.m. UTC | #3
On Wed, May 01, 2024 at 06:00:30PM -0400, Jeff King wrote:
> Bisecting show the culprit is 2386535511 (attr: read attributes from
> HEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a
> bare repository would only look for attributes in the info/attributes
> file. But after, we look at the HEAD tree-ish, too. And pack-objects
> will check the "delta" attribute for every path of every object we are
> packing. And remember that in-tree lookups for foo/bar/baz require
> looking not just for .gitattributes, but also foo/.gitattributes,
> foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.

Thanks for the explanation and bisection. I agree that 2386535511 makes
sense as a likely culprit given what you wrote here.

>   - the problem doesn't show up if the repo has reachability bitmaps.
>     This is because the bitmap result doesn't have the pathnames of each
>     object (we do have the "name hash", but it's not enough for us to do
>     an attr lookup), and so objects we get from a bitmap do not
>     respect the delta attribute at all.
>
>     But when doing a shallow clone, we have to disable bitmaps and do a
>     regular traversal. So even if you have bitmaps, you still run into
>     the problem.
>
>     The example above should not have bitmaps (we do build them by
>     default when repacking bare repos these days, but I don't think
>     we'll do so right after cloning). If you have a local repo that
>     already has bitmaps, you should be able to see the difference by
>     using "git -c pack.usebitmaps=false pack-objects".

Yikes. I was hoping that bitmaps would be a saving grace here for setups
that have bitmap generation enabled, but it makes sense that it doesn't
help if you are doing a shallow clone where you have to disable bitmaps.

>     So even if you are a server which generally enables bitmaps, you can
>     still get bit by this for shallow clones, but also for other
>     non-bitmap invocations, like say "git repack -ad". There I see the
>     same 3-minute slowdown in the enumeration phase.

That's also pretty scary, and a worthwhile callout.

> So what to do? It seems like some kind of caching would help here. We're
> looking up the same paths over and over, for two reasons:
>
>   1. We'll have many objects with the same paths, one for each time the
>      path was modified through history.
>
>   2. Adjacent objects share the higher-level lookups. Both "dir/a" and
>      "dir/b" will need to look up "dir/.gitattributes" (and all the way
>      up to ".gitattributes").

Right. I guess you need to cache something like on the order of the set
of dirnames of all modified paths in the repository (and recursively the
dirnames of those dirnames up until you get to the root).

> So even something simple and stupid like this:

...makes sense.

> restores the v2.42 performance. But there are probably better options:
>
>   - this is caching whole .gitattributes buffers. In pack-objects we
>     only care about a single bit for try_delta. For linux.git it doesn't
>     really matter, as 99% of our entries are just CACHE_MISSING and the
>     real value is avoiding the negative lookups. But the same problem
>     exists to a lesser degree for "git log -p" in a bare repo. So I
>     think it makes sense to try to solve it in the attr layer.
>
>   - the string keys have a lot of duplication. You'll have
>     "foo/.gitattributes", "foo/bar/.gitattributes", and so on. A trie
>     structure split by path component would let you store each component
>     just once. And perhaps have even faster lookups. I think this is
>     roughly the same issue faced by the kernel VFS for doing path
>     lookups, so something dentry/dcache-like would help.
>
>     I don't know how much it matters in practice, though. The sum of all
>     of the paths in HEAD for linux.git is ~3.5MB, which is a rounding
>     error on the needs of the rest of the packing process.

This was my gut reaction when I started reading this bullet point, too.
I have a hard time imagining a repository that would be so large that it
would have a lot of unique paths, but not so large that it would be
otherwise cheap to run pack-objects.

>   - As noted above, most entries are just CACHE_MISSING. So rather than
>     lazily looking up and caching entries, we could just prepopulate the
>     cache. And then you know that if an entry isn't present in the
>     cache, it does not exist in the tree. The downside is that you pay
>     to walk the all of HEAD^{tree}, even if you only have a few lookups
>     to do. That's a good tradeoff for pack-objects (which usually ends
>     up looking for every path anyway), but not for "git diff" (where you
>     only care about a few changed paths.

That seems reasonable to do.

Thanks,
Taylor
Dhruva Krishnamurthy May 2, 2024, 12:45 a.m. UTC | #4
On Wed, May 1, 2024 at 3:00 PM Jeff King <peff@peff.net> wrote:
> Oof. I was able to reproduce this regression, and the impact can be
> pretty severe. You can reproduce it without NFS and without shallow
> clones. Get a bare clone of something big like the kernel:

Wow, that was really fast! I spent quite some time narrowing down and
still was far off. I hope to develop such deep insights and
understanding of git code some day. Thank you very much for sharing
all the details and giving me leads to further explore.

-dhruva
Taylor Blau May 2, 2024, 5:33 p.m. UTC | #5
On Wed, May 01, 2024 at 08:33:52PM -0400, Taylor Blau wrote:
> On Wed, May 01, 2024 at 06:00:30PM -0400, Jeff King wrote:
> > Bisecting show the culprit is 2386535511 (attr: read attributes from
> > HEAD when bare repo, 2023-10-13), which is in v2.43.0. Before that, a
> > bare repository would only look for attributes in the info/attributes
> > file. But after, we look at the HEAD tree-ish, too. And pack-objects
> > will check the "delta" attribute for every path of every object we are
> > packing. And remember that in-tree lookups for foo/bar/baz require
> > looking not just for .gitattributes, but also foo/.gitattributes,
> > foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.
>
> Thanks for the explanation and bisection. I agree that 2386535511 makes
> sense as a likely culprit given what you wrote here.

Here is one possible approach, which is a partial revert of 2386535511.
I thought about suggesting that we revert 2386535511 entirely, but I
think that may be too strong of an approach especially if there are
plans to otherwise improve the performance of attr lookups with some
caching layer.

Instead, this patch changes the behavior to only fallback to "HEAD" in
bare repositories from check-attr, but leaves pack-objects, archive, and
all other builtins alone.

I should note, this is a pretty hacky approach to use the extern'd
git_attr_tree variable from within the check-attr builtin, but I think
that this does do the trick.

Alternatively, if this is too hacky or magical that check-attr does one
thing but every other command does something else, I would personally be
fine with a full revert of 2386535511.

Anyway, here is the patch:

--- 8< ---

Subject: [PATCH] attr.c: only read attributes from HEAD via check-attr

This patch is a partial revert of commit 2386535511d (attr: read
attributes from HEAD when bare repo, 2023-10-13), which caused Git to
start reading from .gitattributes files from HEAD^{tree} when invoked in
a bare repository.

This patch has an unfortunate side-effect of significantly slowing down
pack-objects, for example, when invoked in a bare repository without
using reachability bitmaps.

Prior to 2386535511d, pack-objects would only look at the
info/attributes file when working in a bare repository. But after,
pack-objects ends up looking at every "delta" attribute not just in the
info/attributes file, but for every .gitattributes file in each tree
recursively from the root down to the dirname of whatever path we're
inspecting. In other words, gathering attributes for path foo/bar/baz
requires reading .gitattributes, foo/.gitattributes,
foo/bar/.gitattributes, and foo/bar/baz/.gitattributes.

Restore the pre-2386535511d behavior for commands other than check-attr
(which was the intended target of the change described in 2386535511d).

If we want to cause pack-objects to use HEAD^{tree} as an attributes
source in bare repositories by default again, it would likely come after
some caching layer to avoid the performance penalty.

Reported-by: Dhruva Krishnamurthy <dhruvakm@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 attr.c                  | 7 -------
 builtin/check-attr.c    | 2 ++
 t/t5001-archive-attr.sh | 2 +-
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/attr.c b/attr.c
index 7c380c17317..33473bdce01 100644
--- a/attr.c
+++ b/attr.c
@@ -1220,17 +1220,10 @@ static void compute_default_attr_source(struct object_id *attr_source)
 	if (!default_attr_source_tree_object_name && git_attr_tree) {
 		default_attr_source_tree_object_name = git_attr_tree;
 		ignore_bad_attr_tree = 1;
 	}

-	if (!default_attr_source_tree_object_name &&
-	    startup_info->have_repository &&
-	    is_bare_repository()) {
-		default_attr_source_tree_object_name = "HEAD";
-		ignore_bad_attr_tree = 1;
-	}
-
 	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
 		return;

 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index c1da1d184e9..9b445fe33c6 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -188,10 +188,12 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)

 	if (source) {
 		if (repo_get_oid_tree(the_repository, source, &initialized_oid))
 			die("%s: not a valid tree-ish source", source);
 		set_git_attr_source(source);
+	} else if (startup_info->have_repository && is_bare_repository()) {
+		git_attr_tree = "HEAD";
 	}

 	if (stdin_paths)
 		check_attr_stdin_paths(prefix, check, all_attrs);
 	else {
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index eaf959d8f63..0ff47a239db 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -136,11 +136,11 @@ test_expect_success 'git archive with worktree attributes, bare' '
 	(cd bare && git archive --worktree-attributes HEAD) >bare-worktree.tar &&
 	(mkdir bare-worktree && cd bare-worktree && "$TAR" xf -) <bare-worktree.tar
 '

 test_expect_missing	bare-worktree/ignored
-test_expect_missing	bare-worktree/ignored-by-tree
+test_expect_exists	bare-worktree/ignored-by-tree
 test_expect_exists	bare-worktree/ignored-by-worktree

 test_expect_success 'export-subst' '
 	git log "--pretty=format:A${SUBSTFORMAT}O" HEAD >substfile1.expected &&
 	test_cmp nosubstfile archive/nosubstfile &&
--
2.45.0.1.g3e84e921a0a.dirty

--- >8 ---

Thanks,
Taylor
Junio C Hamano May 2, 2024, 5:44 p.m. UTC | #6
Taylor Blau <me@ttaylorr.com> writes:

> Instead, this patch changes the behavior to only fallback to "HEAD" in
> bare repositories from check-attr, but leaves pack-objects, archive, and
> all other builtins alone.

I thought the whole point of the exercise was to allow server-side
(which typically is bare and cannot use anything from the working
tree) to pay attention to the attributes.  This patch rips that out
and piles even more new and unproven code on top?  I am not sure.
Taylor Blau May 2, 2024, 5:55 p.m. UTC | #7
On Thu, May 02, 2024 at 10:44:20AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Instead, this patch changes the behavior to only fallback to "HEAD" in
> > bare repositories from check-attr, but leaves pack-objects, archive, and
> > all other builtins alone.
>
> I thought the whole point of the exercise was to allow server-side
> (which typically is bare and cannot use anything from the working
> tree) to pay attention to the attributes.  This patch rips that out
> and piles even more new and unproven code on top?  I am not sure.

I thought the point of John's patch was to allow just check-attr to read
from HEAD^{tree} in bare repositories, and not to touch other commands.

I could be misunderstanding the original intent of John's patch (the
commit message there isn't clear whether the change was intended to
target just check-attr or all of Git). But my hope is that it was the
former, which this patch preserves.

I do not know whether servers should in general be trusting
user-provided attributes for things like "delta".

Thanks,
Taylor
Dhruva Krishnamurthy May 2, 2024, 6:34 p.m. UTC | #8
On Thu, May 2, 2024 at 10:44 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Instead, this patch changes the behavior to only fallback to "HEAD" in
> > bare repositories from check-attr, but leaves pack-objects, archive, and
> > all other builtins alone.
>
> I thought the whole point of the exercise was to allow server-side
> (which typically is bare and cannot use anything from the working
> tree) to pay attention to the attributes.  This patch rips that out
> and piles even more new and unproven code on top?  I am not sure.

Yes, I am particularly interested in bare repositories (server/hosting side).
karthik nayak May 2, 2024, 7:01 p.m. UTC | #9
Taylor Blau <me@ttaylorr.com> writes:

> On Thu, May 02, 2024 at 10:44:20AM -0700, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>> > Instead, this patch changes the behavior to only fallback to "HEAD" in
>> > bare repositories from check-attr, but leaves pack-objects, archive, and
>> > all other builtins alone.
>>
>> I thought the whole point of the exercise was to allow server-side
>> (which typically is bare and cannot use anything from the working
>> tree) to pay attention to the attributes.  This patch rips that out
>> and piles even more new and unproven code on top?  I am not sure.
>
> I thought the point of John's patch was to allow just check-attr to read
> from HEAD^{tree} in bare repositories, and not to touch other commands.
>
> I could be misunderstanding the original intent of John's patch (the
> commit message there isn't clear whether the change was intended to
> target just check-attr or all of Git). But my hope is that it was the
> former, which this patch preserves.
>

From the series [1] it becomes more clear that the intention was to
target all commands.

[1]: https://lore.kernel.org/git/pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com/

> I do not know whether servers should in general be trusting
> user-provided attributes for things like "delta".
>
> Thanks,
> Taylor
Junio C Hamano May 2, 2024, 9:08 p.m. UTC | #10
Karthik Nayak <karthik.188@gmail.com> writes:

> Taylor Blau <me@ttaylorr.com> writes:
>
>> I could be misunderstanding the original intent of John's patch (the
>> commit message there isn't clear whether the change was intended to
>> target just check-attr or all of Git). But my hope is that it was the
>> former, which this patch preserves.
>>
>
> From the series [1] it becomes more clear that the intention was to
> target all commands.
>
> [1]: https://lore.kernel.org/git/pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com/

True.  

We could drop [1/2] from the series in the meantime to make it a
GitLab installation specific issue where they explicitly use
attr.tree to point at HEAD ;-) That is not solving anything for
those who set attr.tree (in a sense, they are buying the feature
with overhead of reading attributes from the named tree), but at
least for most people who are used to seeing the bare repository
ignoring the attributes, it would be an improvement to drop the
"bare repositories the tree of the HEAD commit is used to look up
attributes files by default" half from the series.
Dhruva Krishnamurthy May 3, 2024, 5:37 a.m. UTC | #11
On Thu, May 2, 2024 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> We could drop [1/2] from the series in the meantime to make it a
> GitLab installation specific issue where they explicitly use
> attr.tree to point at HEAD ;-) That is not solving anything for
> those who set attr.tree (in a sense, they are buying the feature
> with overhead of reading attributes from the named tree), but at
> least for most people who are used to seeing the bare repository
> ignoring the attributes, it would be an improvement to drop the
> "bare repositories the tree of the HEAD commit is used to look up
> attributes files by default" half from the series.
>

A hack (without knowing side effects if any) is to use an empty tree
for attr source:
$ git config --add attr.tree $(git hash-object -t tree /dev/null)

This gives me performance comparable to git 2.42
diff mbox series

Patch

diff --git a/attr.c b/attr.c
index 679e42258c..b32af9a78b 100644
--- a/attr.c
+++ b/attr.c
@@ -24,6 +24,7 @@ 
 #include "thread-utils.h"
 #include "tree-walk.h"
 #include "object-name.h"
+#include "strmap.h"
 
 const char *git_attr_tree;
 
@@ -795,25 +796,31 @@  static struct attr_stack *read_attr_from_blob(struct index_state *istate,
 					      const struct object_id *tree_oid,
 					      const char *path, unsigned flags)
 {
-	struct object_id oid;
-	unsigned long sz;
-	enum object_type type;
+	static struct strmap cache = STRMAP_INIT;
+	void *CACHE_MISSING = (void *)1;
 	void *buf;
-	unsigned short mode;
 
 	if (!tree_oid)
 		return NULL;
 
-	if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
-		return NULL;
-
-	buf = repo_read_object_file(istate->repo, &oid, &type, &sz);
-	if (!buf || type != OBJ_BLOB) {
-		free(buf);
-		return NULL;
+	buf = strmap_get(&cache, path);
+	if (!buf) {
+		struct object_id oid;
+		unsigned short mode;
+		if (!get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) {
+			unsigned long sz;
+			enum object_type type;
+			buf = repo_read_object_file(istate->repo, &oid, &type, &sz);
+			if (!buf || type != OBJ_BLOB) {
+				free(buf);
+				buf = NULL;
+			}
+		}
+		strmap_put(&cache, path, buf ? buf : CACHE_MISSING);
 	}
 
-	return read_attr_from_buf(buf, path, flags);
+	return buf == CACHE_MISSING ? NULL :
+		read_attr_from_buf(buf, path, flags);
 }
 
 static struct attr_stack *read_attr_from_index(struct index_state *istate,