{"thread":{"id":"45493","subject":"[PATCH] read-cache: call verify_hdr() in a background thread","startedAt":"2017-03-24T13:28:19Z","lastAt":"2017-03-24T17:53:25Z","messageCount":7,"participants":["git@jeffhostetler.com","Jeff King","Jeff Hostetler","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315247","messageId":"1490362071-46932-1-git-send-email-git@jeffhostetler.com","threadId":"45493","inReplyTo":null,"subject":"[PATCH] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-24T13:27:50Z","receivedAt":"2017-03-24T13:28:19Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\n\nThis patch contains a performance optimization to run\nverify_hdr() in a background thread while the foreground\nthread parses the index.  This allows do_read_index() to\ncomplete faster.\n\nThis idea was recently discussed on the mailing list in:\nhttps://public-inbox.org/git/85221b97-759f-b7a9-1256-21515d163cbf@jeffhostetler.com/\nduring a discussion on sha1dc.\n\nOur testing on Windows showed that verifying the SHA1 hash\non the index file takes approximately the same amount of time\nas parsing the file and building the array of cache_entries.\n(It could also be that having 2 threads better ammortizes the\npage faults of reading from the mmap'd file.)\n\nAn earlier version of this change has been in use in GfW since December:\nhttps://github.com/git-for-windows/git/pull/978\n\n\nJeff Hostetler (1):\n  read-cache: call verify_hdr() in a background thread\n\n read-cache.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 86 insertions(+), 1 deletion(-)\n\n-- \n2.7.4\n\n"},{"id":"315248","messageId":"1490362071-46932-2-git-send-email-git@jeffhostetler.com","threadId":"45493","inReplyTo":"1490362071-46932-1-git-send-email-git@jeffhostetler.com","subject":"[PATCH] read-cache: call verify_hdr() in a background thread","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-24T13:27:51Z","receivedAt":"2017-03-24T13:28:22Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhost@microsoft.com>\n\nTeash do_read_index() in read-cache.c to call verify_hdr()\nin a background thread while the forground thread parses\nthe index and builds the_index.\n\nThis is a performance optimization to reduce the overall\ntime required to get the index into memory.\n\nTesting on Windows (using the OpenSSL SHA1 routine) showed\nthat parsing the index and computing the SHA1 take almost\nequal time, so this patch effectively reduces the startup\ntime by 1/2.\n\nSigned-off-by: Jeff Hostetler <jeffhost@microsoft.com>\n---\n read-cache.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 86 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 9054369..27c63a3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1564,6 +1564,83 @@ static void post_read_index_from(struct index_state *istate)\n \ttweak_untracked_cache(istate);\n }\n \n+#ifdef NO_PTHREADS\n+\n+struct verify_hdr_thread_data {\n+\tstruct cache_header *hdr;\n+\tsize_t size;\n+};\n+\n+/*\n+ * Non-threaded version does all the work immediately.\n+ * Returns < 0 on error or bad signature.\n+ */\n+static int verify_hdr_start(struct verify_hdr_thread_data *d)\n+{\n+\treturn verify_hdr(d->hdr, (unsigned long)d->size);\n+}\n+\n+static int verify_hdr_finish(struct verify_hdr_thread_data *d)\n+{\n+\treturn 0;\n+}\n+\n+#else\n+\n+#include <pthread.h>\n+\n+/*\n+ * Require index file to be larger than this threshold before\n+ * we bother using a background thread to verify the SHA.\n+ */\n+#define VERIFY_HDR_THRESHOLD    (1024)\n+\n+struct verify_hdr_thread_data {\n+\tpthread_t thread_id;\n+\tstruct cache_header *hdr;\n+\tsize_t size;\n+\tint result;\n+};\n+\n+/*\n+ * Thread proc to run verify_hdr() computation in a background thread.\n+ */\n+static void *verify_hdr_thread_proc(void *_data)\n+{\n+\tstruct verify_hdr_thread_data *d = _data;\n+\td->result = verify_hdr(d->hdr, (unsigned long)d->size);\n+\treturn NULL;\n+}\n+\n+/*\n+ * Threaded version starts background thread and returns zero\n+ * to indicate that we don't know the hash is bad yet.  If the\n+ * index is too small, we just do the work imediately.\n+ */\n+static int verify_hdr_start(struct verify_hdr_thread_data *d)\n+{\n+\tif (d->size < VERIFY_HDR_THRESHOLD)\n+\t\treturn verify_hdr(d->hdr, (unsigned long)d->size);\n+\n+\tif (pthread_create(&d->thread_id, NULL, verify_hdr_thread_proc, d))\n+\t\tdie_errno(\"unable to start verify_hdr_thread\");\n+\n+\treturn 0;\n+}\n+\n+static int verify_hdr_finish(struct verify_hdr_thread_data *d)\n+{\n+\tif (d->size < VERIFY_HDR_THRESHOLD)\n+\t\treturn 0;\n+\n+\tif (pthread_join(d->thread_id, NULL))\n+\t\tdie_errno(\"unable to join verify_hdr_thread\");\n+\n+\treturn d->result;\n+}\n+\n+#endif\n+\n /* remember to discard_cache() before reading a different cache! */\n int do_read_index(struct index_state *istate, const char *path, int must_exist)\n {\n@@ -1574,6 +1651,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tvoid *mmap;\n \tsize_t mmap_size;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n+\tstruct verify_hdr_thread_data data;\n \n \tif (istate->initialized)\n \t\treturn istate->cache_nr;\n@@ -1600,7 +1678,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tclose(fd);\n \n \thdr = mmap;\n-\tif (verify_hdr(hdr, mmap_size) < 0)\n+\n+\tdata.hdr = hdr;\n+\tdata.size = mmap_size;\n+\tif (verify_hdr_start(&data) < 0)\n \t\tgoto unmap;\n \n \thashcpy(istate->sha1, (const unsigned char *)hdr + mmap_size - 20);\n@@ -1649,6 +1730,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tsrc_offset += 8;\n \t\tsrc_offset += extsize;\n \t}\n+\n+\tif (verify_hdr_finish(&data) < 0)\n+\t\tgoto unmap;\n+\n \tmunmap(mmap, mmap_size);\n \treturn istate->cache_nr;\n \n-- \n2.7.4\n\n"},{"id":"315263","messageId":"20170324153617.smmzju5mygqt5vux@sigill.intra.peff.net","threadId":"45493","inReplyTo":"1490362071-46932-2-git-send-email-git@jeffhostetler.com","subject":"Re: [PATCH] read-cache: call verify_hdr() in a background thread","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T15:36:17Z","receivedAt":"2017-03-24T15:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 01:27:51PM +0000, git@jeffhostetler.com wrote:\n\n> From: Jeff Hostetler <jeffhost@microsoft.com>\n> \n> Teash do_read_index() in read-cache.c to call verify_hdr()\n> in a background thread while the forground thread parses\n> the index and builds the_index.\n> \n> This is a performance optimization to reduce the overall\n> time required to get the index into memory.\n> \n> Testing on Windows (using the OpenSSL SHA1 routine) showed\n> that parsing the index and computing the SHA1 take almost\n> equal time, so this patch effectively reduces the startup\n> time by 1/2.\n\nHave you considered just skipping the sha1 check on read (possibly with\nan option)?\n\nIts purpose is to detect disk bit-rot. Checking it for every single\noperation may be a bit excessive, especially because it gets rewritten a\nlot. Nobody really cared until now because they don't have index files\nthat are hundreds of megabytes.\n\n-Peff\n"},{"id":"315264","messageId":"1743d048-8d6b-c14e-161b-351e6d79fb95@jeffhostetler.com","threadId":"45493","inReplyTo":"20170324153617.smmzju5mygqt5vux@sigill.intra.peff.net","subject":"Re: [PATCH] read-cache: call verify_hdr() in a background thread","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-24T15:46:14Z","receivedAt":"2017-03-24T15:49:39Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/24/2017 11:36 AM, Jeff King wrote:\n> On Fri, Mar 24, 2017 at 01:27:51PM +0000, git@jeffhostetler.com wrote:\n>\n>> From: Jeff Hostetler <jeffhost@microsoft.com>\n>>\n>> Teash do_read_index() in read-cache.c to call verify_hdr()\n>> in a background thread while the forground thread parses\n>> the index and builds the_index.\n>>\n>> This is a performance optimization to reduce the overall\n>> time required to get the index into memory.\n>>\n>> Testing on Windows (using the OpenSSL SHA1 routine) showed\n>> that parsing the index and computing the SHA1 take almost\n>> equal time, so this patch effectively reduces the startup\n>> time by 1/2.\n>\n> Have you considered just skipping the sha1 check on read (possibly with\n> an option)?\n>\n> Its purpose is to detect disk bit-rot. Checking it for every single\n> operation may be a bit excessive, especially because it gets rewritten a\n> lot. Nobody really cared until now because they don't have index files\n> that are hundreds of megabytes.\n\nYes, we have done that in a version of the client customized\nfor the virtual file system effort and have been using it\ninternally without incident for quite a while.\n\nAnd we can get a patch for that instead (or include it with\nthis one) if there's interest.\n\nI tried to limit my proposal here to increasing performance while\npreserving the existing behavior.\n\nJeff\n"},{"id":"315270","messageId":"20170324163540.GB31294@aiede.mtv.corp.google.com","threadId":"45493","inReplyTo":"1490362071-46932-2-git-send-email-git@jeffhostetler.com","subject":"Re: [PATCH] read-cache: call verify_hdr() in a background thread","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T16:35:40Z","receivedAt":"2017-03-24T16:42:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"git@jeffhostetler.com wrote:\n\n> From: Jeff Hostetler <jeffhost@microsoft.com>\n>\n> Teash do_read_index() in read-cache.c to call verify_hdr()\n\ns/Teash/Teach/\n\n> in a background thread while the forground thread parses\n\ns/forground/foreground/\n\n> the index and builds the_index.\n>\n> This is a performance optimization to reduce the overall\n> time required to get the index into memory.\n>\n> Testing on Windows (using the OpenSSL SHA1 routine) showed\n> that parsing the index and computing the SHA1 take almost\n> equal time, so this patch effectively reduces the startup\n> time by 1/2.\n>\n> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>\n\nNice.  Do you have example commands I can run to reproduce\nthat benchmark?  (Even better if you can phrase that as a\npatch against t/perf/.)\n\n[...]\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1564,6 +1564,83 @@ static void post_read_index_from(struct index_state *istate)\n>  \ttweak_untracked_cache(istate);\n>  }\n>  \n> +#ifdef NO_PTHREADS\n> +\n> +struct verify_hdr_thread_data {\n> +\tstruct cache_header *hdr;\n> +\tsize_t size;\n\n'size' appears to always be cast to an unsigned long when it's\nused.  Why not use unsigned long consistently?\n\n> +};\n> +\n> +/*\n> + * Non-threaded version does all the work immediately.\n> + * Returns < 0 on error or bad signature.\n> + */\n> +static int verify_hdr_start(struct verify_hdr_thread_data *d)\n> +{\n> +\treturn verify_hdr(d->hdr, (unsigned long)d->size);\n> +}\n> +\n> +static int verify_hdr_finish(struct verify_hdr_thread_data *d)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +#else\n> +\n> +#include <pthread.h>\n\nPlease put this at the top of the file with other #includes.  One\nsimple way to do that is to #include \"thread-utils.h\" at the top of\nthe file unconditionally.\n\n> +\n> +/*\n> + * Require index file to be larger than this threshold before\n> + * we bother using a background thread to verify the SHA.\n> + */\n> +#define VERIFY_HDR_THRESHOLD    (1024)\n\nnits: (1) no need for parens for a numerical macro like this\n(2) comment can be made briefer and more explicit:\n\n/*\n * Index size threshold in bytes before it's worth bothering to\n * use a background thread to verify the index file.\n */\n\nHow was this value chosen?\n\n> +\n> +struct verify_hdr_thread_data {\n> +\tpthread_t thread_id;\n> +\tstruct cache_header *hdr;\n> +\tsize_t size;\n> +\tint result;\n> +};\n\nAll structs are data.  Other parts of git seem to name this kind of\ncallback cookie *_cb_data, so perhaps verify_hdr_cb_data?\n\nOn the other hand this seems to also be used by the caller as a handle\nto the async verify_hdr process. Maybe verify_hdr_state?\n\nThis seems to be doing something similar to the existing 'struct\nasync' interface.  Could it use that instead, or does it incur too\nmuch overhead?  An advantage would be avoiding having to handle the\nNO_PTHREADS ifdef-ery.\n\n> +\n> +/*\n> + * Thread proc to run verify_hdr() computation in a background thread.\n> + */\n> +static void *verify_hdr_thread_proc(void *_data)\n\nPlease don't name identifiers with a leading underscore --- those are\nreserved names.\n\n> +{\n> +\tstruct verify_hdr_thread_data *d = _data;\n> +\td->result = verify_hdr(d->hdr, (unsigned long)d->size);\n> +\treturn NULL;\n> +}\n> +\n> +/*\n> + * Threaded version starts background thread and returns zero\n> + * to indicate that we don't know the hash is bad yet.  If the\n> + * index is too small, we just do the work imediately.\n> + */\n> +static int verify_hdr_start(struct verify_hdr_thread_data *d)\n\nThis comment restates what the code says.  Is there background or\nsomething about the intent behind the code that could be said instead\nto help the reader?  Otherwise I'd suggest removing the comment.\n\n[...]\n> @@ -1574,6 +1651,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n>  \tvoid *mmap;\n>  \tsize_t mmap_size;\n>  \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n> +\tstruct verify_hdr_thread_data data;\n>  \n>  \tif (istate->initialized)\n>  \t\treturn istate->cache_nr;\n> @@ -1600,7 +1678,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n>  \tclose(fd);\n>  \n>  \thdr = mmap;\n> -\tif (verify_hdr(hdr, mmap_size) < 0)\n> +\n> +\tdata.hdr = hdr;\n> +\tdata.size = mmap_size;\n> +\tif (verify_hdr_start(&data) < 0)\n>  \t\tgoto unmap;\n\nWhat happens if there is an error before the code reaches the end of\nthe function?  I think there needs to be a verify_hdr_finish call in\nthe 'unmap:' cleanup section.\n\nThe rest looks reasonable.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315284","messageId":"23685f59-138f-51c8-61f4-6f46d153ff89@jeffhostetler.com","threadId":"45493","inReplyTo":"20170324163540.GB31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH] read-cache: call verify_hdr() in a background thread","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-03-24T17:33:24Z","receivedAt":"2017-03-24T17:33:35Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/24/2017 12:35 PM, Jonathan Nieder wrote:\n> git@jeffhostetler.com wrote:\n>\n>> From: Jeff Hostetler <jeffhost@microsoft.com>\n>>\n>> Teash do_read_index() in read-cache.c to call verify_hdr()\n>...\n> Nice.  Do you have example commands I can run to reproduce\n> that benchmark?  (Even better if you can phrase that as a\n> patch against t/perf/.)\n\nI debated doing a t/perf and/or t/helper to demonstrate this\nlike I did for the lazy-init-name-hash changes the other day,\nbut decided against it.  I'll put together something and\ninclude it in the next version.\n\n>\n> [...]\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1564,6 +1564,83 @@ static void post_read_index_from(struct index_state *istate)\n>>  \ttweak_untracked_cache(istate);\n>>  }\n>>\n>> +#ifdef NO_PTHREADS\n>> +\n>> +struct verify_hdr_thread_data {\n>> +\tstruct cache_header *hdr;\n>> +\tsize_t size;\n>\n> 'size' appears to always be cast to an unsigned long when it's\n> used.  Why not use unsigned long consistently?\n>\n>> +};\n>> +\n>> +/*\n>> + * Non-threaded version does all the work immediately.\n>> + * Returns < 0 on error or bad signature.\n>> + */\n>> +static int verify_hdr_start(struct verify_hdr_thread_data *d)\n>> +{\n>> +\treturn verify_hdr(d->hdr, (unsigned long)d->size);\n>> +}\n>> +\n>> +static int verify_hdr_finish(struct verify_hdr_thread_data *d)\n>> +{\n>> +\treturn 0;\n>> +}\n>> +\n>> +#else\n>> +\n>> +#include <pthread.h>\n>\n> Please put this at the top of the file with other #includes.  One\n> simple way to do that is to #include \"thread-utils.h\" at the top of\n> the file unconditionally.\n>\n>> +\n>> +/*\n>> + * Require index file to be larger than this threshold before\n>> + * we bother using a background thread to verify the SHA.\n>> + */\n>> +#define VERIFY_HDR_THRESHOLD    (1024)\n>\n> nits: (1) no need for parens for a numerical macro like this\n> (2) comment can be made briefer and more explicit:\n>\n> /*\n>  * Index size threshold in bytes before it's worth bothering to\n>  * use a background thread to verify the index file.\n>  */\n>\n> How was this value chosen?\n\nThis was somewhat at random.  I'll update with the t/perf stuff.\n\n>\n>> +\n>> +struct verify_hdr_thread_data {\n>> +\tpthread_t thread_id;\n>> +\tstruct cache_header *hdr;\n>> +\tsize_t size;\n>> +\tint result;\n>> +};\n>\n> All structs are data.  Other parts of git seem to name this kind of\n> callback cookie *_cb_data, so perhaps verify_hdr_cb_data?\n>\n> On the other hand this seems to also be used by the caller as a handle\n> to the async verify_hdr process. Maybe verify_hdr_state?\n>\n> This seems to be doing something similar to the existing 'struct\n> async' interface.  Could it use that instead, or does it incur too\n> much overhead?  An advantage would be avoiding having to handle the\n> NO_PTHREADS ifdef-ery.\n>\n>> +\n>> +/*\n>> + * Thread proc to run verify_hdr() computation in a background thread.\n>> + */\n>> +static void *verify_hdr_thread_proc(void *_data)\n>\n> Please don't name identifiers with a leading underscore --- those are\n> reserved names.\n>\n>> +{\n>> +\tstruct verify_hdr_thread_data *d = _data;\n>> +\td->result = verify_hdr(d->hdr, (unsigned long)d->size);\n>> +\treturn NULL;\n>> +}\n\nI was just modeling the code on what I saw in preload-index.c.\nThere the #ifdef side defines the trivial functions. Then the #else\nand the #include, the struct thread_data, and then the\npreload_thread(void *_data) function declaration.  But blame reports\nthat code dating back to 2008.  Is there an example of a newer\npreferred style somewhere ?\n\n>> +\n>> +/*\n>> + * Threaded version starts background thread and returns zero\n>> + * to indicate that we don't know the hash is bad yet.  If the\n>> + * index is too small, we just do the work imediately.\n>> + */\n>> +static int verify_hdr_start(struct verify_hdr_thread_data *d)\n>\n> This comment restates what the code says.  Is there background or\n> something about the intent behind the code that could be said instead\n> to help the reader?  Otherwise I'd suggest removing the comment.\n>\n> [...]\n>\n> What happens if there is an error before the code reaches the end of\n> the function?  I think there needs to be a verify_hdr_finish call in\n> the 'unmap:' cleanup section.\n\nBut the \"unmap\" section calls die().  Do need to join first ??\n(It's OK if we do, just asking.)\n\n>\n> The rest looks reasonable.\n>\n> Thanks and hope that helps,\n> Jonathan\n>\n\nThanks,\nJeff\n"},{"id":"315288","messageId":"20170324175310.GC31294@aiede.mtv.corp.google.com","threadId":"45493","inReplyTo":"23685f59-138f-51c8-61f4-6f46d153ff89@jeffhostetler.com","subject":"Re: [PATCH] read-cache: call verify_hdr() in a background thread","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-24T17:53:10Z","receivedAt":"2017-03-24T17:53:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff Hostetler wrote:\n> On 3/24/2017 12:35 PM, Jonathan Nieder wrote:\n\n>> What happens if there is an error before the code reaches the end of\n>> the function?  I think there needs to be a verify_hdr_finish call in\n>> the 'unmap:' cleanup section.\n>\n> But the \"unmap\" section calls die().  Do need to join first ??\n> (It's OK if we do, just asking.)\n\nGood point.  Now I wonder why the 'unmap:' section exists at all.\n\n\t$ git log -Sunmap: read-cache.c \n\tcommit e83c5163316f89bfbde7d9ab23ca2e25604af290\n\tAuthor: Linus Torvalds <torvalds@ppc970.osdl.org>\n\tDate:   Thu Apr 7 15:13:13 2005 -0700\n\n\t    Initial revision of \"git\", the information manager from hell\n\nSo the unmap: section has been there since day one.  At that point\nit used 'return error(...)', so it made more sense.  Where did the\ndie() come from, then?\n\n\tcommit 5d1a5c02e8ac1c16688ea4a44512245f25a49f8a\n\tAuthor: Linus Torvalds <torvalds@osdl.org>\n\tDate:   Sat Oct 1 13:24:27 2005 -0700\n\n\t    [PATCH] Better error reporting for \"git status\"\n\nI think it should be safe to eliminate the 'unmap' section altogether\nand that that change just forgot to, but in any case it's orthogonal\nto your patch.\n\nThanks and sorry for the confusion,\nJonathan\n"}]}