Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 9, 2016, 20:40 UTC
- 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:
Show 14 quoted lines
> 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.