| Message ID | YNILCDz3LpHX7OX0@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some "log --decorate" optimizations | expand |
On 6/22/2021 12:08 PM, Jeff King wrote: > - obj = parse_object(the_repository, oid); > - if (!obj) > + objtype = oid_object_info(the_repository, oid, NULL); > + if (type < 0) > return 0; Do you mean "if (objtype < 0)" here? There is a 'type' variable, but it is an enum decoration_type and I can't find a reason why it would be negative. oid_object_info() _does_ return -1 if there is a problem loading the object, so that would make sense. This is the only question I had about the entire series. Everything else LGTM. Thanks, -Stolee
On Tue, Jun 22 2021, Jeff King wrote: > When we load the ref decorations, we parse the object pointed to by each > ref in order to get a "struct object". This is unnecessarily expensive; > we really only need the object struct, and don't even look at the parsed > contents. The exception is tags, which we do need to peel. > > We can improve this by looking up the object type first (which is much > cheaper), and skipping the parse entirely for non-tags. This increases > the work slightly for annotated tags (which now do a type lookup _and_ a > parse), but decreases it a lot for other types. On balance, this seems > to be a good tradeoff. > > In my git.git clone, with ~2k refs, most of which are branches, the time > to run "git log -1 --decorate" drops from 34ms to 11ms. Even on my > linux.git clone, which contains mostly tags and only a handful of > branches, the time drops from 30ms to 19ms. And on a more extreme > real-world case with ~220k refs, mostly non-tags, the time drops from > 2.6s to 650ms. > > That command is a lop-sided example, of course, because it does as > little non-loading work as possible. But it does show the absolute time > improvement. Even in something like a full "git log --decorate" on that > extreme repo, we'd still be saving 2s of CPU time. > > Ideally we could push this even further, and avoid parsing even tags, by > relying on the packed-refs "peel" optimization (which we could do by > calling peel_iterated_oid() instead of peeling manually). But we can't > do that here. The packed-refs file only stores the bottom-layer of the > peel (so in a "tag->tag->commit" chain, it stores only the commit as the > peel result). But the decoration code wants to peel the layers > individually, annotating the middle layers of the chain. > > If the packed-refs file ever learns to store all of the peeled layers, > then we could switch to it. Or even if it stored a flag to indicate the > peel was not multi-layer (because most of them aren't), then we could > use it most of the time and fall back to a manual peel for the rare > cases. > > Signed-off-by: Jeff King <peff@peff.net> > --- > log-tree.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/log-tree.c b/log-tree.c > index 7b823786c2..8b700e9c14 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > int flags, void *cb_data) > { > struct object *obj; > + enum object_type objtype; > enum decoration_type type = DECORATION_NONE; > struct decoration_filter *filter = (struct decoration_filter *)cb_data; > > @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > return 0; > } > > - obj = parse_object(the_repository, oid); > - if (!obj) > + objtype = oid_object_info(the_repository, oid, NULL); > + if (type < 0) > return 0; > + obj = lookup_object_by_type(the_repository, oid, objtype); This series looks good. I just wonder if between this and my own lookup_{blob,tree,tag,commit}_type() in [1] whether exposing some function between what we have now in parse_object() and parse_object_buffer() wouldn't also do this for you. I.e. in my patch if you pass a type into parse_object_buffer() I think you'll get the same behavior. To be clear I see nothing wrong with this, it's more of a musing about how some functions in object.c discover the type on their own, others allow passing it in, sometimes (worse before that series of mine) we relay the not-real-but-inferred-type etc. 1. https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/
On Tue, Jun 22, 2021 at 12:35:46PM -0400, Derrick Stolee wrote: > On 6/22/2021 12:08 PM, Jeff King wrote: > > > - obj = parse_object(the_repository, oid); > > - if (!obj) > > + objtype = oid_object_info(the_repository, oid, NULL); > > + if (type < 0) > > return 0; > > Do you mean "if (objtype < 0)" here? There is a 'type' variable, > but it is an enum decoration_type and I can't find a reason why > it would be negative. oid_object_info() _does_ return -1 if there > is a problem loading the object, so that would make sense. Whoops, thanks for catching that. I originally called it "enum object_type type", but then of course the compiler informed that there was already a "type" variable in the function. So I renamed it to "objtype" but missed updating that line. But it still compiled. Yikes. :) It doesn't trigger in the test suite because it only happens if the repository is corrupted. > This is the only question I had about the entire series. Everything > else LGTM. Thanks. Here's an amended version of the patch, in case there are no other fixups necessary (fingers crossed). -Peff -- >8 -- Subject: [PATCH] load_ref_decorations(): avoid parsing non-tag objects When we load the ref decorations, we parse the object pointed to by each ref in order to get a "struct object". This is unnecessarily expensive; we really only need the object struct, and don't even look at the parsed contents. The exception is tags, which we do need to peel. We can improve this by looking up the object type first (which is much cheaper), and skipping the parse entirely for non-tags. This increases the work slightly for annotated tags (which now do a type lookup _and_ a parse), but decreases it a lot for other types. On balance, this seems to be a good tradeoff. In my git.git clone, with ~2k refs, most of which are branches, the time to run "git log -1 --decorate" drops from 34ms to 11ms. Even on my linux.git clone, which contains mostly tags and only a handful of branches, the time drops from 30ms to 19ms. And on a more extreme real-world case with ~220k refs, mostly non-tags, the time drops from 2.6s to 650ms. That command is a lop-sided example, of course, because it does as little non-loading work as possible. But it does show the absolute time improvement. Even in something like a full "git log --decorate" on that extreme repo, we'd still be saving 2s of CPU time. Ideally we could push this even further, and avoid parsing even tags, by relying on the packed-refs "peel" optimization (which we could do by calling peel_iterated_oid() instead of peeling manually). But we can't do that here. The packed-refs file only stores the bottom-layer of the peel (so in a "tag->tag->commit" chain, it stores only the commit as the peel result). But the decoration code wants to peel the layers individually, annotating the middle layers of the chain. If the packed-refs file ever learns to store all of the peeled layers, then we could switch to it. Or even if it stored a flag to indicate the peel was not multi-layer (because most of them aren't), then we could use it most of the time and fall back to a manual peel for the rare cases. Signed-off-by: Jeff King <peff@peff.net> --- log-tree.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/log-tree.c b/log-tree.c index 7b823786c2..c3c8e7e1df 100644 --- a/log-tree.c +++ b/log-tree.c @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, int flags, void *cb_data) { struct object *obj; + enum object_type objtype; enum decoration_type type = DECORATION_NONE; struct decoration_filter *filter = (struct decoration_filter *)cb_data; @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, return 0; } - obj = parse_object(the_repository, oid); - if (!obj) + objtype = oid_object_info(the_repository, oid, NULL); + if (objtype < 0) return 0; + obj = lookup_object_by_type(the_repository, oid, objtype); if (starts_with(refname, "refs/heads/")) type = DECORATION_REF_LOCAL;
On Tue, Jun 22, 2021 at 01:06:48PM -0400, Jeff King wrote: > > Do you mean "if (objtype < 0)" here? There is a 'type' variable, > > but it is an enum decoration_type and I can't find a reason why > > it would be negative. oid_object_info() _does_ return -1 if there > > is a problem loading the object, so that would make sense. > > Whoops, thanks for catching that. I originally called it "enum > object_type type", but then of course the compiler informed that there > was already a "type" variable in the function. So I renamed it to > "objtype" but missed updating that line. But it still compiled. Yikes. :) > > It doesn't trigger in the test suite because it only happens if the > repository is corrupted. I'm tempted by this as a cleanup on top (I don't want to do it inline, since the diff is so noisy). But I'm also content to leave it. -- >8 -- Subject: [PATCH] add_ref_decoration(): rename s/type/deco_type/ Now that we have two types (a decoration type and an object type) in the function, let's give them both unique names to avoid accidentally using one instead of the other. Signed-off-by: Jeff King <peff@peff.net> --- log-tree.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/log-tree.c b/log-tree.c index c3c8e7e1df..4f69ed176d 100644 --- a/log-tree.c +++ b/log-tree.c @@ -135,7 +135,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, { struct object *obj; enum object_type objtype; - enum decoration_type type = DECORATION_NONE; + enum decoration_type deco_type = DECORATION_NONE; struct decoration_filter *filter = (struct decoration_filter *)cb_data; if (filter && !ref_filter_match(refname, filter)) @@ -162,17 +162,17 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, obj = lookup_object_by_type(the_repository, oid, objtype); if (starts_with(refname, "refs/heads/")) - type = DECORATION_REF_LOCAL; + deco_type = DECORATION_REF_LOCAL; else if (starts_with(refname, "refs/remotes/")) - type = DECORATION_REF_REMOTE; + deco_type = DECORATION_REF_REMOTE; else if (starts_with(refname, "refs/tags/")) - type = DECORATION_REF_TAG; + deco_type = DECORATION_REF_TAG; else if (!strcmp(refname, "refs/stash")) - type = DECORATION_REF_STASH; + deco_type = DECORATION_REF_STASH; else if (!strcmp(refname, "HEAD")) - type = DECORATION_REF_HEAD; + deco_type = DECORATION_REF_HEAD; - add_name_decoration(type, refname, obj); + add_name_decoration(deco_type, refname, obj); while (obj->type == OBJ_TAG) { obj = ((struct tag *)obj)->tagged; if (!obj)
On 6/22/2021 1:09 PM, Jeff King wrote: > On Tue, Jun 22, 2021 at 01:06:48PM -0400, Jeff King wrote: > >>> Do you mean "if (objtype < 0)" here? There is a 'type' variable, >>> but it is an enum decoration_type and I can't find a reason why >>> it would be negative. oid_object_info() _does_ return -1 if there >>> is a problem loading the object, so that would make sense. >> >> Whoops, thanks for catching that. I originally called it "enum >> object_type type", but then of course the compiler informed that there >> was already a "type" variable in the function. So I renamed it to >> "objtype" but missed updating that line. But it still compiled. Yikes. :) >> >> It doesn't trigger in the test suite because it only happens if the >> repository is corrupted. > > I'm tempted by this as a cleanup on top (I don't want to do it inline, > since the diff is so noisy). But I'm also content to leave it. Naming is hard. This change seems worth it. Thanks, -Stolee
On Tue, Jun 22 2021, Jeff King wrote: > On Tue, Jun 22, 2021 at 12:35:46PM -0400, Derrick Stolee wrote: > >> On 6/22/2021 12:08 PM, Jeff King wrote: >> >> > - obj = parse_object(the_repository, oid); >> > - if (!obj) >> > + objtype = oid_object_info(the_repository, oid, NULL); >> > + if (type < 0) >> > return 0; >> >> Do you mean "if (objtype < 0)" here? There is a 'type' variable, >> but it is an enum decoration_type and I can't find a reason why >> it would be negative. oid_object_info() _does_ return -1 if there >> is a problem loading the object, so that would make sense. > > Whoops, thanks for catching that. I originally called it "enum > object_type type", but then of course the compiler informed that there > was already a "type" variable in the function. So I renamed it to > "objtype" but missed updating that line. But it still compiled. Yikes. :) [Enter Captain Hindsight] If you use a slightly different coding style and leverage the information the compiler has to work with you'd get it to error for you, e.g. this on your original patch would catch it: diff --git a/log-tree.c b/log-tree.c index 8b700e9c142..7e3a011b533 100644 --- a/log-tree.c +++ b/log-tree.c @@ -157,9 +157,12 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, } objtype = oid_object_info(the_repository, oid, NULL); - if (type < 0) + switch (type) { + case OBJ_BAD: return 0; - obj = lookup_object_by_type(the_repository, oid, objtype); + default: + obj = lookup_object_by_type(the_repository, oid, objtype); + } if (starts_with(refname, "refs/heads/")) type = DECORATION_REF_LOCAL; IMO the real problem is an over-reliance on C being so happy to treat enums as ints (well, with them being ints). If you consistently use labels you get the compiler to do the checking. For me with gcc and clang with that on top: log-tree.c:161:2: error: case value ‘4294967295’ not in enumerated type ‘enum decoration_type’ [-Werror=switch] case OBJ_BAD: ^~~~ log-tree.c:161:7: error: case value not in enumerated type 'enum decoration_type' [-Werror,-Wswitch] case OBJ_BAD: ^ I think we've disagreed on that exact point before recently, i.e. you think we shouldn't rely on OBJ_BAD in that way, and instead check for any negative value: https://lore.kernel.org/git/YHCZh5nLNVEHCWV2@coredump.intra.peff.net/ This sort of thing is a good reason to pick the opposite pattern. You get the same type checking you'd usually get with anything else in C. Yes, it is more verbose e.g. in this case, and particularly (as noted downthread of what I linked to) because "enum object_type" contains so many uncommon things, and really should be split up. In practice I don't think it's too verbose, because once you start consistently using the pattern you'll usually not be doing conversions all over the place, and would just do this sort of thing via a helper that does the type checking, e.g. something like this (or anything else where you don't lose the type & labels): diff --git a/log-tree.c b/log-tree.c index 8b700e9c142..a61fb01ba3f 100644 --- a/log-tree.c +++ b/log-tree.c @@ -130,6 +130,30 @@ static int ref_filter_match(const char *refname, return 1; } +static enum object_type oid_object_info_ok(struct repository *repo, + struct object_id *oid, + enum object_type *typep, + unsigned long *sizep) +{ + enum object_type type = oid_object_info(repo, oid, sizep); + *typep = type; + switch (type) { + case OBJ_BAD: + return 0; + case OBJ_COMMIT: + case OBJ_TREE: + case OBJ_BLOB: + case OBJ_TAG: + return 1; + case OBJ_NONE: + case OBJ_OFS_DELTA: + case OBJ_REF_DELTA: + case OBJ_ANY: + case OBJ_MAX: + BUG("the enum_object type is too large!"); + } +} + static int add_ref_decoration(const char *refname, const struct object_id *oid, int flags, void *cb_data) { @@ -156,8 +180,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, return 0; } - objtype = oid_object_info(the_repository, oid, NULL); - if (type < 0) + if (!oid_object_info_ok(the_repository, oid, &type, NULL)) return 0; obj = lookup_object_by_type(the_repository, oid, objtype); With that pattern GCC narrowlry pulls ahead with showing 4 warnings just about the loss of the type, with Clang at 3 :)
On Tue, Jun 22, 2021 at 07:06:43PM +0200, Ævar Arnfjörð Bjarmason wrote: > > @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > > return 0; > > } > > > > - obj = parse_object(the_repository, oid); > > - if (!obj) > > + objtype = oid_object_info(the_repository, oid, NULL); > > + if (type < 0) > > return 0; > > + obj = lookup_object_by_type(the_repository, oid, objtype); > > This series looks good. I just wonder if between this and my own > lookup_{blob,tree,tag,commit}_type() in [1] whether exposing some > function between what we have now in parse_object() and > parse_object_buffer() wouldn't also do this for you. > > I.e. in my patch if you pass a type into parse_object_buffer() I think > you'll get the same behavior. Maybe, but I'm having trouble seeing what would be a helpful abstraction. I don't think I'd want to use parse_object_buffer() here; we don't have a buffer at all (and obviously it could learn to handle NULL, but that's extra code there). parse_object_buffer() could call this lookup_object_by_type() to get the struct, which would save it a few lines. But it still has to do the if-else chain for each type, because it does other type-specific things. So I dunno. I would be happy if you came up with something, just because it's nice to minimize the number of places that do this if-else/switch on type. But I have a feeling it's diminishing returns in terms of complexity. If we can at least contain it all to object.c, that's something. -Peff
On Tue, Jun 22, 2021 at 08:27:53PM +0200, Ævar Arnfjörð Bjarmason wrote: > > Whoops, thanks for catching that. I originally called it "enum > > object_type type", but then of course the compiler informed that there > > was already a "type" variable in the function. So I renamed it to > > "objtype" but missed updating that line. But it still compiled. Yikes. :) > > [Enter Captain Hindsight] > > If you use a slightly different coding style and leverage the > information the compiler has to work with you'd get it to error for you, > e.g. this on your original patch would catch it: > > diff --git a/log-tree.c b/log-tree.c > index 8b700e9c142..7e3a011b533 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -157,9 +157,12 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > } > > objtype = oid_object_info(the_repository, oid, NULL); > - if (type < 0) > + switch (type) { > + case OBJ_BAD: > return 0; > - obj = lookup_object_by_type(the_repository, oid, objtype); > + default: > + obj = lookup_object_by_type(the_repository, oid, objtype); > + } > > if (starts_with(refname, "refs/heads/")) > type = DECORATION_REF_LOCAL; Yeah, I agree that would find it in this case. I do find that style slightly harder to read, though. And... > IMO the real problem is an over-reliance on C being so happy to treat > enums as ints (well, with them being ints). If you consistently use > labels you get the compiler to do the checking. For me with gcc and > clang with that on top: > > log-tree.c:161:2: error: case value ‘4294967295’ not in enumerated type ‘enum decoration_type’ [-Werror=switch] > case OBJ_BAD: > ^~~~ > log-tree.c:161:7: error: case value not in enumerated type 'enum decoration_type' [-Werror,-Wswitch] > case OBJ_BAD: > ^ ...it would help in this case because OBJ_BAD happens to have a value that is not defined for decoration_type. If it did, then the compiler would be quite happy to consider them equivalent. So I don't disagree with you exactly, but I'm not sure of the tradeoff of always using switches instead of conditionals (which IMHO is less readable) for more compiler safety that only works sometimes is worth it. > I think we've disagreed on that exact point before recently, i.e. you > think we shouldn't rely on OBJ_BAD in that way, and instead check for > any negative value: > https://lore.kernel.org/git/YHCZh5nLNVEHCWV2@coredump.intra.peff.net/ To be clear, my complaint about checking for OBJ_BAD exactly is that it closes the door on other negative return types. And indeed, the switch() you showed above would become a silent bug if we introduced OBJ_BAD_FOR_ANOTHER_READ as "-2" (without any compiler support, because of the "default" case in the switch statement). Now that's somewhat hypothetical, but in the near-term it also means confirming that any of the functions which get converted from "int" to "enum object_type" are not in fact passing back "-2" in any circumstances. That said... > In practice I don't think it's too verbose, because once you start > consistently using the pattern you'll usually not be doing conversions > all over the place, and would just do this sort of thing via a helper > that does the type checking, e.g. something like this (or anything else > where you don't lose the type & labels): > [...] > - objtype = oid_object_info(the_repository, oid, NULL); > - if (type < 0) > + if (!oid_object_info_ok(the_repository, oid, &type, NULL)) > return 0; Yes, that would deal with that problem. It's definitely a different style, but one that I could get used to. It's a lot more object oriented ("you are not allowed to do numeric logic on an object type; you can only use these accessor methods to query it"). If we were going that route, I would stop having "enum object_type" at all, and instead make it "struct object_type { enum { ... } value }". That would prevent anybody from accidentally just looking at it, and instead force people into that object-oriented style. I dunno. It is is a big departure from how we do things now. And the bug here notwithstanding, I don't feel like enum confusion has generally been a big source of error for us. -Peff
On Tue, Jun 22, 2021 at 12:08:40PM -0400, Jeff King wrote: > If the packed-refs file ever learns to store all of the peeled layers, > then we could switch to it. Or even if it stored a flag to indicate the > peel was not multi-layer (because most of them aren't), then we could > use it most of the time and fall back to a manual peel for the rare > cases. Yeah, I would be in favor of either of these. Of the two, the latter seems like the simplest thing, but the former provides you all of the information you could hope for. I suppose that if you are already changing the format of packed-refs, then we might as well do the thing which provides the most information and allows us to optimize *all* cases, not just the vast majority of them. Of course, that's all way outside of the scope of this patch, which shouldn't have to deal with either of those. > Signed-off-by: Jeff King <peff@peff.net> > --- > log-tree.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/log-tree.c b/log-tree.c > index 7b823786c2..8b700e9c14 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > int flags, void *cb_data) > { > struct object *obj; > + enum object_type objtype; > enum decoration_type type = DECORATION_NONE; > struct decoration_filter *filter = (struct decoration_filter *)cb_data; > > @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, > return 0; > } > > - obj = parse_object(the_repository, oid); > - if (!obj) > + objtype = oid_object_info(the_repository, oid, NULL); > + if (type < 0) > return 0; > + obj = lookup_object_by_type(the_repository, oid, objtype); The comments about s/type/obj&/ aside, this looks good to me (and the amended version below looks ready to be picked up, at least in my eyes). One thing I did want to note which is elided by the limited context is that we *do* parse tags like you say, it's just hidden in the "while (obj->type == OBJ_TAG)" loop below. So that's doing the right thing, but it wasn't clear from the limited context here that this patch was immediately correct. Thanks, Taylor
On Tue, Jun 22, 2021 at 10:46:40PM -0400, Taylor Blau wrote: > On Tue, Jun 22, 2021 at 12:08:40PM -0400, Jeff King wrote: > > If the packed-refs file ever learns to store all of the peeled layers, > > then we could switch to it. Or even if it stored a flag to indicate the > > peel was not multi-layer (because most of them aren't), then we could > > use it most of the time and fall back to a manual peel for the rare > > cases. > > Yeah, I would be in favor of either of these. Of the two, the latter > seems like the simplest thing, but the former provides you all of the > information you could hope for. > > I suppose that if you are already changing the format of packed-refs, > then we might as well do the thing which provides the most information > and allows us to optimize *all* cases, not just the vast majority of > them. One reason not to include all of them is that the list can be arbitrarily long, and regular readers of packed-refs (who may not even care about peeling at all) have to skip past it. That matters a little less these since we binary-search it (but you still might be iterating over the ref). So I think either way it is a tradeoff, and you are making assumptions about which cases are less likely. If I were to work on this (and I don't have any immediate plans to do so), I'd probably do whichever is easiest to implement, and to maintain backwards-compatibility. And I suspect that is the "flag" approach, but a lot would depend on the details of our parser and what it permits. -Peff
diff --git a/log-tree.c b/log-tree.c index 7b823786c2..8b700e9c14 100644 --- a/log-tree.c +++ b/log-tree.c @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, int flags, void *cb_data) { struct object *obj; + enum object_type objtype; enum decoration_type type = DECORATION_NONE; struct decoration_filter *filter = (struct decoration_filter *)cb_data; @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, return 0; } - obj = parse_object(the_repository, oid); - if (!obj) + objtype = oid_object_info(the_repository, oid, NULL); + if (type < 0) return 0; + obj = lookup_object_by_type(the_repository, oid, objtype); if (starts_with(refname, "refs/heads/")) type = DECORATION_REF_LOCAL;
When we load the ref decorations, we parse the object pointed to by each ref in order to get a "struct object". This is unnecessarily expensive; we really only need the object struct, and don't even look at the parsed contents. The exception is tags, which we do need to peel. We can improve this by looking up the object type first (which is much cheaper), and skipping the parse entirely for non-tags. This increases the work slightly for annotated tags (which now do a type lookup _and_ a parse), but decreases it a lot for other types. On balance, this seems to be a good tradeoff. In my git.git clone, with ~2k refs, most of which are branches, the time to run "git log -1 --decorate" drops from 34ms to 11ms. Even on my linux.git clone, which contains mostly tags and only a handful of branches, the time drops from 30ms to 19ms. And on a more extreme real-world case with ~220k refs, mostly non-tags, the time drops from 2.6s to 650ms. That command is a lop-sided example, of course, because it does as little non-loading work as possible. But it does show the absolute time improvement. Even in something like a full "git log --decorate" on that extreme repo, we'd still be saving 2s of CPU time. Ideally we could push this even further, and avoid parsing even tags, by relying on the packed-refs "peel" optimization (which we could do by calling peel_iterated_oid() instead of peeling manually). But we can't do that here. The packed-refs file only stores the bottom-layer of the peel (so in a "tag->tag->commit" chain, it stores only the commit as the peel result). But the decoration code wants to peel the layers individually, annotating the middle layers of the chain. If the packed-refs file ever learns to store all of the peeled layers, then we could switch to it. Or even if it stored a flag to indicate the peel was not multi-layer (because most of them aren't), then we could use it most of the time and fall back to a manual peel for the rare cases. Signed-off-by: Jeff King <peff@peff.net> --- log-tree.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)