{"thread":{"id":"29114","subject":"[POC PATCH 1/5] Turn grep's use_threads into a global flag","startedAt":"2011-12-09T08:39:32Z","lastAt":"2012-04-10T13:39:10Z","messageCount":11,"participants":["Thomas Rast","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"180639","messageId":"cover.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":null,"subject":"[POC PATCH 0/5] Threaded loose object and pack access","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:32Z","receivedAt":"2011-12-09T08:39:32Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Well, just to make sure we're all left in a confused mess of partly\nconflicting patches, here's another angle on the same thing:\n\nJeff King wrote:\n> Wow, that's horrible. Leaving aside the parallelism, it's just terrible\n> that reading from the cache is 20 times slower than the worktree. I get\n> similar results on my quad-core machine.\n\nBy poking around in sha1_file.c I got that down to about 10.  It's not\ngreat yet, but it seems a start.\n\nThe goal would be to improve it to the point where a patch lookup that\nalready has all relevant packs open and windows mapped can proceed\nwithout locking.  I'm not sure that's doable short of duplicating the\nwhole pack state (including fds and windows) across threads, but I'll\ngive it some more thought before going that route.\n\n\n\nThomas Rast (5):\n  Turn grep's use_threads into a global flag\n  grep: push locking into read_sha1_*\n  sha1_file_name_buf(): sha1_file_name in caller's buffer\n  sha1_file: stuff various pack reading variables into a struct\n  sha1_file: make the pack machinery thread-safe\n\n builtin/grep.c   |   60 +++++-----------\n cache.h          |    1 +\n replace_object.c |    5 +-\n sha1_file.c      |  213 +++++++++++++++++++++++++++++++++++++++++-------------\n thread-utils.c   |   30 ++++++++\n thread-utils.h   |   23 ++++++\n 6 files changed, 240 insertions(+), 92 deletions(-)\n\n-- \n1.7.8.431.g2abf2\n"},{"id":"180637","messageId":"16b39f83ed64a2705f2db8880680cd5639f94f56.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"[POC PATCH 1/5] Turn grep's use_threads into a global flag","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:33Z","receivedAt":"2011-12-09T08:39:33Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"In preparation for further work on this, turn use_threads into a flag\nshared across the whole code base.  The supporting (un)lock_if_threaded()\nfunctions are to be used for locking; they return immediately when not\nthreading.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/grep.c |   20 ++++++++------------\n thread-utils.c |   16 ++++++++++++++++\n thread-utils.h |   16 ++++++++++++++++\n 3 files changed, 40 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 988ea1d..76f2c4f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,8 +24,6 @@\n \tNULL\n };\n \n-static int use_threads = 1;\n-\n #ifndef NO_PTHREADS\n #define THREADS 8\n static pthread_t threads[THREADS];\n@@ -76,14 +74,12 @@ struct work_item {\n \n static inline void grep_lock(void)\n {\n-\tif (use_threads)\n-\t\tpthread_mutex_lock(&grep_mutex);\n+\tlock_if_threaded(&grep_mutex);\n }\n \n static inline void grep_unlock(void)\n {\n-\tif (use_threads)\n-\t\tpthread_mutex_unlock(&grep_mutex);\n+\tunlock_if_threaded(&grep_mutex);\n }\n \n /* Used to serialize calls to read_sha1_file. */\n@@ -91,14 +87,12 @@ static inline void grep_unlock(void)\n \n static inline void read_sha1_lock(void)\n {\n-\tif (use_threads)\n-\t\tpthread_mutex_lock(&read_sha1_mutex);\n+\tlock_if_threaded(&read_sha1_mutex);\n }\n \n static inline void read_sha1_unlock(void)\n {\n-\tif (use_threads)\n-\t\tpthread_mutex_unlock(&read_sha1_mutex);\n+\tunlock_if_threaded(&read_sha1_mutex);\n }\n \n /* Signalled when a new work_item is added to todo. */\n@@ -984,6 +978,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\targc--;\n \t}\n \n+#ifndef NO_PTHREADS\n+\tuse_threads = 1;\n+#endif\n+\n \tif (show_in_pager == default_pager)\n \t\tshow_in_pager = git_pager(1);\n \tif (show_in_pager) {\n@@ -1011,8 +1009,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tskip_first_line = 1;\n \t\tstart_threads(&opt);\n \t}\n-#else\n-\tuse_threads = 0;\n #endif\n \n \tcompile_grep_patterns(&opt);\ndiff --git a/thread-utils.c b/thread-utils.c\nindex 7f4b76a..fb75a29 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n #include \"thread-utils.h\"\n \n+int use_threads;\n+\n #if defined(hpux) || defined(__hpux) || defined(_hpux)\n #  include <sys/pstat.h>\n #endif\n@@ -59,3 +61,17 @@ int init_recursive_mutex(pthread_mutex_t *m)\n \t}\n \treturn ret;\n }\n+\n+#ifndef NO_PTHREADS\n+void lock_if_threaded(pthread_mutex_t *m)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_lock(m);\n+}\n+\n+void unlock_if_threaded(pthread_mutex_t *m)\n+{\n+\tif (use_threads)\n+\t\tpthread_mutex_unlock(m);\n+}\n+#endif\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 6fb98c3..9a780a2 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -1,11 +1,27 @@\n #ifndef THREAD_COMPAT_H\n #define THREAD_COMPAT_H\n \n+/*\n+ * This variable is used by commands to globally tell affected\n+ * subsystems that they must use thread-safe mechanisms.\n+ */\n+extern int use_threads;\n+\n #ifndef NO_PTHREADS\n #include <pthread.h>\n \n extern int online_cpus(void);\n extern int init_recursive_mutex(pthread_mutex_t*);\n \n+/* These functions do nothing if use_threads==0 or NO_PTHREADS */\n+extern void lock_if_threaded(pthread_mutex_t*);\n+extern void unlock_if_threaded(pthread_mutex_t*);\n+\n+#else\n+\n+#define lock_if_threaded(lock)\n+#define unlock_if_threaded(lock)\n+\n #endif\n+\n #endif /* THREAD_COMPAT_H */\n-- \n1.7.8.431.g2abf2\n"},{"id":"180642","messageId":"c967eb8f334f83ef0cd6272cff709ed556b935d4.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"[POC PATCH 2/5] grep: push locking into read_sha1_*","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:34Z","receivedAt":"2011-12-09T08:39:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Move the locking away from grep (the user) and into read_sha1_* and\nread_object_* (the subsystem).  This will allow future work on the\nlocking granularity there.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/grep.c |   35 ++++-------------------------------\n sha1_file.c    |   12 ++++++++++--\n thread-utils.c |   11 +++++++++++\n thread-utils.h |    6 ++++++\n 4 files changed, 31 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 76f2c4f..6c5bdfa 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -82,19 +82,6 @@ static inline void grep_unlock(void)\n \tunlock_if_threaded(&grep_mutex);\n }\n \n-/* Used to serialize calls to read_sha1_file. */\n-static pthread_mutex_t read_sha1_mutex;\n-\n-static inline void read_sha1_lock(void)\n-{\n-\tlock_if_threaded(&read_sha1_mutex);\n-}\n-\n-static inline void read_sha1_unlock(void)\n-{\n-\tunlock_if_threaded(&read_sha1_mutex);\n-}\n-\n /* Signalled when a new work_item is added to todo. */\n static pthread_cond_t cond_add;\n \n@@ -248,8 +235,8 @@ static void start_threads(struct grep_opt *opt)\n {\n \tint i;\n \n+\tinit_subsystem_locks();\n \tpthread_mutex_init(&grep_mutex, NULL);\n-\tpthread_mutex_init(&read_sha1_mutex, NULL);\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n@@ -296,16 +283,14 @@ static int wait_all(void)\n \t}\n \n \tpthread_mutex_destroy(&grep_mutex);\n-\tpthread_mutex_destroy(&read_sha1_mutex);\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n+\tdestroy_subsystem_locks();\n \n \treturn hit;\n }\n #else /* !NO_PTHREADS */\n-#define read_sha1_lock()\n-#define read_sha1_unlock()\n \n static int wait_all(void)\n {\n@@ -363,21 +348,11 @@ static int grep_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)\n-{\n-\tvoid *data;\n-\n-\tread_sha1_lock();\n-\tdata = read_sha1_file(sha1, type, size);\n-\tread_sha1_unlock();\n-\treturn data;\n-}\n-\n static void *load_sha1(const unsigned char *sha1, unsigned long *size,\n \t\t       const char *name)\n {\n \tenum object_type type;\n-\tvoid *data = lock_and_read_sha1_file(sha1, &type, size);\n+\tvoid *data = read_sha1_file(sha1, &type, size);\n \n \tif (!data)\n \t\terror(_(\"'%s': unable to read %s\"), name, sha1_to_hex(sha1));\n@@ -578,7 +553,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tvoid *data;\n \t\t\tunsigned long size;\n \n-\t\t\tdata = lock_and_read_sha1_file(entry.sha1, &type, &size);\n+\t\t\tdata = read_sha1_file(entry.sha1, &type, &size);\n \t\t\tif (!data)\n \t\t\t\tdie(_(\"unable to read tree (%s)\"),\n \t\t\t\t    sha1_to_hex(entry.sha1));\n@@ -608,10 +583,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tstruct strbuf base;\n \t\tint hit, len;\n \n-\t\tread_sha1_lock();\n \t\tdata = read_object_with_reference(obj->sha1, tree_type,\n \t\t\t\t\t\t  &size, NULL);\n-\t\tread_sha1_unlock();\n \n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), sha1_to_hex(obj->sha1));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 956422b..c3595b3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -18,6 +18,7 @@\n #include \"refs.h\"\n #include \"pack-revindex.h\"\n #include \"sha1-lookup.h\"\n+#include \"thread-utils.h\"\n \n #ifndef O_NOATIME\n #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n@@ -2237,13 +2238,19 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \tvoid *data;\n \tchar *path;\n \tconst struct packed_git *p;\n-\tconst unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)\n+\tconst unsigned char *repl;\n+\n+\tlock_if_threaded(&read_sha1_mutex);\n+\n+\trepl = (flag & READ_SHA1_FILE_REPLACE)\n \t\t? lookup_replace_object(sha1) : sha1;\n \n \terrno = 0;\n \tdata = read_object(repl, type, size);\n-\tif (data)\n+\tif (data) {\n+\t\tunlock_if_threaded(&read_sha1_mutex);\n \t\treturn data;\n+\t}\n \n \tif (errno && errno != ENOENT)\n \t\tdie_errno(\"failed to read object %s\", sha1_to_hex(sha1));\n@@ -2263,6 +2270,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \t\tdie(\"packed object %s (stored in %s) is corrupt\",\n \t\t    sha1_to_hex(repl), p->pack_name);\n \n+\tunlock_if_threaded(&read_sha1_mutex);\n \treturn NULL;\n }\n \ndiff --git a/thread-utils.c b/thread-utils.c\nindex fb75a29..70af3f9 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -62,6 +62,17 @@ int init_recursive_mutex(pthread_mutex_t *m)\n \treturn ret;\n }\n \n+pthread_mutex_t read_sha1_mutex;\n+void init_subsystem_locks(void)\n+{\n+\tpthread_mutex_init(&read_sha1_mutex, NULL);\n+}\n+\n+void destroy_subsystem_locks(void)\n+{\n+\tpthread_mutex_destroy(&read_sha1_mutex);\n+}\n+\n #ifndef NO_PTHREADS\n void lock_if_threaded(pthread_mutex_t *m)\n {\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 9a780a2..3906753 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -17,10 +17,16 @@\n extern void lock_if_threaded(pthread_mutex_t*);\n extern void unlock_if_threaded(pthread_mutex_t*);\n \n+extern pthread_mutex_t read_sha1_mutex;\n+extern void init_subsystem_locks(void);\n+extern void destroy_subsystem_locks(void);\n+\n #else\n \n #define lock_if_threaded(lock)\n #define unlock_if_threaded(lock)\n+#define init_subsystem_locks()\n+#define destroy_subsystem_locks()\n \n #endif\n \n-- \n1.7.8.431.g2abf2\n"},{"id":"180638","messageId":"bdaa6fad1f9ca46e9e1059bc1d6167732a1e9896.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"[POC PATCH 3/5] sha1_file_name_buf(): sha1_file_name in caller's buffer","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:35Z","receivedAt":"2011-12-09T08:39:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"sha1_file_name is non-reentrant because of its use of a static buffer.\nSplit it into just the buffer writing (which can be called even from\nthreads as long as the buffer is stack'd) and a small wrapper that\nuses the static buffer as before.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n sha1_file.c |   29 +++++++++++++++++++----------\n 1 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c3595b3..18648c3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -153,18 +153,11 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)\n }\n \n /*\n- * NOTE! This returns a statically allocated buffer, so you have to be\n- * careful about using it. Do an \"xstrdup()\" if you need to save the\n- * filename.\n- *\n- * Also note that this returns the location for creating.  Reading\n- * SHA1 file can happen from any alternate directory listed in the\n- * DB_ENVIRONMENT environment variable if it is not found in\n- * the primary object database.\n+ * Similar to sha1_file_name but you provide a buffer of size at least\n+ * PATH_MAX.\n  */\n-char *sha1_file_name(const unsigned char *sha1)\n+void sha1_file_name_buf(char *buf, const unsigned char *sha1)\n {\n-\tstatic char buf[PATH_MAX];\n \tconst char *objdir;\n \tint len;\n \n@@ -179,6 +172,22 @@ char *sha1_file_name(const unsigned char *sha1)\n \tbuf[len+3] = '/';\n \tbuf[len+42] = '\\0';\n \tfill_sha1_path(buf + len + 1, sha1);\n+}\n+\n+/*\n+ * NOTE! This returns a statically allocated buffer, so you have to be\n+ * careful about using it. Do an \"xstrdup()\" if you need to save the\n+ * filename.\n+ *\n+ * Also note that this returns the location for creating.  Reading\n+ * SHA1 file can happen from any alternate directory listed in the\n+ * DB_ENVIRONMENT environment variable if it is not found in\n+ * the primary object database.\n+ */\n+char *sha1_file_name(const unsigned char *sha1)\n+{\n+\tstatic char buf[PATH_MAX];\n+\tsha1_file_name_buf(buf, sha1);\n \treturn buf;\n }\n \n-- \n1.7.8.431.g2abf2\n"},{"id":"180641","messageId":"21a0f7e2a7111bf09d34dfc76e8d1308cea6a7ac.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"[POC PATCH 4/5] sha1_file: stuff various pack reading variables into a struct","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:36Z","receivedAt":"2011-12-09T08:39:36Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"In preparation for making these variables thread-local, put various\ndelta-cache related bits of pack reading state into a struct.  For now\nthe accessor function is a dummy that always returns a static instance\nof this struct.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n sha1_file.c |   99 ++++++++++++++++++++++++++++++++++++++++-------------------\n 1 files changed, 67 insertions(+), 32 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 18648c3..7c367f9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1655,21 +1655,50 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \n #define MAX_DELTA_CACHE (256)\n \n-static size_t delta_base_cached;\n-\n-static struct delta_base_cache_lru_list {\n+struct delta_base_cache_lru_list {\n \tstruct delta_base_cache_lru_list *prev;\n \tstruct delta_base_cache_lru_list *next;\n-} delta_base_cache_lru = { &delta_base_cache_lru, &delta_base_cache_lru };\n+};\n+\n+static struct delta_base_cache_lru_list main_delta_base_cache_lru = {\n+\t&main_delta_base_cache_lru, &main_delta_base_cache_lru\n+};\n \n-static struct delta_base_cache_entry {\n+struct delta_base_cache_entry {\n \tstruct delta_base_cache_lru_list lru;\n \tvoid *data;\n \tstruct packed_git *p;\n \toff_t base_offset;\n \tunsigned long size;\n \tenum object_type type;\n-} delta_base_cache[MAX_DELTA_CACHE];\n+};\n+\n+static struct delta_base_cache_entry main_delta_base_cache[MAX_DELTA_CACHE];\n+\n+struct pack_context {\n+\tsize_t delta_base_cached;\n+\tstruct delta_base_cache_entry *delta_base_cache;\n+\tstruct delta_base_cache_lru_list *delta_base_cache_lru;\n+\tstruct packed_git *last_found;\n+};\n+\n+static struct pack_context main_pack_context = {\n+\t0, main_delta_base_cache, &main_delta_base_cache_lru, (void*)1\n+};\n+\n+static struct pack_context *pack_context_alloc(void)\n+{\n+\tstruct pack_context *ctx = xmalloc(sizeof(struct pack_context));\n+\tctx->delta_base_cached = 0;\n+\tctx->delta_base_cache_lru = xmalloc(sizeof(struct delta_base_cache_lru_list));\n+\tctx->delta_base_cache_lru->prev = ctx->delta_base_cache_lru;\n+\tctx->delta_base_cache_lru->next = ctx->delta_base_cache_lru;\n+\tctx->delta_base_cache = xcalloc(MAX_DELTA_CACHE, sizeof(struct delta_base_cache_entry));\n+\tctx->last_found = (void*)1;\n+\treturn ctx;\n+}\n+\n+#define get_thread_pack_context() (&main_pack_context)\n \n static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)\n {\n@@ -1683,7 +1712,8 @@ static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)\n static int in_delta_base_cache(struct packed_git *p, off_t base_offset)\n {\n \tunsigned long hash = pack_entry_hash(p, base_offset);\n-\tstruct delta_base_cache_entry *ent = delta_base_cache + hash;\n+\tstruct delta_base_cache_entry *ent\n+\t\t= get_thread_pack_context()->delta_base_cache + hash;\n \treturn (ent->data && ent->p == p && ent->base_offset == base_offset);\n }\n \n@@ -1692,7 +1722,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,\n {\n \tvoid *ret;\n \tunsigned long hash = pack_entry_hash(p, base_offset);\n-\tstruct delta_base_cache_entry *ent = delta_base_cache + hash;\n+\tstruct pack_context *ctx = get_thread_pack_context();\n+\tstruct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;\n \n \tret = ent->data;\n \tif (!ret || ent->p != p || ent->base_offset != base_offset)\n@@ -1702,7 +1733,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,\n \t\tent->data = NULL;\n \t\tent->lru.next->prev = ent->lru.prev;\n \t\tent->lru.prev->next = ent->lru.next;\n-\t\tdelta_base_cached -= ent->size;\n+\t\tctx->delta_base_cached -= ent->size;\n \t} else {\n \t\tret = xmemdupz(ent->data, ent->size);\n \t}\n@@ -1711,48 +1742,52 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,\n \treturn ret;\n }\n \n-static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)\n+static inline void release_delta_base_cache(struct pack_context *ctx,\n+\t\t\t\t\t    struct delta_base_cache_entry *ent)\n {\n \tif (ent->data) {\n \t\tfree(ent->data);\n \t\tent->data = NULL;\n \t\tent->lru.next->prev = ent->lru.prev;\n \t\tent->lru.prev->next = ent->lru.next;\n-\t\tdelta_base_cached -= ent->size;\n+\t\tctx->delta_base_cached -= ent->size;\n \t}\n }\n \n void clear_delta_base_cache(void)\n {\n \tunsigned long p;\n+\tstruct pack_context *ctx = get_thread_pack_context();\n+\tstruct delta_base_cache_entry *delta_base_cache = ctx->delta_base_cache;\n \tfor (p = 0; p < MAX_DELTA_CACHE; p++)\n-\t\trelease_delta_base_cache(&delta_base_cache[p]);\n+\t\trelease_delta_base_cache(ctx, &delta_base_cache[p]);\n }\n \n static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tvoid *base, unsigned long base_size, enum object_type type)\n {\n \tunsigned long hash = pack_entry_hash(p, base_offset);\n-\tstruct delta_base_cache_entry *ent = delta_base_cache + hash;\n+\tstruct pack_context *ctx = get_thread_pack_context();\n+\tstruct delta_base_cache_entry *ent = ctx->delta_base_cache + hash;\n \tstruct delta_base_cache_lru_list *lru;\n \n-\trelease_delta_base_cache(ent);\n-\tdelta_base_cached += base_size;\n+\trelease_delta_base_cache(ctx, ent);\n+\tctx->delta_base_cached += base_size;\n \n-\tfor (lru = delta_base_cache_lru.next;\n-\t     delta_base_cached > delta_base_cache_limit\n-\t     && lru != &delta_base_cache_lru;\n+\tfor (lru = ctx->delta_base_cache_lru->next;\n+\t     ctx->delta_base_cached > delta_base_cache_limit\n+\t     && lru != ctx->delta_base_cache_lru;\n \t     lru = lru->next) {\n \t\tstruct delta_base_cache_entry *f = (void *)lru;\n \t\tif (f->type == OBJ_BLOB)\n-\t\t\trelease_delta_base_cache(f);\n+\t\t\trelease_delta_base_cache(ctx, f);\n \t}\n-\tfor (lru = delta_base_cache_lru.next;\n-\t     delta_base_cached > delta_base_cache_limit\n-\t     && lru != &delta_base_cache_lru;\n+\tfor (lru = ctx->delta_base_cache_lru->next;\n+\t     ctx->delta_base_cached > delta_base_cache_limit\n+\t     && lru != ctx->delta_base_cache_lru;\n \t     lru = lru->next) {\n \t\tstruct delta_base_cache_entry *f = (void *)lru;\n-\t\trelease_delta_base_cache(f);\n+\t\trelease_delta_base_cache(ctx, f);\n \t}\n \n \tent->p = p;\n@@ -1760,10 +1795,10 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tent->type = type;\n \tent->data = base;\n \tent->size = base_size;\n-\tent->lru.next = &delta_base_cache_lru;\n-\tent->lru.prev = delta_base_cache_lru.prev;\n-\tdelta_base_cache_lru.prev->next = &ent->lru;\n-\tdelta_base_cache_lru.prev = &ent->lru;\n+\tent->lru.next = ctx->delta_base_cache_lru;\n+\tent->lru.prev = ctx->delta_base_cache_lru->prev;\n+\tctx->delta_base_cache_lru->prev->next = &ent->lru;\n+\tctx->delta_base_cache_lru->prev = &ent->lru;\n }\n \n static void *read_object(const unsigned char *sha1, enum object_type *type,\n@@ -2021,14 +2056,14 @@ int is_pack_valid(struct packed_git *p)\n \n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \toff_t offset;\n+\tstruct pack_context *ctx = get_thread_pack_context();\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = (last_found == (void *)1) ? packed_git : last_found;\n+\tp = (ctx->last_found == (void *)1) ? packed_git : ctx->last_found;\n \n \tdo {\n \t\tif (p->num_bad_objects) {\n@@ -2055,16 +2090,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\te->offset = offset;\n \t\t\te->p = p;\n \t\t\thashcpy(e->sha1, sha1);\n-\t\t\tlast_found = p;\n+\t\t\tctx->last_found = p;\n \t\t\treturn 1;\n \t\t}\n \n \t\tnext:\n-\t\tif (p == last_found)\n+\t\tif (p == ctx->last_found)\n \t\t\tp = packed_git;\n \t\telse\n \t\t\tp = p->next;\n-\t\tif (p == last_found)\n+\t\tif (p == ctx->last_found)\n \t\t\tp = p->next;\n \t} while (p);\n \treturn 0;\n-- \n1.7.8.431.g2abf2\n"},{"id":"180640","messageId":"dd2bf650b382f5aca727b7d93a48598fb1a2f7d9.1323419666.git.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"[POC PATCH 5/5] sha1_file: make the pack machinery thread-safe","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:39:37Z","receivedAt":"2011-12-09T08:39:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"More precisely speaking, this pushes the locking down from\nread_object() into bits of the pack machinery that cannot (yet) run in\nparallel.\n\nThere are several hacks here:\n\na) prepare_packed_git() must be called before any parallel accesses\n   happen.  It now unconditionally opens and maps all index files.\n\nb) similarly, prepare_replace_object() must be called before any\n   parallel read_sha1_file() happens\n\nThis simplification lets us avoid locking outright to guard the index\naccesses; locking is then mainly required for open_packed_git(),\n[un]use_pack(), and such.\n\nThe ultimate goal would of course be to let at least _some_ pack\naccesses happen without any locking whatsoever.  But grep already\nbenefits from it with a nice speed boost on non-worktree greps.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/grep.c   |    9 ++++++\n cache.h          |    1 +\n replace_object.c |    5 ++-\n sha1_file.c      |   81 +++++++++++++++++++++++++++++++++++++++++++++++------\n thread-utils.c   |    9 ++++--\n thread-utils.h   |    3 +-\n 6 files changed, 93 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 6c5bdfa..212497d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -980,6 +980,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n \t\t    opt.funcbody)\n \t\t\tskip_first_line = 1;\n+\t\t/*\n+\t\t * This does the non-threadsafe work early.  FIXME:\n+\t\t * grep shouldn't have to know about this mess.\n+\t\t */\n+\t\tuse_threads = 0;\n+\t\tprepare_replace_object();\n+\t\tprepare_packed_git();\n+\t\tuse_threads = 1;\n+\n \t\tstart_threads(&opt);\n \t}\n #endif\ndiff --git a/cache.h b/cache.h\nindex 8c98d05..379dd44 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -764,6 +764,7 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh\n \t\treturn sha1;\n \treturn do_lookup_replace_object(sha1);\n }\n+extern void prepare_replace_object(void);\n \n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern int sha1_object_info(const unsigned char *, unsigned long *);\ndiff --git a/replace_object.c b/replace_object.c\nindex d0b1548..b303392 100644\n--- a/replace_object.c\n+++ b/replace_object.c\n@@ -2,6 +2,7 @@\n #include \"sha1-lookup.h\"\n #include \"refs.h\"\n #include \"commit.h\"\n+#include \"thread-utils.h\"\n \n static struct replace_object {\n \tunsigned char sha1[2][20];\n@@ -76,13 +77,15 @@ static int register_replace_ref(const char *refname,\n \treturn 0;\n }\n \n-static void prepare_replace_object(void)\n+void prepare_replace_object(void)\n {\n \tstatic int replace_object_prepared;\n \n \tif (replace_object_prepared)\n \t\treturn;\n \n+\tassert(!use_threads);\n+\n \tfor_each_replace_ref(register_replace_ref, NULL);\n \treplace_object_prepared = 1;\n \tif (!replace_object_nr)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7c367f9..b61692e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -429,7 +429,8 @@ void prepare_alt_odb(void)\n \n static int has_loose_object_local(const unsigned char *sha1)\n {\n-\tchar *name = sha1_file_name(sha1);\n+\tchar name[PATH_MAX];\n+\tsha1_file_name_buf(name, sha1);\n \treturn !access(name, F_OK);\n }\n \n@@ -650,9 +651,12 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \n void release_pack_memory(size_t need, int fd)\n {\n-\tsize_t cur = pack_mapped;\n+\tsize_t cur;\n+\tlock_if_threaded(&pack_access_mutex);\n+\tcur = pack_mapped;\n \twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))\n \t\t; /* nothing */\n+\tunlock_if_threaded(&pack_access_mutex);\n }\n \n void *xmmap(void *start, size_t length,\n@@ -689,9 +693,12 @@ void close_pack_windows(struct packed_git *p)\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n+\n \tif (w) {\n+\t\tlock_if_threaded(&pack_access_mutex);\n \t\tw->inuse_cnt--;\n \t\t*w_cursor = NULL;\n+\t\tunlock_if_threaded(&pack_access_mutex);\n \t}\n }\n \n@@ -712,10 +719,13 @@ void close_pack_index(struct packed_git *p)\n  * must subsist at this point.  If ever objects from this pack are requested\n  * again, the new version of the pack will be reinitialized through\n  * reprepare_packed_git().\n+ *\n+ * NOT THREAD-SAFE\n  */\n void free_pack_by_name(const char *pack_name)\n {\n \tstruct packed_git *p, **pp = &packed_git;\n+\tassert(!use_threads);\n \n \twhile (*pp) {\n \t\tp = *pp;\n@@ -821,13 +831,33 @@ static int open_packed_git_1(struct packed_git *p)\n \n static int open_packed_git(struct packed_git *p)\n {\n-\tif (!open_packed_git_1(p))\n+\tlock_if_threaded(&pack_access_mutex);\n+\t/*\n+\t * is_pack_valid() took the easy route and did not\n+\t * lock.  This is probably okay; if the pack was\n+\t * *ever* open, it was valid unless another process is\n+\t * actively trying to corrupt it, in which case:\n+\t * meh.\n+\t *\n+\t * However, a concurrent open_packed_git() may already have\n+\t * opened it before we get here.  So we test again in a locked\n+\t * section.  If it beat us to it, then we have no work left to\n+\t * do.\n+\t */\n+\tif (p->pack_fd != -1) {\n+\t\tunlock_if_threaded(&pack_access_mutex);\n \t\treturn 0;\n+\t}\n+\tif (!open_packed_git_1(p)) {\n+\t\tunlock_if_threaded(&pack_access_mutex);\n+\t\treturn 0;\n+\t}\n \tif (p->pack_fd != -1) {\n \t\tclose(p->pack_fd);\n \t\tpack_open_fds--;\n \t\tp->pack_fd = -1;\n \t}\n+\tunlock_if_threaded(&pack_access_mutex);\n \treturn -1;\n }\n \n@@ -858,6 +888,9 @@ unsigned char *use_pack(struct packed_git *p,\n \t */\n \tif (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))\n \t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n+\n+\tlock_if_threaded(&pack_access_mutex);\n+\n \tif (offset > (p->pack_size - 20))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n \n@@ -916,6 +949,9 @@ unsigned char *use_pack(struct packed_git *p,\n \toffset -= win->offset;\n \tif (left)\n \t\t*left = win->len - xsize_t(offset);\n+\n+\tunlock_if_threaded(&pack_access_mutex);\n+\n \treturn win->base + offset;\n }\n \n@@ -1044,6 +1080,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tif (!p)\n \t\t\tcontinue;\n \t\tinstall_packed_git(p);\n+\t\topen_pack_index(p);\n \t}\n \tclosedir(dir);\n }\n@@ -1102,6 +1139,12 @@ static void rearrange_packed_git(void)\n \tfree(ary);\n }\n \n+/*\n+ * NOT THREAD-SAFE\n+ *\n+ * However, it's ok if you run this early, before starting threads,\n+ * and then use the pack machinery from threads.\n+ */\n static int prepare_packed_git_run_once = 0;\n void prepare_packed_git(void)\n {\n@@ -1109,6 +1152,7 @@ void prepare_packed_git(void)\n \n \tif (prepare_packed_git_run_once)\n \t\treturn;\n+\tassert (!use_threads);\n \tprepare_packed_git_one(get_object_directory(), 1);\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n@@ -1180,8 +1224,10 @@ static int git_open_noatime(const char *name)\n static int open_sha1_file(const unsigned char *sha1)\n {\n \tint fd;\n-\tchar *name = sha1_file_name(sha1);\n+\tchar namebuf[PATH_MAX];\n+\tchar *name = namebuf;\n \tstruct alternate_object_database *alt;\n+\tsha1_file_name_buf(name, sha1);\n \n \tfd = git_open_noatime(name);\n \tif (fd >= 0)\n@@ -1698,7 +1744,22 @@ static struct pack_context *pack_context_alloc(void)\n \treturn ctx;\n }\n \n+#ifdef NO_PTHREADS\n #define get_thread_pack_context() (&main_pack_context)\n+#else\n+static struct pack_context *get_thread_pack_context(void)\n+{\n+\tstruct pack_context *ctx;\n+\tif (!use_threads)\n+\t\treturn &main_pack_context;\n+\tctx = pthread_getspecific(pack_context_key);\n+\tif (ctx)\n+\t\treturn ctx;\n+\tctx = pack_context_alloc();\n+\tpthread_setspecific(pack_context_key, ctx);\n+\treturn ctx;\n+}\n+#endif\n \n static unsigned long pack_entry_hash(struct packed_git *p, off_t base_offset)\n {\n@@ -2219,6 +2280,10 @@ static void *read_packed_sha1(const unsigned char *sha1,\n \treturn data;\n }\n \n+/*\n+ * WARNING: must never be called concurrently with read_sha1_file and\n+ * friends!  They do lookups in the cached_objects without locking.\n+ */\n int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,\n \t\t      unsigned char *sha1)\n {\n@@ -2280,19 +2345,15 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \t\t\t      unsigned flag)\n {\n \tvoid *data;\n-\tchar *path;\n \tconst struct packed_git *p;\n \tconst unsigned char *repl;\n \n-\tlock_if_threaded(&read_sha1_mutex);\n-\n \trepl = (flag & READ_SHA1_FILE_REPLACE)\n \t\t? lookup_replace_object(sha1) : sha1;\n \n \terrno = 0;\n \tdata = read_object(repl, type, size);\n \tif (data) {\n-\t\tunlock_if_threaded(&read_sha1_mutex);\n \t\treturn data;\n \t}\n \n@@ -2305,7 +2366,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \t\t    sha1_to_hex(repl), sha1_to_hex(sha1));\n \n \tif (has_loose_object(repl)) {\n-\t\tpath = sha1_file_name(sha1);\n+\t\tchar path[PATH_MAX];\n+\t\tsha1_file_name_buf(path, sha1);\n \t\tdie(\"loose object %s (stored in %s) is corrupt\",\n \t\t    sha1_to_hex(repl), path);\n \t}\n@@ -2314,7 +2376,6 @@ void *read_sha1_file_extended(const unsigned char *sha1,\n \t\tdie(\"packed object %s (stored in %s) is corrupt\",\n \t\t    sha1_to_hex(repl), p->pack_name);\n \n-\tunlock_if_threaded(&read_sha1_mutex);\n \treturn NULL;\n }\n \ndiff --git a/thread-utils.c b/thread-utils.c\nindex 70af3f9..0da2b65 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -62,15 +62,18 @@ int init_recursive_mutex(pthread_mutex_t *m)\n \treturn ret;\n }\n \n-pthread_mutex_t read_sha1_mutex;\n+pthread_mutex_t pack_access_mutex;\n+pthread_key_t pack_context_key;\n void init_subsystem_locks(void)\n {\n-\tpthread_mutex_init(&read_sha1_mutex, NULL);\n+\tinit_recursive_mutex(&pack_access_mutex);\n+\tpthread_key_create(&pack_context_key, NULL);\n }\n \n void destroy_subsystem_locks(void)\n {\n-\tpthread_mutex_destroy(&read_sha1_mutex);\n+\tpthread_mutex_destroy(&pack_access_mutex);\n+\tpthread_key_delete(pack_context_key);\n }\n \n #ifndef NO_PTHREADS\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 3906753..7d3cc0a 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -17,7 +17,8 @@\n extern void lock_if_threaded(pthread_mutex_t*);\n extern void unlock_if_threaded(pthread_mutex_t*);\n \n-extern pthread_mutex_t read_sha1_mutex;\n+extern pthread_mutex_t pack_access_mutex;\n+extern pthread_key_t pack_context_key;\n extern void init_subsystem_locks(void);\n extern void destroy_subsystem_locks(void);\n \n-- \n1.7.8.431.g2abf2\n"},{"id":"180643","messageId":"201112090945.26367.trast@student.ethz.ch","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"Re: [POC PATCH 0/5] Threaded loose object and pack access","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-09T08:45:26Z","receivedAt":"2011-12-09T08:45:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n> Well, just to make sure we're all left in a confused mess of partly\n> conflicting patches, here's another angle on the same thing:\n\nBleh, obviously that was intended to be a reply to\n\n  http://thread.gmane.org/gmane.comp.version-control.git/185932/focus=186231\n\nand CC'd to Peff.\n\nSorry for the mess.  I'll go sulking with a few cups of coffee.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180803","messageId":"CACsJy8DxupdnKturE1Ows6+OcC22wPer0=Q56g+P93mP_+BKZA@mail.gmail.com","threadId":"29114","inReplyTo":"cover.1323419666.git.trast@student.ethz.ch","subject":"Re: [POC PATCH 0/5] Threaded loose object and pack access","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-10T15:51:40Z","receivedAt":"2011-12-10T15:51:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 9, 2011 at 3:39 PM, Thomas Rast <trast@student.ethz.ch> wrote:\n> Well, just to make sure we're all left in a confused mess of partly\n> conflicting patches, here's another angle on the same thing:\n>\n> Jeff King wrote:\n>> Wow, that's horrible. Leaving aside the parallelism, it's just terrible\n>> that reading from the cache is 20 times slower than the worktree. I get\n>> similar results on my quad-core machine.\n>\n> By poking around in sha1_file.c I got that down to about 10.  It's not\n> great yet, but it seems a start.\n>\n> The goal would be to improve it to the point where a patch lookup that\n> already has all relevant packs open and windows mapped can proceed\n> without locking.  I'm not sure that's doable short of duplicating the\n> whole pack state (including fds and windows) across threads, but I'll\n> give it some more thought before going that route.\n\nAnother potential user for parallel pack access is fsck. Although fsck\naccess pattern may be different from grep, fsck would open and read\nthrough all packs.\n-- \nDuy\n"},{"id":"188796","messageId":"CACsJy8AyphUD-vFwwgaW0eWG3ekgHA+tcAwV2zk5YGorkW0TzQ@mail.gmail.com","threadId":"29114","inReplyTo":"dd2bf650b382f5aca727b7d93a48598fb1a2f7d9.1323419666.git.trast@student.ethz.ch","subject":"Re: [POC PATCH 5/5] sha1_file: make the pack machinery thread-safe","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-04-09T14:43:56Z","receivedAt":"2012-04-09T14:43:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 9, 2011 at 3:39 PM, Thomas Rast <trast@student.ethz.ch> wrote:\n> More precisely speaking, this pushes the locking down from\n> read_object() into bits of the pack machinery that cannot (yet) run in\n> parallel.\n>\n> There are several hacks here:\n>\n> a) prepare_packed_git() must be called before any parallel accesses\n>   happen.  It now unconditionally opens and maps all index files.\n>\n> b) similarly, prepare_replace_object() must be called before any\n>   parallel read_sha1_file() happens\n>\n> This simplification lets us avoid locking outright to guard the index\n> accesses; locking is then mainly required for open_packed_git(),\n> [un]use_pack(), and such.\n>\n> The ultimate goal would of course be to let at least _some_ pack\n> accesses happen without any locking whatsoever.  But grep already\n> benefits from it with a nice speed boost on non-worktree greps.\n\n(I'm running into multithread pack access problem in rev-list..)\n\nWhy not put the global pointer \"struct packed_git *packed_git\" to\n\"struct pack_context\" and avoid locking entirely? Resource usage is\nlike we run <n> different processes, I think, which is not too bad. We\nmay want to share a few static pack_* variables such as\npack_open_fds.. to avoid hitting system limits too fast.\n-- \nDuy\n"},{"id":"188859","messageId":"8762d7zs67.fsf@thomas.inf.ethz.ch","threadId":"29114","inReplyTo":"CACsJy8AyphUD-vFwwgaW0eWG3ekgHA+tcAwV2zk5YGorkW0TzQ@mail.gmail.com","subject":"Re: [POC PATCH 5/5] sha1_file: make the pack machinery thread-safe","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-04-10T12:29:20Z","receivedAt":"2012-04-10T12:29:20Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Fri, Dec 9, 2011 at 3:39 PM, Thomas Rast <trast@student.ethz.ch> wrote:\n>> More precisely speaking, this pushes the locking down from\n>> read_object() into bits of the pack machinery that cannot (yet) run in\n>> parallel.\n>>\n>> There are several hacks here:\n>>\n>> a) prepare_packed_git() must be called before any parallel accesses\n>>   happen.  It now unconditionally opens and maps all index files.\n>>\n>> b) similarly, prepare_replace_object() must be called before any\n>>   parallel read_sha1_file() happens\n>>\n>> This simplification lets us avoid locking outright to guard the index\n>> accesses; locking is then mainly required for open_packed_git(),\n>> [un]use_pack(), and such.\n>>\n>> The ultimate goal would of course be to let at least _some_ pack\n>> accesses happen without any locking whatsoever.  But grep already\n>> benefits from it with a nice speed boost on non-worktree greps.\n>\n> (I'm running into multithread pack access problem in rev-list..)\n>\n> Why not put the global pointer \"struct packed_git *packed_git\" to\n> \"struct pack_context\" and avoid locking entirely? Resource usage is\n> like we run <n> different processes, I think, which is not too bad. We\n> may want to share a few static pack_* variables such as\n> pack_open_fds.. to avoid hitting system limits too fast.\n\nI was hesitating to do that because I think it's not the best solution\nyet.  At least for 64bit systems, I thought of doing some or all of:\n\n* opening/mapping the pack indexes immediately to avoid locking there\n  (perhaps the POC already does this, I haven't looked again).  If you\n  have many packs this isn't cheap because the index must be verified.\n\n* mapping small packs immediately\n\n* mapping \"the\" big pack immediately (many repos will have a huge pack\n  from the initial clone)\n\nPut another way, my current concern is that on 64bit systems it's\nincredibly easy to share (who cares about a few GBs of mmap()?), whereas\non 32bit systems it probably matters much more, but there we also suffer\nmore from not sharing.\n\nAm I making any sense?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"188860","messageId":"CACsJy8CwJ-sfj2yTqp+LJYt1bhsDqQ21ob_hp9iZRV82cF=sPg@mail.gmail.com","threadId":"29114","inReplyTo":"8762d7zs67.fsf@thomas.inf.ethz.ch","subject":"Re: [POC PATCH 5/5] sha1_file: make the pack machinery thread-safe","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-04-10T13:39:10Z","receivedAt":"2012-04-10T13:39:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 10, 2012 at 7:29 PM, Thomas Rast <trast@inf.ethz.ch> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> On Fri, Dec 9, 2011 at 3:39 PM, Thomas Rast <trast@student.ethz.ch> wrote:\n>>> More precisely speaking, this pushes the locking down from\n>>> read_object() into bits of the pack machinery that cannot (yet) run in\n>>> parallel.\n>>>\n>>> There are several hacks here:\n>>>\n>>> a) prepare_packed_git() must be called before any parallel accesses\n>>>   happen.  It now unconditionally opens and maps all index files.\n>>>\n>>> b) similarly, prepare_replace_object() must be called before any\n>>>   parallel read_sha1_file() happens\n>>>\n>>> This simplification lets us avoid locking outright to guard the index\n>>> accesses; locking is then mainly required for open_packed_git(),\n>>> [un]use_pack(), and such.\n>>>\n>>> The ultimate goal would of course be to let at least _some_ pack\n>>> accesses happen without any locking whatsoever.  But grep already\n>>> benefits from it with a nice speed boost on non-worktree greps.\n>>\n>> (I'm running into multithread pack access problem in rev-list..)\n>>\n>> Why not put the global pointer \"struct packed_git *packed_git\" to\n>> \"struct pack_context\" and avoid locking entirely? Resource usage is\n>> like we run <n> different processes, I think, which is not too bad. We\n>> may want to share a few static pack_* variables such as\n>> pack_open_fds.. to avoid hitting system limits too fast.\n>\n> I was hesitating to do that because I think it's not the best solution\n> yet.  At least for 64bit systems, I thought of doing some or all of:\n>\n> * opening/mapping the pack indexes immediately to avoid locking there\n>  (perhaps the POC already does this, I haven't looked again).  If you\n>  have many packs this isn't cheap because the index must be verified.\n\nSharing mmapped pack indexes makes sense. We do full mmap there so it\neats address space in 32 bit systems (but still not a lot, linux-2.6\npack index is about 60MB).\n\nI tried but could not find the index verifying code (i.e. recalculate\nsha-1 and match with stored one) anywhere, so I suppose opening packs\nand indexes is cheap.\n\n> * mapping small packs immediately\n\nWe need to partition file handle space to avoid running out of file handles.\n\n> * mapping \"the\" big pack immediately (many repos will have a huge pack\n>  from the initial clone)\n\nWe have sliding pack windows exactly for that: accessing >4GB packs in\n32bit systems. So address space should not be an issue here.\n\n> Put another way, my current concern is that on 64bit systems it's\n> incredibly easy to share (who cares about a few GBs of mmap()?), whereas\n> on 32bit systems it probably matters much more, but there we also suffer\n> more from not sharing.\n\nHaving said all that, lock-free pack access does not work for me yet.\nI keep get crashes deep in cache_or_unpack_entry :(\n-- \nDuy\n"}]}