Re: [PATCH v2 11/24] pack-bitmap-write: pass ownership of intermediate bitmaps

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 25, 2020, 01:00 UTC
Message-ID: <20201125010022.938266-1-jonathantanmy@google.com>
In-Reply-To: <466dd3036a8ca7dc9718a53f17cf87e0eb22c066.1605649533.git.me@ttaylorr.com>

Show 12 quoted lines

> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index f2f0b6b2c2..d2d46ff5f4 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -333,6 +333,7 @@ void bitmap_writer_build(struct packing_data *to_pack)
>  		struct commit *commit = bb.commits[i-1];
>  		struct bb_commit *ent = bb_data_at(&bb.data, commit);
>  		struct commit *child;
> +		int reused = 0;
>  
>  		fill_bitmap_commit(ent, commit);
>

Before the following chunk is the start of a loop:

"while ((child = pop_commit(&ent->children))) {"

Show 20 quoted lines

> @@ -348,10 +349,15 @@ void bitmap_writer_build(struct packing_data *to_pack)
>  
>  			if (child_ent->bitmap)
>  				bitmap_or(child_ent->bitmap, ent->bitmap);
> -			else
> +			else if (reused)
>  				child_ent->bitmap = bitmap_dup(ent->bitmap);
> +			else {
> +				child_ent->bitmap = ent->bitmap;
> +				reused = 1;
> +			}
>  		}
> -		bitmap_free(ent->bitmap);
> +		if (!reused)
> +			bitmap_free(ent->bitmap);
>  		ent->bitmap = NULL;
>  	}
>  	bitmap_builder_clear(&bb);
> -- 
> 2.29.2.312.gabc4d358d8

So this is clearly correct.

I asked myself if this optimization is worth it when we're going to drastically reduce the number of steps in patch 18, but I think that the answer is still yes.