On Sat, May 12, 2018 at 11:07 AM, Jeff King <peff@xxxxxxxx> 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
