Re: [PATCH v3] index-pack: retain child bases in delta cache

[Derrick Stolee <stolee@gmail.com>]

On 6/2/26 8:05 PM, Arijit Banerjee via GitGitGadget wrote:

>      Changes since v2:
>      
>       * Addressed Jeff King's review question by releasing cached base data
>         after all direct children have been dispatched, while keeping the
>         existing subtree bookkeeping intact.
>       * Re-ran t/t5302-pack-index.sh, p5302-pack-index.sh, and end-to-end
>         full clone spot checks with the precise-release version.
...
> +static int base_data_has_remaining_direct_children(struct base_data *c)
> +{
> +	return c->ref_first <= c->ref_last ||
> +	       c->ofs_first <= c->ofs_last;
> +}
> +

I'm glad you were able to find some bookkeeping that already exists to
help with this decision.



>   static void prune_base_data(struct base_data *retain)
>   {
>   	struct list_head *pos;
> @@ -1201,8 +1207,12 @@ static void *threaded_second_pass(void *data)
>   		}
>   
>   		work_lock();
> -		if (parent)
> +		if (parent) {
>   			parent->retain_data--;
> +			if (!parent->retain_data &&
> +			    !base_data_has_remaining_direct_children(parent))
> +				free_base_data(parent);
> +		}

This appears like the correct place to do this.

>   		if (child && child->data) {
>   			/*
> @@ -1212,7 +1222,6 @@ static void *threaded_second_pass(void *data)
>   			list_add(&child->list, &work_head);
>   			base_cache_used += child->size;
>   			prune_base_data(NULL);
> -			free_base_data(child);

And still we don't want this universal free.

Thanks for re-running your performance numbers after this change. I didn't
see any significant difference in the relative changes.

I don't think we have a way of measuring "memory pressure" during the
performance test suite. Did you see any evidence that this change has the
intended effect of reducing process memory proactively instead of relying
on the cache evictions?

Thanks,
-Stolee