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

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Access to e->delta_size_ (and by extension
> pack->delta_size[e - pack->objects]) is unprotected as before, the
> thread scheduler in pack-objects must make sure "e" is never updated
> by two different threads.

OK.  Do we need to worry about "e" (e.g. "e->delta_size_valid")
being accessed while/before it is set by another thread?

oe_delta_size() makes unprotected accesses to .delta_size_ and
pack->delta_size[e - pack->objects], so we apparently do not, and
oe_set_delta_size() only protects the allocation call and does not
prevent a reader in oe_delta_size() from first reading the _valid
field, noticing that it is 0 as initialized, and goes on to read the
pack->delta_size[] slot for the entry, while the writer is setting
the size to .delta_size_ field and flipping _valid bit, without ever
storing the size in the pack->delta_size[] array.

> @@ -130,6 +131,7 @@ struct packing_data {
>  	uint32_t index_size;
>  
>  	unsigned int *in_pack_pos;
> +	unsigned long *delta_size;
>  
>  	/*
>  	 * Only one of these can be non-NULL and they have different
> @@ -140,10 +142,29 @@ struct packing_data {
>  	struct packed_git **in_pack_by_idx;
>  	struct packed_git **in_pack;
>  
> +#ifndef NO_PTHREADS
> +	pthread_mutex_t lock;

I am wondering if we want the variable to say what data it is
protecting from simultaneous accesses, or leave it as generic so
that any new caller that wants to lock any (new) thing that is
associated with a packing_data structure can grab it for other
purposes.  The design of this patch clearly is the latter, which is
OK for now, I think.

> @@ -332,18 +353,34 @@ static inline unsigned long oe_delta_size(struct packing_data *pack,
>  {
>  	if (e->delta_size_valid)
>  		return e->delta_size_;
> -	return oe_size(pack, e);
> +
> +	/*
> +	 * pack->detla_size[] can't be NULL because oe_set_delta_size()
> +	 * must have been called when a new delta is saved with
> +	 * oe_set_delta().
> +	 * If oe_delta() returns NULL (i.e. default state, which means
> +	 * delta_size_valid is also false), then the caller must never
> +	 * call oe_delta_size().
> +	 */
> +	return pack->delta_size[e - pack->objects];
>  }
>  
>  static inline void oe_set_delta_size(struct packing_data *pack,
>  				     struct object_entry *e,
>  				     unsigned long size)
>  {
> -	e->delta_size_ = size;
> -	e->delta_size_valid = e->delta_size_ == size;
> -	if (!e->delta_size_valid && size != oe_size(pack, e))
> -		BUG("this can only happen in check_object() "
> -		    "where delta size is the same as entry size");
> +	if (size < pack->oe_delta_size_limit) {
> +		e->delta_size_ = size;
> +		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;
> +		e->delta_size_valid = 0;
> +	}
>  }