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

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 3, 2026, 12:24 UTC
Message-ID: <c4a32a6f-70bf-4ff4-abbf-d6e301246b5b@gmail.com>
In-Reply-To: <pull.2131.v3.git.1780445118653.gitgitgadget@gmail.com>

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

Show 7 quoted lines

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

...

Show 6 quoted lines

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

Show 14 quoted lines

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

Show 7 quoted lines

>   		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