All of lore.kernel.org
 help / color / mirror / Atom feed
From: ZheNing Hu <adlternative@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] ref-filter: hacky "streaming" mode
Date: Tue, 7 Sep 2021 13:28:33 +0800	[thread overview]
Message-ID: <CAOLTT8QbdNBSY95bCa+UNJBqsJEEHbnaKfZLzvN2Qzd-Np8Lqg@mail.gmail.com> (raw)
In-Reply-To: <YTTARcEvpXWSDfYW@coredump.intra.peff.net>

Jeff King <peff@peff.net> 于2021年9月5日周日 下午9:04写道:
>
> On Sun, Sep 05, 2021 at 04:20:02PM +0800, ZheNing Hu wrote:
>
> > But here is a terrible fact: we did not use ref_array_sort() for sorting here.
> > So in fact, for_each_fullref_in() does the sorting work () for us by
> > default sort (%(refname)),
> > This may be due to the file system or some implementation of ref_iterator.
> > But this limit the application of this optimization when we use other
> > atoms to sort.
>
> Right. This technique does not help at all if you use --sort. I do think
> it's reasonable to rely on the sorted output of the for_each_ref()
> functions; other parts of Git likely do as well, so I think we'd try
> pretty hard to maintain that (and no, it's not a filesystem thing; we do
> make sure to sort it ourselves; see the calls to sort_ref_dir()).
>
> > > +       /*
> > > +        * the %(symref) placeholder is broken with pretty_print_ref(),
> > > +        * which our streaming code uses. I suspect this is a sign of breakage
> > > +        * in other callers like verify_tag(), which should be fixed. But for
> > > +        * now just disable streaming.
> > > +        *
> > > +        * Note that this implies we've parsed the format already with
> > > +        * verify_ref_format().
> > > +        */
> > > +       if (need_symref)
> > > +               return;
> > > +
> >
> > I haven’t taken a closer look at why pretty_print_ref() does not
> > support %(symref),
> > we can skip it first.
>
> I think it's just because pretty_print_ref() does not take a "flag"
> parameter for the caller. So it never sees that REF_ISSYMREF is set.
>

yeah, pretty_print_ref() does not set the flag, this is a defect of
pretty_print_ref(), maybe we need to find a way to set this flag.
I also deliberately avoided %(symref) when refactoring git cat-file --batch,
perhaps the improvements here can also be applied to git cat-file --batch

> There aren't many callers of that function, so I think nobody ever
> really noticed. But you can see the bug like this:
>
>   git init repo
>   cd repo
>
>   git commit --allow-empty -m foo
>   git tag -s -m bar annotated &&
>   git symbolic-ref refs/tags/symref refs/tags/annotated
>
>   # this is ok; ref-filter handles the flags
>   git tag --list --format='list: %(refname) %(symref)'
>
>   # not ok; we do not tell the formatter about the flags, so it does
>   # not notice that "symref" is a symref
>   git tag --verify --format='verify: %(refname) %(symref)' annotated symref
>
> I notice that the --verify output also shows the short refname, which
> makes me wonder if %(symref) would have other bugs there (because it
> has to re-resolve the ref to come up with the symref destination).
>

This may be easy to fix:

diff --git a/builtin/tag.c b/builtin/tag.c
index 452558ec95..4be5d36366 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -152,11 +152,11 @@ static int verify_tag(const char *name, const char *ref,
        if (format->format)
                flags = GPG_VERIFY_OMIT_STATUS;

-       if (gpg_verify_tag(oid, name, flags))
+       if (gpg_verify_tag(oid, ref, flags))
                return -1;

        if (format->format)
-               pretty_print_ref(name, oid, format);
+               pretty_print_ref(ref, oid, format);

        return 0;
 }

> > Unfortunately, this optimization may not be helpful for git cat-file --batch,
> > see [1], batch_object_write() directly constructs a ref_array_item and call
> > format_ref_array_item() to grab data, It does not use ref_array. So it also
> > does not have this malloc() | free() overhead.
>
> Right. It would only be helped by the "quick" formats in the second
> patch (and by skipping the malloc/free of the individual
> ref_array_items).
>

Yes, your test is indeed worth thinking about: how to avoid
intermediate data to reduce overhead.

> -Peff

Thanks.
--
ZheNing Hu

  reply	other threads:[~2021-09-07  5:28 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-04 12:40 [hacky PATCH 0/2] speeding up trivial for-each-ref invocations Jeff King
2021-09-04 12:41 ` [PATCH 1/2] ref-filter: hacky "streaming" mode Jeff King
2021-09-05  8:20   ` ZheNing Hu
2021-09-05 13:04     ` Jeff King
2021-09-07  5:28       ` ZheNing Hu [this message]
2021-09-07 18:01         ` Jeff King
2021-09-09 14:45           ` ZheNing Hu
2021-09-10 14:26             ` Jeff King
2021-09-15 12:27               ` ZheNing Hu
2021-09-15 14:23                 ` ZheNing Hu
2021-09-16 21:45                   ` Jeff King
2021-09-20  7:42                     ` ZheNing Hu
2021-09-16 21:31                 ` Jeff King
2021-09-05 13:15     ` Jeff King
2021-09-07  5:42       ` ZheNing Hu
2021-09-04 12:42 ` [PATCH 2/2] ref-filter: implement "quick" formats Jeff King
2021-09-05  8:20   ` ZheNing Hu
2021-09-05 13:07     ` Jeff King
2021-09-06 13:34       ` ZheNing Hu
2021-09-07 20:06       ` Junio C Hamano
2021-09-05  8:19 ` [hacky PATCH 0/2] speeding up trivial for-each-ref invocations ZheNing Hu
2021-09-05 12:49   ` Jeff King
2021-09-06 13:30     ` ZheNing Hu
2021-09-07 17:28       ` Jeff King
2021-09-09 13:20         ` ZheNing Hu
2021-09-06  6:54 ` Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOLTT8QbdNBSY95bCa+UNJBqsJEEHbnaKfZLzvN2Qzd-Np8Lqg@mail.gmail.com \
    --to=adlternative@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.