Re: [PATCH 03/16] pack-objects: use a faster hash table

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2013, 22:48 UTC
Message-ID: <7vk3lhzu0q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1372116193-32762-4-git-send-email-tanoku@gmail.com>

Vicent Marti <tanoku@gmail.com> writes:

Show 29 quoted lines

> @@ -901,19 +896,19 @@ static int no_try_delta(const char *path)
>  	return 0;
>  }
>  
> -static int add_object_entry(const unsigned char *sha1, enum object_type type,
> -			    const char *name, int exclude)
> +static int add_object_entry_1(const unsigned char *sha1, enum object_type type,
> +			    uint32_t hash, int exclude, struct packed_git *found_pack,
> +				off_t found_offset)
>  {
>  	struct object_entry *entry;
> -	struct packed_git *p, *found_pack = NULL;
> -	off_t found_offset = 0;
> -	int ix;
> -	unsigned hash = name_hash(name);
> +	struct packed_git *p;
> +	khiter_t ix;
> +	int hash_ret;
>  
> -	ix = nr_objects ? locate_object_entry_hash(sha1) : -1;
> -	if (ix >= 0) {
> +	ix = kh_put_sha1(packed_objects, sha1, &hash_ret);
> +	if (hash_ret == 0) {
>  		if (exclude) {
> -			entry = objects + object_ix[ix] - 1;
> +			entry = kh_value(packed_objects, ix);
>  			if (!entry->preferred_base)
>  				nr_result--;
>  			entry->preferred_base = 1;

After this, the function returns. The original did not add to the table the object name we are looking at, but the new code first adds it to the table with the unconditional kh_put_sha1() above. Is a call to kh_del_sha1() missing here ...

Show 8 quoted lines

> @@ -921,38 +916,42 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
>  		return 0;
>  	}
>  
> -	if (!exclude && local && has_loose_object_nonlocal(sha1))
> +	if (!exclude && local && has_loose_object_nonlocal(sha1)) {
> +		kh_del_sha1(packed_objects, ix);
>  		return 0;

... like this one, which seems to compensate for "ahh, after all we realize we do not want to add this one to the table"?

Show 34 quoted lines

> @@ -966,19 +965,30 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
>  		entry->in_pack_offset = found_offset;
>  	}
>  
> -	if (object_ix_hashsz * 3 <= nr_objects * 4)
> -		rehash_objects();
> -	else
> -		object_ix[-1 - ix] = nr_objects;
> +	kh_value(packed_objects, ix) = entry;
> +	kh_key(packed_objects, ix) = entry->idx.sha1;
> +	objects[nr_objects++] = entry;
>  
>  	display_progress(progress_state, nr_objects);
>  
> -	if (name && no_try_delta(name))
> -		entry->no_try_delta = 1;
> -
>  	return 1;
>  }
>  
> +static int add_object_entry(const unsigned char *sha1, enum object_type type,
> +			    const char *name, int exclude)
> +{
> +	if (add_object_entry_1(sha1, type, name_hash(name), exclude, NULL, 0)) {
> +		struct object_entry *entry = objects[nr_objects - 1];
> +
> +		if (name && no_try_delta(name))
> +			entry->no_try_delta = 1;
> +
> +		return 1;
> +	}
> +
> +	return 0;
> +}

It is somewhat unclear what we are getting from the split of the main part of this function into *_1(), other than the *_1() function now has a very deep indentation inside "if (!found_pack)", which is always true because the caller always passes NULL to found_pack. Perhaps this is an unrelated refactoring that is needed for later steps and does not have anything to do with the use of new hash function?