From: Junio C Hamano <gitster@pobox•com>
To: Duy Nguyen <pclouds@gmail•com>
Cc: Git Mailing List <git@vger•kernel.org>
Subject: Re: [PATCH 2/2] index-pack: kill union delta_base to save memory
Date: Sat, 04 Jul 2015 15:24:48 -0700 [thread overview]
Message-ID: <xmqqvbdz36j3.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CACsJy8D872sj9WQec_FZrTxx=gqy++L1XLxJdEtEQNpGpFYr=Q@mail.gmail.com> (Duy Nguyen's message of "Sat, 4 Jul 2015 01:29:19 +0700")
Duy Nguyen <pclouds@gmail•com> writes:
>>> @@ -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.
Actually, the above is not analyzed correctly. I thought
nr_unresolved was ref + ofs, but look at the caller in the
fix_thin_pack codepath:
int nr_unresolved = nr_ofs_deltas + nr_ref_deltas - nr_resolved_deltas;
int nr_objects_initial = nr_objects;
if (nr_unresolved <= 0)
die(_("confusion beyond insanity"));
REALLOC_ARRAY(objects, nr_objects + nr_unresolved + 1);
memset(objects + nr_objects + 1, 0,
nr_unresolved * sizeof(*objects));
f = sha1fd(output_fd, curr_pack);
fix_unresolved_deltas(f, nr_unresolved);
If there were tons of nr_resolved_deltas and only small number of
nr_ofs_deltas, then the allocation in question may actually be
under-allocating.
So...
next prev parent reply other threads:[~2015-07-05 9:23 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-18 10:47 [PATCH 0/2] nd/slim-index-pack-memory-usage update Nguyễn Thái Ngọc Duy
2015-04-18 10:47 ` [PATCH 1/2] index-pack: reduce object_entry size to save memory Nguyễn Thái Ngọc Duy
2015-04-18 10:47 ` [PATCH 2/2] index-pack: kill union delta_base " Nguyễn Thái Ngọc Duy
2015-07-03 16:51 ` Junio C Hamano
2015-07-03 18:29 ` Duy Nguyen
2015-07-04 1:21 ` [PATCH] index-pack: fix overallocation of sorted_by_pos array Junio C Hamano
2015-07-04 22:30 ` [PATCH] index-pack: fix allocation " Junio C Hamano
2015-07-06 23:23 ` Junio C Hamano
2015-07-07 0:36 ` Duy Nguyen
2015-07-07 15:49 ` Junio C Hamano
2015-07-07 16:06 ` Jeff King
2015-07-08 11:56 ` [PATCH 1/2] index-pack: rename the field "type" to "in_pack_type" Nguyễn Thái Ngọc Duy
2015-07-08 11:56 ` [PATCH 2/2] pack-objects: rename the field "type" to "real_type" Nguyễn Thái Ngọc Duy
2015-07-08 13:47 ` Jeff King
2015-07-08 13:57 ` Duy Nguyen
2015-07-08 14:11 ` Jeff King
2015-07-04 22:24 ` Junio C Hamano [this message]
-- strict thread matches above, loose matches on Subject: below --
2015-02-20 1:58 [PATCH 0/2] nd/slim-index-pack-memory-usage updates Nguyễn Thái Ngọc Duy
2015-02-20 1:58 ` [PATCH 2/2] index-pack: kill union delta_base to save memory Nguyễn Thái Ngọc Duy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvbdz36j3.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox