Re: [PATCH] pack-objects: trace pack bytes written

[email protected] writes:

> From: Friel <[email protected]>
>
> We want to measure how compression settings affect push performance on
> the client. Different settings can produce different-sized packs from
> the same objects. Trace2 records the object count, but we also need the
> pack size to compare those settings.
>
> Add a write_pack_file/wrote_bytes Trace2 datum alongside
> write_pack_file/wrote. Count packs written to stdout or disk, including
> each pack's header and trailing checksum. When pack.packSizeLimit splits
> the output, report the sum of the pack sizes.
>
> Signed-off-by: Friel <[email protected]>
> ---
>  builtin/pack-objects.c |  7 +++++++
>  t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 1ec5b6f206..bbf1adb437 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1337,6 +1337,7 @@ static void write_pack_file(void)
>  	uint32_t nr_remaining = nr_result;
>  	time_t last_mtime = 0;
>  	struct object_entry **write_order;
> +	off_t bytes_written = 0;
>  
>  	if (progress > pack_to_stdout)
>  		progress_state = start_progress(the_repository,
> @@ -1347,6 +1348,7 @@ static void write_pack_file(void)
>  	do {
>  		unsigned char hash[GIT_MAX_RAWSZ];
>  		char *pack_tmp_name = NULL;
> +		off_t pack_bytes;
>  
>  		if (pack_to_stdout) {
>  			/*
> @@ -1389,6 +1391,8 @@ static void write_pack_file(void)
>  			display_progress(progress_state, written);
>  		}
>  
> +		pack_bytes = hashfile_total(f) +
> +			the_repository->hash_algo->rawsz;
>  		if (pack_to_stdout) {
>  			/*
>  			 * We never fsync when writing to stdout since we may
> @@ -1419,6 +1423,7 @@ static void write_pack_file(void)
>  				write_bitmap_index = 0;
>  			}
>  		}
> +		bytes_written += pack_bytes;

I may very well be misreading the code, but it is unclear to me what
role pack_bytes is playing, why we want to compute it before the
finialization if/else cascade above, and increment bytes_written
after that finalization if/else cascade above.

IOW, wouldn't it be equivalent to get rid of hunks 1347 and 1419,
and in hunk 1389 to this instead?

		bytes_written += hashfile_total(f) + the_hash_algo->rawsz;

The numbers for non stdout case are not that interesting (we can see
how bit the on-disk files are very easily), but counting in the
common code path (i.e., hunk 1389) sounds like the cleanest
approach.  I just found that the code with two variables confusing.

Thanks.