{"thread":{"id":"54086","subject":"[PATCH 0/3] index-pack threading defaults","startedAt":"2020-08-21T17:51:57Z","lastAt":"2020-08-24T17:55:59Z","messageCount":13,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"404169","messageId":"20200821175153.GA3263018@coredump.intra.peff.net","threadId":"54086","inReplyTo":null,"subject":"[PATCH 0/3] index-pack threading defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:51:53Z","receivedAt":"2020-08-21T17:51:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I decided to revisit index-pack's hard-coded default of 3 threads. And\nit turns out that higher numbers perform much better. :)\n\nThat value was determined experimentally in 2012. I'm not sure of the\nexact reason it's different now (modern processors are better at\nparallelism, or modern git is better at parallelism, or the original\nexperiment was just a fluke). But regardless, I can get on the order of\na two-to-one speedup by bumping the thread count. See the final patch\nfor timings and more specific discussion.\n\nThe first two patches are just perf-test improvements (actually, the\nslowness to value ratio of p5302 from the first patch is why I was\nlooking at this at all).\n\n  [1/3]: p5302: disable thread-count parameter tests by default\n  [2/3]: p5302: count up to online-cpus for thread tests\n  [3/3]: index-pack: adjust default threading cap\n\n builtin/index-pack.c       | 19 ++++++++++++---\n t/perf/README              |  9 ++++++++\n t/perf/p5302-pack-index.sh | 47 +++++++++++++++++++-------------------\n t/perf/perf-lib.sh         |  2 ++\n 4 files changed, 51 insertions(+), 26 deletions(-)\n\n-Peff\n"},{"id":"404170","messageId":"20200821175339.GA3263141@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200821175153.GA3263018@coredump.intra.peff.net","subject":"[PATCH 1/3] p5302: disable thread-count parameter tests by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:53:39Z","receivedAt":"2020-08-21T17:53:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The primary function of the perf suite is to detect regressions (or\nimprovements) between versions of Git. The only numbers we show a direct\ncomparison for are timings between the same test run on two different\nversions.\n\nHowever, it can sometimes be used to collect other information.  For\ninstance, p5302 runs the same index-pack operation with different thread\ncounts. The output doesn't directly compare these, but anybody\ninterested in working on index-pack can manually compare the results.\n\nFor a normal regression run of the full perf-suite, though, this incurs\na significant cost to generate numbers nobody will actually look at;\nabout 25% of the total time of the test suite is spent in p5302. And the\nlow-thread-count runs are the most expensive part of it, since they're\n(unsurprisingly) not using as many threads.\n\nLet's skip these tests by default, but make it possible for people\nworking on index-pack to still run them by setting an environment\nvariable. Rather than make this specific to p5302, let's introduce a\ngeneric mechanism. This makes it possible to run the full suite with\nevery possible test if somebody really wants to burn some CPU.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/README              |  9 +++++++++\n t/perf/p5302-pack-index.sh | 10 +++++-----\n t/perf/perf-lib.sh         |  2 ++\n 3 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex c7b70e2d28..bd649afa97 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -84,6 +84,15 @@ You can set the following variables (also in your config.mak):\n \tprobably be about linux.git size for optimal results.\n \tBoth default to the git.git you are running from.\n \n+    GIT_PERF_EXTRA\n+\tBoolean to enable additional tests. Most test scripts are\n+\twritten to detect regressions between two versions of Git, and\n+\tthe output will compare timings for individual tests between\n+\tthose versions. Some scripts have additional tests which are not\n+\trun by default, that show patterns within a single version of\n+\tGit (e.g., performance of index-pack as the number of threads\n+\tchanges). These can be enabled with GIT_PERF_EXTRA.\n+\n You can also pass the options taken by ordinary git tests; the most\n useful one is:\n \ndiff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh\nindex a9b3e112d9..23011ab739 100755\n--- a/t/perf/p5302-pack-index.sh\n+++ b/t/perf/p5302-pack-index.sh\n@@ -13,31 +13,31 @@ test_expect_success 'repack' '\n \texport PACK\n '\n \n-test_perf 'index-pack 0 threads' '\n+test_perf PERF_EXTRA 'index-pack 0 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK\n '\n \n-test_perf 'index-pack 1 thread ' '\n+test_perf PERF_EXTRA 'index-pack 1 thread ' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git GIT_FORCE_THREADS=1 git index-pack --threads=1 --stdin < $PACK\n '\n \n-test_perf 'index-pack 2 threads' '\n+test_perf PERF_EXTRA 'index-pack 2 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git git index-pack --threads=2 --stdin < $PACK\n '\n \n-test_perf 'index-pack 4 threads' '\n+test_perf PERF_EXTRA 'index-pack 4 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git git index-pack --threads=4 --stdin < $PACK\n '\n \n-test_perf 'index-pack 8 threads' '\n+test_perf PERF_EXTRA 'index-pack 8 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git git index-pack --threads=8 --stdin < $PACK\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 13e389367a..821581a885 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -245,3 +245,5 @@ test_at_end_hook_ () {\n test_export () {\n \texport \"$@\"\n }\n+\n+test_lazy_prereq PERF_EXTRA 'test_bool_env GIT_PERF_EXTRA false'\n-- \n2.28.0.694.g07780f7063\n\n"},{"id":"404171","messageId":"20200821175451.GB3263141@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200821175153.GA3263018@coredump.intra.peff.net","subject":"[PATCH 2/3] p5302: count up to online-cpus for thread tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:54:51Z","receivedAt":"2020-08-21T17:54:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When PERF_EXTRA is enabled, p5302 checks the performance of index-pack\nwith various numbers of threads. This can be useful for deciding what\nthe default should be (which is currently capped at 3 threads based on\nthe results of this script).\n\nHowever, we only go up to 8 threads, and modern machines may have more.\nLet's get the number of CPUs from test-tool, and test various numbers of\nthreads between one and that maximum.\n\nNote that the current tests aren't all identical, as we have to set\nGIT_FORCE_THREADS for the --threads=1 test (which measures the overhead\nof starting a single worker thread versus the \"0\" case of using the main\nthread). To keep the loop simple, we'll keep the \"0\" case out of it, and\nset GIT_FORCE_THREADS=1 for all of the other cases (it's a noop for all\nbut the \"1\" case, since numbers higher than 1 would always need\nthreads).\n\nNote also that we could skip running \"test-tool\" if PERF_EXTRA isn't\nset. However, there's some small value in knowing the number of threads,\nso that we can mark each test as skipped in the output.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/p5302-pack-index.sh | 47 +++++++++++++++++++-------------------\n 1 file changed, 24 insertions(+), 23 deletions(-)\n\ndiff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh\nindex 23011ab739..228593d9ad 100755\n--- a/t/perf/p5302-pack-index.sh\n+++ b/t/perf/p5302-pack-index.sh\n@@ -13,35 +13,36 @@ test_expect_success 'repack' '\n \texport PACK\n '\n \n-test_perf PERF_EXTRA 'index-pack 0 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK\n+# Rather than counting up and doubling each time, count down from the endpoint,\n+# halving each time. That ensures that our final test uses as many threads as\n+# CPUs, even if it isn't a power of 2.\n+test_expect_success 'set up thread-counting tests' '\n+\tt=$(test-tool online-cpus) &&\n+\tthreads= &&\n+\twhile test $t -gt 0\n+\tdo\n+\t\tthreads=\"$t $threads\"\n+\t\tt=$((t / 2))\n+\tdone\n '\n \n-test_perf PERF_EXTRA 'index-pack 1 thread ' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git GIT_FORCE_THREADS=1 git index-pack --threads=1 --stdin < $PACK\n-'\n-\n-test_perf PERF_EXTRA 'index-pack 2 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=2 --stdin < $PACK\n-'\n-\n-test_perf PERF_EXTRA 'index-pack 4 threads' '\n+test_perf PERF_EXTRA 'index-pack 0 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=4 --stdin < $PACK\n+\tGIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK\n '\n \n-test_perf PERF_EXTRA 'index-pack 8 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=8 --stdin < $PACK\n-'\n+for t in $threads\n+do\n+\tTHREADS=$t\n+\texport THREADS\n+\ttest_perf PERF_EXTRA \"index-pack $t threads\" '\n+\t\trm -rf repo.git &&\n+\t\tgit init --bare repo.git &&\n+\t\tGIT_DIR=repo.git GIT_FORCE_THREADS=1 \\\n+\t\tgit index-pack --threads=$THREADS --stdin <$PACK\n+\t'\n+done\n \n test_perf 'index-pack default number of threads' '\n \trm -rf repo.git &&\n-- \n2.28.0.694.g07780f7063\n\n"},{"id":"404172","messageId":"20200821175800.GC3263141@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200821175153.GA3263018@coredump.intra.peff.net","subject":"[PATCH 3/3] index-pack: adjust default threading cap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:58:00Z","receivedAt":"2020-08-21T17:58:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b8a2486f15 (index-pack: support multithreaded delta resolving,\n2012-05-06) describes an experiment that shows that setting the number\nof threads for index-pack higher than 3 does not help.\n\nI repeated that experiment using a more modern version of Git and a more\nmodern CPU and got different results.\n\nHere are timings for p5302 against linux.git run on my laptop, a Core\ni9-9880H with 8 cores plus hyperthreading (so online-cpus returns 16):\n\n  5302.3: index-pack 0 threads                   256.28(253.41+2.79)\n  5302.4: index-pack 1 threads                   257.03(254.03+2.91)\n  5302.5: index-pack 2 threads                   149.39(268.34+3.06)\n  5302.6: index-pack 4 threads                   94.96(294.10+3.23)\n  5302.7: index-pack 8 threads                   68.12(339.26+3.89)\n  5302.8: index-pack 16 threads                  70.90(655.03+7.21)\n  5302.9: index-pack default number of threads   116.91(290.05+3.21)\n\nYou can see that wall-clock times continue to improve dramatically up to\nthe number of cores, but bumping beyond that (into hyperthreading\nterritory) does not help (and in fact hurts a little).\n\nHere's the same experiment on a machine with dual Xeon 6230's, totaling\n40 cores (80 with hyperthreading):\n\n  5302.3: index-pack 0 threads                    310.04(302.73+6.90)\n  5302.4: index-pack 1 threads                    310.55(302.68+7.40)\n  5302.5: index-pack 2 threads                    178.17(304.89+8.20)\n  5302.6: index-pack 5 threads                    99.53(315.54+9.56)\n  5302.7: index-pack 10 threads                   72.80(327.37+12.79)\n  5302.8: index-pack 20 threads                   60.68(357.74+21.66)\n  5302.9: index-pack 40 threads                   58.07(454.44+67.96)\n  5302.10: index-pack 80 threads                  59.81(720.45+334.52)\n  5302.11: index-pack default number of threads   134.18(309.32+7.98)\n\nThe results are similar; things stop improving at 40 threads. Curiously,\ngoing from 20 to 40 really doesn't help much, either (and increases CPU\ntime considerably). So that may represent an actual barrier to\nparallelism, where we lose out due to context-switching and loss of\ncache locality, but don't reap the wall-clock benefits due to contention\nof our coarse-grained locks.\n\nSo what's a good default value? It's clear that the current cap of 3 is\ntoo low; our default values are 42% and 57% slower than the best times\non each machine. The results on the 40-core machine imply that 20\nthreads is an actual barrier regardless of the number of cores, so we'll\ntake that as a maximum. We get the best results on these machines at\nhalf of the online-cpus value. That's presumably a result of the\nhyperthreading. That's common on multi-core Intel processors, but not\nnecessarily elsewhere. But if we take it as an assumption, we can\nperform optimally on hyperthreaded machines and still do much better\nthan the status quo on other machines, as long as we never half below\nthe current value of 3.\n\nSo that's what this patch does.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSomebody with an AMD machine with a bunch of cores may want to revisit\nthis. I don't think it makes anything worse, but they may want to not do\nthe halving. I don't know if there's an easy way for us to determine the\ndifference via online_cpus(), though.\n\n builtin/index-pack.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f865666db9..9721bf1ffe 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1798,9 +1798,22 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \n \tif (HAVE_THREADS && !nr_threads) {\n \t\tnr_threads = online_cpus();\n-\t\t/* An experiment showed that more threads does not mean faster */\n-\t\tif (nr_threads > 3)\n-\t\t\tnr_threads = 3;\n+\t\t/*\n+\t\t * Experiments show that going above 20 threads doesn't help,\n+\t\t * no matter how many cores you have. Below that, we tend to\n+\t\t * max at half the number of online_cpus(), presumably because\n+\t\t * half of those are hyperthreads rather than full cores. We'll\n+\t\t * never reduce the level below \"3\", though, to match a\n+\t\t * historical value that nobody complained about.\n+\t\t */\n+\t\tif (nr_threads < 4)\n+\t\t\t; /* too few cores to consider capping */\n+\t\telse if (nr_threads < 6)\n+\t\t\tnr_threads = 3; /* historic cap */\n+\t\telse if (nr_threads < 40)\n+\t\t\tnr_threads /= 2;\n+\t\telse\n+\t\t\tnr_threads = 20; /* hard cap */\n \t}\n \n \tcurr_pack = open_pack_file(pack_name);\n-- \n2.28.0.694.g07780f7063\n"},{"id":"404173","messageId":"20200821175845.GA3263218@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200821175451.GB3263141@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] p5302: count up to online-cpus for thread tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T17:58:45Z","receivedAt":"2020-08-21T17:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2020 at 01:54:51PM -0400, Jeff King wrote:\n\n>  t/perf/p5302-pack-index.sh | 47 +++++++++++++++++++-------------------\n>  1 file changed, 24 insertions(+), 23 deletions(-)\n\nI meant to generate this diff with --patience, which is much more\nreadable:\n\ndiff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh\nindex 23011ab739..228593d9ad 100755\n--- a/t/perf/p5302-pack-index.sh\n+++ b/t/perf/p5302-pack-index.sh\n@@ -13,35 +13,36 @@ test_expect_success 'repack' '\n \texport PACK\n '\n \n+# Rather than counting up and doubling each time, count down from the endpoint,\n+# halving each time. That ensures that our final test uses as many threads as\n+# CPUs, even if it isn't a power of 2.\n+test_expect_success 'set up thread-counting tests' '\n+\tt=$(test-tool online-cpus) &&\n+\tthreads= &&\n+\twhile test $t -gt 0\n+\tdo\n+\t\tthreads=\"$t $threads\"\n+\t\tt=$((t / 2))\n+\tdone\n+'\n+\n test_perf PERF_EXTRA 'index-pack 0 threads' '\n \trm -rf repo.git &&\n \tgit init --bare repo.git &&\n \tGIT_DIR=repo.git git index-pack --threads=1 --stdin < $PACK\n '\n \n-test_perf PERF_EXTRA 'index-pack 1 thread ' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git GIT_FORCE_THREADS=1 git index-pack --threads=1 --stdin < $PACK\n-'\n-\n-test_perf PERF_EXTRA 'index-pack 2 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=2 --stdin < $PACK\n-'\n-\n-test_perf PERF_EXTRA 'index-pack 4 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=4 --stdin < $PACK\n-'\n-\n-test_perf PERF_EXTRA 'index-pack 8 threads' '\n-\trm -rf repo.git &&\n-\tgit init --bare repo.git &&\n-\tGIT_DIR=repo.git git index-pack --threads=8 --stdin < $PACK\n-'\n+for t in $threads\n+do\n+\tTHREADS=$t\n+\texport THREADS\n+\ttest_perf PERF_EXTRA \"index-pack $t threads\" '\n+\t\trm -rf repo.git &&\n+\t\tgit init --bare repo.git &&\n+\t\tGIT_DIR=repo.git GIT_FORCE_THREADS=1 \\\n+\t\tgit index-pack --threads=$THREADS --stdin <$PACK\n+\t'\n+done\n \n test_perf 'index-pack default number of threads' '\n \trm -rf repo.git &&\n-- \n2.28.0.694.g07780f7063\n\n"},{"id":"404174","messageId":"CAPig+cRQG6EN7Zq_fYMQOM7y9a6rgwWORZhN=px21-7RorWNdg@mail.gmail.com","threadId":"54086","inReplyTo":"20200821175800.GC3263141@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] index-pack: adjust default threading cap","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-21T18:08:55Z","receivedAt":"2020-08-21T18:09:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 21, 2020 at 1:58 PM Jeff King <peff@peff.net> wrote:\n> So what's a good default value? It's clear that the current cap of 3 is\n> too low; our default values are 42% and 57% slower than the best times\n> on each machine. The results on the 40-core machine imply that 20\n> threads is an actual barrier regardless of the number of cores, so we'll\n> take that as a maximum. We get the best results on these machines at\n> half of the online-cpus value. That's presumably a result of the\n> hyperthreading. That's common on multi-core Intel processors, but not\n> necessarily elsewhere. But if we take it as an assumption, we can\n> perform optimally on hyperthreaded machines and still do much better\n> than the status quo on other machines, as long as we never half below\n> the current value of 3.\n\nI'm not familiar with the index-pack machinery, so this response may\nbe silly, but the first question which came to my mind was whether or\nnot SSD vs. spinning-platter disk impacts these results, and which of\nthe two you were using for the tests (which I don't think was\nmentioned in any of the commit messages). So, basically, I'm wondering\nabout the implication of this change for those of us still stuck with\nold spinning-platter disks.\n"},{"id":"404179","messageId":"20200821184152.GA3263614@coredump.intra.peff.net","threadId":"54086","inReplyTo":"CAPig+cRQG6EN7Zq_fYMQOM7y9a6rgwWORZhN=px21-7RorWNdg@mail.gmail.com","subject":"Re: [PATCH 3/3] index-pack: adjust default threading cap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T18:41:52Z","receivedAt":"2020-08-21T18:41:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2020 at 02:08:55PM -0400, Eric Sunshine wrote:\n\n> On Fri, Aug 21, 2020 at 1:58 PM Jeff King <peff@peff.net> wrote:\n> > So what's a good default value? It's clear that the current cap of 3 is\n> > too low; our default values are 42% and 57% slower than the best times\n> > on each machine. The results on the 40-core machine imply that 20\n> > threads is an actual barrier regardless of the number of cores, so we'll\n> > take that as a maximum. We get the best results on these machines at\n> > half of the online-cpus value. That's presumably a result of the\n> > hyperthreading. That's common on multi-core Intel processors, but not\n> > necessarily elsewhere. But if we take it as an assumption, we can\n> > perform optimally on hyperthreaded machines and still do much better\n> > than the status quo on other machines, as long as we never half below\n> > the current value of 3.\n> \n> I'm not familiar with the index-pack machinery, so this response may\n> be silly, but the first question which came to my mind was whether or\n> not SSD vs. spinning-platter disk impacts these results, and which of\n> the two you were using for the tests (which I don't think was\n> mentioned in any of the commit messages). So, basically, I'm wondering\n> about the implication of this change for those of us still stuck with\n> old spinning-platter disks.\n\nThey were both SSD machines, but it wouldn't matter for these tests\nbecause they easily fit the whole pack into memory anyway.\n\nBut in the general case, I don't think disk performance would be\nrelevant. Delta resolution is very CPU-bound, because it's\nde-compressing data and then computing its SHA-1. So linux.git, for\ninstance, is looking at ~1.3GB on disk that expands to 87.5GB of bytes\nto run through SHA-1.\n\nAnd it would be pretty unlikely to hit the disk anyway, as the thing we\nprimarily index is incoming packs which we've literally just written. So\nI'd expect them to be in cache.\n\nOf course, if you can get different numbers from p5302, I'd be curious\nto hear them. :)\n\nA more plausible downside might be that memory usage would increase as\nwe operate on multiple deltas at once. But pack-objects is already much\nmore hungry here, as it runs online_cpus() delta-compression threads\nsimultaneously, each of which may have up to window_size entries in\nmemory at once.\n\n-Peff\n"},{"id":"404181","messageId":"20200821184459.GB3263614@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200821175153.GA3263018@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] index-pack threading defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T18:44:59Z","receivedAt":"2020-08-21T18:45:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2020 at 01:51:53PM -0400, Jeff King wrote:\n\n> That value was determined experimentally in 2012. I'm not sure of the\n> exact reason it's different now (modern processors are better at\n> parallelism, or modern git is better at parallelism, or the original\n> experiment was just a fluke). But regardless, I can get on the order of\n> a two-to-one speedup by bumping the thread count. See the final patch\n> for timings and more specific discussion.\n\nAfter writing a response elsewhere in the thread, it occurred to me that\na good candidate for explaining this may be that our modern sha1dc\nimplementation is way slower than what we were using in 2012 (which\nwould have been either block-sha1, or the even-faster openssl\nimplementation). And since a good chunk of index-pack's time is going to\ncomputing sha1 hashes on the resulting objects, that means that since\n2012, we're spending relatively more time in the hash computation (which\nparallelizes per-object) and less time in the other parts that happen\nunder a lock.\n\n-Peff\n"},{"id":"404191","messageId":"xmqqmu2n964x.fsf@gitster.c.googlers.com","threadId":"54086","inReplyTo":"20200821184459.GB3263614@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] index-pack threading defaults","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T18:59:58Z","receivedAt":"2020-08-21T19:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Aug 21, 2020 at 01:51:53PM -0400, Jeff King wrote:\n>\n>> That value was determined experimentally in 2012. I'm not sure of the\n>> exact reason it's different now (modern processors are better at\n>> parallelism, or modern git is better at parallelism, or the original\n>> experiment was just a fluke). But regardless, I can get on the order of\n>> a two-to-one speedup by bumping the thread count. See the final patch\n>> for timings and more specific discussion.\n>\n> After writing a response elsewhere in the thread, it occurred to me that\n> a good candidate for explaining this may be that our modern sha1dc\n> implementation is way slower than what we were using in 2012 (which\n> would have been either block-sha1, or the even-faster openssl\n> implementation). And since a good chunk of index-pack's time is going to\n> computing sha1 hashes on the resulting objects, that means that since\n> 2012, we're spending relatively more time in the hash computation (which\n> parallelizes per-object) and less time in the other parts that happen\n> under a lock.\n\nBelievable conjecture that is.  You could benchmark again with\nblock-sha1 on today's hardware, but because the performance profile\nwith sha1dc is what matters in the real world anyway...\n\nThanks.\n"},{"id":"404193","messageId":"20200821191416.GB1165@coredump.intra.peff.net","threadId":"54086","inReplyTo":"xmqqmu2n964x.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/3] index-pack threading defaults","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-21T19:14:16Z","receivedAt":"2020-08-21T19:14:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2020 at 11:59:58AM -0700, Junio C Hamano wrote:\n\n> > After writing a response elsewhere in the thread, it occurred to me that\n> > a good candidate for explaining this may be that our modern sha1dc\n> > implementation is way slower than what we were using in 2012 (which\n> > would have been either block-sha1, or the even-faster openssl\n> > implementation). And since a good chunk of index-pack's time is going to\n> > computing sha1 hashes on the resulting objects, that means that since\n> > 2012, we're spending relatively more time in the hash computation (which\n> > parallelizes per-object) and less time in the other parts that happen\n> > under a lock.\n> \n> Believable conjecture that is.  You could benchmark again with\n> block-sha1 on today's hardware, but because the performance profile\n> with sha1dc is what matters in the real world anyway...\n\nYeah, I agree on the \"real world\" part, but I'm the curious sort, so\nhere are numbers compiled against openssl (which is generally even\nfaster than block-sha1, and would thus emphasize the results of our\nhypothesis):\n\n  5302.3: index-pack 0 threads                   108.78(106.39+2.31)\n  5302.4: index-pack 1 threads                   110.65(108.08+2.49)\n  5302.5: index-pack 2 threads                   67.57(110.83+2.75) \n  5302.6: index-pack 4 threads                   48.18(123.82+3.02) \n  5302.7: index-pack 8 threads                   39.07(153.45+4.13) \n  5302.8: index-pack 16 threads                  38.38(265.78+7.71) \n  5302.9: index-pack default number of threads   54.64(117.35+2.73)\n\nSo it's actually pretty similar. Things continue getting faster as we go\npast 3 threads. Though our total improvement is less (29% better with 8\nthreads compared to 3, versus 42% better when using sha1dc). So I think\nit's _part_ of the reason, but not all of it.\n\n-Peff\n"},{"id":"404246","messageId":"20200822011607.GX8085@camp.crustytoothpaste.net","threadId":"54086","inReplyTo":"20200821175800.GC3263141@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] index-pack: adjust default threading cap","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-08-22T01:16:07Z","receivedAt":"2020-08-22T01:16:50Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-08-21 at 17:58:00, Jeff King wrote:\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index f865666db9..9721bf1ffe 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1798,9 +1798,22 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (HAVE_THREADS && !nr_threads) {\n>  \t\tnr_threads = online_cpus();\n> -\t\t/* An experiment showed that more threads does not mean faster */\n> -\t\tif (nr_threads > 3)\n> -\t\t\tnr_threads = 3;\n> +\t\t/*\n> +\t\t * Experiments show that going above 20 threads doesn't help,\n> +\t\t * no matter how many cores you have. Below that, we tend to\n> +\t\t * max at half the number of online_cpus(), presumably because\n> +\t\t * half of those are hyperthreads rather than full cores. We'll\n> +\t\t * never reduce the level below \"3\", though, to match a\n> +\t\t * historical value that nobody complained about.\n> +\t\t */\n> +\t\tif (nr_threads < 4)\n> +\t\t\t; /* too few cores to consider capping */\n> +\t\telse if (nr_threads < 6)\n> +\t\t\tnr_threads = 3; /* historic cap */\n> +\t\telse if (nr_threads < 40)\n> +\t\t\tnr_threads /= 2;\n\nI was going to ask if we could make the halving conditional on x86_64,\nbut it turns out POWER and UltraSPARC also have SMT, so that doesn't\nmake sense.  I expect that most users who have more than 6 \"cores\" are\ngoing to be on one of those systems or possibly ARM64, and since the\nperformance penalty of using half as many cores isn't that significant,\nI'm not sure it's worth worrying about further.  This will be an\nimprovement regardless.\n\nWhich is just a long way of saying, this patch seems fine to me.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"404331","messageId":"20200824173735.GA673908@coredump.intra.peff.net","threadId":"54086","inReplyTo":"20200822011607.GX8085@camp.crustytoothpaste.net","subject":"Re: [PATCH 3/3] index-pack: adjust default threading cap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-24T17:37:35Z","receivedAt":"2020-08-24T17:37:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 22, 2020 at 01:16:07AM +0000, brian m. carlson wrote:\n\n> > +\t\tif (nr_threads < 4)\n> > +\t\t\t; /* too few cores to consider capping */\n> > +\t\telse if (nr_threads < 6)\n> > +\t\t\tnr_threads = 3; /* historic cap */\n> > +\t\telse if (nr_threads < 40)\n> > +\t\t\tnr_threads /= 2;\n> \n> I was going to ask if we could make the halving conditional on x86_64,\n> but it turns out POWER and UltraSPARC also have SMT, so that doesn't\n> make sense.  I expect that most users who have more than 6 \"cores\" are\n> going to be on one of those systems or possibly ARM64, and since the\n> performance penalty of using half as many cores isn't that significant,\n> I'm not sure it's worth worrying about further.  This will be an\n> improvement regardless.\n> \n> Which is just a long way of saying, this patch seems fine to me.\n\nOK, good. :) I agree there may be room for more improvement on those\nsystems. But lacking access to any, my goal was to make things better on\nsystems I _could_ test on, and not make things worse on other systems.\nSo I'd be very happy if people on other platforms (especially non-intel\nones) wanted to run:\n\n  cd t/perf\n  GIT_PERF_EXTRA=1 \\\n  GIT_PERF_LARGE_REPO=/path/to/clone/of/linux.git \\\n  ./p5302-pack-index.sh\n\nand report the results.\n\nI do have a slightly-old AMD machine with 4 cores (an A8-7600). Here's\nwhat it says:\n\n  5302.3: index-pack 0 threads                   447.67(436.62+6.57)\n  5302.4: index-pack 1 threads                   450.80(441.26+7.20)\n  5302.5: index-pack 2 threads                   265.62(459.56+7.30)\n  5302.6: index-pack 4 threads                   177.06(477.56+8.22)\n  5302.7: index-pack default number of threads   202.60(473.15+7.61)\n\nSo it does get better with 4 threads (but we continue to cap it at 3).\nI wonder whether we should just do:\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9721bf1ffe..d7453d0c09 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1809,7 +1809,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tif (nr_threads < 4)\n \t\t\t; /* too few cores to consider capping */\n \t\telse if (nr_threads < 6)\n-\t\t\tnr_threads = 3; /* historic cap */\n+\t\t\tnr_threads = nr_threads;\n \t\telse if (nr_threads < 40)\n \t\t\tnr_threads /= 2;\n \t\telse\n\nThat does probably make things slightly worse for a 6-core hyperthreaded\nIntel machine. And it doesn't help an actual 8-core AMD machine at all.\n\n-Peff\n"},{"id":"404346","messageId":"20200824175547.GA9245@flurp.local","threadId":"54086","inReplyTo":"20200824173735.GA673908@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] index-pack: adjust default threading cap","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-24T17:55:47Z","receivedAt":"2020-08-24T17:55:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 24, 2020 at 01:37:35PM -0400, Jeff King wrote:\n> So I'd be very happy if people on other platforms (especially non-intel\n> ones) wanted to run:\n> \n>   cd t/perf\n>   GIT_PERF_EXTRA=1 \\\n>   GIT_PERF_LARGE_REPO=/path/to/clone/of/linux.git \\\n>   ./p5302-pack-index.sh\n> \n> and report the results.\n> \n> I do have a slightly-old AMD machine with 4 cores (an A8-7600). Here's\n> what it says:\n> \n>   5302.3: index-pack 0 threads                   447.67(436.62+6.57)\n>   5302.4: index-pack 1 threads                   450.80(441.26+7.20)\n>   5302.5: index-pack 2 threads                   265.62(459.56+7.30)\n>   5302.6: index-pack 4 threads                   177.06(477.56+8.22)\n>   5302.7: index-pack default number of threads   202.60(473.15+7.61)\n\nI tested on my 9.5 year old 4-core iMac with 2.5 GHz Intel Core i5,\nspinning platter hard disk, and 12 GB memory (recently upgraded from 4\nGB). I used git.git rather than linux.git because I didn't want to\nwait several days for the results (plus this is my primary machine\nwhich I'm actively using). Results were similar to what you saw:\n\n  5302.3: index-pack 0 threads                   14.85(14.10+0.66)\n  5302.4: index-pack 1 threads                   14.65(13.83+0.63)\n  5302.5: index-pack 2 threads                   9.11(14.40+0.80) \n  5302.6: index-pack 4 threads                   6.89(17.03+1.32) \n  5302.7: index-pack default number of threads   7.72(16.15+1.15) \n\nThanks for providing exact instructions for running the test; I had\nnever done it before, thus didn't know how.\n"}]}