Thread (11 messages) 11 messages, 4 authors, 2026-08-21

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

From: Junio C Hamano <hidden>
Date: 2026-08-18 01:08:27

friel@openai.com writes:
quoted hunk ↗ jump to hunk
From: Friel <redacted>

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 <redacted>
---
 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help