[PATCH] pack-objects: trace pack bytes written

[PATCH] pack-objects: trace pack bytes written

[friel]

From: Friel <friel@openai.com>

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 <friel@openai.com>
---
 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;
 
 		if (!pack_to_stdout) {
 			struct stat st;
@@ -1510,6 +1515,8 @@ static void write_pack_file(void)
 		    written, nr_result);
 	trace2_data_intmax("pack-objects", the_repository,
 			   "write_pack_file/wrote", nr_result);
+	trace2_data_intmax("pack-objects", the_repository,
+			   "write_pack_file/wrote_bytes", bytes_written);
 }
 
 static int no_try_delta(const char *path)
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 9dabb3615a..aac139e6a0 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -33,6 +33,30 @@ test_expect_success 'setup' '
 	} >expect
 '
 
+test_expect_success 'pack-object traces bytes written to stdout' '
+	test_when_finished "rm -f pack.trace pack.pack" &&
+	GIT_TRACE2_EVENT="$PWD/pack.trace" \
+		git pack-objects --quiet --revs --stdout >pack.pack <<-EOF &&
+	$commit
+	EOF
+	bytes=$(test_file_size pack.pack) &&
+	test_grep "\"key\":\"write_pack_file/wrote_bytes\",\"value\":\"$bytes\"" pack.trace
+'
+
+test_expect_success 'pack-object traces bytes written to split pack files' '
+	test_when_finished "rm -f split.trace traced-pack-*" &&
+	GIT_TRACE2_EVENT="$PWD/split.trace" \
+		git -c pack.packSizeLimit=3m pack-objects --quiet traced-pack <obj-list &&
+	test 2 = $(ls traced-pack-*.pack | wc -l) &&
+	bytes=0 &&
+	for pack in traced-pack-*.pack
+	do
+		pack_size=$(test_file_size "$pack") &&
+		bytes=$((bytes + pack_size)) || return 1
+	done &&
+	test_grep "\"key\":\"write_pack_file/wrote_bytes\",\"value\":\"$bytes\"" split.trace
+'
+
 test_expect_success 'setup pack-object <stdin' '
 	git init pack-object-stdin &&
 	test_commit -C pack-object-stdin one &&

base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16

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

[Junio C Hamano]

friel@openai.com writes:

> From: Friel <friel@openai.com>
>
> 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 <friel@openai.com>
> ---
>  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.