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

[Duy Nguyen <pclouds@gmail.com>]

On Sat, Jul 21, 2018 at 8:56 AM Elijah Newren <newren@gmail.com> wrote:
> > -- 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