Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 17, 2021, 16:09 UTC
Message-ID: <CAP8UFD0AvakWdpJKBDrEZstVvFOGdjVUkWsBXn7geiUdVzAZGQ@mail.gmail.com>
In-Reply-To: <CAOLTT8TJ_VBh8mreExXHdCFb1e6hB1-Exsn6OFdo2sZNkVFJkA@mail.gmail.com>

Hi,

On Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu <adlternative@gmail.com> wrote:

Show 11 quoted lines

>
> Hi,
>
> In the implementation of %(raw) atom
> (bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()
> to copy the content of the object. But if we can reuse the content
> of the object?
>
> Since git cat-file --batch needs to use ref-filter
> as the backend, if the object buffer can be reused correctly here,
> we can save a lot of copies and improve its performance by about 6%.

Yeah, that would be great.

Show 21 quoted lines

> Tracing back to the source, the object buffer is allocated from
> oid_object_info_extended(), but in parse_object_buffer() we may lose
> the ownership of the buffer (maybe the buffer is eaten), but I browsed the
> source code of for-each-ref.c or cat-file.c, and I don’t seem to find that the
> buffers which have been eaten are released by the program.
>
> So can we reuse it?
>
> This is what I want to do:
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 93ce2a6ef2..1f6c1daabd 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct
> atom_value *val, int deref, struct exp
>                         unsigned long buf_size = data->size;
>
>                         if (atom->u.raw_data.option == RAW_BARE) {
> -                               v->s = xmemdupz(buf, buf_size);
> +                               v->s = buf;

It seems to me that this could work only if 'buf' isn't freed. Have you checked that? Did we leak 'buf' before this patch? Otherwise when are we freeing it?

Show 10 quoted lines

>                                 v->s_size = buf_size;
>                         } else if (atom->u.raw_data.option == RAW_LENGTH) {
>                                 v->s = xstrfmt("%"PRIuMAX, (uintmax_t)buf_size);
> @@ -1768,8 +1768,6 @@ static int get_object(struct ref_array_item
> *ref, int deref, struct object **obj
>         }
>
>         grab_common_values(ref->value, deref, oi);
> -       if (!eaten)
> -               free(oi->content);

This change might not be needed. 'oi->content' seems to come from the 'buf' above, either directly or through a copy, but if we can indeed own the 'buf' completely, then we should be able to dispose of it how we want.

>         return 0;
>  }