git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: ZheNing Hu <adlternative@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git List" <git@vger.kernel.org>, "Jeff King" <peff@peff.net>,
	"Christian Couder" <chriscool@tuxfamily.org>,
	"Hariom Verma" <hariom18599@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output
Date: Thu, 8 Apr 2021 20:05:39 +0800	[thread overview]
Message-ID: <CAOLTT8RYjp1GKQpdPSzSNnn9o3+tuKNcGEUTp5HD9_Jut01JMQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqczv5zvj3.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年4月8日周四 上午4:31写道:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Subject: Re: [PATCH v2] [GSOC] ref-filter: use single strbuf for all output
>
> The implementation changed so much from the initial attempt, for
> which the above title may have been appropriate, that reusing single
> strbuf over and over is not the most important part of the change
> anymore, I am afraid.  Besides, it uses TWO strbufs ;-)
>
> Subject: [PATCH] ref-filter: introduce show_ref_array_items() helper
>
> or something like that?
>

Yep, I may think that its core is still reusing strbufs, but
"introduce show_ref_array_items()"  will be more accurate.

> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > When we use `git for-each-ref`, every ref will call
> > `show_ref_array_item()` and allocate its own final strbuf
> > and error strbuf. Instead, we can reuse these two strbuf
> > for each step ref's output.
> >
> > The performance for `git for-each-ref` on the Git repository
> > itself with performance testing tool `hyperfine` changes from
> > 18.7 ms ± 0.4 ms to 18.2 ms ± 0.3 ms.
> >
> > This approach is similar to the one used by 79ed0a5
> > (cat-file: use a single strbuf for all output, 2018-08-14)
> > to speed up the cat-file builtin.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     [GSOC] ref-filter: use single strbuf for all output
> >
> >     Now git for-each-ref can reuse two buffers for all refs output, the
> >     performance is slightly improved.
> >
> >     Now there may be a question : Should the original interface
> >     show_ref_array_items be retained?
> > ...
> >        /*  Callback function for parsing the sort option */
>
> Again, not a very useful range-diff as the implementation changed so much.
>

This makes me wonder if I should give up GGG in the future.
I also don’t want a rang-diff with a big difference.

