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

[ZheNing Hu <adlternative@gmail.com>]

Christian Couder <christian.couder@gmail.com> 于2021年8月18日周三 下午4:54写道：
>
> On Wed, Aug 18, 2021 at 6:51 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Christian Couder <christian.couder@gmail.com> 于2021年8月18日周三 上午12:10写道：
> > >
> > > Hi,
> > >
> > > On Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu <adlternative@gmail.com> wrote:
> > > >
> > > > 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.
> > >
> > > > 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?
> > >
> > This is how I use gdb find out if this buffer have been freed:
>
> I was asking about 'buf' before the patch.
>
> Before the patch, we were doing:
>
> v->s = xmemdupz(buf, buf_size);
>
> which means that in v->s there is a copy of 'buf', not 'buf' itself.
> So I was wondering if 'buf' was freed then, not the copy in in v->s.

I think the 'buf' is not freed in ref-filter logic.

But one thing worth noting is:

parse_object_buffer() may take this buffer away, and store it in
tree->buffer or use set_commit_buffer() to store it in commit_slab.

So in theory, as long as we don’t use parse_object_buffer(), this
dynamically allocated memory can be used freely for us. If we use
parse_object_buffer() and free the buffer, there seems to be a risk,
but it does not appear so far.

--
ZheNing Hu