From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 02 May 2013 07:05:01 GMT
Subject: Re: [PATCH] lookup_object: prioritize recently found objects
Message-ID: <5182101D.4050807@viscovery.net>
In-Reply-To: <20130502064630.GA15208@sigill.intra.peff.net>

Am 5/2/2013 8:46, schrieb Jeff King:
> On Thu, May 02, 2013 at 08:44:07AM +0200, Johannes Sixt wrote:
>> Am 5/1/2013 22:34, schrieb Jeff King:
>>>  struct object *lookup_object(const unsigned char *sha1)
>>>  {
>>> -	unsigned int i;
>>> +	unsigned int i, first;
>>>  	struct object *obj;
>>>  
>>>  	if (!obj_hash)
>>>  		return NULL;
>>>  
>>> -	i = hashtable_index(sha1);
>>> +	first = i = hashtable_index(sha1);
>>>  	while ((obj = obj_hash[i]) != NULL) {
>>>  		if (!hashcmp(sha1, obj->sha1))
>>>  			break;
>>> @@ -85,6 +85,11 @@ struct object *lookup_object(const unsigned char *sha1)
>>>  		if (i == obj_hash_size)
>>>  			i = 0;
>>>  	}
>>> +	if (obj && i != first) {
>>> +		struct object *tmp = obj_hash[i];
>>> +		obj_hash[i] = obj_hash[first];
>>> +		obj_hash[first] = tmp;
>>> +	}
>>>  	return obj;
>>>  }
>>
>> This is one of the places where I think the code does not speak for itself
>> and a comment is warranted: The new if statement is not about correctness,
>> but about optimization:
> 
> I figured the lengthy description in the commit message would be
> sufficient,

It's absolutely sufficient *if* one reads the commit message. In this
case, though it goes more like "this function should be trivial, and it is
-- up to this if statement; what the heck is it good for?" and the reader
is forced to dig the history.

BTW, do you notice that the function is now modifying an object (the hash
table) even though this is rather unexpected from a "lookup" function?

-- Hannes