{"thread":{"id":"51136","subject":"[PATCH] hash-object: don't pointlessly zlib compress without -w","startedAt":"2019-05-20T22:29:47Z","lastAt":"2019-05-22T05:32:17Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"375964","messageId":"20190520222932.22843-1-avarab@gmail.com","threadId":"51136","inReplyTo":null,"subject":"[PATCH] hash-object: don't pointlessly zlib compress without -w","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-20T22:29:32Z","receivedAt":"2019-05-20T22:29:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When hash-object hashes something the size of core.bigFileThreshold or\nlarger (512MB by default) it'll be streamed through\nstream_to_pack().\n\nThat added in 568508e765 (\"bulk-checkin: replace fast-import based\nimplementation\", 2011-10-28) would compress the file with zlib, but\nwas oblivious as to whether the content would actually be written out\nto disk, which isn't the case unless hash-object is called with the\n\"-w\" option.\n\nHashing is much slower if we need to compress the content, so let's\ncheck if the HASH_WRITE_OBJECT flag has been given.\n\nAn accompanying perf test shows how much this improves things. With\nCFLAGS=-O3 and OPENSSL_SHA1=Y the relevant change is (manually\nreformatted to avoid long lines):\n\n 1007.6: 'git hash-object ' with threshold=32M\n -> 1.57(1.55+0.01) 0.09(0.09+0.00) -94.3%\n 1007.7: 'git hash-object --stdin < ' with threshold=32M\n -> 1.57(1.57+0.00) 0.09(0.07+0.01) -94.3%\n 1007.8: 'echo | git hash-object --stdin-paths' threshold=32M\n -> 1.59(1.56+0.00) 0.09(0.08+0.00) -94.3%\n\nThe same tests using \"-w\" still take that long, since those will need\nto zlib compress the relevant object. With the sha1collisiondetection\nlibrary (our default) there's less of a difference since the hashing\nitself is slower, or respectively:\n\n 1.71(1.65+0.01) 0.19(0.18+0.01) -88.9%\n 1.70(1.66+0.02) 0.19(0.19+0.00) -88.8%\n 1.69(1.66+0.00) 0.19(0.18+0.00) -88.8%\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bulk-checkin.c | 3 ++-\n t/perf/p1007-hash-object.sh | 53 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 1 deletion(-)\n create mode 100755 t/perf/p1007-hash-object.sh\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 39ee7d6107..a26126ee76 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -105,8 +105,9 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n \tint status = Z_OK;\n \tint write_object = (flags & HASH_WRITE_OBJECT);\n \toff_t offset = 0;\n+\tint level = write_object ? pack_compression_level : Z_NO_COMPRESSION;\n \n-\tgit_deflate_init(&s, pack_compression_level);\n+\tgit_deflate_init(&s, level);\n \n \thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n \ts.next_out = obuf + hdrlen;\ndiff --git a/t/perf/p1007-hash-object.sh b/t/perf/p1007-hash-object.sh\nnew file mode 100755\nindex 0000000000..8df6dc59a5\n--- /dev/null\n+++ b/t/perf/p1007-hash-object.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description=\"Tests performance of hash-object\"\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_lazy_prereq SHA1SUM_AND_SANE_DD_AND_URANDOM '\n+\t>empty &&\n+\tsha1sum empty >empty.sha1sum &&\n+\tgrep -q -w da39a3ee5e6b4b0d3255bfef95601890afd80709 empty.sha1sum &&\n+\tdd if=/dev/urandom of=random.test bs=1024 count=1 &&\n+\tstat -c %s random.test >random.size &&\n+\tgrep -q -x 1024 random.size\n+'\n+\n+if test_have_prereq !SHA1SUM_AND_SANE_DD_AND_URANDOM\n+then\n+\tskip_all='failed prereq check for sha1sum/dd/stat'\n+\ttest_perf 'dummy p0013 test (skipped all tests)' 'true'\n+\ttest_done\n+fi\n+\n+test_expect_success 'setup 64MB file.random file' '\n+\tdd if=/dev/urandom of=file.random count=$((64*1024)) bs=1024\n+'\n+\n+test_perf 'sha1sum(1) on file.random (for comparison)' '\n+\tsha1sum file.random\n+'\n+\n+for threshold in 32M 64M\n+do\n+\tfor write in '' ' -w'\n+\tdo\n+\t\tfor literally in ' --literally -t commit' ''\n+\t\tdo\n+\t\t\ttest_perf \"'git hash-object$write$literally ' with threshold=$threshold\" \"\n+\t\t\t\tgit -c core.bigFileThreshold=$threshold hash-object$write$literally file.random\n+\t\t\t\"\n+\n+\t\t\ttest_perf \"'git hash-object$write$literally --stdin < ' with threshold=$threshold\" \"\n+\t\t\t\tgit -c core.bigFileThreshold=$threshold hash-object$write$literally --stdin | git hash-object$write$literally --stdin-paths' threshold=$threshold\" \"\n+\t\t\t\techo file.random | git -c core.bigFileThreshold=$threshold hash-object$write$literally --stdin-paths\n+\t\t\t\"\n+\t\tdone\n+\tdone\n+done\n+\n+test_done\n-- \n2.21.0.1020.gf2820cf01a\n\n"},{"id":"376051","messageId":"20190522053213.GD29933@sigill.intra.peff.net","threadId":"51136","inReplyTo":"20190520222932.22843-1-avarab@gmail.com","subject":"Re: [PATCH] hash-object: don't pointlessly zlib compress without -w","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-22T05:32:13Z","receivedAt":"2019-05-22T05:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 21, 2019 at 12:29:32AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> That added in 568508e765 (\"bulk-checkin: replace fast-import based\n> implementation\", 2011-10-28) would compress the file with zlib, but\n> was oblivious as to whether the content would actually be written out\n> to disk, which isn't the case unless hash-object is called with the\n> \"-w\" option.\n\nYeah, this seems like an obvious and easy improvement. I'd guess that\nnot many people use hash-object without \"-w\", so nobody ever really\ncared much.\n\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 39ee7d6107..a26126ee76 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -105,8 +105,9 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n> \tint status = Z_OK;\n> \tint write_object = (flags & HASH_WRITE_OBJECT);\n> \toff_t offset = 0;\n> +\tint level = write_object ? pack_compression_level : Z_NO_COMPRESSION;\n> \n> -\tgit_deflate_init(&s, pack_compression_level);\n> +\tgit_deflate_init(&s, level);\n\nHmm. It really seems like if we're not writing, that we could avoid this\nwhole pack-encoding business altogether, and just stream the contents\nthrough the hash, no matter the size. E.g., we're paying extra here to\ncompute a pack crc32 that nobody cares about, not to mention the zlib\ncrc.\n\nThat said, this is just not that common a case, and you've regained the\nvast majority of the performance with this simple change. So it may not\nbe worth pursuing further.\n\n> +test_lazy_prereq SHA1SUM_AND_SANE_DD_AND_URANDOM '\n> +\t>empty &&\n> +\tsha1sum empty >empty.sha1sum &&\n> +\tgrep -q -w da39a3ee5e6b4b0d3255bfef95601890afd80709 empty.sha1sum &&\n> +\tdd if=/dev/urandom of=random.test bs=1024 count=1 &&\n> +\tstat -c %s random.test >random.size &&\n> +\tgrep -q -x 1024 random.size\n> +'\n\nI don't think this needs to be cryptographically random. You could just\nuse \"test-tool genrandom\" instead, which gets rid of the need for dd and\n/dev/urandom (and as a bonus, is more deterministic since it's a seeded\nLCG).\n\n> +test_perf 'sha1sum(1) on file.random (for comparison)' '\n> +\tsha1sum file.random\n> +'\n\nI'm not sure this is worth the prereq it entails. Who really cares how\nfast sha1sum is? What we generally care about in the perf suite is how\nGit speeds compare. Primarily between versions (to show off\nimprovements, or find regressions), but sometimes between two builds\n(but sadly the perf output is not good at comparing those numbers\nautomatically).\n\nI know this sets a baseline, but it feels a bit like apples to oranges.\n\n> +for threshold in 32M 64M\n> +do\n> +\tfor write in '' ' -w'\n> +\tdo\n> +\t\tfor literally in ' --literally -t commit' ''\n\nIs this \"-t commit\" worth it? I guess it proves that --literally is\nbeing used, since we'd choke otherwise. But for a perf test, it seems\nlike you'd want to change as few variables as possible.\n\n> +\t\t\ttest_perf \"'git hash-object$write$literally --stdin < ' with threshold=$threshold\" \"\n\nThis test title violates our shell style (whitespace after \"<\"). ;)\n\n(Yes, I know it was probably to avoid \"<<\").\n\n> +\t\t\ttest_perf \"'echo | git hash-object$write$literally --stdin-paths' threshold=$threshold\" \"\n> +\t\t\t\techo file.random | git -c core.bigFileThreshold=$threshold hash-object$write$literally --stdin-paths\n\nYou have to bunch up the option variables in the title to get the\nwhitespace right. But in the command itself, you can do:\n\n hash-object $write $literally ...\n\nand the shell's whitespace handling will make it work. Maybe worth it to\nmake the source a bit more readable.\n\n-Peff\n"}]}