{"thread":{"id":"16787","subject":"[RFC PATCH 0/2] Add support for multi threaded checkout","startedAt":"2008-12-18T20:51:57Z","lastAt":"2008-12-19T01:04:29Z","messageCount":11,"participants":["Pickens, James E","James Pickens","Nicolas Pitre","Nicolas Morey-Chaisemartin","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"98281","messageId":"3BA20DF9B35F384F8B7395B001EC3FB3265B2A01@azsmsx507.amr.corp.intel.com","threadId":"16787","inReplyTo":null,"subject":"[RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"Pickens, James E","fromEmail":"james.e.pickens@intel.com","sentAt":"2008-12-18T20:51:57Z","receivedAt":"2008-12-18T20:51:57Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"\nThere was some discussion a while back about improving git performance on\nNFS (http://article.gmane.org/gmane.comp.version-control.git/100950).\nThis led to Linus adding the 'preload_index' function, which improves\nperformance of several commands using multi threading.  He also briefly\ndescribed how to do the same for 'git checkout'.  Well, I finally found\nsome time to work on it, and will post patches shortly.  This is my first\npatch; apologies if I screwed something up.\n\nPatch 1 adds the functionality, and 2 adds a config option to\nenable/disable it.\n\nMuch of the patch is literally copy/paste from preload-index.c into\nunpack-trees.c.  Many of the functions called during checkout are not\nthread safe, so I added a mutex in entry.c to serialize basically\neverything except writing the files to disk.  I also added a mutex in\nunpack-trees.c for the progress meter.\n\nIt passes the test suite, and seems fairly safe to my naïve eyes.\n\nHere are some benchmarks, cloning a linux kernel repo I had on an NFS\ndrive:\n\n                   NFS->NFS    NFS->Local\nmaster (53682f0c)    2:46.1          13.3\nwith threads           36.6          18.2\n\nSo it improved performance on NFS significantly.  Unfortunately it also\ndegraded performance on the local disk significantly.  I'm hoping someone\nwill suggest a way to mitigate that... I think it would be reasonable to\ndisable the threading except when the work dir is on NFS, but I don't\nknow how to detect that.  Even in that case it will have *some* impact\nfrom locking/unlocking the mutex, but I think it would be in the noise.\n\nJames\n"},{"id":"98283","messageId":"1229633811-3877-1-git-send-email-james.e.pickens@intel.com","threadId":"16787","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB3265B2A01@azsmsx507.amr.corp.intel.com","subject":"[PATCH 1/2] Add support for multi threaded checkout","fromName":"James Pickens","fromEmail":"james.e.pickens@intel.com","sentAt":"2008-12-18T20:56:50Z","receivedAt":"2008-12-18T20:56:50Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"This speeds up operations like 'git clone' on NFS drives tremendously, but\nslows down the same operations on local disks.\n\nPartitioning the work and launching threads is done in unpack-trees.c.  The code\nis mostly copied from preload_index.c.  The maximum number of threads is set to\n8, which seemed to give a reasonable tradeoff between performance improvement on\nNFS and degradation on local disks.\n\nSome code was added to entry.c for serialization.  Most of the contents of\ncheckout_entry and write_entry are serialized, except writing the checked out\nfiles to disk.\n---\n entry.c        |   42 +++++++++++++++++---\n unpack-trees.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 151 insertions(+), 6 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex aa2ee46..764d2db 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -1,6 +1,21 @@\n #include \"cache.h\"\n #include \"blob.h\"\n \n+#ifdef NO_PTHREADS\n+\n+#define checkout_lock()\t\t(void)0\n+#define checkout_unlock()\t(void)0\n+\n+#else\n+\n+#include <pthread.h>\n+\n+static pthread_mutex_t checkout_mutex = PTHREAD_MUTEX_INITIALIZER;\n+#define checkout_lock()\t\tpthread_mutex_lock(&checkout_mutex)\n+#define checkout_unlock()\tpthread_mutex_unlock(&checkout_mutex)\n+\n+#endif\n+\n static void create_directories(const char *path, const struct checkout *state)\n {\n \tint len = strlen(path);\n@@ -100,7 +115,7 @@ static void *read_blob_entry(struct cache_entry *ce, const char *path, unsigned\n \n static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)\n {\n-\tint fd;\n+\tint fd, retval;\n \tlong wrote;\n \n \tswitch (ce->ce_mode & S_IFMT) {\n@@ -109,10 +124,15 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tunsigned long size;\n \n \tcase S_IFREG:\n+\t\tcheckout_lock();\n \t\tnew = read_blob_entry(ce, path, &size);\n-\t\tif (!new)\n-\t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n+\n+\t\tif (!new) {\n+\t\t\tretval = error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, sha1_to_hex(ce->sha1));\n+\t\t\tcheckout_unlock();\n+\t\t\treturn retval;\n+\t\t}\n \n \t\t/*\n \t\t * Convert from git internal format to working tree format\n@@ -124,6 +144,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t\tnew = strbuf_detach(&buf, &newsize);\n \t\t\tsize = newsize;\n \t\t}\n+\t\tcheckout_unlock();\n \n \t\tif (to_tempfile) {\n \t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n@@ -143,10 +164,17 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t\treturn error(\"git checkout-index: unable to write file %s\", path);\n \t\tbreak;\n \tcase S_IFLNK:\n+\t\tcheckout_lock();\n \t\tnew = read_blob_entry(ce, path, &size);\n-\t\tif (!new)\n-\t\t\treturn error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n+\n+\t\tif (!new) {\n+\t\t\tretval = error(\"git checkout-index: unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, sha1_to_hex(ce->sha1));\n+\t\t\tcheckout_unlock();\n+\t\t\treturn retval;\n+\t\t}\n+\t\tcheckout_unlock();\n+\n \t\tif (to_tempfile || !has_symlinks) {\n \t\t\tif (to_tempfile) {\n \t\t\t\tstrcpy(path, \".merge_link_XXXXXX\");\n@@ -192,7 +220,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \n int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)\n {\n-\tstatic char path[PATH_MAX + 1];\n+\tchar path[PATH_MAX + 1];\n \tstruct stat st;\n \tint len = state->base_dir_len;\n \n@@ -229,6 +257,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n+\tcheckout_lock();\n \tcreate_directories(path, state);\n+\tcheckout_unlock();\n \treturn write_entry(ce, path, state, 0);\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 54f301d..30b9862 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -8,6 +8,10 @@\n #include \"progress.h\"\n #include \"refs.h\"\n \n+#ifndef NO_PTHREADS\n+#include <pthread.h>\n+#endif\n+\n /*\n  * Error messages expected by scripts out of plumbing commands such as\n  * read-tree.  Non-scripted Porcelain is not required to use these messages\n@@ -85,6 +89,115 @@ static void unlink_entry(struct cache_entry *ce)\n }\n \n static struct checkout state;\n+\n+#ifdef NO_PTHREADS\n+#define progress_lock()\t\t(void)0\n+#define progress_unlock()\t(void)0\n+\n+static int threaded_checkout(struct index_state *index, int update, struct progress *prog, unsigned *prog_cnt)\n+{\n+\treturn 0; /* do nothing */\n+}\n+\n+#else\n+\n+#include <pthread.h>\n+\n+static pthread_mutex_t progress_mutex = PTHREAD_MUTEX_INITIALIZER;\n+#define progress_lock()\t\tpthread_mutex_lock(&progress_mutex)\n+#define progress_unlock()\tpthread_mutex_unlock(&progress_mutex)\n+\n+/*\n+ * Mostly randomly chosen maximum thread counts: we\n+ * cap the parallelism to 8 threads, and we want\n+ * to have at least 500 files per thread for it to\n+ * be worth starting a thread.\n+ */\n+#define MAX_PARALLEL (8)\n+#define THREAD_COST (500)\n+\n+struct thread_data {\n+\tpthread_t pthread;\n+\tstruct index_state *index;\n+\tstruct checkout *state;\n+\tint update, offset, nr, errs;\n+\tstruct progress *progress;\n+\tunsigned *progress_cnt;\n+};\n+\n+static void *checkout_thread(void *_data)\n+{\n+\tint nr;\n+\tstruct thread_data *p = _data;\n+\tstruct index_state *index = p->index;\n+\tstruct cache_entry **cep = index->cache + p->offset;\n+\n+\tp->errs = 0;\n+\n+\tnr = p->nr;\n+\tif (0 == nr) {\n+\t\treturn NULL;\n+\t}\n+\n+\tif (nr + p->offset > index->cache_nr)\n+\t\tnr = index->cache_nr - p->offset;\n+\n+\tdo {\n+\t\tstruct cache_entry *ce = *cep++;\n+\n+\t\tif (ce->ce_flags & CE_UPDATE) {\n+\t\t\tprogress_lock();\n+\t\t\tdisplay_progress(p->progress, ++(*p->progress_cnt));\n+\t\t\tprogress_unlock();\n+\t\t\tce->ce_flags &= ~CE_UPDATE;\n+\t\t\tif (p->update) {\n+\t\t\t\tp->errs |= checkout_entry(ce, p->state, NULL);\n+\t\t\t\tfflush(stdout);\n+\t\t\t}\n+\t\t}\n+\t} while (--nr > 0);\n+\treturn NULL;\n+}\n+\n+static int threaded_checkout(struct index_state *index, int update, struct progress *prog, unsigned *prog_cnt)\n+{\n+\tint threads, work, offset, i;\n+\tstruct thread_data data[MAX_PARALLEL];\n+\tint errs = 0;\n+\n+\tthreads = index->cache_nr / THREAD_COST;\n+\tif (threads > MAX_PARALLEL)\n+\t\tthreads = MAX_PARALLEL;\n+\telse if (threads == 0)\n+\t\treturn 0;\n+\n+\toffset = 0;\n+\twork = (index->cache_nr + threads - 1) / threads;\n+\tfor (i = 0; i < threads; i++) {\n+\t\tstruct thread_data *p = data+i;\n+\t\tp->index = index;\n+\t\tp->offset = offset;\n+\t\tp->nr = work;\n+\t\tp->state = &state;\n+\t\tp->update = update;\n+\t\tp->progress = prog;\n+\t\tp->progress_cnt = prog_cnt;\n+\t\toffset += work;\n+\t\tif (pthread_create(&p->pthread, NULL, checkout_thread, p))\n+\t\t\tdie(\"unable to create threaded checkout\");\n+\t}\n+\tfor (i = 0; i < threads; i++) {\n+\t\tstruct thread_data *p = data+i;\n+\t\tif (pthread_join(p->pthread, NULL))\n+\t\t\tdie(\"unable to join threaded checkout\");\n+\t\terrs |= p->errs;\n+\t}\n+\n+\treturn errs;\n+}\n+\n+#endif\n+\n static int check_updates(struct unpack_trees_options *o)\n {\n \tunsigned cnt = 0, total = 0;\n@@ -118,6 +231,8 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t}\n \t}\n \n+\terrs |= threaded_checkout(index, o->update, progress, &cnt);\n+\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n-- \n1.6.0.4.1116.gc5d7\n"},{"id":"98282","messageId":"1229633811-3877-2-git-send-email-james.e.pickens@intel.com","threadId":"16787","inReplyTo":"1229633811-3877-1-git-send-email-james.e.pickens@intel.com","subject":"[PATCH 2/2] Add core.threadedcheckout config option","fromName":"James Pickens","fromEmail":"james.e.pickens@intel.com","sentAt":"2008-12-18T20:56:51Z","receivedAt":"2008-12-18T20:56:51Z","isPatch":true,"sender":{"key":"james.e.pickens@intel.com","avatar":null},"body":"Setting it to 'true' enables multi threaded checkout.  Default is false.\n---\n Documentation/config.txt |    8 ++++++++\n cache.h                  |    1 +\n config.c                 |    5 +++++\n environment.c            |    3 +++\n unpack-trees.c           |    3 +++\n 5 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 21ea165..22ac76b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -422,6 +422,14 @@ relatively high IO latencies.  With this set to 'true', git will do the\n index comparison to the filesystem data in parallel, allowing\n overlapping IO's.\n \n+core.threadedcheckout::\n+\tEnable parallel checkout for operations like 'git checkout'\n++\n+This can speed up operations like 'git clone' and 'git checkout' especially\n+on filesystems like NFS that have relatively high IO latencies.  With this\n+set to 'true', git will write the checked out files to disk in parallel,\n+allowing overlapping IO's.\n+\n alias.*::\n \tCommand aliases for the linkgit:git[1] command wrapper - e.g.\n \tafter defining \"alias.last = cat-file commit HEAD\", the invocation\ndiff --git a/cache.h b/cache.h\nindex 231c06d..0777597 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -512,6 +512,7 @@ extern size_t delta_base_cache_limit;\n extern int auto_crlf;\n extern int fsync_object_files;\n extern int core_preload_index;\n+extern int core_threaded_checkout;\n \n enum safe_crlf {\n \tSAFE_CRLF_FALSE = 0,\ndiff --git a/config.c b/config.c\nindex 790405a..819693e 100644\n--- a/config.c\n+++ b/config.c\n@@ -495,6 +495,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.threadedcheckout\")) {\n+\t\tcore_threaded_checkout = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n }\ndiff --git a/environment.c b/environment.c\nindex e278bce..2450b65 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -46,6 +46,9 @@ enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n /* Parallel index stat data preload? */\n int core_preload_index = 0;\n \n+/* Parallel checkout? */\n+int core_threaded_checkout = 0;\n+\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n static char *work_tree;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 30b9862..635b7dc 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -165,6 +165,9 @@ static int threaded_checkout(struct index_state *index, int update, struct progr\n \tstruct thread_data data[MAX_PARALLEL];\n \tint errs = 0;\n \n+\tif (!core_threaded_checkout)\n+\t\treturn 0;\n+\n \tthreads = index->cache_nr / THREAD_COST;\n \tif (threads > MAX_PARALLEL)\n \t\tthreads = MAX_PARALLEL;\n-- \n1.6.0.4.1116.gc5d7\n"},{"id":"98284","messageId":"alpine.LFD.2.00.0812181600210.30035@xanadu.home","threadId":"16787","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB3265B2A01@azsmsx507.amr.corp.intel.com","subject":"Re: [RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-12-18T21:02:08Z","receivedAt":"2008-12-18T21:02:08Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 18 Dec 2008, Pickens, James E wrote:\n\n>                    NFS->NFS    NFS->Local\n> master (53682f0c)    2:46.1          13.3\n> with threads           36.6          18.2\n> \n> So it improved performance on NFS significantly.\n\nAre those figures repeatable over multiple consecutive runs?\n\n\nNicolas\n"},{"id":"98285","messageId":"885649360812181313q6d43354jf73b3f39d5844016@mail.gmail.com","threadId":"16787","inReplyTo":"alpine.LFD.2.00.0812181600210.30035@xanadu.home","subject":"Re: [RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2008-12-18T21:13:34Z","receivedAt":"2008-12-18T21:13:34Z","isPatch":true,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"[Resending since I forgot to copy the list]\n\nOn Thu, Dec 18, 2008, Nicolas Pitre <nico@cam.org> wrote:\n> On Thu, 18 Dec 2008, Pickens, James E wrote:\n>\n>>                    NFS->NFS    NFS->Local\n>> master (53682f0c)    2:46.1          13.3\n>> with threads           36.6          18.2\n>>\n>> So it improved performance on NFS significantly.\n>\n> Are those figures repeatable over multiple consecutive runs?\n\nRoughly, yes.  There is some variance of course, probably\nmore than usual since all these operations involve NFS.\nThe numbers are the best of several runs in each case.\n\nJames\n"},{"id":"98286","messageId":"494ABDC9.9060001@morey-chaisemartin.com","threadId":"16787","inReplyTo":"3BA20DF9B35F384F8B7395B001EC3FB3265B2A01@azsmsx507.amr.corp.intel.com","subject":"Re: [RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"devel@morey-chaisemartin.com","sentAt":"2008-12-18T21:16:57Z","receivedAt":"2008-12-18T21:16:57Z","isPatch":true,"sender":{"key":"devel@morey-chaisemartin.com","avatar":null},"body":"Pickens, James E a écrit :\n> Even in that case it will have *some* impact\n> from locking/unlocking the mutex, but I think it would be in the noise.\n>\n> James\n> -\nI guess you could do something like :\n\n#define checkout_lock()\t\tcore_threaded_checkout ?pthread_mutex_lock(&checkout_mutex) : (void) 0\n#define checkout_unlock()\t\tcore_threaded_checkout ?pthread_mutex_unlock(&checkout_mutex) : (void) 0\n\nIt should be faster when you don't actually use threaded checkouts, as you won't unnecessarily lock/unlock your mutex. \n\nHave you looked at the perf from local to local? I'm just curious.\n\nNicolas Morey-Chaisemartin\n"},{"id":"98291","messageId":"alpine.LFD.2.00.0812181333150.14014@localhost.localdomain","threadId":"16787","inReplyTo":"1229633811-3877-1-git-send-email-james.e.pickens@intel.com","subject":"Re: [PATCH 1/2] Add support for multi threaded checkout","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-18T21:41:48Z","receivedAt":"2008-12-18T21:41:48Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Dec 2008, James Pickens wrote:\n>\n> This speeds up operations like 'git clone' on NFS drives tremendously, but\n> slows down the same operations on local disks.\n> \n> Partitioning the work and launching threads is done in unpack-trees.c.  The code\n> is mostly copied from preload_index.c.  The maximum number of threads is set to\n> 8, which seemed to give a reasonable tradeoff between performance improvement on\n> NFS and degradation on local disks.\n\nHmm. I don't really like this very much.\n\nWhy? Because as your locking shows, we can really only parallelise the \nactual write-out anyway, and rather than do any locking there, wouldn't it \nbe better to have a notion of \"queued work\" (which would be just the \nwrite-out) to be done in parallel?\n\nSo instead of doing all the unpacking etc in parallel (with locking around \nit to serialize it), I'd suggest doing ll the unpacking serially since \nthat isn't the problem anyway (and since you have to protect it with a \nlock anyway), and just have a \"write out and free the buffer\" phase that \nis done in the threads.\n\nThe alternative would be to actually do what your patch suggests, but \nactually try to make the code git SHA1 object handling be thread-safe. At \nthat point, the ugly locking in write_entry() would go away, and you might \nactually improve performance on SMP thanks to doing the CPU part in \nparallel.\n\nBut as-is, I think the patch is a bit ugly. The reason I liked the index \npre-reading was that it could be done entirely locklessly, so it really \ndid parallelize it _fully_ (even if the IO latency part was the much \nbigger issue), and that was also why it actually ended up helping even on \na local disk (only if you have multiple cores, of course).\n\n\t\tLinus\n"},{"id":"98290","messageId":"885649360812181342u2978038fj3a11670acd9fd873@mail.gmail.com","threadId":"16787","inReplyTo":"494ABDC9.9060001@morey-chaisemartin.com","subject":"Re: [RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2008-12-18T21:42:21Z","receivedAt":"2008-12-18T21:42:21Z","isPatch":true,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Thu, Dec 18, 2008 at 2:16 PM, Nicolas Morey-Chaisemartin\n<devel@morey-chaisemartin.com> wrote:\n> I guess you could do something like :\n>\n> #define checkout_lock()         core_threaded_checkout ?pthread_mutex_lock(&checkout_mutex) : (void) 0\n> #define checkout_unlock()               core_threaded_checkout ?pthread_mutex_unlock(&checkout_mutex) : (void) 0\n>\n> It should be faster when you don't actually use threaded checkouts, as you won't unnecessarily lock/unlock your mutex.\n>\n> Have you looked at the perf from local to local? I'm just curious.\n\n\nI had looked at it before but didn't record any numbers.  I just took the\nfollowing timings (2 runs each):\n\nmaster             13.78    12.79\nthreads enabled    16.84    20.45\nthreads disabled   14.07    13.27\n\nJames\n"},{"id":"98300","messageId":"885649360812181535h36d24b0gb31acddded452a0@mail.gmail.com","threadId":"16787","inReplyTo":"alpine.LFD.2.00.0812181333150.14014@localhost.localdomain","subject":"Re: [PATCH 1/2] Add support for multi threaded checkout","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2008-12-18T23:35:01Z","receivedAt":"2008-12-18T23:35:01Z","isPatch":true,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Thu, Dec 18, 2008, Linus Torvalds <torvalds@linux-foundation.org> wrote:\n> So instead of doing all the unpacking etc in parallel (with locking around\n> it to serialize it), I'd suggest doing ll the unpacking serially since\n> that isn't the problem anyway (and since you have to protect it with a\n> lock anyway), and just have a \"write out and free the buffer\" phase that\n> is done in the threads.\n\nThat's certainly a more elegant way to do it, but unless I'm missing\nsomething, it requires rewriting a good bit of code.  The main reason I\nwent with the locking was to keep the patch as simple and non-intrusive\nas possible.\n\n> The alternative would be to actually do what your patch suggests, but\n> actually try to make the code git SHA1 object handling be thread-safe. At\n> that point, the ugly locking in write_entry() would go away, and you might\n> actually improve performance on SMP thanks to doing the CPU part in\n> parallel.\n\nI started down that path at one point, and quickly got in over my head.\nMaking all that code thread safe looks like a big task to me.  From my\nperspective, I get a ~350% speedup from this easy patch, and I might get\nan additional 25% (blind guess) from a much more difficult patch.  It\ndidn't seem worth the effort.\n\nJames\n"},{"id":"98306","messageId":"alpine.LFD.2.00.0812181606250.14014@localhost.localdomain","threadId":"16787","inReplyTo":"885649360812181535h36d24b0gb31acddded452a0@mail.gmail.com","subject":"Re: [PATCH 1/2] Add support for multi threaded checkout","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-19T00:13:13Z","receivedAt":"2008-12-19T00:13:13Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Dec 2008, James Pickens wrote:\n\n> On Thu, Dec 18, 2008, Linus Torvalds <torvalds@linux-foundation.org> wrote:\n> > So instead of doing all the unpacking etc in parallel (with locking around\n> > it to serialize it), I'd suggest doing ll the unpacking serially since\n> > that isn't the problem anyway (and since you have to protect it with a\n> > lock anyway), and just have a \"write out and free the buffer\" phase that\n> > is done in the threads.\n> \n> That's certainly a more elegant way to do it, but unless I'm missing\n> something, it requires rewriting a good bit of code.  The main reason I\n> went with the locking was to keep the patch as simple and non-intrusive\n> as possible.\n\nYeah, I looked a bit more at it, and one problem is that we don't just \nwrite out the file, we also refresh the cache with the stat information \nafter writing it out. If we just write the thing out, it would be simpler: \nwe'd just make the thread locklessly write things out and free the data \nfrom a simple set of lockless threads - no need for any access to git data \nstructures.\n\nHo humm. I'll think about it a bit more.\n\n\t\tLinus\n"},{"id":"98315","messageId":"885649360812181704l3a905ebfrb90f391e86004efc@mail.gmail.com","threadId":"16787","inReplyTo":"494ABDC9.9060001@morey-chaisemartin.com","subject":"Re: [RFC PATCH 0/2] Add support for multi threaded checkout","fromName":"James Pickens","fromEmail":"jepicken@gmail.com","sentAt":"2008-12-19T01:04:29Z","receivedAt":"2008-12-19T01:04:29Z","isPatch":true,"sender":{"key":"jepicken@gmail.com","avatar":null},"body":"On Thu, Dec 18, 2008, Nicolas Morey-Chaisemartin\n<devel@morey-chaisemartin.com> wrote:\n> I guess you could do something like :\n>\n> #define checkout_lock()         core_threaded_checkout ?pthread_mutex_lock(&checkout_mutex) : (void) 0\n> #define checkout_unlock()               core_threaded_checkout ?pthread_mutex_unlock(&checkout_mutex) : (void) 0\n\nI tried that, and to make it easier to see the impact, I changed the\n'wrote = write_in_full(...)' calls in entry.c to 'wrote = size'.  That\nmakes git just create a bunch of empty files instead of writing the real\ncontents to disk.  Here's the result, with core.threadedcheckout set to\nfalse, best of 2 runs:\n\noriginal patch:     3.19\noriginal + above:   3.18\n\nSo the cost of locking/unlocking the mutex looks vanishingly small in the\nsingle thread case.\n\nThis also puts an upper bound on the time required for a single thread to\nunpack the data.\n\nJames\n"}]}