>
> >  builtin/for-each-ref.c |  4 +---
> >  ref-filter.c           | 20 ++++++++++++++++++++
> >  ref-filter.h           |  5 +++++
> >  3 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> > index cb9c81a04606..d630402230f3 100644
> > --- a/builtin/for-each-ref.c
> > +++ b/builtin/for-each-ref.c
> > @@ -16,7 +16,6 @@ static char const * const for_each_ref_usage[] = {
> >
> >  int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> >  {
> > -     int i;
> >       struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
> >       int maxcount = 0, icase = 0;
> >       struct ref_array array;
> > @@ -80,8 +79,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> >
> >       if (!maxcount || array.nr < maxcount)
> >               maxcount = array.nr;
> > -     for (i = 0; i < maxcount; i++)
> > -             show_ref_array_item(array.items[i], &format);
> > +     show_ref_array_items(array.items, &format, maxcount);
>
> The intention of this call is to pass an array and the number of
> elements in the array as a pair to the function, right?  When you
> design the API for a new helper function, do not split them apart by
> inserting an unrelated parameter in the middle.
>

Eh, are you saying that `maxcount` is irrelevant here? There should be
`maxcount`, because we need to limit the number of iterations here.

> > diff --git a/ref-filter.c b/ref-filter.c
> > index f0bd32f71416..27bbf9b6c8ac 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -2435,6 +2435,26 @@ int format_ref_array_item(struct ref_array_item *info,
> >       return 0;
> >  }
> >
> > +void show_ref_array_items(struct ref_array_item **info,
> > +                      const struct ref_format *format,
> > +                      size_t n)
>
> IOW,
>
>         void show_ref_array_items(const struct ref_format *format,
>                                   struct ref_array_item *info[], size_t n)
>

Yes, it will be more obvious in the form of an array.

> > +{
> > +     struct strbuf final_buf = STRBUF_INIT;
> > +     struct strbuf error_buf = STRBUF_INIT;
> > +     size_t i;
> > +
> > +     for (i = 0; i < n; i++) {
> > +             if (format_ref_array_item(info[i], format, &final_buf, &error_buf))
> > +                     die("%s", error_buf.buf);
>
> OK, the contents of error_buf is already localized, so it is correct
> not to have _() around the "%s" here.
>
> > +             fwrite(final_buf.buf, 1, final_buf.len, stdout);
> > +             strbuf_reset(&error_buf);
> > +             strbuf_reset(&final_buf);
> > +             putchar('\n');
>
> This is inherited code, but splitting fwrite() and putchar() apart
> like this makes the code hard to follow.  Perhaps clean it up later
> when nothing else is going on in the code as leftoverbits, outside
> the topic.
>

Ok, swap the position of reset and putchar.

> > +     }
> > +     strbuf_release(&error_buf);
> > +     strbuf_release(&final_buf);
> > +}
> > +
> >  void show_ref_array_item(struct ref_array_item *info,
> >                        const struct ref_format *format)
> >  {
>
> Isn't the point of the new helper function so that this can become a
> thin wrapper around it, i.e.
>
>         void show_ref_array_item(...)
>         {
>                 show_ref_array_items(format, &info, 1);
>         }
>

Maybe it makes sense. But as Peff said, Maybe we can just delete it.

> > diff --git a/ref-filter.h b/ref-filter.h
> > index 19ea4c413409..eb7e79a6676d 100644
> > --- a/ref-filter.h
> > +++ b/ref-filter.h
> > @@ -121,6 +121,11 @@ int format_ref_array_item(struct ref_array_item *info,
> >                         struct strbuf *error_buf);
> >  /*  Print the ref using the given format and quote_style */
> >  void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
> > +/*  Print the refs using the given format and quote_style and maxcount */
> > +void show_ref_array_items(struct ref_array_item **info,
> > +                      const struct ref_format *format,
> > +                      size_t n);
>
> The inconsistency between "maxcount" vs "n" is irritating.  Calling
> the parameter with a name that has the word "info" (because the new
> parameter is about that array) and a word like "nelem" to hint that
> it is the number of elements in the array) would be sensible.
>
> void show_ref_array_items(const struct ref_format *format,
>                           struct ref_array_item *info[], size_t info_count);
>
> or something along the line, perhaps?
>

Aha, I guess this is the reason for the misunderstanding above.
Yes, `info_count` is the correct meaning and the meaning of `n` is
wrong.

Thanks.
--
ZheNing Hu

  reply	other threads:[~2021-04-08 12:06 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-05 14:01 [PATCH] [GSOC] ref-filter: use single strbuf for all output ZheNing Hu via GitGitGadget
2021-04-05 17:05 ` Eric Sunshine
2021-04-06  8:53   ` ZheNing Hu
2021-04-05 21:02 ` Derrick Stolee
2021-04-06  8:58   ` ZheNing Hu
2021-04-05 22:17 ` Jeff King
2021-04-06  9:49   ` ZheNing Hu
2021-04-06 10:35     ` ZheNing Hu
2021-04-06 14:00       ` Jeff King
2021-04-06 14:35         ` ZheNing Hu
2021-04-06 18:34 ` René Scharfe
2021-04-07 13:57   ` ZheNing Hu
2021-04-07 15:26 ` [PATCH v2] " ZheNing Hu via GitGitGadget
2021-04-07 20:31   ` Junio C Hamano
2021-04-08 12:05     ` ZheNing Hu [this message]
2021-04-07 21:27   ` Jeff King
2021-04-08 12:18     ` ZheNing Hu
2021-04-08 14:32       ` Jeff King
2021-04-08 14:43         ` ZheNing Hu
2021-04-08 14:51           ` Jeff King
2021-04-08 15:12             ` ZheNing Hu

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=CAOLTT8RYjp1GKQpdPSzSNnn9o3+tuKNcGEUTp5HD9_Jut01JMQ@mail.gmail.com \
    --to=adlternative@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=hariom18599@gmail.com \
    --cc=l.s.r@web.de \
    --cc=peff@peff.net \
    --cc=stolee@gmail.com \
    --cc=sunshine@sunshineco.com \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).