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

From: Elijah Newren <newren@gmail.com>
Date: Jul 21, 2018, 06:56 UTC
Message-ID: <CABPp-BG_kYw1fWvq0YTTic7AV=nYuoKe9E4QMYVN0R_16xPKaw@mail.gmail.com>
In-Reply-To: <20180721044711.GA31376@duynguyen.home>

On Fri, Jul 20, 2018 at 9:47 PM, Duy Nguyen <pclouds@gmail.com> wrote:

> On Fri, Jul 20, 2018 at 10:43:25AM -0700, Elijah Newren wrote:

Show 8 quoted lines

>> Out of curiosity, would it be possible to use the delta_size_ field
>> for deltas that are small enough, and only use an external data
>> structure (perhaps a hash rather than an array) for the few deltas
>> that are large?
>
> We could. And because repack time is still a bit higher in your
> linux.git case. Let's try this. No locking in common case and very
> small locked region when we hit large deltas

Sweet, I'm starting some tests with it...

> -- 8< --

...

Show 5 quoted lines

> 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 @@

...

Show 16 quoted lines

> @@ -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?)

Show 33 quoted lines

>  }
>
> -uint32_t *new_delta_size_array(struct packing_data *pdata);
>  static inline void oe_set_delta_size(struct packing_data *pack,
>                                      struct object_entry *e,
>                                      unsigned long size)
>  {
> -       packing_data_lock(pack);
> -       if (!pack->delta_size && size < pack->oe_delta_size_limit) {
> -               packing_data_unlock(pack);
> +       if (size < pack->oe_delta_size_limit) {
>                 e->delta_size_ = size;
> -               return;
> +               e->delta_size_valid = 1;
> +       } else {
> +               packing_data_lock(pack);
> +               if (!pack->delta_size)
> +                       ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
> +               packing_data_unlock(pack);
> +               pack->delta_size[e - pack->objects] = size;
>         }
> -       /*
> -        * We have had at least one delta size exceeding OE_DELTA_SIZE_BITS
> -        * limit. delta_size_ will not be used anymore. All delta sizes are
> -        * now from the delta_size[] array.
> -        */
> -       if (!pack->delta_size)
> -               pack->delta_size = new_delta_size_array(pack);
> -       pack->delta_size[e - pack->objects] = size;
> -       packing_data_unlock(pack);
>  }
>
>  #endif