{"thread":{"id":"45528","subject":"[PATCH v3 1/2] read-cache: core.checksumindex","startedAt":"2017-03-28T19:08:47Z","lastAt":"2017-03-31T13:24:41Z","messageCount":13,"participants":["git@jeffhostetler.com","Jeff King","Jeff Hostetler","Junio C Hamano","Thomas Gummerer"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"315589","messageId":"20170328190732.59486-2-git@jeffhostetler.com","threadId":"45528","inReplyTo":"20170328190732.59486-1-git@jeffhostetler.com","subject":"[PATCH v3 1/2] read-cache: core.checksumindex","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-28T19:07:31Z","receivedAt":"2017-03-28T19:08:47Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nTeach git to skip verification of the index SHA in verify_hdr()\nin read_index().\n\nThis is a performance optimization. The index file SHA verification\ncan be considered an ancient relic from the early days of git and only\nuseful for detecting disk corruption. For small repositories, this\nSHA calculation is not that significant, but for gigantic repositories\nthis calculation adds significant time to every command.\n\nAdded \"core.checksumindex\" to enable/disable the SHA verification.\n\nSigned-off-by: Jeff Hostetler \n---\n read-cache.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 9054369..2ab4b74 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1376,12 +1376,24 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \tgit_SHA_CTX c;\n \tunsigned char sha1[20];\n \tint hdr_version;\n+\tint do_checksum = 0;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(\"bad signature\");\n \thdr_version = ntohl(hdr->hdr_version);\n \tif (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)\n \t\treturn error(\"bad index version %d\", hdr_version);\n+\n+\t/*\n+\t * Since we run very early in command startup, git_config()\n+\t * may not have been called yet and the various \"core_*\"\n+\t * global variables haven't been set. So look it up\n+\t * explicitly.\n+\t */\n+\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n+\tif (!do_checksum)\n+\t\treturn 0;\n+\n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, hdr, size - 20);\n \tgit_SHA1_Final(sha1, &c);\n-- \n2.9.3\n\n"},{"id":"315590","messageId":"20170328190732.59486-3-git@jeffhostetler.com","threadId":"45528","inReplyTo":"20170328190732.59486-1-git@jeffhostetler.com","subject":"[PATCH v3 2/2] test-core-checksum-index: core.checksumindex test helper","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-28T19:07:32Z","receivedAt":"2017-03-28T19:08:48Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreated test helper to measure read_index() with and without\ncore.checksumindex set and report performance.\n\nSigned-off-by: Jeff Hostetler \n---\n Makefile | 1 +\n t/helper/.gitignore | 1 +\n t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 79 insertions(+)\n create mode 100644 t/helper/test-core-checksum-index.c\n\ndiff --git a/Makefile b/Makefile\nindex 9ec6065..6049427 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -607,6 +607,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n TEST_PROGRAMS_NEED_X += test-config\n+TEST_PROGRAMS_NEED_X += test-core-checksum-index\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/t/helper/.gitignore b/t/helper/.gitignore\nindex d6e8b36..f651a6b 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -1,6 +1,7 @@\n /test-chmtime\n /test-ctype\n /test-config\n+/test-core-checksum-index\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/t/helper/test-core-checksum-index.c b/t/helper/test-core-checksum-index.c\nnew file mode 100644\nindex 0000000..0452dc2\n--- /dev/null\n+++ b/t/helper/test-core-checksum-index.c\n@@ -0,0 +1,77 @@\n+#include \"cache.h\"\n+#include \"parse-options.h\"\n+\n+uint64_t time_runs(int do_checksum, int count)\n+{\n+\tuint64_t t0, t1;\n+\tuint64_t sum = 0;\n+\tuint64_t avg;\n+\tint i;\n+\n+\tgit_config_set_gently(\"core.checksumindex\",\n+\t\t(do_checksum ? \"true\" : \"false\"));\n+\n+\tfor (i = 0; i < count; i++) {\n+\t\tt0 = getnanotime();\n+\t\tread_cache();\n+\t\tt1 = getnanotime();\n+\n+\t\tsum += (t1 - t0);\n+\n+\t\tprintf(\"%f %d [cache_nr %d]\\n\",\n+\t\t\t ((double)(t1 - t0))/1000000000,\n+\t\t\t do_checksum,\n+\t\t\t the_index.cache_nr);\n+\t\tfflush(stdout);\n+\n+\t\tdiscard_cache();\n+\t}\n+\n+\tavg = sum / count;\n+\tif (count > 1) {\n+\t\tprintf(\"%f %d avg\\n\",\n+\t\t\t (double)avg/1000000000,\n+\t\t\t do_checksum);\n+\t\tfflush(stdout);\n+\t}\n+\n+\treturn avg;\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tint original_do_checksum;\n+\tint count = 1;\n+\tconst char *usage[] = {\n+\t\t\"test-core-checksum-index\",\n+\t\tNULL\n+\t};\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER('c', \"count\", &count, \"number of passes\"),\n+\t\tOPT_END(),\n+\t};\n+\tconst char *prefix;\n+\tuint64_t avg_0, avg_1;\n+\n+\tprefix = setup_git_directory();\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (count < 1)\n+\t\tdie(\"count must greater than zero\");\n+\n+\t/* throw away call to get the index in the disk cache */\n+\tread_cache();\n+\tdiscard_cache();\n+\n+\tgit_config_get_bool(\"core.checksumindex\", &original_do_checksum);\n+\n+\tavg_1 = time_runs(1, count);\n+\tavg_0 = time_runs(0, count);\n+\n+\tgit_config_set_gently(\"core.checksumindex\",\n+\t\t((original_do_checksum) ? \"true\" : \"false\"));\n+\n+\tif (avg_0 >= avg_1)\n+\t\tdie(\"skipping index checksum verification did not help\");\n+\treturn 0;\n+}\n-- \n2.9.3\n\n"},{"id":"315591","messageId":"20170328190732.59486-1-git@jeffhostetler.com","threadId":"45528","inReplyTo":null,"subject":"[PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-28T19:07:30Z","receivedAt":"2017-03-28T19:08:49Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nVersion 3 of this patch series simplifies this effort to just turn\non/off the hash verification using a \"core.checksumindex\" config variable.\n\nI've preserved the original checksum validation code so that we can\nforce it on in fsck if desired.\n\nIt eliminates the original threading model completely.\n\nJeff Hostetler (2):\n read-cache: core.checksumindex\n test-core-checksum-index: core.checksumindex test helper\n\n Makefile | 1 +\n read-cache.c | 12 ++++++\n t/helper/.gitignore | 1 +\n t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 91 insertions(+)\n create mode 100644 t/helper/test-core-checksum-index.c\n\n-- \n2.9.3\n\n"},{"id":"315592","messageId":"20170328191628.dprziuhpv7khvocu@sigill.intra.peff.net","threadId":"45528","inReplyTo":"20170328190732.59486-1-git@jeffhostetler.com","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:16:28Z","receivedAt":"2017-03-28T19:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2017 at 07:07:30PM +0000, git@jeffhostetler.com wrote:\n\n> From: Jeff Hostetler \n> \n> Version 3 of this patch series simplifies this effort to just turn\n> on/off the hash verification using a \"core.checksumindex\" config variable.\n> \n> I've preserved the original checksum validation code so that we can\n> force it on in fsck if desired.\n> \n> It eliminates the original threading model completely.\n> \n> Jeff Hostetler (2):\n> read-cache: core.checksumindex\n> test-core-checksum-index: core.checksumindex test helper\n> \n> Makefile | 1 +\n> read-cache.c | 12 ++++++\n> t/helper/.gitignore | 1 +\n> t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++\n\nDo we still need test-core-checksum-index? Can we just time ls-files or\nsomething in t/perf?\n\n-Peff\n"},{"id":"315615","messageId":"35f220df-aa63-b80f-8970-429850202cdd@jeffhostetler.com","threadId":"45528","inReplyTo":"20170328191628.dprziuhpv7khvocu@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-28T19:50:34Z","receivedAt":"2017-03-28T19:51:47Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/28/2017 3:16 PM, Jeff King wrote:\n> On Tue, Mar 28, 2017 at 07:07:30PM +0000, git@jeffhostetler.com wrote:\n>\n>> From: Jeff Hostetler \n>>\n>> Version 3 of this patch series simplifies this effort to just turn\n>> on/off the hash verification using a \"core.checksumindex\" config variable.\n>>\n>> I've preserved the original checksum validation code so that we can\n>> force it on in fsck if desired.\n>>\n>> It eliminates the original threading model completely.\n>>\n>> Jeff Hostetler (2):\n>> read-cache: core.checksumindex\n>> test-core-checksum-index: core.checksumindex test helper\n>>\n>> Makefile | 1 +\n>> read-cache.c | 12 ++++++\n>> t/helper/.gitignore | 1 +\n>> t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++\n>\n> Do we still need test-core-checksum-index? Can we just time ls-files or\n> something in t/perf?\n\nIt was a convenient way to isolate, average, and compare\nread_index() times, but I suppose we could do something\nlike that.\n\nI did confirm that a ls-files does show a slight 0.008\nsecond difference on the 58K file Linux tree when toggled\non or off.\n\nBut I'm tempted to suggest that we just omit my helper exe\nand not worry about a test -- since we don't have any test\nrepos large enough to really demonstrate the differences.\nMy concern is that that 0.008 would be lost in the noise\nof the rest of the test and make for an unreliable result.\n\nJeff\n"},{"id":"315616","messageId":"20170328195605.xy4pnhy74s6wgwps@sigill.intra.peff.net","threadId":"45528","inReplyTo":"35f220df-aa63-b80f-8970-429850202cdd@jeffhostetler.com","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:56:06Z","receivedAt":"2017-03-28T19:56:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2017 at 03:50:34PM -0400, Jeff Hostetler wrote:\n\n> It was a convenient way to isolate, average, and compare\n> read_index() times, but I suppose we could do something\n> like that.\n> \n> I did confirm that a ls-files does show a slight 0.008\n> second difference on the 58K file Linux tree when toggled\n> on or off.\n\nYeah, I agree it helps isolate the change. I'm just not sure we want to\ncarry a bunch of function-specific perf-testing code. And one of the\nnice things about testing a real command is that it's...a real command.\nSo it's an actual improvement a user might see.\n\n> But I'm tempted to suggest that we just omit my helper exe\n> and not worry about a test -- since we don't have any test\n> repos large enough to really demonstrate the differences.\n> My concern is that that 0.008 would be lost in the noise\n> of the rest of the test and make for an unreliable result.\n\nYeah, I think that would be fine. You _could_ write a t/perf test and\nthen use your 400MB monstrosity as GIT_PERF_LARGE_REPO. But given that\nmost people don't have such a thing, there's not much value over you\njust showing off the perf improvement in the commit message.\n\nWe could also have a t/perf test that generates a monstrous index and\nshows that it's faster. But frankly, I don't think this is all that\ninteresting as a performance regression test. It's not like there's\nsomething subtle about the performance improvement; we stopped computing\nthe SHA-1, and (gasp!) it takes exactly one SHA-1 computation's less\ntime.\n\nSo just mentioning the test case and the improvement in the commit\nmessage is sufficient, IMHO.\n\n-Peff\n"},{"id":"315812","messageId":"xmqqtw6a35qc.fsf@gitster.mtv.corp.google.com","threadId":"45528","inReplyTo":"20170328195605.xy4pnhy74s6wgwps@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T19:49:15Z","receivedAt":"2017-03-30T19:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So just mentioning the test case and the improvement in the commit\n> message is sufficient, IMHO.\n\nSo here is how I butchered [v3 1/2] to tentatively queue it on 'pu'.\n\nNotable suggested changes I have in this one are:\n\n * I stole the numbers from the cover letter of v2 and added them at\n the end of the log message.\n\n * As the checksum is not a useless relic, but is an integrity\n check, I dropped the \"ancient relic\" from the proposed log\n message. It is just that the modern disks are reliable enough to\n make it worthwhile to think about a trade-off this patch makes\n between performance and integrity.\n\n * As it is customary, the configuration variable starts as an opt\n in feature. In a few releases, perhaps we can flip the default,\n but we do not do so from day one.\n\n * Updated the code around the call to config-get-bool to avoid\n asking the same question twice.\n\n * Added minimum documentation.\n\nBy the way, are we sure we have something that validates the\nchecksum regardless of the configuration setting? If not, we may\nwant to tweak this further so that we can force the validation from\n\"git fsck\" or something. I am not going to do that myself, but it\nmay be necessary before this graduates to 'master'.\n\nThanks.\n\n-- >8 --\nFrom: Jeff Hostetler \nDate: Tue, 28 Mar 2017 19:07:31 +0000\nSubject: [PATCH] read-cache: core.checksumindex\n\nTeach git to skip verification of the SHA-1 checksum at the end of\nthe index file in verify_hdr() called from read_index() when the\ncore.checksumIndex configuration variable is set to false.\n\nThe checksum verification is for detecting disk corruption, and for\nsmall projects, the time it takes to compute SHA-1 is not that\nsignificant, but for gigantic repositories this calculation adds\nsignificant time to every command.\n\nOn the Linux kernel repository, the effect is rather trivial.\nThe time to reading its index with 58k entries drops from 0.0284 sec\ndown to 0.0155 sec.\n\nOn my Windows source tree (450MB index), I'm seeing a savings of 0.6\nseconds -- read_index() went from 1.2 to 0.6 seconds.\n\nSigned-off-by: Jeff Hostetler \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 8 ++++++++\n read-cache.c | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1df1965457..bc7b216d43 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -329,6 +329,14 @@ advice.*::\n \t\tshow directions on how to proceed from the current state.\n --\n \n+core.checksumIndex::\n+\tTell Git to validate the checksum at the end of the index\n+\tfile to detect corruption. Defaults to `true`. Those who\n+\twork on a project with too many files may want to set this\n+\tvariable to `false` to make it faster to load the index (in\n+\texchange for reliability, but in general modern disks are\n+\treliable enough for most people).\n+\n core.fileMode::\n \tTells Git if the executable bit of files in the working tree\n \tis to be honored.\ndiff --git a/read-cache.c b/read-cache.c\nindex e447751823..3195702cf7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1376,12 +1376,28 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \tgit_SHA_CTX c;\n \tunsigned char sha1[20];\n \tint hdr_version;\n+\tstatic int do_checksum = -1;\n \n \tif (hdr->hdr_signature != htonl(CACHE_SIGNATURE))\n \t\treturn error(\"bad signature\");\n \thdr_version = ntohl(hdr->hdr_version);\n \tif (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)\n \t\treturn error(\"bad index version %d\", hdr_version);\n+\n+\tif (do_checksum < 0) {\n+\t\t/*\n+\t\t * Since we run very early in command startup, git_config()\n+\t\t * may not have been called yet and the various \"core_*\"\n+\t\t * global variables haven't been set. So look it up\n+\t\t * explicitly.\n+\t\t */\n+\t\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n+\t\tif (do_checksum < 0)\n+\t\t\tdo_checksum = 0; /* default to false */\n+\t}\n+\tif (!do_checksum)\n+\t\treturn 0;\n+\n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, hdr, size - 20);\n \tgit_SHA1_Final(sha1, &c);\n-- \n2.12.2-727-gf32eb5229d\n\n"},{"id":"315815","messageId":"20170330195820.kexvl44x7ncthlcx@sigill.intra.peff.net","threadId":"45528","inReplyTo":"xmqqtw6a35qc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-30T19:58:20Z","receivedAt":"2017-03-30T19:58:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2017 at 12:49:15PM -0700, Junio C Hamano wrote:\n\n> Notable suggested changes I have in this one are:\n> \n> * I stole the numbers from the cover letter of v2 and added them at\n> the end of the log message.\n\nYeah, good.\n\n> * As the checksum is not a useless relic, but is an integrity\n> check, I dropped the \"ancient relic\" from the proposed log\n> message. It is just that the modern disks are reliable enough to\n> make it worthwhile to think about a trade-off this patch makes\n> between performance and integrity.\n\nYeah, I'd agree this is more of a tradeoff than a cleanup.\n\n> * As it is customary, the configuration variable starts as an opt\n> in feature. In a few releases, perhaps we can flip the default,\n> but we do not do so from day one.\n\nI think this is so user-invisible that it doesn't really matter much,\nbut I'm OK with taking it slow.\n\n> * Updated the code around the call to config-get-bool to avoid\n> asking the same question twice.\n\nA few comments on that below.\n\n> * Added minimum documentation.\n\nYep, looks good.\n\n> By the way, are we sure we have something that validates the\n> checksum regardless of the configuration setting? If not, we may\n> want to tweak this further so that we can force the validation from\n> \"git fsck\" or something. I am not going to do that myself, but it\n> may be necessary before this graduates to 'master'.\n\nYes, I'd agree this shouldn't graduate without the matching change to\nteach fsck to flip the switch back.\n\n> +\tstatic int do_checksum = -1;\n> [...]\n> +\tif (do_checksum < 0) {\n> +\t\t/*\n> +\t\t * Since we run very early in command startup, git_config()\n> +\t\t * may not have been called yet and the various \"core_*\"\n> +\t\t * global variables haven't been set. So look it up\n> +\t\t * explicitly.\n> +\t\t */\n> +\t\tgit_config_get_bool(\"core.checksumindex\", &do_checksum);\n> +\t\tif (do_checksum < 0)\n> +\t\t\tdo_checksum = 0; /* default to false */\n> +\t}\n> +\tif (!do_checksum)\n> +\t\treturn 0;\n\nThis is basically introducing a new caching layer, but there's no way to\ninvalidate it (if, say, we looked at the config once and then we knew\nthat the config changed). I think it's probably OK, because the main\nreason for the config to change is reading it before/after repository\nsetup. But since this is index code, it shouldn't be called at all\nbefore repository setup.\n\nStill, I'm not sure the extra layer of cache is all that valuable. It\nshould be a single hash lookup in the config cache (in an operation that\notherwise reads the entire index).\n\n-Peff\n"},{"id":"315818","messageId":"20170330200648.GH27158@hank","threadId":"45528","inReplyTo":"20170328195605.xy4pnhy74s6wgwps@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-30T20:06:48Z","receivedAt":"2017-03-30T20:06:44Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/28, Jeff King wrote:\n> On Tue, Mar 28, 2017 at 03:50:34PM -0400, Jeff Hostetler wrote:\n> \n> > It was a convenient way to isolate, average, and compare\n> > read_index() times, but I suppose we could do something\n> > like that.\n> > \n> > I did confirm that a ls-files does show a slight 0.008\n> > second difference on the 58K file Linux tree when toggled\n> > on or off.\n> \n> Yeah, I agree it helps isolate the change. I'm just not sure we want to\n> carry a bunch of function-specific perf-testing code. And one of the\n> nice things about testing a real command is that it's...a real command.\n> So it's an actual improvement a user might see.\n> \n> > But I'm tempted to suggest that we just omit my helper exe\n> > and not worry about a test -- since we don't have any test\n> > repos large enough to really demonstrate the differences.\n> > My concern is that that 0.008 would be lost in the noise\n> > of the rest of the test and make for an unreliable result.\n> \n> Yeah, I think that would be fine. You _could_ write a t/perf test and\n> then use your 400MB monstrosity as GIT_PERF_LARGE_REPO. But given that\n> most people don't have such a thing, there's not much value over you\n> just showing off the perf improvement in the commit message.\n\nSorry if this was already discussed, but we already do have a perf\ntest for the index (p0002), and a corresponding helper program which\njust does read_cache() and discard_cache(). Maybe we could re-use\nthat and add a second test running the same using the new config?\n\n> We could also have a t/perf test that generates a monstrous index and\n> shows that it's faster. But frankly, I don't think this is all that\n> interesting as a performance regression test. It's not like there's\n> something subtle about the performance improvement; we stopped computing\n> the SHA-1, and (gasp!) it takes exactly one SHA-1 computation's less\n> time.\n> \n> So just mentioning the test case and the improvement in the commit\n> message is sufficient, IMHO.\n> \n> -Peff\n"},{"id":"315825","messageId":"20170330203916.ckp62syupioo3hxc@sigill.intra.peff.net","threadId":"45528","inReplyTo":"20170330200648.GH27158@hank","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-30T20:39:16Z","receivedAt":"2017-03-30T20:39:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2017 at 09:06:48PM +0100, Thomas Gummerer wrote:\n\n> > Yeah, I think that would be fine. You _could_ write a t/perf test and\n> > then use your 400MB monstrosity as GIT_PERF_LARGE_REPO. But given that\n> > most people don't have such a thing, there's not much value over you\n> > just showing off the perf improvement in the commit message.\n> \n> Sorry if this was already discussed, but we already do have a perf\n> test for the index (p0002), and a corresponding helper program which\n> just does read_cache() and discard_cache(). Maybe we could re-use\n> that and add a second test running the same using the new config?\n\nOh, indeed. Yes, I would think the results of p0002 would probably show\noff Jeff's improvements.\n\n-Peff\n"},{"id":"315828","messageId":"xmqqa882336t.fsf@gitster.mtv.corp.google.com","threadId":"45528","inReplyTo":"20170330195820.kexvl44x7ncthlcx@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T20:44:10Z","receivedAt":"2017-03-30T20:44:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Still, I'm not sure the extra layer of cache is all that valuable. It\n> should be a single hash lookup in the config cache (in an operation that\n> otherwise reads the entire index).\n\nOK, let's drop that part, then.\n"},{"id":"315884","messageId":"094f16d2-e1d9-1478-df6a-4f24f17e184e@jeffhostetler.com","threadId":"45528","inReplyTo":"20170330203916.ckp62syupioo3hxc@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-31T13:23:39Z","receivedAt":"2017-03-31T13:24:07Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/30/2017 4:39 PM, Jeff King wrote:\n> On Thu, Mar 30, 2017 at 09:06:48PM +0100, Thomas Gummerer wrote:\n>\n>>> Yeah, I think that would be fine. You _could_ write a t/perf test and\n>>> then use your 400MB monstrosity as GIT_PERF_LARGE_REPO. But given that\n>>> most people don't have such a thing, there's not much value over you\n>>> just showing off the perf improvement in the commit message.\n>>\n>> Sorry if this was already discussed, but we already do have a perf\n>> test for the index (p0002), and a corresponding helper program which\n>> just does read_cache() and discard_cache(). Maybe we could re-use\n>> that and add a second test running the same using the new config?\n>\n> Oh, indeed. Yes, I would think the results of p0002 would probably show\n> off Jeff's improvements.\n>\n> -Peff\n>\n\nLet me re-roll it with Junio's cleanup, update fsck to force it on,\nand look at using p0002.\n\nThanks,\nJeff\n"},{"id":"315886","messageId":"cf8baa5b-34ca-658e-0de5-03b69ab86521@jeffhostetler.com","threadId":"45528","inReplyTo":"xmqqa882336t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/2] read-cache: call verify_hdr() in a background thread","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-31T13:20:38Z","receivedAt":"2017-03-31T13:24:41Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/30/2017 4:44 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Still, I'm not sure the extra layer of cache is all that valuable. It\n>> should be a single hash lookup in the config cache (in an operation that\n>> otherwise reads the entire index).\n>\n> OK, let's drop that part, then.\n>\n\nYes, let's omit the caching there. That way perf tests can run it\nmultiple times with it on or off as they see fit. And in the normal\ncase it will only be executed once anyway.\n"}]}