Re: [PATCH 03/12] shallow.c: use commit-slab for commit depth instead of commit->util

[Duy Nguyen <pclouds@gmail.com>]

On Sat, May 12, 2018 at 11:07 AM, Jeff King <peff@peff.net> wrote:
> On Sat, May 12, 2018 at 10:00:19AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> @@ -82,25 +84,29 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>>       struct object_array stack = OBJECT_ARRAY_INIT;
>>       struct commit *commit = NULL;
>>       struct commit_graft *graft;
>> +     struct commit_depth depths;
>>
>> +     init_commit_depth(&depths);
>>       while (commit || i < heads->nr || stack.nr) {
>>               struct commit_list *p;
>>               if (!commit) {
>>                       if (i < heads->nr) {
>> +                             int **depth_slot;
>>                               commit = (struct commit *)
>>                                       deref_tag(heads->objects[i++].item, NULL, 0);
>>                               if (!commit || commit->object.type != OBJ_COMMIT) {
>>                                       commit = NULL;
>>                                       continue;
>>                               }
>> -                             if (!commit->util)
>> -                                     commit->util = xmalloc(sizeof(int));
>> -                             *(int *)commit->util = 0;
>> +                             depth_slot = commit_depth_at(&depths, commit);
>> +                             if (!*depth_slot)
>> +                                     *depth_slot = xmalloc(sizeof(int));
>> +                             **depth_slot = 0;
>
> It looks like we could save ourselves an extra layer of indirection (and
> tiny heap allocations) by just storing an "int" directly in the slab.
> Do we ever use the NULL as a sentinel value?
>
> Here we just allocate it if not set. Let's see if we can find some
> others...
>
>> @@ -116,25 +122,32 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>>               }
>>               commit->object.flags |= not_shallow_flag;
>>               for (p = commit->parents, commit = NULL; p; p = p->next) {
>> -                     if (!p->item->util) {
>> -                             int *pointer = xmalloc(sizeof(int));
>> -                             p->item->util = pointer;
>> -                             *pointer =  cur_depth;
>> +                     int **depth_slot = commit_depth_at(&depths, p->item);
>> +                     if (!*depth_slot) {
>> +                             *depth_slot = xmalloc(sizeof(int));
>> +                             **depth_slot = cur_depth;
>>                       } else {
>> -                             int *pointer = p->item->util;
>> -                             if (cur_depth >= *pointer)
>> +                             if (cur_depth >= **depth_slot)
>>                                       continue;
>> -                             *pointer = cur_depth;
>> +                             **depth_slot = cur_depth;
>>                       }
>
> Here we malloc again if it's not set. But we do behave slightly
> differently when we see NULL, in that we do not bother to even compare
> against cur_depth. So if we were to directly store ints, we'd see "0" as
> the sentinel depth here, which would not match our "cur_depth >=
> depth_slot" check.
>
> So no, it wouldn't work to directly store depths with the code as
> written.  I'm not sure if the depth can ever be 0. If not, then it would
> be a suitable sentinel as:
>
>   int *slot = commit_depth_at(&depths, p->item);
>   if (!*slot || cur_depth < *slot)
>         *slot = cur_depth;
>
> But somebody would have to dig into the possible values of cur_depth
> there (which would make sense to do as a separate patch anyway, since
> the point of this is to be a direct conversion).

I actually tried that first, going with storing int directly in the
slab instead of int*. And some shallow tests failed so I didn't bother
(the goal was to get rid of 'util' pointer, not to optimize more)
-- 
Duy