{"thread":{"id":"66185","subject":"[PATCH] pack-objects: trace pack bytes written","startedAt":"2026-08-17T23:39:17Z","lastAt":"2026-08-21T03:33:55Z","messageCount":11,"participants":["friel@openai.com","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"550729","messageId":"20260817233914.8740-2-friel@openai.com","threadId":"66185","inReplyTo":null,"subject":"[PATCH] pack-objects: trace pack bytes written","fromName":"","fromEmail":"friel@openai.com","sentAt":"2026-08-17T23:39:15Z","receivedAt":"2026-08-17T23:39:17Z","isPatch":true,"body":"From: Friel \n\nWe want to measure how compression settings affect push performance on\nthe client. Different settings can produce different-sized packs from\nthe same objects. Trace2 records the object count, but we also need the\npack size to compare those settings.\n\nAdd a write_pack_file/wrote_bytes Trace2 datum alongside\nwrite_pack_file/wrote. Count packs written to stdout or disk, including\neach pack's header and trailing checksum. When pack.packSizeLimit splits\nthe output, report the sum of the pack sizes.\n\nSigned-off-by: Friel \n---\n builtin/pack-objects.c | 7 +++++++\n t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1ec5b6f206..bbf1adb437 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1337,6 +1337,7 @@ static void write_pack_file(void)\n \tuint32_t nr_remaining = nr_result;\n \ttime_t last_mtime = 0;\n \tstruct object_entry **write_order;\n+\toff_t bytes_written = 0;\n \n \tif (progress > pack_to_stdout)\n \t\tprogress_state = start_progress(the_repository,\n@@ -1347,6 +1348,7 @@ static void write_pack_file(void)\n \tdo {\n \t\tunsigned char hash[GIT_MAX_RAWSZ];\n \t\tchar *pack_tmp_name = NULL;\n+\t\toff_t pack_bytes;\n \n \t\tif (pack_to_stdout) {\n \t\t\t/*\n@@ -1389,6 +1391,8 @@ static void write_pack_file(void)\n \t\t\tdisplay_progress(progress_state, written);\n \t\t}\n \n+\t\tpack_bytes = hashfile_total(f) +\n+\t\t\tthe_repository->hash_algo->rawsz;\n \t\tif (pack_to_stdout) {\n \t\t\t/*\n \t\t\t * We never fsync when writing to stdout since we may\n@@ -1419,6 +1423,7 @@ static void write_pack_file(void)\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t}\n \t\t}\n+\t\tbytes_written += pack_bytes;\n \n \t\tif (!pack_to_stdout) {\n \t\t\tstruct stat st;\n@@ -1510,6 +1515,8 @@ static void write_pack_file(void)\n \t\t written, nr_result);\n \ttrace2_data_intmax(\"pack-objects\", the_repository,\n \t\t\t \"write_pack_file/wrote\", nr_result);\n+\ttrace2_data_intmax(\"pack-objects\", the_repository,\n+\t\t\t \"write_pack_file/wrote_bytes\", bytes_written);\n }\n \n static int no_try_delta(const char *path)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 9dabb3615a..aac139e6a0 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -33,6 +33,30 @@ test_expect_success 'setup' '\n \t} >expect\n '\n \n+test_expect_success 'pack-object traces bytes written to stdout' '\n+\ttest_when_finished \"rm -f pack.trace pack.pack\" &&\n+\tGIT_TRACE2_EVENT=\"$PWD/pack.trace\" \\\n+\t\tgit pack-objects --quiet --revs --stdout >pack.pack <<-EOF &&\n+\t$commit\n+\tEOF\n+\tbytes=$(test_file_size pack.pack) &&\n+\ttest_grep \"\\\"key\\\":\\\"write_pack_file/wrote_bytes\\\",\\\"value\\\":\\\"$bytes\\\"\" pack.trace\n+'\n+\n+test_expect_success 'pack-object traces bytes written to split pack files' '\n+\ttest_when_finished \"rm -f split.trace traced-pack-*\" &&\n+\tGIT_TRACE2_EVENT=\"$PWD/split.trace\" \\\n+\t\tgit -c pack.packSizeLimit=3m pack-objects --quiet traced-pack From: Friel \n>\n> We want to measure how compression settings affect push performance on\n> the client. Different settings can produce different-sized packs from\n> the same objects. Trace2 records the object count, but we also need the\n> pack size to compare those settings.\n>\n> Add a write_pack_file/wrote_bytes Trace2 datum alongside\n> write_pack_file/wrote. Count packs written to stdout or disk, including\n> each pack's header and trailing checksum. When pack.packSizeLimit splits\n> the output, report the sum of the pack sizes.\n>\n> Signed-off-by: Friel \n> ---\n> builtin/pack-objects.c | 7 +++++++\n> t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 31 insertions(+)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 1ec5b6f206..bbf1adb437 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1337,6 +1337,7 @@ static void write_pack_file(void)\n> \tuint32_t nr_remaining = nr_result;\n> \ttime_t last_mtime = 0;\n> \tstruct object_entry **write_order;\n> +\toff_t bytes_written = 0;\n> \n> \tif (progress > pack_to_stdout)\n> \t\tprogress_state = start_progress(the_repository,\n> @@ -1347,6 +1348,7 @@ static void write_pack_file(void)\n> \tdo {\n> \t\tunsigned char hash[GIT_MAX_RAWSZ];\n> \t\tchar *pack_tmp_name = NULL;\n> +\t\toff_t pack_bytes;\n> \n> \t\tif (pack_to_stdout) {\n> \t\t\t/*\n> @@ -1389,6 +1391,8 @@ static void write_pack_file(void)\n> \t\t\tdisplay_progress(progress_state, written);\n> \t\t}\n> \n> +\t\tpack_bytes = hashfile_total(f) +\n> +\t\t\tthe_repository->hash_algo->rawsz;\n> \t\tif (pack_to_stdout) {\n> \t\t\t/*\n> \t\t\t * We never fsync when writing to stdout since we may\n> @@ -1419,6 +1423,7 @@ static void write_pack_file(void)\n> \t\t\t\twrite_bitmap_index = 0;\n> \t\t\t}\n> \t\t}\n> +\t\tbytes_written += pack_bytes;\n\nI may very well be misreading the code, but it is unclear to me what\nrole pack_bytes is playing, why we want to compute it before the\nfinialization if/else cascade above, and increment bytes_written\nafter that finalization if/else cascade above.\n\nIOW, wouldn't it be equivalent to get rid of hunks 1347 and 1419,\nand in hunk 1389 to this instead?\n\n\t\tbytes_written += hashfile_total(f) + the_hash_algo->rawsz;\n\nThe numbers for non stdout case are not that interesting (we can see\nhow bit the on-disk files are very easily), but counting in the\ncommon code path (i.e., hunk 1389) sounds like the cleanest\napproach. I just found that the code with two variables confusing.\n\nThanks.\n\n"},{"id":"550857","messageId":"c6a8cdac36d2202055d637ebcc97e484122cdcd4.1787158152.git.friel@openai.com","threadId":"66185","inReplyTo":"xmqqo6f02q2f.fsf@gitster.g","subject":"[PATCH v2] pack-objects: trace pack bytes written","fromName":"","fromEmail":"friel@openai.com","sentAt":"2026-08-19T23:28:10Z","receivedAt":"2026-08-19T23:28:13Z","isPatch":true,"body":"From: Friel \n\nWe want to measure how compression settings affect push performance on\nthe client. Different settings can produce different-sized packs from\nthe same objects. Trace2 records the object count, but we also need the\npack size to compare those settings.\n\nAdd a write_pack_file/wrote_bytes Trace2 datum alongside\nwrite_pack_file/wrote. Count packs written to stdout or disk, including\neach pack's header and trailing checksum. When pack.packSizeLimit splits\nthe output, report the sum of the pack sizes.\n\nSigned-off-by: Friel \n---\nJunio, you're right. Updating bytes_written before finalization is\nequivalent. I've dropped pack_bytes; everything else is unchanged.\nThanks.\n\n builtin/pack-objects.c | 5 +++++\n t/t5300-pack-object.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1ec5b6f206..252530172c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1337,6 +1337,7 @@ static void write_pack_file(void)\n \tuint32_t nr_remaining = nr_result;\n \ttime_t last_mtime = 0;\n \tstruct object_entry **write_order;\n+\toff_t bytes_written = 0;\n \n \tif (progress > pack_to_stdout)\n \t\tprogress_state = start_progress(the_repository,\n@@ -1389,6 +1390,8 @@ static void write_pack_file(void)\n \t\t\tdisplay_progress(progress_state, written);\n \t\t}\n \n+\t\tbytes_written += hashfile_total(f) +\n+\t\t\tthe_repository->hash_algo->rawsz;\n \t\tif (pack_to_stdout) {\n \t\t\t/*\n \t\t\t * We never fsync when writing to stdout since we may\n@@ -1510,6 +1513,8 @@ static void write_pack_file(void)\n \t\t written, nr_result);\n \ttrace2_data_intmax(\"pack-objects\", the_repository,\n \t\t\t \"write_pack_file/wrote\", nr_result);\n+\ttrace2_data_intmax(\"pack-objects\", the_repository,\n+\t\t\t \"write_pack_file/wrote_bytes\", bytes_written);\n }\n \n static int no_try_delta(const char *path)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 9dabb3615a..aac139e6a0 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -33,6 +33,30 @@ test_expect_success 'setup' '\n \t} >expect\n '\n \n+test_expect_success 'pack-object traces bytes written to stdout' '\n+\ttest_when_finished \"rm -f pack.trace pack.pack\" &&\n+\tGIT_TRACE2_EVENT=\"$PWD/pack.trace\" \\\n+\t\tgit pack-objects --quiet --revs --stdout >pack.pack <<-EOF &&\n+\t$commit\n+\tEOF\n+\tbytes=$(test_file_size pack.pack) &&\n+\ttest_grep \"\\\"key\\\":\\\"write_pack_file/wrote_bytes\\\",\\\"value\\\":\\\"$bytes\\\"\" pack.trace\n+'\n+\n+test_expect_success 'pack-object traces bytes written to split pack files' '\n+\ttest_when_finished \"rm -f split.trace traced-pack-*\" &&\n+\tGIT_TRACE2_EVENT=\"$PWD/split.trace\" \\\n+\t\tgit -c pack.packSizeLimit=3m pack-objects --quiet traced-pack hash_algo->rawsz;\n \t\tif (pack_to_stdout) {\n \t\t\t/*\n @@ -1423,7 +1422,6 @@ static void write_pack_file(void)\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t}\n \t\t}\n -\t\tbytes_written += pack_bytes;\n \n \t\tif (!pack_to_stdout) {\n \t\t\tstruct stat st;\n\nbase-commit: 18e66859d87fb4b76599f73460b54f0848c76b16\n"},{"id":"550863","messageId":"aoaTjWMSO8og_iFw@pks.im","threadId":"66185","inReplyTo":"c6a8cdac36d2202055d637ebcc97e484122cdcd4.1787158152.git.friel@openai.com","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-20T05:41:33Z","receivedAt":"2026-08-20T05:41:40Z","isPatch":true,"body":"On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 1ec5b6f206..252530172c 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1389,6 +1390,8 @@ static void write_pack_file(void)\n> \t\t\tdisplay_progress(progress_state, written);\n> \t\t}\n> \n> +\t\tbytes_written += hashfile_total(f) +\n> +\t\t\tthe_repository->hash_algo->rawsz;\n> \t\tif (pack_to_stdout) {\n> \t\t\t/*\n> \t\t\t * We never fsync when writing to stdout since we may\n\nI guess the addition here accounts for the trailing hash written by the\nhashfile. If so, shouldn't we also use the algortihm that the hashfile\nuses in the first place via `f->algop->rawsz`?\n\n> @@ -1510,6 +1513,8 @@ static void write_pack_file(void)\n> \t\t written, nr_result);\n> \ttrace2_data_intmax(\"pack-objects\", the_repository,\n> \t\t\t \"write_pack_file/wrote\", nr_result);\n> +\ttrace2_data_intmax(\"pack-objects\", the_repository,\n> +\t\t\t \"write_pack_file/wrote_bytes\", bytes_written);\n> }\n> \n> static int no_try_delta(const char *path)\n\nThe \"write_pack_file/wrote\" event is quite awkwardly named, if you ask\nme, as it's not immediately obvious what exactly it's counting, and the\nsecond metric may make this even more confusing. In retrospect it\nwould've been preferable to call this \"wrote_objects\" to clarify.\n\nI don't really think we guarantee any kind of stability around those\ntraces, so we could in theory change it here, too. But I don't feel like\nmy argument is strong enough to really warrant such a change, so maybe\nwe should just leave it as-is.\n\nThanks!\n\nPatrick\n"},{"id":"550879","messageId":"20260820082102.GA2973952@coredump.intra.peff.net","threadId":"66185","inReplyTo":"aoaTjWMSO8og_iFw@pks.im","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-20T08:21:02Z","receivedAt":"2026-08-20T08:21:03Z","isPatch":true,"body":"On Thu, Aug 20, 2026 at 07:41:33AM +0200, Patrick Steinhardt wrote:\n\n> On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 1ec5b6f206..252530172c 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)\n> > \t\t\tdisplay_progress(progress_state, written);\n> > \t\t}\n> > \n> > +\t\tbytes_written += hashfile_total(f) +\n> > +\t\t\tthe_repository->hash_algo->rawsz;\n> > \t\tif (pack_to_stdout) {\n> > \t\t\t/*\n> > \t\t\t * We never fsync when writing to stdout since we may\n> \n> I guess the addition here accounts for the trailing hash written by the\n> hashfile. If so, shouldn't we also use the algortihm that the hashfile\n> uses in the first place via `f->algop->rawsz`?\n\nPerhaps, though that is used to write the hash (via CSUM_HASH_IN_STREAM)\nonly in two of the conditional blocks. In the third we finalize the\nhashfile and then use fixup_pack_header_footer(), passing the_hash_algo\ndirectly (not even the_repository->hash_algo, though of course they mean\nthe same thing).\n\nIt all works out, of course, because we created the hashfile struct\nearlier using the_repository->hash_algo. So I think this is mostly\nacademic in the first place, but your suggestion harmonizes two of the\nconditional blocks while creating disagreement with the third.\n\nI think something like this would \"fix\" it by consistently using the\nhashfile's algo in all three blocks:\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4a5fcbe5f5..0fdff72f41 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1413,9 +1413,9 @@ static void write_pack_file(void)\n \t\t\t * If we wrote the wrong number of entries in the\n \t\t\t * header, rewrite it like in fast-import.\n \t\t\t */\n-\n+\t\t\tconst struct git_hash_algo *algo = f->algop;\n \t\t\tint fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);\n-\t\t\tfixup_pack_header_footer(the_hash_algo, fd, hash,\n+\t\t\tfixup_pack_header_footer(algo, fd, hash,\n \t\t\t\t\t\t pack_tmp_name, nr_written,\n \t\t\t\t\t\t hash, offset);\n \t\t\tclose(fd);\n\n\nBut there's a subtle yet interesting difference here! f->algop won't\nnecessarily be the same pointer as the_hash_algo. If we compiled with an\nunsafe variant, that will be used for hashfiles. If we're just looking\nat rawsz that's OK; the two variants should be identical (other than\nperformance and collision detection), so taking rawsz from either is\nfine.\n\nBut fixup_pack_header_footer() actually recomputes the hash (as it must\nif we tweak the header). Right now it does it using the \"normal\"\nvariant, but we should be able to use the unsafe one (which my diff\nsnippet above would start to do).\n\nOf course this whole thing is absurdly pessimal in the first place. If\nwe are just going to throw out the hashfile's checksum, then why bother\ncomputing it in the first place? Because we don't trust a disk write at\nall, and actually verify the original hash computation as we read the\nbytes back in! So we'll actually sha1 the written packfile three times.\nYikes. I wonder if it's really worth being so paranoid. But that is how\nit has always been.\n\nAnyway, that is a bit of a tangent from the patch in question. I think\neither spelling is OK for the purposes of this patch. If somebody wants\nto pursue harmonizing the paths (and maybe even doing some timings to\nsee if switching to the unsafe variant is noticeable here, and what the\ntotal cost of this triple-write approach is), that can happen\nseparately.\n\n-Peff\n"},{"id":"550882","messageId":"aobFLJuiuM1EuNpv@pks.im","threadId":"66185","inReplyTo":"20260820082102.GA2973952@coredump.intra.peff.net","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-20T09:13:16Z","receivedAt":"2026-08-20T09:13:24Z","isPatch":true,"body":"On Thu, Aug 20, 2026 at 04:21:02AM -0400, Jeff King wrote:\n> On Thu, Aug 20, 2026 at 07:41:33AM +0200, Patrick Steinhardt wrote:\n> > On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:\n> > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > > index 1ec5b6f206..252530172c 100644\n> > > --- a/builtin/pack-objects.c\n> > > +++ b/builtin/pack-objects.c\n> > > @@ -1389,6 +1390,8 @@ static void write_pack_file(void)\n> > > \t\t\tdisplay_progress(progress_state, written);\n> > > \t\t}\n> > > \n> > > +\t\tbytes_written += hashfile_total(f) +\n> > > +\t\t\tthe_repository->hash_algo->rawsz;\n> > > \t\tif (pack_to_stdout) {\n> > > \t\t\t/*\n> > > \t\t\t * We never fsync when writing to stdout since we may\n> > \n> > I guess the addition here accounts for the trailing hash written by the\n> > hashfile. If so, shouldn't we also use the algortihm that the hashfile\n> > uses in the first place via `f->algop->rawsz`?\n> \n> Perhaps, though that is used to write the hash (via CSUM_HASH_IN_STREAM)\n> only in two of the conditional blocks. In the third we finalize the\n> hashfile and then use fixup_pack_header_footer(), passing the_hash_algo\n> directly (not even the_repository->hash_algo, though of course they mean\n> the same thing).\n> \n> It all works out, of course, because we created the hashfile struct\n> earlier using the_repository->hash_algo. So I think this is mostly\n> academic in the first place, but your suggestion harmonizes two of the\n> conditional blocks while creating disagreement with the third.\n> \n> I think something like this would \"fix\" it by consistently using the\n> hashfile's algo in all three blocks:\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 4a5fcbe5f5..0fdff72f41 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1413,9 +1413,9 @@ static void write_pack_file(void)\n> \t\t\t * If we wrote the wrong number of entries in the\n> \t\t\t * header, rewrite it like in fast-import.\n> \t\t\t */\n> -\n> +\t\t\tconst struct git_hash_algo *algo = f->algop;\n> \t\t\tint fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);\n> -\t\t\tfixup_pack_header_footer(the_hash_algo, fd, hash,\n> +\t\t\tfixup_pack_header_footer(algo, fd, hash,\n> \t\t\t\t\t\t pack_tmp_name, nr_written,\n> \t\t\t\t\t\t hash, offset);\n> \t\t\tclose(fd);\n> \n> \n> But there's a subtle yet interesting difference here! f->algop won't\n> necessarily be the same pointer as the_hash_algo. If we compiled with an\n> unsafe variant, that will be used for hashfiles. If we're just looking\n> at rawsz that's OK; the two variants should be identical (other than\n> performance and collision detection), so taking rawsz from either is\n> fine.\n> \n> But fixup_pack_header_footer() actually recomputes the hash (as it must\n> if we tweak the header). Right now it does it using the \"normal\"\n> variant, but we should be able to use the unsafe one (which my diff\n> snippet above would start to do).\n\nYeah, I agree that switching over to the unsafe algortihm is sensible.\nBeing able to speed up hashing of packfiles was one of the prime\nmotivations of introducing the unsafe variants in the first place, so\nthe fact that we still use the safe variant here feels like a plain\noversight to me.\n\n> Of course this whole thing is absurdly pessimal in the first place. If\n> we are just going to throw out the hashfile's checksum, then why bother\n> computing it in the first place? Because we don't trust a disk write at\n> all, and actually verify the original hash computation as we read the\n> bytes back in! So we'll actually sha1 the written packfile three times.\n> Yikes. I wonder if it's really worth being so paranoid. But that is how\n> it has always been.\n\nThat's... awful. Honestly, if we cannot trust what we're writing to disk\nwe're going to be kind of screwed anyway. We don't re-verify loose\nobjects, refs or whatever other data structures we write to disk either.\nSo doing this thrice here feels wrong.\n\n> Anyway, that is a bit of a tangent from the patch in question. I think\n> either spelling is OK for the purposes of this patch. If somebody wants\n> to pursue harmonizing the paths (and maybe even doing some timings to\n> see if switching to the unsafe variant is noticeable here, and what the\n> total cost of this triple-write approach is), that can happen\n> separately.\n\nI agree that this is definitely out of scope of this patch series.\n\nPatrick\n"},{"id":"550913","messageId":"xmqq4igou7o7.fsf@gitster.g","threadId":"66185","inReplyTo":"20260820082102.GA2973952@coredump.intra.peff.net","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-20T15:35:04Z","receivedAt":"2026-08-20T15:35:07Z","isPatch":true,"body":"Jeff King writes:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 4a5fcbe5f5..0fdff72f41 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1413,9 +1413,9 @@ static void write_pack_file(void)\n> \t\t\t * If we wrote the wrong number of entries in the\n> \t\t\t * header, rewrite it like in fast-import.\n> \t\t\t */\n> -\n> +\t\t\tconst struct git_hash_algo *algo = f->algop;\n> \t\t\tint fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);\n> -\t\t\tfixup_pack_header_footer(the_hash_algo, fd, hash,\n> +\t\t\tfixup_pack_header_footer(algo, fd, hash,\n> \t\t\t\t\t\t pack_tmp_name, nr_written,\n> \t\t\t\t\t\t hash, offset);\n> \t\t\tclose(fd);\n>\n> ...\n> But fixup_pack_header_footer() actually recomputes the hash (as it must\n> if we tweak the header). Right now it does it using the \"normal\"\n> variant, but we should be able to use the unsafe one (which my diff\n> snippet above would start to do).\n\nI am amused. This is an interesting find.\n\nThanks.\n\n"},{"id":"550965","messageId":"20260821004019.GA296407@coredump.intra.peff.net","threadId":"66185","inReplyTo":"aobFLJuiuM1EuNpv@pks.im","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-21T00:40:19Z","receivedAt":"2026-08-21T00:40:22Z","isPatch":true,"body":"On Thu, Aug 20, 2026 at 11:13:16AM +0200, Patrick Steinhardt wrote:\n\n> > But there's a subtle yet interesting difference here! f->algop won't\n> > necessarily be the same pointer as the_hash_algo. If we compiled with an\n> > unsafe variant, that will be used for hashfiles. If we're just looking\n> > at rawsz that's OK; the two variants should be identical (other than\n> > performance and collision detection), so taking rawsz from either is\n> > fine.\n> > \n> > But fixup_pack_header_footer() actually recomputes the hash (as it must\n> > if we tweak the header). Right now it does it using the \"normal\"\n> > variant, but we should be able to use the unsafe one (which my diff\n> > snippet above would start to do).\n> \n> Yeah, I agree that switching over to the unsafe algortihm is sensible.\n> Being able to speed up hashing of packfiles was one of the prime\n> motivations of introducing the unsafe variants in the first place, so\n> the fact that we still use the safe variant here feels like a plain\n> oversight to me.\n\nYes, though I think the oversight can be forgiven here. The unsafe\nvariants are purely for performance, so we started by converting a few\nhot code paths, knowing that it was OK to leave other spots using the\ncollision-detecting implementation. The main one we cared about is\n\"pack-objects --stdout\" to serve fetches.\n\nBut this particular case is almost never exercised! It triggers only\nwhen --max-pack-size causes us to split the result into multiple packs\n(we can't write the header up front in that case, because we don't know\nhow many objects we'll fit into the output). So I doubt anybody would\nhave noticed or cared about the performance difference.\n\nBut it also means that cleaning up the triple-hash is tricky. The three\nhashes in this code path are:\n\n a. we hash as we write, via struct hashfile\n\n b. we hash as we read back the data to verify it\n\n c. we re-hash the data on top of the fixed-up header\n\nWe can obviously drop (b) if we choose. We can't drop (c); it's the\nfinal value that goes into the on-disk packfile. So we'd like to drop\n(a), which is pointless (except for cross-checking step b).\n\nBut we don't know if we're in this code path until we've finished\nwriting the file! If the output is smaller than --max-pack-size, then we\njust write the hash from (a) directly, and neither (b) nor (c) happens\nat all. This is the \"nr_written == nr_remaining\" conditional, the second\nin the chain.\n\nWe could pessimistically assume that we'll need to do (c), and skip the\nhash for (a). But that is worse for the usual case that we don't split\nthe packfiles. Instead of hashing as the data is written, we have to\nre-read it (passing all those bytes through memory again).\n\nSo realistically the best we can do is drop (b).\n\nOf course what I'd _really_ like to do is rip out --max-pack-size\nentirely. I don't think it's generally helpful, and it introduces all\nkinds of weird corner cases and complications like this. But obviously\nthat's a much bigger change, and naturally if I seriously proposed it\nsomebody would come out of the woodwork so with obscure case where it's\nuseful.\n\n> > Of course this whole thing is absurdly pessimal in the first place. If\n> > we are just going to throw out the hashfile's checksum, then why bother\n> > computing it in the first place? Because we don't trust a disk write at\n> > all, and actually verify the original hash computation as we read the\n> > bytes back in! So we'll actually sha1 the written packfile three times.\n> > Yikes. I wonder if it's really worth being so paranoid. But that is how\n> > it has always been.\n> \n> That's... awful. Honestly, if we cannot trust what we're writing to disk\n> we're going to be kind of screwed anyway. We don't re-verify loose\n> objects, refs or whatever other data structures we write to disk either.\n> So doing this thrice here feels wrong.\n\nThere was an attitude in the early days of Git that we should be\nchecking hashes and checksums all the time. I.e., that the validity of\nthe data was the most precious thing, and we should notice an on-disk\ncorruption as quickly and reliably as possible, similar to filesystems\nthat checksum the data.\n\nBut over time we've relaxed that quite a bit because of the quite\nnoticeable costs. For example, we used to re-hash every object we\nloaded, but these days we have PARSE_OBJECT_SKIP_HASH_CHECK, and\nfeatures like the commit graph.\n\nI think this is a case where we could similarly relax. Especially\nbecause this is just the pack checksum. The actual object contents are\nstill protected by their respective hashes.\n\n-Peff\n"},{"id":"550967","messageId":"20260821004739.GA297273@coredump.intra.peff.net","threadId":"66185","inReplyTo":"c6a8cdac36d2202055d637ebcc97e484122cdcd4.1787158152.git.friel@openai.com","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-21T00:47:39Z","receivedAt":"2026-08-21T00:47:40Z","isPatch":true,"body":"On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:\n\n> From: Friel \n> \n> We want to measure how compression settings affect push performance on\n> the client. Different settings can produce different-sized packs from\n> the same objects. Trace2 records the object count, but we also need the\n> pack size to compare those settings.\n> \n> Add a write_pack_file/wrote_bytes Trace2 datum alongside\n> write_pack_file/wrote. Count packs written to stdout or disk, including\n> each pack's header and trailing checksum. When pack.packSizeLimit splits\n> the output, report the sum of the pack sizes.\n> \n> Signed-off-by: Friel \n> ---\n> Junio, you're right. Updating bytes_written before finalization is\n> equivalent. I've dropped pack_bytes; everything else is unchanged.\n> Thanks.\n\nThe downthread discussion went pretty far off-topic, so for those who do\nnot want to read it, the summary is: this patch looks good to me. ;)\n\n-Peff\n"},{"id":"550973","messageId":"xmqqpkzcrvun.fsf@gitster.g","threadId":"66185","inReplyTo":"20260821004019.GA296407@coredump.intra.peff.net","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-21T03:33:20Z","receivedAt":"2026-08-21T03:33:22Z","isPatch":true,"body":"Jeff King writes:\n\n> Of course what I'd _really_ like to do is rip out --max-pack-size\n> entirely. I don't think it's generally helpful, and it introduces all\n> kinds of weird corner cases and complications like this. But obviously\n> that's a much bigger change, and naturally if I seriously proposed it\n> somebody would come out of the woodwork so with obscure case where it's\n> useful.\n\n;-) Perhaps Git 3.0 boundary?\n\n> I think this is a case where we could similarly relax. Especially\n> because this is just the pack checksum. The actual object contents are\n> still protected by their respective hashes.\n\nYes. Dropping the \"(b) validate as we re-read\" step is a reasonable\nthing to do with the least disruption from that viewpoint.\n\nThanks.\n\n\n"},{"id":"550974","messageId":"xmqqlda0rvtq.fsf@gitster.g","threadId":"66185","inReplyTo":"20260821004739.GA297273@coredump.intra.peff.net","subject":"Re: [PATCH v2] pack-objects: trace pack bytes written","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-21T03:33:53Z","receivedAt":"2026-08-21T03:33:55Z","isPatch":true,"body":"Jeff King writes:\n\n> On Wed, Aug 19, 2026 at 04:28:10PM -0700, friel@openai.com wrote:\n>\n>> From: Friel \n>> \n>> We want to measure how compression settings affect push performance on\n>> the client. Different settings can produce different-sized packs from\n>> the same objects. Trace2 records the object count, but we also need the\n>> pack size to compare those settings.\n>> \n>> Add a write_pack_file/wrote_bytes Trace2 datum alongside\n>> write_pack_file/wrote. Count packs written to stdout or disk, including\n>> each pack's header and trailing checksum. When pack.packSizeLimit splits\n>> the output, report the sum of the pack sizes.\n>> \n>> Signed-off-by: Friel \n>> ---\n>> Junio, you're right. Updating bytes_written before finalization is\n>> equivalent. I've dropped pack_bytes; everything else is unchanged.\n>> Thanks.\n>\n> The downthread discussion went pretty far off-topic, so for those who do\n> not want to read it, the summary is: this patch looks good to me. ;)\n\nIt looks good to me, too. Thanks, all.\n"}]}