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

[PATCH v2] 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>
---
Junio, you're right. Updating bytes_written before finalization is
equivalent. I've dropped pack_bytes; everything else is unchanged.
Thanks.

 builtin/pack-objects.c |  5 +++++
 t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1ec5b6f206..252530172c 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,
@@ -1389,6 +1390,8 @@ static void write_pack_file(void)
 			display_progress(progress_state, written);
 		}
 
+		bytes_written += hashfile_total(f) +
+			the_repository->hash_algo->rawsz;
 		if (pack_to_stdout) {
 			/*
 			 * We never fsync when writing to stdout since we may
@@ -1510,6 +1513,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 &&

Interdiff against v1:
  diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
  index bbf1adb437..252530172c 100644
  --- a/builtin/pack-objects.c
  +++ b/builtin/pack-objects.c
  @@ -1348,7 +1348,6 @@ 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) {
   			/*
  @@ -1391,7 +1390,7 @@ static void write_pack_file(void)
   			display_progress(progress_state, written);
   		}
   
  -		pack_bytes = hashfile_total(f) +
  +		bytes_written += hashfile_total(f) +
   			the_repository->hash_algo->rawsz;
   		if (pack_to_stdout) {
   			/*
  @@ -1423,7 +1422,6 @@ static void write_pack_file(void)
   				write_bitmap_index = 0;
   			}
   		}
  -		bytes_written += pack_bytes;
   
   		if (!pack_to_stdout) {
   			struct stat st;

base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16

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

[Patrick Steinhardt]

On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 1ec5b6f206..252530172c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
>  			display_progress(progress_state, written);
>  		}
>  
> +		bytes_written += hashfile_total(f) +
> +			the_repository->hash_algo->rawsz;
>  		if (pack_to_stdout) {
>  			/*
>  			 * We never fsync when writing to stdout since we may

I guess the addition here accounts for the trailing hash written by the
hashfile. If so, shouldn't we also use the algortihm that the hashfile
uses in the first place via `f->algop->rawsz`?

> @@ -1510,6 +1513,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)

The "write_pack_file/wrote" event is quite awkwardly named, if you ask
me, as it's not immediately obvious what exactly it's counting, and the
second metric may make this even more confusing. In retrospect it
would've been preferable to call this "wrote_objects" to clarify.

I don't really think we guarantee any kind of stability around those
traces, so we could in theory change it here, too. But I don't feel like
my argument is strong enough to really warrant such a change, so maybe
we should just leave it as-is.

Thanks!

Patrick

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

[Jeff King]

On Thu, Aug 20, 2026 at 07:41:33AM +0200, Patrick Steinhardt wrote:

> On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 1ec5b6f206..252530172c 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
> >  			display_progress(progress_state, written);
> >  		}
> >  
> > +		bytes_written += hashfile_total(f) +
> > +			the_repository->hash_algo->rawsz;
> >  		if (pack_to_stdout) {
> >  			/*
> >  			 * We never fsync when writing to stdout since we may
> 
> I guess the addition here accounts for the trailing hash written by the
> hashfile. If so, shouldn't we also use the algortihm that the hashfile
> uses in the first place via `f->algop->rawsz`?

Perhaps, though that is used to write the hash (via CSUM_HASH_IN_STREAM)
only in two of the conditional blocks. In the third we finalize the
hashfile and then use fixup_pack_header_footer(), passing the_hash_algo
directly (not even the_repository->hash_algo, though of course they mean
the same thing).

It all works out, of course, because we created the hashfile struct
earlier using the_repository->hash_algo. So I think this is mostly
academic in the first place, but your suggestion harmonizes two of the
conditional blocks while creating disagreement with the third.

I think something like this would "fix" it by consistently using the
hashfile's algo in all three blocks:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 4a5fcbe5f5..0fdff72f41 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1413,9 +1413,9 @@ static void write_pack_file(void)
 			 * If we wrote the wrong number of entries in the
 			 * header, rewrite it like in fast-import.
 			 */
-
+			const struct git_hash_algo *algo = f->algop;
 			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
-			fixup_pack_header_footer(the_hash_algo, fd, hash,
+			fixup_pack_header_footer(algo, fd, hash,
 						 pack_tmp_name, nr_written,
 						 hash, offset);
 			close(fd);


But there's a subtle yet interesting difference here! f->algop won't
necessarily be the same pointer as the_hash_algo. If we compiled with an
unsafe variant, that will be used for hashfiles. If we're just looking
at rawsz that's OK; the two variants should be identical (other than
performance and collision detection), so taking rawsz from either is
fine.

But fixup_pack_header_footer() actually recomputes the hash (as it must
if we tweak the header). Right now it does it using the "normal"
variant, but we should be able to use the unsafe one (which my diff
snippet above would start to do).

Of course this whole thing is absurdly pessimal in the first place. If
we are just going to throw out the hashfile's checksum, then why bother
computing it in the first place? Because we don't trust a disk write at
all, and actually verify the original hash computation as we read the
bytes back in! So we'll actually sha1 the written packfile three times.
Yikes. I wonder if it's really worth being so paranoid. But that is how
it has always been.

Anyway, that is a bit of a tangent from the patch in question. I think
either spelling is OK for the purposes of this patch. If somebody wants
to pursue harmonizing the paths (and maybe even doing some timings to
see if switching to the unsafe variant is noticeable here, and what the
total cost of this triple-write approach is), that can happen
separately.

-Peff

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

[Patrick Steinhardt]

On Thu, Aug 20, 2026 at 04:21:02AM -0400, Jeff King wrote:
> On Thu, Aug 20, 2026 at 07:41:33AM +0200, Patrick Steinhardt wrote:
> > On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:
> > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > > index 1ec5b6f206..252530172c 100644
> > > --- a/builtin/pack-objects.c
> > > +++ b/builtin/pack-objects.c
> > > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)
> > >  			display_progress(progress_state, written);
> > >  		}
> > >  
> > > +		bytes_written += hashfile_total(f) +
> > > +			the_repository->hash_algo->rawsz;
> > >  		if (pack_to_stdout) {
> > >  			/*
> > >  			 * We never fsync when writing to stdout since we may
> > 
> > I guess the addition here accounts for the trailing hash written by the
> > hashfile. If so, shouldn't we also use the algortihm that the hashfile
> > uses in the first place via `f->algop->rawsz`?
> 
> Perhaps, though that is used to write the hash (via CSUM_HASH_IN_STREAM)
> only in two of the conditional blocks. In the third we finalize the
> hashfile and then use fixup_pack_header_footer(), passing the_hash_algo
> directly (not even the_repository->hash_algo, though of course they mean
> the same thing).
> 
> It all works out, of course, because we created the hashfile struct
> earlier using the_repository->hash_algo. So I think this is mostly
> academic in the first place, but your suggestion harmonizes two of the
> conditional blocks while creating disagreement with the third.
> 
> I think something like this would "fix" it by consistently using the
> hashfile's algo in all three blocks:
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 4a5fcbe5f5..0fdff72f41 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1413,9 +1413,9 @@ static void write_pack_file(void)
>  			 * If we wrote the wrong number of entries in the
>  			 * header, rewrite it like in fast-import.
>  			 */
> -
> +			const struct git_hash_algo *algo = f->algop;
>  			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
> -			fixup_pack_header_footer(the_hash_algo, fd, hash,
> +			fixup_pack_header_footer(algo, fd, hash,
>  						 pack_tmp_name, nr_written,
>  						 hash, offset);
>  			close(fd);
> 
> 
> But there's a subtle yet interesting difference here! f->algop won't
> necessarily be the same pointer as the_hash_algo. If we compiled with an
> unsafe variant, that will be used for hashfiles. If we're just looking
> at rawsz that's OK; the two variants should be identical (other than
> performance and collision detection), so taking rawsz from either is
> fine.
> 
> But fixup_pack_header_footer() actually recomputes the hash (as it must
> if we tweak the header). Right now it does it using the "normal"
> variant, but we should be able to use the unsafe one (which my diff
> snippet above would start to do).

Yeah, I agree that switching over to the unsafe algortihm is sensible.
Being able to speed up hashing of packfiles was one of the prime
motivations of introducing the unsafe variants in the first place, so
the fact that we still use the safe variant here feels like a plain
oversight to me.

> Of course this whole thing is absurdly pessimal in the first place. If
> we are just going to throw out the hashfile's checksum, then why bother
> computing it in the first place? Because we don't trust a disk write at
> all, and actually verify the original hash computation as we read the
> bytes back in! So we'll actually sha1 the written packfile three times.
> Yikes. I wonder if it's really worth being so paranoid. But that is how
> it has always been.

That's... awful. Honestly, if we cannot trust what we're writing to disk
we're going to be kind of screwed anyway. We don't re-verify loose
objects, refs or whatever other data structures we write to disk either.
So doing this thrice here feels wrong.

> Anyway, that is a bit of a tangent from the patch in question. I think
> either spelling is OK for the purposes of this patch. If somebody wants
> to pursue harmonizing the paths (and maybe even doing some timings to
> see if switching to the unsafe variant is noticeable here, and what the
> total cost of this triple-write approach is), that can happen
> separately.

I agree that this is definitely out of scope of this patch series.

Patrick