From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 09 Jun 2016 20:40:59 GMT
Subject: Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic
Message-ID: <xmqqlh2egko4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqtwh2gmlv.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> instead. But that is still unspecified, so we rather go with
>>
>> static int compare_ce(const void *one, const void *two, void *cb_data)
>> {
>>     const struct cache_entry *ce_one = one, *ce_two = two;
>>     return strcmp(ce_one->name, ce_two->name);
>> }
>
> As I said below, I do not think it is worth addressing by making the
> code's behaviour on real systems worse.  As long as what you have as
> the key into priority queue is a pointer to cache_entry, you cannot
> make it better from that point of view.

... because having to strcmp() their names would be much more
expensive than the pointer comparison.

However, I think you could use a pointer into a single array as
an element of prio_queue.  I notice here:

 	for (; suc->current < suc->list.nr; suc->current++) {
-		const struct cache_entry *ce = suc->list.entries[suc->current];
+		ce = suc->list.entries[suc->current];
 		if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
+			*ce_task_cb = (struct cache_entry *) ce;
 			suc->current++;
 			return 1;
 		}
 	}
 
that list.entries[] can serve as such an array.  If you pass around
the pointer to its element instead, i.e.

-		ce = suc->list.entries[suc->current];
+		ceP = &suc->list.entries[suc->current];
- 		if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
+ 		if (prepare_to_clone_next_submodule(*ceP, child, suc, err)) {
+			*ce_task_cb = (struct cache_entry *) ce;
-			*ce_task_cb = ceP;
		...
	}
	/*
	 * The loop above tried cloning each submodule once,
	 * now try the stragglers again.
	 */
-	ce = (struct cache_entry *) prio_queue_get(&suc->failed_queue);
+	ceP = (struct cache_entry **) prio_queue_get(&suc->failed_queue);

then the elements you are pushing into prio-queue would not be ce
(pointer to a cache entry), but would be a pointer of an array that
holds many pointers to cache entries, so it becomes kosher to
compare them for ordering.

That would probably not add too much overhead at runtime; it may
have to involve a bit of code restructuring, so I do not know if it
is worth it, though.