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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2018, 18:04 UTC
Message-ID: <xmqq8t617rqv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180722080421.12887-1-pclouds@gmail.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.

Show 14 quoted lines

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

Show 39 quoted lines

> @@ -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;
> +	}
>  }