Re: [PATCH 2/2] index-pack: kill union delta_base to save memory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jul 3, 2015 at 11:51 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:
>
>> Once we know the number of objects in the input pack, we allocate an
>> array of nr_objects of struct delta_entry. On x86-64, this struct is
>> 32 bytes long. The union delta_base, which is part of struct
>> delta_entry, provides enough space to store either ofs-delta (8 bytes)
>> or ref-delta (20 bytes).
>
> Sorry for responding to a patch 7000+ messages ago, but some kind
> folks at Google were puzzled by this code, and I think they found a
> small bug.
>
>>  static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
>>  {
>> -     struct delta_entry **sorted_by_pos;
>> +     struct ref_delta_entry **sorted_by_pos;
>>       int i, n = 0;
>>
>>       /*
>> @@ -1282,28 +1344,25 @@ static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
>>        * resolving deltas in the same order as their position in the pack.
>>        */
>>       sorted_by_pos = xmalloc(nr_unresolved * sizeof(*sorted_by_pos));
>> -     for (i = 0; i < nr_deltas; i++) {
>> -             if (objects[deltas[i].obj_no].real_type != OBJ_REF_DELTA)
>> -                     continue;
>> -             sorted_by_pos[n++] = &deltas[i];
>> -     }
>> +     for (i = 0; i < nr_ref_deltas; i++)
>> +             sorted_by_pos[n++] = &ref_deltas[i];
>>       qsort(sorted_by_pos, n, sizeof(*sorted_by_pos), delta_pos_compare);
>
> You allocate an array of nr_unresolved (which is the sum of
> nr_ref_deltas and nr_ofs_deltas in the new world order with patch)
> entries, fill only the first nr_ref_deltas entries of it, and then
> sort that first n (= nr_ref_deltas) entries.  The qsort and the
> subsequent loop only looks at the first n entries, so nothing is
> filling or reading these nr_ofs_deltas entres at the end.
>
> I do not see any wrong behaviour other than temporary wastage of
> nr_ofs_deltas pointers that would be caused by this, but this
> allocation is misleading.
>
> Also, the old code before this change had to use 'i' and 'n' because
> some of the things we see in the (old) deltas[] array we scanned
> with 'i' would not make it into the sorted_by_pos[] array in the old
> world order, but now because you have only ref delta in a separate
> ref_deltas[] array, they increment lock&step.  That also puzzled me
> while re-reading this code.
>
> Perhaps something like this is needed?

Yeah I can see the confusion when reading the code without checking
its history. You probably want to kill the argument nr_unresolved too
because it's not needed anymore after your patch.

So what's the bug you mentioned? All I got from the above was wastage
and confusion, no bug.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]