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

[Jeff King <peff@peff.net>]

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).

-Peff