Re: [PATCH] pack-objects: fix performance issues on packing large deltas

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 21, 2018, 07:14 UTC
Message-ID: <CACsJy8AMTdF22oFqJAscsQGgqRv3Xt7kXTTWY3SsKHVPByYnSw@mail.gmail.com>
In-Reply-To: <CABPp-BG_kYw1fWvq0YTTic7AV=nYuoKe9E4QMYVN0R_16xPKaw@mail.gmail.com>

On Sat, Jul 21, 2018 at 8:56 AM Elijah Newren <newren@gmail.com> wrote:

Show 27 quoted lines

> > -- 8< --
> ...
> > diff --git a/pack-objects.h b/pack-objects.h
> > index 9f977ae800..11890e7217 100644
> > --- a/pack-objects.h
> > +++ b/pack-objects.h
> > @@ -15,7 +15,7 @@
> ...
> > @@ -353,37 +354,26 @@ static inline void oe_set_size(struct packing_data *pack,
> >  static inline unsigned long oe_delta_size(struct packing_data *pack,
> >                                           const struct object_entry *e)
> >  {
> > -       unsigned long size;
> > -
> > -       packing_data_lock(pack);
> > -       if (pack->delta_size)
> > -               size = pack->delta_size[e - pack->objects];
> > +       if (e->delta_size_valid)
> > +               return e->delta_size_;
> >         else
> > -               size = e->delta_size_;
> > -       packing_data_unlock(pack);
> > -       return size;
> > +               return pack->delta_size[e - pack->objects];
>
> Should this last line be wrapped with packing_data_lock/unlock calls?
> (And similarly in oe_set_delta_size when writing to this array?)

No. Data related to "e" alone could be accessed unprotected. This is what try_delta has been doing (it's "trg_entry" in there). Because we know that pack->delta_size[e - pack->objects] is for "e" and no other entry, we can have the same treatment. If we access other entries' data though like in my new_delta_size_array(), that could be racy.

-- 
Duy