{"thread":{"id":"51621","subject":"[GSoC][PATCH 0/4] grep: re-enable threads when cached, w/ parallel inflation","startedAt":"2019-08-10T20:27:45Z","lastAt":"2020-01-30T13:28:07Z","messageCount":47,"participants":["Matheus Tavares","Jonathan Tan","Jeff King","Matheus Tavares Bernardino","Christian Couder","Victor Leschuk","SZEDER Gábor","Junio C Hamano","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"380269","messageId":"cover.1565468806.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":null,"subject":"[GSoC][PATCH 0/4] grep: re-enable threads when cached, w/ parallel inflation","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-10T20:27:26Z","receivedAt":"2019-08-10T20:27:45Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This series focuses on allowing parallel access to zlib inflation and\nusing that to perform a faster git-grep in the non-worktree case.\n\nThreads were disabled for this case at 53b8d93 (\"grep: disable\nthreading in non-worktree case\", 12-12-2011), due to performance drops.\n\nHowever, by allowing threads to perform inflation in parallel, we can\nregain the speedup. This is a good hotspot for parallelism as some test\ncases[1] showed that it can account for up to 48% of execution time.\nAnd besides that, inflation tasks are already independent of each other.\n\nAs a result, grepping 'abcd[02]' (\"Regex 1\") and\n'(static|extern) (int|double) \\*' (\"Regex 2\") at chromium's\nrepository[2], I got (means of 30 executions):\n\n Threads | Regex 1 | Regex 2\n ---------|------------|-----------\n 1 | 17.3557s | 20.8410s\n 2 | 9.7170s | 11.2415s\n 8 | 6.1723s | 6.9378s\n\nAs a reference, just enabling threads in the non-worktree case,\nwithout parallel inflation, I got:\n\n Threads | Regex 1 | Regex 2\n ---------|------------|-----------\n 1 | 17.1359s | 20.8306s\n 2 | 14.5036s | 15.4172s\n 8 | 13.6304s | 13.8659s\n\nFor now, the optimization is not supported when --textconv or\n--recurse-submodules are used, but I hope to send another patchset for\nthat still during GSoC. We may also try to allow even more parallelism,\nrefining the added 'obj_read_lock'.\n\n[1]: https://matheustavares.gitlab.io/posts/week-6-working-at-zlib-inflation#multithreading-zlib-inflation\n[2]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\ntravis build: https://travis-ci.org/matheustavares/git/builds/570255029\n\nMatheus Tavares (4):\n object-store: add lock to read_object_file_extended()\n grep: allow locks to be enabled individually\n grep: disable grep_read_mutex when possible\n grep: re-enable threads in some non-worktree cases\n\n Documentation/git-grep.txt | 12 ++++++++\n builtin/grep.c | 22 +++++++++++---\n grep.c | 4 +--\n grep.h | 8 +++--\n object-store.h | 4 +++\n packfile.c | 7 +++++\n sha1-file.c | 61 ++++++++++++++++++++++++++++++++++----\n 7 files changed, 105 insertions(+), 13 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"380270","messageId":"052de4c139bf4962182e6cb8f4aa315aa6130124.1565468806.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1565468806.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH 1/4] object-store: add lock to read_object_file_extended()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-10T20:27:27Z","receivedAt":"2019-08-10T20:28:04Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow read_object_file_extended() to be called by multiple threads\nprotecting it with a lock. The lock usage can be toggled with\nenable_obj_read_lock() and disable_obj_read_lock().\n\nProbably there are many spots in read_object_file_extended()'s call\nchain that could be executed unlocked (and thus, in parallel). But, for\nnow, we are only interested in allowing parallel access to zlib\ninflation. This is one of the sections where object reading spends most\nof the time and it's already thread-safe. So, to take advantage of that,\nthe lock is released when entering it and re-acquired right after. We\nmay refine the lock to also exploit other possible parallel spots in the\nfuture, but threaded zlib inflation should already give great speedups.\n\nNote that add_delta_base_cache() was also modified to skip adding\nalready present entries to the cache. This wasn't possible before, but\nnow it is since phase I and phase III of unpack_entry() may execute\nconcurrently.\n\nSigned-off-by: Matheus Tavares \n---\n object-store.h | 4 ++++\n packfile.c | 7 ++++++\n sha1-file.c | 61 +++++++++++++++++++++++++++++++++++++++++++++-----\n 3 files changed, 67 insertions(+), 5 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 7f7b3cdd80..cfc9484995 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -159,6 +159,10 @@ const char *loose_object_path(struct repository *r, struct strbuf *buf,\n void *map_loose_object(struct repository *r, const struct object_id *oid,\n \t\t unsigned long *size);\n \n+void enable_obj_read_lock(void);\n+void disable_obj_read_lock(void);\n+void obj_read_lock(void);\n+void obj_read_unlock(void);\n void *read_object_file_extended(struct repository *r,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tenum object_type *type,\ndiff --git a/packfile.c b/packfile.c\nindex fc43a6c52c..de93dc50e2 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1115,7 +1115,9 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1468,6 +1470,9 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tstruct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));\n \tstruct list_head *lru, *tmp;\n \n+\tif (get_delta_base_cache_entry(p, base_offset))\n+\t\treturn;\n+\n \tdelta_base_cached += base_size;\n \n \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n@@ -1597,7 +1602,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tif (!stream.avail_out)\n \t\t\tbreak; /* the payload is larger than it should be */\n \t\tcurpos += stream.next_in - in;\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 84fd02f107..f5ff51aedb 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1560,16 +1560,54 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n \treturn 0;\n }\n \n+static pthread_mutex_t obj_read_mutex;\n+static int obj_read_use_lock = 0;\n+\n+/*\n+ * Enabling the object read lock allows multiple threads to safely call the\n+ * following functions in parallel: repo_read_object_file(), read_object_file()\n+ * and read_object_file_extended().\n+ */\n+void enable_obj_read_lock(void)\n+{\n+\tif (obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 1;\n+\tpthread_mutex_init(&obj_read_mutex, NULL);\n+}\n+\n+void disable_obj_read_lock(void)\n+{\n+\tif (!obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 0;\n+\tpthread_mutex_destroy(&obj_read_mutex);\n+}\n+\n+void obj_read_lock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_lock(&obj_read_mutex);\n+}\n+\n+void obj_read_unlock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_unlock(&obj_read_mutex);\n+}\n+\n /*\n * This function dies on corrupt objects; the callers who want to\n * deal with them should arrange to call read_object() and give error\n * messages themselves.\n */\n-void *read_object_file_extended(struct repository *r,\n-\t\t\t\tconst struct object_id *oid,\n-\t\t\t\tenum object_type *type,\n-\t\t\t\tunsigned long *size,\n-\t\t\t\tint lookup_replace)\n+static void *do_read_object_file_extended(struct repository *r,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t enum object_type *type,\n+\t\t\t\t\t unsigned long *size,\n+\t\t\t\t\t int lookup_replace)\n {\n \tvoid *data;\n \tconst struct packed_git *p;\n@@ -1602,6 +1640,19 @@ void *read_object_file_extended(struct repository *r,\n \treturn NULL;\n }\n \n+void *read_object_file_extended(struct repository *r,\n+\t\t\t\tconst struct object_id *oid,\n+\t\t\t\tenum object_type *type,\n+\t\t\t\tunsigned long *size,\n+\t\t\t\tint lookup_replace)\n+{\n+\tvoid *data;\n+\tobj_read_lock();\n+\tdata = do_read_object_file_extended(r, oid, type, size, lookup_replace);\n+\tobj_read_unlock();\n+\treturn data;\n+}\n+\n void *read_object_with_reference(struct repository *r,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t const char *required_type_name,\n-- \n2.22.0\n\n"},{"id":"380271","messageId":"235de7de2874bd089b106be75121e1616308ed55.1565468806.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1565468806.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH 2/4] grep: allow locks to be enabled individually","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-10T20:27:28Z","receivedAt":"2019-08-10T20:28:04Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep has some internal locks to protect thread-unsafe operations\nwhen running with threads. The usage of these locks can be toggled\nthrough the variable 'grep_use_locks'. However, it's not currently\npossible to enable each lock individually. And since object reading has\nits own locks now, it is desirable to disable the respective grep lock\n(and only that) in cases where we can do so. To do that, transform\n'grep_use_locks' from a binary variable to a bitmask, which controls\neach lock individually.\n\nThe actual disabling of grep_read_lock, when possible, will be done in\nthe following patch.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 2 +-\n grep.c | 4 ++--\n grep.h | 8 ++++++--\n 3 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..a871bad8ad 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -205,7 +205,7 @@ static void start_threads(struct grep_opt *opt)\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n-\tgrep_use_locks = 1;\n+\tgrep_use_locks = GREP_USE_ALL_LOCKS;\n \n \tfor (i = 0; i < ARRAY_SIZE(todo); i++) {\n \t\tstrbuf_init(&todo[i].out, 0);\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..3aca0db435 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1523,13 +1523,13 @@ pthread_mutex_t grep_attr_mutex;\n \n static inline void grep_attr_lock(void)\n {\n-\tif (grep_use_locks)\n+\tif (grep_use_locks & GREP_USE_ATTR_LOCK)\n \t\tpthread_mutex_lock(&grep_attr_mutex);\n }\n \n static inline void grep_attr_unlock(void)\n {\n-\tif (grep_use_locks)\n+\tif (grep_use_locks & GREP_USE_ATTR_LOCK)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n \ndiff --git a/grep.h b/grep.h\nindex 1875880f37..02bffacfa2 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -229,6 +229,10 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs);\n struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n int grep_threads_ok(const struct grep_opt *opt);\n \n+#define GREP_USE_READ_LOCK (1 << 0)\n+#define GREP_USE_ATTR_LOCK (1 << 1)\n+#define GREP_USE_ALL_LOCKS (~0)\n+\n /*\n * Mutex used around access to the attributes machinery if\n * opt->use_threads. Must be initialized/destroyed by callers!\n@@ -239,13 +243,13 @@ extern pthread_mutex_t grep_read_mutex;\n \n static inline void grep_read_lock(void)\n {\n-\tif (grep_use_locks)\n+\tif (grep_use_locks & GREP_USE_READ_LOCK)\n \t\tpthread_mutex_lock(&grep_read_mutex);\n }\n \n static inline void grep_read_unlock(void)\n {\n-\tif (grep_use_locks)\n+\tif (grep_use_locks & GREP_USE_READ_LOCK)\n \t\tpthread_mutex_unlock(&grep_read_mutex);\n }\n \n-- \n2.22.0\n\n"},{"id":"380272","messageId":"d2e3f4eac24d26210f8962ebd82fd24a99c91fdf.1565468806.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1565468806.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH 3/4] grep: disable grep_read_mutex when possible","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-10T20:27:29Z","receivedAt":"2019-08-10T20:28:04Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep uses 'grep_read_mutex' to protect some object reading\noperations. But these have their own internal lock now, which ensure a\nbetter performance (with more parallel regions). So, disable the former\nwhen it's possible to use the latter, with enable_obj_read_lock().\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex a871bad8ad..fa51392222 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -205,7 +205,17 @@ static void start_threads(struct grep_opt *opt)\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n-\tgrep_use_locks = GREP_USE_ALL_LOCKS;\n+\tif (recurse_submodules || opt->allow_textconv) {\n+\t\t/*\n+\t\t * textconv and submodules' operations are not thread-safe yet\n+\t\t * so we must use grep_read_lock when grepping multithreaded\n+\t\t * with these options.\n+\t\t */\n+\t\tgrep_use_locks = GREP_USE_ALL_LOCKS;\n+\t} else {\n+\t\tgrep_use_locks = GREP_USE_ATTR_LOCK;\n+\t\tenable_obj_read_lock();\n+\t}\n \n \tfor (i = 0; i < ARRAY_SIZE(todo); i++) {\n \t\tstrbuf_init(&todo[i].out, 0);\n@@ -227,7 +237,7 @@ static void start_threads(struct grep_opt *opt)\n \t}\n }\n \n-static int wait_all(void)\n+static int wait_all(struct grep_opt *opt)\n {\n \tint hit = 0;\n \tint i;\n@@ -263,6 +273,9 @@ static int wait_all(void)\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n \tgrep_use_locks = 0;\n+\tif (!recurse_submodules && !opt->allow_textconv) {\n+\t\tdisable_obj_read_lock();\n+\t}\n \n \treturn hit;\n }\n@@ -1140,7 +1153,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (num_threads > 1)\n-\t\thit |= wait_all();\n+\t\thit |= wait_all(&opt);\n \tif (hit && show_in_pager)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n-- \n2.22.0\n\n"},{"id":"380273","messageId":"8c26abe9156e069ad4d19e9f0ce131cd1453f030.1565468806.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1565468806.git.matheus.bernardino@usp.br","subject":"[GSoC][PATCH 4/4] grep: re-enable threads in some non-worktree cases","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-10T20:27:30Z","receivedAt":"2019-08-10T20:28:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"They were disabled at 53b8d93 (\"grep: disable threading in non-worktree\ncase\", 12-12-2011), due to observable performance drops. But now that\nzlib inflation can be performed in parallel, for some of git-grep's\noptions, we can regain the speedup.\n\nGrepping 'abcd[02]' (\"Regex 1\") and '(static|extern) (int|double) \\*'\n(\"Regex 2\") at chromium's repository[1] I got:\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 17.3557s | 20.8410s\n 2 | 9.7170s | 11.2415s\n 8 | 6.1723s | 6.9378s\n\nThese are all means of 30 executions after 2 warmup runs. All tests were\nexecuted on an i7-7700HQ with 16GB of RAM and SSD. But to make sure the\noptimization also performs well on HDD, the tests were repeated on an\nAMD Turion 64 X2 TL-62 (dual-core) with 4GB of RAM and HDD (SATA-150,\n5400 rpm):\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 40.3347s | 47.6173s\n 2 | 27.6547s | 35.1797s\n\nUnfortunately, textconv and submodules' operations remain thread-unsafe,\nneeding locks to be safely executed when threaded. Because of that, it's\nnot currently worthy to grep in parallel with them. So, when --textconv\nor --recurse-submodules are given for a non-worktree case, threads are\nkept disabled. In order to clarify this behavior, let's also add a\n\"NOTES\" section to Documentation/git-grep.txt explaining the thread\nusage details.\n\n[1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/git-grep.txt | 12 ++++++++++++\n builtin/grep.c | 3 ++-\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 2d27969057..9686875fbc 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -330,6 +330,18 @@ EXAMPLES\n `git grep solution -- :^Documentation`::\n \tLooks for `solution`, excluding files in `Documentation`.\n \n+NOTES\n+-----\n+\n+The --threads option (and grep.threads configuration) will be ignored when\n+--open-files-in-pager is used, forcing a single-threaded execution.\n+\n+When grepping the index file (with --cached or giving tree objects), the\n+following options will also suppress thread creation:\n+\n+\t--recurse_submodules\n+\t--textconv\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex fa51392222..e5a9da471a 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1073,7 +1073,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\n \n-\tif (list.nr || cached || show_in_pager) {\n+\tif (show_in_pager ||\n+\t ((list.nr || cached) && (recurse_submodules || opt.allow_textconv))) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n \t\tnum_threads = 1;\n-- \n2.22.0\n\n"},{"id":"383112","messageId":"cover.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1565468806.git.matheus.bernardino@usp.br","subject":"[PATCH v2 00/11] grep: improve threading and fix race conditions","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:46Z","receivedAt":"2019-09-30T01:51:11Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This series focus on re-enabling threads at git-grep for the\nnon-worktree case. They are currently disabled due to being slower than\nsingle-threaded grep in this case. However, by allowing parallel zlib\ninflation when reading objects, speedups of up to 3x were observed.\n\nThe patchset also contains some fixes for race conditions found in the\nworktree git-grep and thread optimizations to hopefully increase\noverall performance.\n\nThis version was almost entirely re-written from scratch so I thought a\nrange-diff wouldn't be very useful. The major differences from the first\none are the race condition fixes and being able to run --textconv and\n--recurse-submodules threaded now.\n\nMatheus Tavares (11):\n grep: fix race conditions on userdiff calls\n grep: fix race conditions at grep_submodule()\n grep: fix racy calls in grep_objects()\n replace-object: make replace operations thread-safe\n object-store: allow threaded access to object reading\n grep: replace grep_read_mutex by internal obj read lock\n submodule-config: add skip_if_read option to repo_read_gitmodules()\n grep: allow submodule functions to run in parallel\n grep: protect packed_git [re-]initialization\n grep: re-enable threads in non-worktree case\n grep: move driver pre-load out of critical section\n\n .tsan-suppressions | 6 +++\n Documentation/git-grep.txt | 11 +++++\n builtin/grep.c | 90 +++++++++++++++++++-------------------\n grep.c | 32 ++++++++------\n grep.h | 13 ------\n object-store.h | 37 ++++++++++++++++\n object.c | 2 +\n packfile.c | 9 ++++\n replace-object.c | 11 ++++-\n replace-object.h | 7 ++-\n sha1-file.c | 57 +++++++++++++++++++++---\n submodule-config.c | 18 +++-----\n submodule-config.h | 2 +-\n unpack-trees.c | 4 +-\n 14 files changed, 205 insertions(+), 94 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"383113","messageId":"0f31cb0c126e824008d35d5cba52dd1c3c115c00.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 01/11] grep: fix race conditions on userdiff calls","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:47Z","receivedAt":"2019-09-30T01:51:16Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep uses an internal grep_read_mutex to protect object reading\noperations. Similarly, there's a grep_attr_mutex to protect calls to the\ngitattributes machinery. However, two of the three functions protected\nby the last mutex may also perform object reading, as seen bellow:\n\n- userdiff_get_textconv() > notes_cache_init() >\n notes_cache_match_validity() > lookup_commit_reference_gently() >\n parse_object() > repo_has_object_file() >\n repo_has_object_file_with_flags() > oid_object_info_extended()\n\n- userdiff_find_by_path() > git_check_attr() > collect_some_attrs() >\n prepare_attr_stack() > read_attr() > read_attr_from_index() >\n read_blob_data_from_index() > read_object_file()\n\nAs these calls are not protected by grep_read_mutex, there might be race\nconditions with other threads performing object reading (e.g. threads\ncalling fill_textconv() at grep.c:fill_textconv_grep()). To prevent\nthat, let's make sure to acquire the lock before both of these calls.\n\nNote: this patch might slow down the threaded grep in worktree, for the\nsake of thread-safeness. However, in the following patches we should\nregain performance by replacing grep_read_mutex for an internal object\nreading lock and allowing parallel inflation during object reading.\n\nSigned-off-by: Matheus Tavares \n---\n grep.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..b29946def2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1809,7 +1809,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t * is not thread-safe.\n \t\t */\n \t\tgrep_attr_lock();\n+\t\tgrep_read_lock();\n \t\ttextconv = userdiff_get_textconv(opt->repo, gs->driver);\n+\t\tgrep_read_unlock();\n \t\tgrep_attr_unlock();\n \t}\n \n@@ -2177,8 +2179,11 @@ void grep_source_load_driver(struct grep_source *gs,\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tif (gs->path)\n+\tif (gs->path) {\n+\t\tgrep_read_lock();\n \t\tgs->driver = userdiff_find_by_path(istate, gs->path);\n+\t\tgrep_read_unlock();\n+\t}\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\n-- \n2.23.0\n\n"},{"id":"383114","messageId":"be32683f1d59786550138169200d29bf67a822ca.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 02/11] grep: fix race conditions at grep_submodule()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:48Z","receivedAt":"2019-09-30T01:51:20Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"There're currently two function calls in builtin/grep.c:grep_submodule()\nwhich might result in race conditions:\n\n- submodule_from_path(): it has config_with_options() in its call stack\n which, in turn, may have read_object_file() in its own. Therefore,\n calling the first function without acquiring grep_read_mutex may end\n up causing a race condition with other object read operations\n performed by worker threads (for example, at the fill_textconv()\n call in grep.c:fill_textconv_grep()).\n- parse_object_or_die(): it falls into the same problem, having\n repo_has_object_file(the_repository, ...) in its call stack. Besides\n that, parse_object(), which is also called by parse_object_or_die(),\n is thread-unsafe and also called by object reading functions.\n\nIt's unlikely to really fall into a data race with these operations as\nthe volume of calls to them is usually very low. But we better protect\nourselves against this possibility, anyway. So, to solve these issues,\nmove both of these function calls into the critical section of\ngrep_read_mutex.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2699001fbd..626dbe554d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -407,8 +407,7 @@ static int grep_submodule(struct grep_opt *opt,\n {\n \tstruct repository subrepo;\n \tstruct repository *superproject = opt->repo;\n-\tconst struct submodule *sub = submodule_from_path(superproject,\n-\t\t\t\t\t\t\t &null_oid, path);\n+\tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n \tint hit;\n \n@@ -419,6 +418,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * object.\n \t */\n \tgrep_read_lock();\n+\tsub = submodule_from_path(superproject, &null_oid, path);\n \n \tif (!is_submodule_active(superproject, path)) {\n \t\tgrep_read_unlock();\n@@ -455,9 +455,8 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n-\n \t\tgrep_read_lock();\n+\t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n \t\tdata = read_object_with_reference(&subrepo,\n \t\t\t\t\t\t &object->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-- \n2.23.0\n\n"},{"id":"383115","messageId":"34aeb218bf266ac1a8fabcd9e8b307130d31eb0b.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 03/11] grep: fix racy calls in grep_objects()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:49Z","receivedAt":"2019-09-30T01:51:23Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"deref_tag() calls is_promisor_object() and parse_object(), both of which\nperform lazy initializations and other thread-unsafe operations. If it\nwas only called by grep_objects() this wouldn't be a problem as the\nlatter is only executed by the main thread. However, deref_tag() is also\npresent in read_object_file()'s call stack. So calling deref_tag() in\ngrep_objects() without acquiring the grep_read_mutex may incur in a race\ncondition with object reading operations (such as the ones internally\nperformed by fill_textconv(), called at fill_textconv_grep()). The same\nproblem happens with the call to gitmodules_config_oid() which also has\nparse_object() in its call stack. Fix that protecting both call with the\nsaid grep_read_mutex.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 626dbe554d..fa8b9996d1 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -658,13 +658,18 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n+\n+\t\tgrep_read_lock();\n \t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n \t\t\t\t NULL, 0);\n+\t\tgrep_read_unlock();\n \n \t\t/* load the gitmodules file for this rev */\n \t\tif (recurse_submodules) {\n \t\t\tsubmodule_free(opt->repo);\n+\t\t\tgrep_read_lock();\n \t\t\tgitmodules_config_oid(&real_obj->oid);\n+\t\t\tgrep_read_unlock();\n \t\t}\n \t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name,\n \t\t\t\tlist->objects[i].path)) {\n-- \n2.23.0\n\n"},{"id":"383116","messageId":"5deee3cf11e6f67c696617a9264395fb1ab04f73.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 04/11] replace-object: make replace operations thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:50Z","receivedAt":"2019-09-30T01:51:27Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"replace-object functions are very close to being thread-safe: the only\ncurrent racy section is the lazy initialization at\nprepare_replace_object(). The following patches will protect some object\nreading operations to be called threaded, but before that, replace\nfunctions must be protected. To do so, add a mutex to struct\nraw_object_store and acquire it before lazy initializing the\nreplace_map. This won't cause any noticeable performance drop as the\nmutex will no longer be used after the replace_map is initialized.\n\nLater, when the replace functions are called in parallel, thread\ndebuggers might point our use of the added replace_map_initialized flag\nas a data race. However, as this boolean variable is initialized as\nfalse and it's only updated once, there's no real harm. It's perfectly\nfine if the value is updated right after a thread read it in\nreplace-map.h:lookup_replace_object() (there'll only be a performance\npenalty for the affected threads at that moment). We could cease the\ndebugger warning protecting the variable reading at the said function.\nHowever, this would negatively affect performance for all threads\ncalling it, at any time, so it's not really worthy since the warning\ndoesn't represent a real problem. Instead, to make sure we don't get\nfalse positives (at ThreadSanitizer, at least) an entry for the\nrespective function is added to .tsan-suppressions.\n\nSigned-off-by: Matheus Tavares \n---\n .tsan-suppressions | 6 ++++++\n object-store.h | 2 ++\n object.c | 2 ++\n replace-object.c | 11 ++++++++++-\n replace-object.h | 7 ++++++-\n 5 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nindex 8c85014a0a..5ba86d6845 100644\n--- a/.tsan-suppressions\n+++ b/.tsan-suppressions\n@@ -8,3 +8,9 @@\n # in practice it (hopefully!) doesn't matter.\n race:^want_color$\n race:^transfer_debug$\n+\n+# A boolean value, which tells whether the replace_map has been initialized or\n+# not, is read racily with an update. As this variable is written to only once,\n+# and it's OK if the value change right after reading it, this shouldn't be a\n+# problem.\n+race:^lookup_replace_object$\ndiff --git a/object-store.h b/object-store.h\nindex 7f7b3cdd80..b22e20ad7d 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -110,6 +110,8 @@ struct raw_object_store {\n \t * (see git-replace(1)).\n \t */\n \tstruct oidmap *replace_map;\n+\tunsigned replace_map_initialized : 1;\n+\tpthread_mutex_t replace_mutex; /* protect object replace functions */\n \n \tstruct commit_graph *commit_graph;\n \tunsigned commit_graph_attempted : 1; /* if loading has been attempted */\ndiff --git a/object.c b/object.c\nindex 07bdd5b26e..7ef5856f57 100644\n--- a/object.c\n+++ b/object.c\n@@ -480,6 +480,7 @@ struct raw_object_store *raw_object_store_new(void)\n \n \tmemset(o, 0, sizeof(*o));\n \tINIT_LIST_HEAD(&o->packed_git_mru);\n+\tpthread_mutex_init(&o->replace_mutex, NULL);\n \treturn o;\n }\n \n@@ -507,6 +508,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n \n \toidmap_free(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n+\tpthread_mutex_destroy(&o->replace_mutex);\n \n \tfree_commit_graph(o->commit_graph);\n \to->commit_graph = NULL;\ndiff --git a/replace-object.c b/replace-object.c\nindex e295e87943..7bd9aba6ee 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -34,14 +34,23 @@ static int register_replace_ref(struct repository *r,\n \n void prepare_replace_object(struct repository *r)\n {\n-\tif (r->objects->replace_map)\n+\tif (r->objects->replace_map_initialized)\n \t\treturn;\n \n+\tpthread_mutex_lock(&r->objects->replace_mutex);\n+\tif (r->objects->replace_map_initialized) {\n+\t\tpthread_mutex_unlock(&r->objects->replace_mutex);\n+\t\treturn;\n+\t}\n+\n \tr->objects->replace_map =\n \t\txmalloc(sizeof(*r->objects->replace_map));\n \toidmap_init(r->objects->replace_map, 0);\n \n \tfor_each_replace_ref(r, register_replace_ref, NULL);\n+\tr->objects->replace_map_initialized = 1;\n+\n+\tpthread_mutex_unlock(&r->objects->replace_mutex);\n }\n \n /* We allow \"recursive\" replacement. Only within reason, though */\ndiff --git a/replace-object.h b/replace-object.h\nindex 04ed7a85a2..3fbc32eb7b 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -24,12 +24,17 @@ const struct object_id *do_lookup_replace_object(struct repository *r,\n * name (replaced recursively, if necessary). The return value is\n * either sha1 or a pointer to a permanently-allocated value. When\n * object replacement is suppressed, always return sha1.\n+ *\n+ * Note: some thread debuggers might point a data race on the\n+ * replace_map_initialized reading in this function. However, we know there's no\n+ * problem in the value being updated by one thread right after another one read\n+ * it here (and it should be written to only once, anyway).\n */\n static inline const struct object_id *lookup_replace_object(struct repository *r,\n \t\t\t\t\t\t\t const struct object_id *oid)\n {\n \tif (!read_replace_refs ||\n-\t (r->objects->replace_map &&\n+\t (r->objects->replace_map_initialized &&\n \t r->objects->replace_map->map.tablesize == 0))\n \t\treturn oid;\n \treturn do_lookup_replace_object(r, oid);\n-- \n2.23.0\n\n"},{"id":"383117","messageId":"4c5652ab34f0989856aba919ca84b2b091dcad98.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:51Z","receivedAt":"2019-09-30T01:51:31Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow object reading to be performed by multiple threads protecting it\nwith an internal lock. The lock usage can be toggled with\nenable_obj_read_lock() and disable_obj_read_lock(). Currently, the\nfunctions which can be safely called in parallel are:\nread_object_file_extended(), repo_read_object_file(),\nread_object_file(), read_object_with_reference(), read_object(),\noid_object_info() and oid_object_info_extended(). It's also possible to\nuse obj_read_lock() and obj_read_unlock() to protect other sections that\ncannot execute in parallel with object reading.\n\nProbably there are many spots in the functions listed above that could\nbe executed unlocked (and thus, in parallel). But, for now, we are most\ninterested in allowing parallel access to zlib inflation. This is one of\nthe sections where object reading spends most of the time and it's\nalready thread-safe. So, to take advantage of that, the respective lock\nis released when calling git_inflate() and re-acquired right after, for\nevery calling spot in oid_object_info_extended()'s call chain. We may\nrefine the lock to also exploit other possible parallel spots in the\nfuture, but threaded zlib inflation should already give great speedups\nfor now.\n\nNote that add_delta_base_cache() was also modified to skip adding\nalready present entries to the cache. This wasn't possible before, but\nnow it is since phase I and phase III of unpack_entry() may execute\nconcurrently.\n\nAnother important thing to notice is that the object reading lock only\nworks in conjunction with the 'struct raw_object_store's replace_mutex.\nOtherwise, there would still be racy spots in object reading\nfunctions.\n\nSigned-off-by: Matheus Tavares \n---\n object-store.h | 35 +++++++++++++++++++++++++++++++\n packfile.c | 7 +++++++\n sha1-file.c | 57 +++++++++++++++++++++++++++++++++++++++++++++-----\n 3 files changed, 94 insertions(+), 5 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex b22e20ad7d..8f63f21ad2 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -6,6 +6,7 @@\n #include \"list.h\"\n #include \"sha1-array.h\"\n #include \"strbuf.h\"\n+#include \"thread-utils.h\"\n \n struct object_directory {\n \tstruct object_directory *next;\n@@ -230,6 +231,40 @@ int has_loose_object_nonlocal(const struct object_id *);\n \n void assert_oid_type(const struct object_id *oid, enum object_type expect);\n \n+/*\n+ * Enabling the object read lock allows multiple threads to safely call the\n+ * following functions in parallel: repo_read_object_file(), read_object_file(),\n+ * read_object_file_extended(), read_object_with_reference(), read_object(),\n+ * oid_object_info() and oid_object_info_extended().\n+ *\n+ * obj_read_lock() and obj_read_unlock() may also be used to protect other\n+ * section which cannot execute in parallel with object reading. Since the used\n+ * lock is a recursive mutex, these sections can even contain calls to object\n+ * reading functions. However, beware that in these cases zlib inflation won't\n+ * be performed in parallel, losing performance.\n+ *\n+ * TODO: oid_object_info_extended()'s call stack has a recursive behavior. If\n+ * any of its callees end up calling it, this recursive call won't benefit from\n+ * parallel inflation.\n+ */\n+void enable_obj_read_lock(void);\n+void disable_obj_read_lock(void);\n+\n+extern int obj_read_use_lock;\n+extern pthread_mutex_t obj_read_mutex;\n+\n+static inline void obj_read_lock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_lock(&obj_read_mutex);\n+}\n+\n+static inline void obj_read_unlock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_unlock(&obj_read_mutex);\n+}\n+\n struct object_info {\n \t/* Request */\n \tenum object_type *typep;\ndiff --git a/packfile.c b/packfile.c\nindex 1a7d69fe32..a336972174 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1098,7 +1098,9 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1451,6 +1453,9 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tstruct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));\n \tstruct list_head *lru, *tmp;\n \n+\tif (get_delta_base_cache_entry(p, base_offset))\n+\t\treturn;\n+\n \tdelta_base_cached += base_size;\n \n \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n@@ -1580,7 +1585,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tif (!stream.avail_out)\n \t\t\tbreak; /* the payload is larger than it should be */\n \t\tcurpos += stream.next_in - in;\ndiff --git a/sha1-file.c b/sha1-file.c\nindex e85f249a5d..b4f2f5cb94 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1148,6 +1148,8 @@ static int unpack_loose_short_header(git_zstream *stream,\n \t\t\t\t unsigned char *map, unsigned long mapsize,\n \t\t\t\t void *buffer, unsigned long bufsiz)\n {\n+\tint ret;\n+\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n@@ -1156,7 +1158,11 @@ static int unpack_loose_short_header(git_zstream *stream,\n \tstream->avail_out = bufsiz;\n \n \tgit_inflate_init(stream);\n-\treturn git_inflate(stream, 0);\n+\tobj_read_unlock();\n+\tret = git_inflate(stream, 0);\n+\tobj_read_lock();\n+\n+\treturn ret;\n }\n \n int unpack_loose_header(git_zstream *stream,\n@@ -1201,7 +1207,9 @@ static int unpack_loose_header_to_strbuf(git_zstream *stream, unsigned char *map\n \tstream->avail_out = bufsiz;\n \n \tdo {\n+\t\tobj_read_unlock();\n \t\tstatus = git_inflate(stream, 0);\n+\t\tobj_read_lock();\n \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n@@ -1241,8 +1249,11 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\t */\n \t\tstream->next_out = buf + bytes;\n \t\tstream->avail_out = size - bytes;\n-\t\twhile (status == Z_OK)\n+\t\twhile (status == Z_OK) {\n+\t\t\tobj_read_unlock();\n \t\t\tstatus = git_inflate(stream, Z_FINISH);\n+\t\t\tobj_read_lock();\n+\t\t}\n \t}\n \tif (status == Z_STREAM_END && !stream->avail_in) {\n \t\tgit_inflate_end(stream);\n@@ -1412,10 +1423,32 @@ static int loose_object_info(struct repository *r,\n \treturn (status < 0) ? status : 0;\n }\n \n+int obj_read_use_lock = 0;\n+pthread_mutex_t obj_read_mutex;\n+\n+void enable_obj_read_lock(void)\n+{\n+\tif (obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 1;\n+\tinit_recursive_mutex(&obj_read_mutex);\n+}\n+\n+void disable_obj_read_lock(void)\n+{\n+\tif (!obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 0;\n+\tpthread_mutex_destroy(&obj_read_mutex);\n+}\n+\n int fetch_if_missing = 1;\n \n-int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n-\t\t\t struct object_info *oi, unsigned flags)\n+static int do_oid_object_info_extended(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct object_info *oi, unsigned flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n \tstruct pack_entry e;\n@@ -1423,6 +1456,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \tconst struct object_id *real = oid;\n \tint already_retried = 0;\n \n+\n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(r, oid);\n \n@@ -1498,7 +1532,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, real->hash);\n-\t\treturn oid_object_info_extended(r, real, oi, 0);\n+\t\treturn do_oid_object_info_extended(r, real, oi, 0);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\n \t\toi->u.packed.pack = e.p;\n@@ -1509,6 +1543,17 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \treturn 0;\n }\n \n+int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n+\t\t\t struct object_info *oi, unsigned flags)\n+{\n+\tint ret;\n+\tobj_read_lock();\n+\tret = do_oid_object_info_extended(r, oid, oi, flags);\n+\tobj_read_unlock();\n+\treturn ret;\n+}\n+\n+\n /* returns enum object_type or negative */\n int oid_object_info(struct repository *r,\n \t\t const struct object_id *oid,\n@@ -1581,6 +1626,7 @@ void *read_object_file_extended(struct repository *r,\n \tif (data)\n \t\treturn data;\n \n+\tobj_read_lock();\n \tif (errno && errno != ENOENT)\n \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n \n@@ -1596,6 +1642,7 @@ void *read_object_file_extended(struct repository *r,\n \tif ((p = has_packed_and_bad(r, repl->hash)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), p->pack_name);\n+\tobj_read_unlock();\n \n \treturn NULL;\n }\n-- \n2.23.0\n\n"},{"id":"383118","messageId":"48b632d7a0278f4abb4f0b0390f316a631a9d0ef.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 06/11] grep: replace grep_read_mutex by internal obj read lock","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:52Z","receivedAt":"2019-09-30T01:51:35Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep uses 'grep_read_mutex' to protect its calls to object reading\noperations. But these have their own internal lock now, which ensures a\nbetter performance (allowing parallel access to more regions). So, let's\nremove the former and, instead, activate the latter with\nenable_obj_read_lock().\n\nSections that are currently protected by 'grep_read_mutex' but are not\ninternally protected by the object reading lock should be surrounded by\nobj_read_lock() and obj_read_unlock(). These guarantee mutual exclusion\nwith object reading operations, keeping the current behavior and\navoiding race conditions. Namely, these places are:\n\n In grep.c:\n\n - fill_textconv() at fill_textconv_grep().\n - userdiff_get_textconv() at grep_source_1().\n\n In builtin/grep.c:\n\n - parse_object_or_die() and the submodule functions at\n grep_submodule().\n - deref_tag() and gitmodules_config_oid() at grep_objects().\n\nIf these functions become thread-safe, in the future, we might remove\nthe locking and probably get some speedup.\n\nNote that some of the submodule functions will already be thread-safe\n(or close to being thread-safe) with the internal object reading lock.\nHowever, as some of them will require additional modifications to be\nremoved from the critical section, this will be done in its own patch.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 46 ++++++++++++++++------------------------------\n grep.c | 39 +++++++++++++++++++--------------------\n grep.h | 13 -------------\n 3 files changed, 35 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex fa8b9996d1..5a404ee1db 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -200,12 +200,12 @@ static void start_threads(struct grep_opt *opt)\n \tint i;\n \n \tpthread_mutex_init(&grep_mutex, NULL);\n-\tpthread_mutex_init(&grep_read_mutex, NULL);\n \tpthread_mutex_init(&grep_attr_mutex, NULL);\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n \tgrep_use_locks = 1;\n+\tenable_obj_read_lock();\n \n \tfor (i = 0; i < ARRAY_SIZE(todo); i++) {\n \t\tstrbuf_init(&todo[i].out, 0);\n@@ -257,12 +257,12 @@ static int wait_all(void)\n \tfree(threads);\n \n \tpthread_mutex_destroy(&grep_mutex);\n-\tpthread_mutex_destroy(&grep_read_mutex);\n \tpthread_mutex_destroy(&grep_attr_mutex);\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n \tgrep_use_locks = 0;\n+\tdisable_obj_read_lock();\n \n \treturn hit;\n }\n@@ -295,16 +295,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n \treturn st;\n }\n \n-static void *lock_and_read_oid_file(const struct object_id *oid, enum object_type *type, unsigned long *size)\n-{\n-\tvoid *data;\n-\n-\tgrep_read_lock();\n-\tdata = read_object_file(oid, type, size);\n-\tgrep_read_unlock();\n-\treturn data;\n-}\n-\n static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\t const char *filename, int tree_name_len,\n \t\t const char *path)\n@@ -413,20 +403,20 @@ static int grep_submodule(struct grep_opt *opt,\n \n \t/*\n \t * NEEDSWORK: submodules functions need to be protected because they\n-\t * access the object store via config_from_gitmodules(): the latter\n-\t * uses get_oid() which, for now, relies on the global the_repository\n-\t * object.\n+\t * call config_from_gitmodules(): the latter contains in its call stack\n+\t * many thread-unsafe operations that are racy with object reading, such\n+\t * as parse_object() and is_promisor_object().\n \t */\n-\tgrep_read_lock();\n+\tobj_read_lock();\n \tsub = submodule_from_path(superproject, &null_oid, path);\n \n \tif (!is_submodule_active(superproject, path)) {\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \t\treturn 0;\n \t}\n \n \tif (repo_submodule_init(&subrepo, superproject, sub)) {\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \t\treturn 0;\n \t}\n \n@@ -443,7 +433,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * object.\n \t */\n \tadd_to_alternates_memory(subrepo.objects->odb->path);\n-\tgrep_read_unlock();\n+\tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n \tsubopt.repo = &subrepo;\n@@ -455,13 +445,12 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tgrep_read_lock();\n+\t\tobj_read_lock();\n \t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n+\t\tobj_read_unlock();\n \t\tdata = read_object_with_reference(&subrepo,\n \t\t\t\t\t\t &object->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-\t\tgrep_read_unlock();\n-\n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n \n@@ -586,7 +575,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_oid_file(&entry.oid, &type, &size);\n+\t\t\tdata = read_object_file(&entry.oid, &type, &size);\n \t\t\tif (!data)\n \t\t\t\tdie(_(\"unable to read tree (%s)\"),\n \t\t\t\t oid_to_hex(&entry.oid));\n@@ -624,12 +613,9 @@ 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\tgrep_read_lock();\n \t\tdata = read_object_with_reference(opt->repo,\n \t\t\t\t\t\t &obj->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-\t\tgrep_read_unlock();\n-\n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&obj->oid));\n \n@@ -659,17 +645,17 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \n-\t\tgrep_read_lock();\n+\t\tobj_read_lock();\n \t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n \t\t\t\t NULL, 0);\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \n \t\t/* load the gitmodules file for this rev */\n \t\tif (recurse_submodules) {\n \t\t\tsubmodule_free(opt->repo);\n-\t\t\tgrep_read_lock();\n+\t\t\tobj_read_lock();\n \t\t\tgitmodules_config_oid(&real_obj->oid);\n-\t\t\tgrep_read_unlock();\n+\t\t\tobj_read_unlock();\n \t\t}\n \t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name,\n \t\t\t\tlist->objects[i].path)) {\ndiff --git a/grep.c b/grep.c\nindex b29946def2..0ca400f7b6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1533,11 +1533,6 @@ static inline void grep_attr_unlock(void)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n \n-/*\n- * Same as git_attr_mutex, but protecting the thread-unsafe object db access.\n- */\n-pthread_mutex_t grep_read_mutex;\n-\n static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n@@ -1734,13 +1729,20 @@ static int fill_textconv_grep(struct repository *r,\n \t}\n \n \t/*\n-\t * fill_textconv is not remotely thread-safe; it may load objects\n-\t * behind the scenes, and it modifies the global diff tempfile\n-\t * structure.\n+\t * fill_textconv is not remotely thread-safe; it modifies the global\n+\t * diff tempfile structure, writes to the_repo's odb and might\n+\t * internally call thread-unsafe functions such as the\n+\t * prepare_packed_git() lazy-initializator. Because of the last two, we\n+\t * must ensure mutual exclusion between this call and the object reading\n+\t * API, thus we use obj_read_lock() here.\n+\t *\n+\t * TODO: allowing text conversion to run in parallel with object\n+\t * reading operations might increase performance in the multithreaded\n+\t * non-worktreee git-grep with --textconv.\n \t */\n-\tgrep_read_lock();\n+\tobj_read_lock();\n \tsize = fill_textconv(r, driver, df, &buf);\n-\tgrep_read_unlock();\n+\tobj_read_unlock();\n \tfree_filespec(df);\n \n \t/*\n@@ -1806,13 +1808,16 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\tgrep_source_load_driver(gs, opt->repo->index);\n \t\t/*\n \t\t * We might set up the shared textconv cache data here, which\n-\t\t * is not thread-safe.\n+\t\t * is not thread-safe. Also, get_oid_with_context() and\n+\t\t * parse_object() might be internally called. As they are not\n+\t\t * currenty thread-safe and might be racy with object reading,\n+\t\t * obj_read_lock() must be called.\n \t\t */\n+\t\tobj_read_lock();\n \t\tgrep_attr_lock();\n-\t\tgrep_read_lock();\n \t\ttextconv = userdiff_get_textconv(opt->repo, gs->driver);\n-\t\tgrep_read_unlock();\n \t\tgrep_attr_unlock();\n+\t\tobj_read_unlock();\n \t}\n \n \t/*\n@@ -2111,10 +2116,7 @@ static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n \n-\tgrep_read_lock();\n \tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n-\tgrep_read_unlock();\n-\n \tif (!gs->buf)\n \t\treturn error(_(\"'%s': unable to read %s\"),\n \t\t\t gs->name,\n@@ -2179,11 +2181,8 @@ void grep_source_load_driver(struct grep_source *gs,\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tif (gs->path) {\n-\t\tgrep_read_lock();\n+\tif (gs->path)\n \t\tgs->driver = userdiff_find_by_path(istate, gs->path);\n-\t\tgrep_read_unlock();\n-\t}\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..54bf3a1ed4 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -235,18 +235,5 @@ int grep_threads_ok(const struct grep_opt *opt);\n */\n extern int grep_use_locks;\n extern pthread_mutex_t grep_attr_mutex;\n-extern pthread_mutex_t grep_read_mutex;\n-\n-static inline void grep_read_lock(void)\n-{\n-\tif (grep_use_locks)\n-\t\tpthread_mutex_lock(&grep_read_mutex);\n-}\n-\n-static inline void grep_read_unlock(void)\n-{\n-\tif (grep_use_locks)\n-\t\tpthread_mutex_unlock(&grep_read_mutex);\n-}\n \n #endif\n-- \n2.23.0\n\n"},{"id":"383119","messageId":"38940b38af5337646b0ddd4b06d3efb1b97aec81.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 07/11] submodule-config: add skip_if_read option to repo_read_gitmodules()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:53Z","receivedAt":"2019-09-30T01:51:39Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Currently, submodule-config.c doesn't have an externally acessible\nfunction to read gitmodules only if it wasn't already read. But this\nexactly behavior is internally implemented by gitmodules_read_check(),\nto perform a lazy load. Let's merge this function with\nrepo_read_gitmodules() adding an 'skip_if_read' which allow both\ninternal and external callers to access this functionality. This\nsimplifies a little the code. The added option will also be used in the\nfollowing patch.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 2 +-\n submodule-config.c | 18 ++++++------------\n submodule-config.h | 2 +-\n unpack-trees.c | 4 ++--\n 4 files changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5a404ee1db..1c4ff4a75f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -420,7 +420,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\treturn 0;\n \t}\n \n-\trepo_read_gitmodules(&subrepo);\n+\trepo_read_gitmodules(&subrepo, 0);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4264ee216f..8c4333120a 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -660,10 +660,13 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \treturn parse_config(var, value, ¶meter);\n }\n \n-void repo_read_gitmodules(struct repository *repo)\n+void repo_read_gitmodules(struct repository *repo, int skip_if_read)\n {\n \tsubmodule_cache_check_init(repo);\n \n+\tif (repo->submodule_cache->gitmodules_read && skip_if_read)\n+\t\treturn;\n+\n \tif (repo_read_index(repo) < 0)\n \t\treturn;\n \n@@ -689,20 +692,11 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tthe_repository->submodule_cache->gitmodules_read = 1;\n }\n \n-static void gitmodules_read_check(struct repository *repo)\n-{\n-\tsubmodule_cache_check_init(repo);\n-\n-\t/* read the repo's .gitmodules file if it hasn't been already */\n-\tif (!repo->submodule_cache->gitmodules_read)\n-\t\trepo_read_gitmodules(repo);\n-}\n-\n const struct submodule *submodule_from_name(struct repository *r,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\tconst char *name)\n {\n-\tgitmodules_read_check(r);\n+\trepo_read_gitmodules(r, 1);\n \treturn config_from(r->submodule_cache, treeish_name, name, lookup_name);\n }\n \n@@ -710,7 +704,7 @@ const struct submodule *submodule_from_path(struct repository *r,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\tconst char *path)\n {\n-\tgitmodules_read_check(r);\n+\trepo_read_gitmodules(r, 1);\n \treturn config_from(r->submodule_cache, treeish_name, path, lookup_path);\n }\n \ndiff --git a/submodule-config.h b/submodule-config.h\nindex 1b4e2da658..7a76ef8cd8 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -39,7 +39,7 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t const char *arg, int unset);\n int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-void repo_read_gitmodules(struct repository *repo);\n+void repo_read_gitmodules(struct repository *repo, int skip_if_read);\n void gitmodules_config_oid(const struct object_id *commit_oid);\n const struct submodule *submodule_from_name(struct repository *r,\n \t\t\t\t\t const struct object_id *commit_or_tree,\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9c25126aec..689575944c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -292,11 +292,11 @@ static void load_gitmodules_file(struct index_state *index,\n \tif (pos >= 0) {\n \t\tstruct cache_entry *ce = index->cache[pos];\n \t\tif (!state && ce->ce_flags & CE_WT_REMOVE) {\n-\t\t\trepo_read_gitmodules(the_repository);\n+\t\t\trepo_read_gitmodules(the_repository, 0);\n \t\t} else if (state && (ce->ce_flags & CE_UPDATE)) {\n \t\t\tsubmodule_free(the_repository);\n \t\t\tcheckout_entry(ce, state, NULL, NULL);\n-\t\t\trepo_read_gitmodules(the_repository);\n+\t\t\trepo_read_gitmodules(the_repository, 0);\n \t\t}\n \t}\n }\n-- \n2.23.0\n\n"},{"id":"383120","messageId":"8070e154070e6eaef6f31077d15349e74147ff84.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 08/11] grep: allow submodule functions to run in parallel","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:54Z","receivedAt":"2019-09-30T01:51:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Now that object reading operations are internally protected, the\nsubmodule initialization functions at builtin/grep.c:grep_submodule()\nare very close to being thread-safe. Let's take a look at each call and\nremove from the critical section what we can, for better performance:\n\n- submodule_from_path() and is_submodule_active() cannot be called in\n parallel yet only because they call repo_read_gitmodules() which\n contains, in its call stack, operations that would otherwise be in\n race condition with object reading (for example parse_object() and\n is_promisor_remote()). However, they only call repo_read_gitmodules()\n if it wasn't read before. So let's pre-read it before firing the\n threads and allow these two functions to safelly be called in\n parallel.\n\n- repo_submodule_init() is already thread-safe, so remove it from the\n critical section without other necessary changes.\n\n- The repo_read_gitmodules(&subrepo) call at grep_submodule() is safe as\n no other thread is performing object reading operations in the subrepo\n yet. However, threads might be working in the superproject, and this\n function calls add_to_alternates_memory() internally, which is racy\n with object readings in the superproject. So it must be kept\n protected for now. Let's add a \"NEEDSWORK\" to it, informing why it\n cannot be removed from the critical section yet.\n\n- Finally, add_to_alternates_memory() must be kept protected by the same\n reason of the above item.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 38 ++++++++++++++++++++++----------------\n 1 file changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 1c4ff4a75f..c973ac46a7 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -401,25 +401,23 @@ static int grep_submodule(struct grep_opt *opt,\n \tstruct grep_opt subopt;\n \tint hit;\n \n-\t/*\n-\t * NEEDSWORK: submodules functions need to be protected because they\n-\t * call config_from_gitmodules(): the latter contains in its call stack\n-\t * many thread-unsafe operations that are racy with object reading, such\n-\t * as parse_object() and is_promisor_object().\n-\t */\n-\tobj_read_lock();\n \tsub = submodule_from_path(superproject, &null_oid, path);\n \n-\tif (!is_submodule_active(superproject, path)) {\n-\t\tobj_read_unlock();\n+\tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n-\t}\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n-\t\tobj_read_unlock();\n+\tif (repo_submodule_init(&subrepo, superproject, sub))\n \t\treturn 0;\n-\t}\n \n+\t/*\n+\t * NEEDSWORK: repo_read_gitmodules() might call\n+\t * add_to_alternates_memory() via config_from_gitmodules(). This\n+\t * operation causes a race condition with concurrent object readings\n+\t * performed by the worker threads. That's why we need obj_read_lock()\n+\t * here. It should be removed once it's no longer necessary to add the\n+\t * subrepo's odbs to the in-memory alternates list.\n+\t */\n+\tobj_read_lock();\n \trepo_read_gitmodules(&subrepo, 0);\n \n \t/*\n@@ -1052,6 +1050,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\n \n+\tif (recurse_submodules && (!use_index || untracked))\n+\t\tdie(_(\"option not supported with --recurse-submodules\"));\n+\n \tif (list.nr || cached || show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n@@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t && (opt.pre_context || opt.post_context ||\n \t\t\topt.file_break || opt.funcbody))\n \t\t\tskip_first_line = 1;\n+\n+\t\t/*\n+\t\t * Pre-read gitmodules (if not read already) to prevent racy\n+\t\t * lazy reading in worker threads.\n+\t\t */\n+\t\tif (recurse_submodules)\n+\t\t\trepo_read_gitmodules(the_repository, 1);\n+\n \t\tstart_threads(&opt);\n \t} else {\n \t\t/*\n@@ -1105,9 +1114,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (recurse_submodules && (!use_index || untracked))\n-\t\tdie(_(\"option not supported with --recurse-submodules\"));\n-\n \tif (!show_in_pager && !opt.status_only)\n \t\tsetup_pager();\n \n-- \n2.23.0\n\n"},{"id":"383121","messageId":"ede35868d797ea5a3560422afe7280e977bc9f59.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 09/11] grep: protect packed_git [re-]initialization","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:55Z","receivedAt":"2019-09-30T01:51:45Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Some fields in struct raw_object_store are lazy initialized by the\nthread-unsafe packfile.c:prepare_packed_git(). Although this function is\npresent in the call stack of git-grep threads, all paths to it are\ncurrently protected by obj_read_lock() (and the main thread usually\nindirectly calls it before firing the worker threads, anyway). However,\nit's possible that future modifications add new unprotected paths to it,\nintroducing a race condition. Because errors derived from it wouldn't\nhappen often, it could be hard to detect. So to prevent future\nheadaches, let's force eager initialization of packed_git when setting\ngit-grep up. There'll be a small overhead in the cases where we didn't\nreally needed to prepare packed_git during execution but this shouldn't\nbe very noticeable.\n\nAlso, packed_git may be re-initialized by\npackfile.c:reprepare_packed_git(). Again, all paths to it in git-grep\nare already protected by obj_read_lock() but it may suffer from the same\nproblem in the future. So let's also internally protect it with\nobj_read_lock() (which is a recursive mutex).\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 8 ++++++--\n packfile.c | 2 ++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex c973ac46a7..0947596bcd 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,6 +24,7 @@\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"object-store.h\"\n+#include \"packfile.h\"\n \n static char const * const grep_usage[] = {\n \tN_(\"git grep [] [-e] [...] [[--] ...]\"),\n@@ -1074,11 +1075,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tskip_first_line = 1;\n \n \t\t/*\n-\t\t * Pre-read gitmodules (if not read already) to prevent racy\n-\t\t * lazy reading in worker threads.\n+\t\t * Pre-read gitmodules (if not read already) and force eager\n+\t\t * initialization of packed_git to prevent racy lazy\n+\t\t * reading/initialization once worker threads are started.\n \t\t */\n \t\tif (recurse_submodules)\n \t\t\trepo_read_gitmodules(the_repository, 1);\n+\t\tif (startup_info->have_repository)\n+\t\t\t(void)get_packed_git(the_repository);\n \n \t\tstart_threads(&opt);\n \t} else {\ndiff --git a/packfile.c b/packfile.c\nindex a336972174..5b32dac4ce 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1016,12 +1016,14 @@ void reprepare_packed_git(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n+\tobj_read_lock();\n \tfor (odb = r->objects->odb; odb; odb = odb->next)\n \t\todb_clear_loose_cache(odb);\n \n \tr->objects->approximate_object_count_valid = 0;\n \tr->objects->packed_git_initialized = 0;\n \tprepare_packed_git(r);\n+\tobj_read_unlock();\n }\n \n struct packed_git *get_packed_git(struct repository *r)\n-- \n2.23.0\n\n"},{"id":"383122","messageId":"c1fe6545a90563419731932b0bb17869dab1cda7.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 10/11] grep: re-enable threads in non-worktree case","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:56Z","receivedAt":"2019-09-30T01:51:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"They were disabled at 53b8d93 (\"grep: disable threading in non-worktree\ncase\", 12-12-2011), due to observable performance drops (to the point\nthat using a single thread would be faster than multiple threads). But\nnow that zlib inflation can be performed in parallel we can regain the\nspeedup, so let's re-enable threads in non-worktree grep.\n\nGrepping 'abcd[02]' (\"Regex 1\") and '(static|extern) (int|double) \\*'\n(\"Regex 2\") at chromium's repository[1] I got:\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 17.2920s | 20.9624s\n 2 | 9.6512s | 11.3184s\n 4 | 6.7723s | 7.6268s\n 8** | 6.2886s | 6.9843s\n\nThese are all means of 30 executions after 2 warmup runs. All tests were\nexecuted on an i7-7700HQ (quad core w/ hyper-threading), 16GB of RAM and\nSSD, running Manjaro Linux. But to make sure the optimization also\nperforms well on HDD, the tests were repeated on another machine with an\ni5-4210U (dual core w/ hyper-threading), 8GB of RAM and HDD (SATA III,\n5400 rpm), also running Manjaro Linux:\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 18.4035s | 22.5368s\n 2 | 12.5063s | 14.6409s\n 4** | 10.9136s | 12.7106s\n\n** Note that in these cases we relied on hyper-threading, and that's\n probably why we don't see a big difference in time.\n\nUnfortunately, multithreaded git-grep might be slow in the non-worktree\ncase when --textconv is used and there're too many text conversions.\nProbably the reason for this is that the object read lock is used to\nprotect fill_textconv() and therefore there is a mutual exclusion\nbetween textconv execution and object reading. Because both are time\nconsuming operations, not being able to perform them in parallel can\ncause performance drops. To inform the users about this (and other\nthreading detais), let's also add a \"NOTES ON THREADS\" section to\nDocumentation/git-grep.txt.\n\n[1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/git-grep.txt | 11 +++++++++++\n builtin/grep.c | 2 +-\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 2d27969057..00fc59d565 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -330,6 +330,17 @@ EXAMPLES\n `git grep solution -- :^Documentation`::\n \tLooks for `solution`, excluding files in `Documentation`.\n \n+NOTES ON THREADS\n+----------------\n+\n+The `--threads` option (and the grep.threads configuration) will be ignored when\n+`--open-files-in-pager` is used, forcing a single-threaded execution.\n+\n+When grepping the object store (with `--cached` or giving tree objects), running\n+with multiple threads might perform slower than single threaded if `--textconv`\n+is given and there're too many text conversions. So if you experience low\n+performance in this case, it might be desirable to use `--threads=1`.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 0947596bcd..163f14b60d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1054,7 +1054,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (recurse_submodules && (!use_index || untracked))\n \t\tdie(_(\"option not supported with --recurse-submodules\"));\n \n-\tif (list.nr || cached || show_in_pager) {\n+\tif (show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n \t\tnum_threads = 1;\n-- \n2.23.0\n\n"},{"id":"383123","messageId":"7b0358d1596e6673d12f9592cf05f5193247f3ec.1569808052.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v2 11/11] grep: move driver pre-load out of critical section","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-30T01:50:57Z","receivedAt":"2019-09-30T01:51:52Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In builtin/grep.c:add_work() we pre-load the userdiff drivers before\nadding the grep_source in the todo list. This operation is currently\nbeing performed after acquiring the grep_mutex, but as it's already\nthread-safe, we don't need to protect it here. So let's move it out of\nthe critical section which should avoid thread contention and improve\nperformance.\n\nRunning[1] `git grep --threads=8 abcd[02] HEAD` on chromium's\nrepository[2], I got the following mean times for 30 executions after 2\nwarmups:\n\n Original | 6.2886s\n-------------------------|-----------\n Out of critical section | 5.7852s\n\n[1]: Tests performed on an i7-7700HQ with 16GB of RAM and SSD, running\n Manjaro Linux.\n[2]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 163f14b60d..d275b76647 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -92,8 +92,11 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n+static void add_work(struct grep_opt *opt, struct grep_source *gs)\n {\n+\tif (opt->binary != GREP_BINARY_TEXT)\n+\t\tgrep_source_load_driver(gs, opt->repo->index);\n+\n \tgrep_lock();\n \n \twhile ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {\n@@ -101,9 +104,6 @@ static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n \t}\n \n \ttodo[todo_end].source = *gs;\n-\tif (opt->binary != GREP_BINARY_TEXT)\n-\t\tgrep_source_load_driver(&todo[todo_end].source,\n-\t\t\t\t\topt->repo->index);\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n-- \n2.23.0\n\n"},{"id":"383231","messageId":"e452947091b96a02ca3292f8cd9793a8d661fca4.1569950899.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"48b632d7a0278f4abb4f0b0390f316a631a9d0ef.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH] squash! grep: replace grep_read_mutex by internal obj read lock","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-01T19:23:34Z","receivedAt":"2019-10-01T19:24:47Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Signed-off-by: Matheus Tavares \n---\n\nThis is just a small fixup to be squashed into patch 6: with multiple\nlocks, the locking order must be consistent across all critical sections\nto avoid dead-lock. Since grep_attr_lock() is called before\nobj_read_lock() in grep_source_load_driver(), it must also be called\nfirst here.\n\n\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0ca400f7b6..85ee89b5d6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1813,11 +1813,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t * currenty thread-safe and might be racy with object reading,\n \t\t * obj_read_lock() must be called.\n \t\t */\n-\t\tobj_read_lock();\n \t\tgrep_attr_lock();\n+\t\tobj_read_lock();\n \t\ttextconv = userdiff_get_textconv(opt->repo, gs->driver);\n-\t\tgrep_attr_unlock();\n \t\tobj_read_unlock();\n+\t\tgrep_attr_unlock();\n \t}\n \n \t/*\n-- \n2.23.0\n\n"},{"id":"385973","messageId":"20191112025418.254880-1-jonathantanmy@google.com","threadId":"51621","inReplyTo":"4c5652ab34f0989856aba919ca84b2b091dcad98.1569808052.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-12T02:54:18Z","receivedAt":"2019-11-12T02:54:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> @@ -1580,7 +1585,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> \tdo {\n> \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n> \t\tstream.next_in = in;\n> +\t\tobj_read_unlock();\n> \t\tst = git_inflate(&stream, Z_FINISH);\n> +\t\tobj_read_lock();\n> \t\tif (!stream.avail_out)\n> \t\t\tbreak; /* the payload is larger than it should be */\n> \t\tcurpos += stream.next_in - in;\n\nAs I see it, the main purpose of this patch set is to move the mutex\nguarding object reading from builtin/grep.c (grep_read_mutex) to\nobject-store.h (obj_read_mutex), so that we can add \"holes\" (non-mutex\nsections) such as the one quoted above, in order that zlib inflation can\nhappen outside the mutex.\n\nMy concern is that the presence of these \"holes\" make object reading\nnon-thread-safe, defeating the purpose of obj_read_mutex. In particular,\nthe section quoted above assumes that the window section returned by\nuse_pack() is still valid throughout the inflation, but that window\ncould have been invalidated by things like an excess of windows open,\nreprepare_packed_git(), etc.\n\nI thought of this for a while but couldn't think of a good solution. If\nwe introduced a reference counting mechanism into Git, that would allow\nus to hold the window open outside the mutex, but things like\nreprepare_packed_git() would still be difficult.\n\nIf there's a good solution that only works for unpack_compressed_entry()\nand not the other parts that also inflate, that might be sufficient (we\ncan inflate outside mutex just for this function, and inflate inside\nmutex for the rest) - we would have to benchmark to be sure, but I think\nthat Matheus's main use case concerns grepping over a repo with a\npackfile, not loose objects.\n"},{"id":"386094","messageId":"20191113052044.GB3547@sigill.intra.peff.net","threadId":"51621","inReplyTo":"20191112025418.254880-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-13T05:20:44Z","receivedAt":"2019-11-13T05:20:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2019 at 06:54:18PM -0800, Jonathan Tan wrote:\n\n> > @@ -1580,7 +1585,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> > \tdo {\n> > \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n> > \t\tstream.next_in = in;\n> > +\t\tobj_read_unlock();\n> > \t\tst = git_inflate(&stream, Z_FINISH);\n> > +\t\tobj_read_lock();\n> > \t\tif (!stream.avail_out)\n> > \t\t\tbreak; /* the payload is larger than it should be */\n> > \t\tcurpos += stream.next_in - in;\n> \n> As I see it, the main purpose of this patch set is to move the mutex\n> guarding object reading from builtin/grep.c (grep_read_mutex) to\n> object-store.h (obj_read_mutex), so that we can add \"holes\" (non-mutex\n> sections) such as the one quoted above, in order that zlib inflation can\n> happen outside the mutex.\n> \n> My concern is that the presence of these \"holes\" make object reading\n> non-thread-safe, defeating the purpose of obj_read_mutex. In particular,\n> the section quoted above assumes that the window section returned by\n> use_pack() is still valid throughout the inflation, but that window\n> could have been invalidated by things like an excess of windows open,\n> reprepare_packed_git(), etc.\n\nYeah, I don't think the code above is safe. The map window can be\nmodified by other threads.\n\n> I thought of this for a while but couldn't think of a good solution. If\n> we introduced a reference counting mechanism into Git, that would allow\n> us to hold the window open outside the mutex, but things like\n> reprepare_packed_git() would still be difficult.\n\nI think you could put a reader-writer lock into each window. The code\nhere would take the reader lock, and multiple readers could use it at\nthe same time. Any time the window needs to be shifted, resized, or\ndiscarded, that code would take the writer lock, waiting for (and then\nblocking out) any readers.\n\nA pthread_rwlock would work, but it would be the first use in Git. I\nthink we'd need to find an equivalent for compat/win32/pthread.h.\n\n-Peff\n"},{"id":"386161","messageId":"CAHd-oW4PtO2CKpd3HFgrJWmQf3MN+MHt5c7V6OGx33LgU-CrOQ@mail.gmail.com","threadId":"51621","inReplyTo":"20191113052044.GB3547@sigill.intra.peff.net","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-11-14T05:57:42Z","receivedAt":"2019-11-14T05:57:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Peff and Jonathan\n\nOn Wed, Nov 13, 2019 at 2:20 AM Jeff King wrote:\n>\n> On Mon, Nov 11, 2019 at 06:54:18PM -0800, Jonathan Tan wrote:\n[...]\n> > My concern is that the presence of these \"holes\" make object reading\n> > non-thread-safe, defeating the purpose of obj_read_mutex. In particular,\n> > the section quoted above assumes that the window section returned by\n> > use_pack() is still valid throughout the inflation, but that window\n> > could have been invalidated by things like an excess of windows open,\n> > reprepare_packed_git(), etc.\n\nThank you for spotting this issue!\n\n> > I thought of this for a while but couldn't think of a good solution. If\n> > we introduced a reference counting mechanism into Git, that would allow\n> > us to hold the window open outside the mutex, but things like\n> > reprepare_packed_git() would still be difficult.\n>\n> I think you could put a reader-writer lock into each window. The code\n> here would take the reader lock, and multiple readers could use it at\n> the same time. Any time the window needs to be shifted, resized, or\n> discarded, that code would take the writer lock, waiting for (and then\n> blocking out) any readers.\n\nGreat idea, I'll work on that. Thanks!\n\nAbout the other parallel inflation calls on loose objects at\nunpack_loose_short_header(), unpack_loose_header_to_strbuf() and\nunpack_loose_rest(): could they suffer from a similar race problem?\nFWIU, the inflation input used in these cases comes from\nmap_loose_object(), and it's not referenced outside this scope. So I\nthink there's no risk of one thread munmapping the object file while\nother is inflating it. Is that right?\n\n> A pthread_rwlock would work, but it would be the first use in Git. I\n> think we'd need to find an equivalent for compat/win32/pthread.h.\n\nThese[1][2] seems to be the equivalent options on Windows. I'll have\nto read these docs more carefully, but [2] seems to be more\ninteresting in terms of speed. Also, the extra features of [1] are not\nreally needed for our use case here.\n\n[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/reader-writer-spin-locks\n[2]: https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks\n"},{"id":"386163","messageId":"20191114060134.GB10643@sigill.intra.peff.net","threadId":"51621","inReplyTo":"CAHd-oW4PtO2CKpd3HFgrJWmQf3MN+MHt5c7V6OGx33LgU-CrOQ@mail.gmail.com","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-14T06:01:34Z","receivedAt":"2019-11-14T06:01:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2019 at 02:57:42AM -0300, Matheus Tavares Bernardino wrote:\n\n> About the other parallel inflation calls on loose objects at\n> unpack_loose_short_header(), unpack_loose_header_to_strbuf() and\n> unpack_loose_rest(): could they suffer from a similar race problem?\n> FWIU, the inflation input used in these cases comes from\n> map_loose_object(), and it's not referenced outside this scope. So I\n> think there's no risk of one thread munmapping the object file while\n> other is inflating it. Is that right?\n\nRight, I think loose objects would be fine, because the mmap'd content\nis local to that stack variable.\n\n> > A pthread_rwlock would work, but it would be the first use in Git. I\n> > think we'd need to find an equivalent for compat/win32/pthread.h.\n> \n> These[1][2] seems to be the equivalent options on Windows. I'll have\n> to read these docs more carefully, but [2] seems to be more\n> interesting in terms of speed. Also, the extra features of [1] are not\n> really needed for our use case here.\n> \n> [1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/reader-writer-spin-locks\n> [2]: https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks\n\nYeah, looks like it, but I don't have any expertise there (nor a Windows\nsystem to test on).\n\n-Peff\n"},{"id":"386197","messageId":"20191114181552.137071-1-jonathantanmy@google.com","threadId":"51621","inReplyTo":"20191114060134.GB10643@sigill.intra.peff.net","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-14T18:15:52Z","receivedAt":"2019-11-14T18:15:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > > A pthread_rwlock would work, but it would be the first use in Git. I\n> > > think we'd need to find an equivalent for compat/win32/pthread.h.\n> > \n> > These[1][2] seems to be the equivalent options on Windows. I'll have\n> > to read these docs more carefully, but [2] seems to be more\n> > interesting in terms of speed. Also, the extra features of [1] are not\n> > really needed for our use case here.\n> > \n> > [1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/reader-writer-spin-locks\n> > [2]: https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks\n> \n> Yeah, looks like it, but I don't have any expertise there (nor a Windows\n> system to test on).\n\nOne thing to note is that that if we do this, we'll be having one rwlock\nper pack window. I couldn't find out what the Windows limits were, but\nit seems that pthreads does not mandate having no limit [1]:\n\n> Defining symbols for the maximum number of mutexes and condition\n> variables was considered but rejected because the number of these\n> objects may change dynamically. Furthermore, many implementations\n> place these objects into application memory; thus, there is no\n> explicit maximum.\n\n[1] https://linux.die.net/man/3/pthread_mutex_init\n"},{"id":"386249","messageId":"20191115041215.GB21654@sigill.intra.peff.net","threadId":"51621","inReplyTo":"20191114181552.137071-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T04:12:15Z","receivedAt":"2019-11-15T04:12:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2019 at 10:15:52AM -0800, Jonathan Tan wrote:\n\n> > > > A pthread_rwlock would work, but it would be the first use in Git. I\n> > > > think we'd need to find an equivalent for compat/win32/pthread.h.\n> > > \n> > > These[1][2] seems to be the equivalent options on Windows. I'll have\n> > > to read these docs more carefully, but [2] seems to be more\n> > > interesting in terms of speed. Also, the extra features of [1] are not\n> > > really needed for our use case here.\n> > > \n> > > [1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/reader-writer-spin-locks\n> > > [2]: https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks\n> > \n> > Yeah, looks like it, but I don't have any expertise there (nor a Windows\n> > system to test on).\n> \n> One thing to note is that that if we do this, we'll be having one rwlock\n> per pack window. I couldn't find out what the Windows limits were, but\n> it seems that pthreads does not mandate having no limit [1]:\n\nYeah, interesting point. We shouldn't _usually_ have too many windows at\nonce, but I think this would be the first place where we allocate a\nnon-constant number of thread mechanisms. And there are degenerate cases\nwhere you might have tens of thousands of packs.\n\nI suspect it's a non-issue in practice, though. Any limits there are\nlikely related to kernel resources like descriptors. And those\ndegenerate cases already run into issues there (did you know that Linux\nsystems limit the number of mmaps a single process can have? I didn't\nuntil I tried repacking a repo with 35,000 packs).\n\n-Peff\n"},{"id":"388599","messageId":"CAHd-oW5qT5LmUd6GTL=O+-yXPmq5Uy9gk3ohL_2r+_K+6UJS3Q@mail.gmail.com","threadId":"51621","inReplyTo":"20191115041215.GB21654@sigill.intra.peff.net","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-12-19T22:27:42Z","receivedAt":"2019-12-19T22:27:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Peff and Jonathan\n\nSorry for the delay in re-rolling this series, it was a very busy end\nof semester. But I finally completed my degree and had time to get\nback to it :)\n\nI tried the rwlock approach, but there were some subtle difficulties.\nFor example, we should hold the lock in write mode when free()-ing the\nwindow, and thus, the lock couldn't be declared inside the struct\npack_window.\n\nAlso, it seemed that protecting the window reading at git_inflate()\nwouldn't be enough: suppose a thread has just read from a window in\ngit_inflate() (holding the rwlock) and is now waiting for the\nobj_read_mutex to continue its object reading operation. If another\nthread (that already has the obj_read_mutex) acquires the rwlock in\nsequence, it could free() the said window. It might not sound like a\nproblem since the first thread has already finished reading from it.\nBut since a pointer to the window would still be in the first thread's\nstack as a window cursor, it could be later passed down to use_pack()\nleading to a segfault. I couldn't come up with a solution for this\nyet.\n\nHowever, re-inspecting the code, it seemed to me that we might already\nhave a thread-safe mechanism. The window disposal operations (at\nclose_pack_windows() and unuse_one_window()) are only performed if\nwindow.inuse_cnt == 0. So as a thread which reads from the window will\nalso previously increment its inuse_cnt, wouldn't the reading\noperation be already protected?\n\nAnother concern would be close_pack_fd(), which can close packs even\nwith in-use windows. However, as the mmap docs[1] says: \"closing the\nfile descriptor does not unmap the region\".\n\nFinally, we also considered reprepare_packed_git() as a possible\nconflicting operation. But the function called by it to handle\npackfile opening, prepare_pack(), won't reopen already available\npacks. Therefore, IIUC, it will leave the opened windows intact.\n\nSo, aren't perhaps the window readings performed outside the\nobj_read_mutex critical section already thread-safe?\n\nThanks,\nMatheus\n\n[1]: https://linux.die.net/man/2/mmap\n"},{"id":"389552","messageId":"CAHd-oW7WhSs2PDSwhksu0Kh5pAJE3rw3nmc-8VJ9dkJitz_N8A@mail.gmail.com","threadId":"51621","inReplyTo":"CAHd-oW5qT5LmUd6GTL=O+-yXPmq5Uy9gk3ohL_2r+_K+6UJS3Q@mail.gmail.com","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-09T22:02:24Z","receivedAt":"2020-01-09T22:02:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, folks\n\nOn Thu, Dec 19, 2019 at 5:27 PM Matheus Tavares Bernardino\n wrote:\n>\n[...]\n> However, re-inspecting the code, it seemed to me that we might already\n> have a thread-safe mechanism. The window disposal operations (at\n> close_pack_windows() and unuse_one_window()) are only performed if\n> window.inuse_cnt == 0. So as a thread which reads from the window will\n> also previously increment its inuse_cnt, wouldn't the reading\n> operation be already protected?\n>\n> Another concern would be close_pack_fd(), which can close packs even\n> with in-use windows. However, as the mmap docs[1] says: \"closing the\n> file descriptor does not unmap the region\".\n>\n> Finally, we also considered reprepare_packed_git() as a possible\n> conflicting operation. But the function called by it to handle\n> packfile opening, prepare_pack(), won't reopen already available\n> packs. Therefore, IIUC, it will leave the opened windows intact.\n>\n> So, aren't perhaps the window readings performed outside the\n> obj_read_mutex critical section already thread-safe?\n\nAny thoughts on this?\n\n> Thanks,\n> Matheus\n>\n> [1]: https://linux.die.net/man/2/mmap\n"},{"id":"389590","messageId":"CAP8UFD3QHNoePGsc9t0fVxTJpPy+KBCbPfXKGoNOx_2bCf1Hxg@mail.gmail.com","threadId":"51621","inReplyTo":"CAHd-oW7WhSs2PDSwhksu0Kh5pAJE3rw3nmc-8VJ9dkJitz_N8A@mail.gmail.com","subject":"Re: [PATCH v2 05/11] object-store: allow threaded access to object reading","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-10T19:07:07Z","receivedAt":"2020-01-10T19:07:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Matheus,\n\nOn Thu, Jan 9, 2020 at 11:02 PM Matheus Tavares Bernardino\n wrote:\n>\n> On Thu, Dec 19, 2019 at 5:27 PM Matheus Tavares Bernardino\n> wrote:\n\n> > However, re-inspecting the code, it seemed to me that we might already\n> > have a thread-safe mechanism. The window disposal operations (at\n> > close_pack_windows() and unuse_one_window()) are only performed if\n> > window.inuse_cnt == 0. So as a thread which reads from the window will\n> > also previously increment its inuse_cnt, wouldn't the reading\n> > operation be already protected?\n> >\n> > Another concern would be close_pack_fd(), which can close packs even\n> > with in-use windows. However, as the mmap docs[1] says: \"closing the\n> > file descriptor does not unmap the region\".\n> >\n> > Finally, we also considered reprepare_packed_git() as a possible\n> > conflicting operation. But the function called by it to handle\n> > packfile opening, prepare_pack(), won't reopen already available\n> > packs. Therefore, IIUC, it will leave the opened windows intact.\n> >\n> > So, aren't perhaps the window readings performed outside the\n> > obj_read_mutex critical section already thread-safe?\n>\n> Any thoughts on this?\n\nMy advice, if you don't hear from anyone in the next few days, is to\nupdate the commit message, the cover letter and the comments inside\nthe patches with the information you researched and to resubmit a new\nversion of the patch series.\n\nThanks,\nChristian.\n"},{"id":"389842","messageId":"cover.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1569808052.git.matheus.bernardino@usp.br","subject":"[PATCH v3 00/12] grep: improve threading and fix race conditions","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:48Z","receivedAt":"2020-01-16T02:40:16Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This series focus on re-enabling threads at git-grep for the\nobject store case. They are currently disabled due to being slower than\nsingle-threaded grep in this case. However, by allowing parallel zlib\ninflation when reading objects, speedups of up to 3.3x were observed.\n\nThe patchset also contains some fixes for race conditions found in the\nworking tree git-grep and other thread optimizations. With them, the\nworking tree case reached speedups of up to 1.52x.\n\nChanges since v2:\n- Updated commit message of patch 6 and added comment in the code to\n document how we ensure that the unlocked inflation code is thread-safe\n (as suggested by Christian[1]).\n- Added patch to adjust the default number of threads in git-grep\n according to the number of available cores.\n- Fixed some typos in the commit messages\n- Rebased with master (d0654dc)\n\nNote: this patchset is part of my GSoC project, and also of my\nbachelor's capstone project (which had the same theme/goal). In case\nsomeone might want to take a look, the project's final essay can be\nfound here[2] :) Besides extra details, it contains more performance\ntests and plots of the resulting speedups.\n\n\n[1]: https://public-inbox.org/git/CAP8UFD3QHNoePGsc9t0fVxTJpPy+KBCbPfXKGoNOx_2bCf1Hxg@mail.gmail.com/\n[2]: https://matheustavares.gitlab.io/assets/tavares-final-essay.pdf\ntravis build: https://travis-ci.org/matheustavares/git/builds/637708218\n\nMatheus Tavares (12):\n grep: fix race conditions on userdiff calls\n grep: fix race conditions at grep_submodule()\n grep: fix racy calls in grep_objects()\n replace-object: make replace operations thread-safe\n object-store: allow threaded access to object reading\n grep: replace grep_read_mutex by internal obj read lock\n submodule-config: add skip_if_read option to repo_read_gitmodules()\n grep: allow submodule functions to run in parallel\n grep: protect packed_git [re-]initialization\n grep: re-enable threads in non-worktree case\n grep: move driver pre-load out of critical section\n grep: use no. of cores as the default no. of threads\n\n .tsan-suppressions | 6 +++\n Documentation/git-grep.txt | 15 +++++-\n builtin/grep.c | 93 +++++++++++++++++++-------------------\n grep.c | 32 +++++++------\n grep.h | 13 ------\n object-store.h | 37 +++++++++++++++\n object.c | 2 +\n packfile.c | 34 ++++++++++++++\n replace-object.c | 11 ++++-\n replace-object.h | 7 ++-\n sha1-file.c | 57 +++++++++++++++++++++--\n submodule-config.c | 18 +++-----\n submodule-config.h | 2 +-\n unpack-trees.c | 4 +-\n 14 files changed, 233 insertions(+), 98 deletions(-)\n\nRange-diff against v2:\n 1: 0f31cb0c12 ! 1: e2f3d377f5 grep: fix race conditions on userdiff calls\n @@ Commit message\n git-grep uses an internal grep_read_mutex to protect object reading\n operations. Similarly, there's a grep_attr_mutex to protect calls to the\n gitattributes machinery. However, two of the three functions protected\n - by the last mutex may also perform object reading, as seen bellow:\n + by the last mutex may also perform object reading, as seen below:\n \n - userdiff_get_textconv() > notes_cache_init() >\n notes_cache_match_validity() > lookup_commit_reference_gently() >\n @@ Commit message\n that, let's make sure to acquire the lock before both of these calls.\n \n Note: this patch might slow down the threaded grep in worktree, for the\n - sake of thread-safeness. However, in the following patches we should\n + sake of thread-safeness. However, in the following patches, we should\n regain performance by replacing grep_read_mutex for an internal object\n reading lock and allowing parallel inflation during object reading.\n \n 2: be32683f1d = 2: 6f0899701b grep: fix race conditions at grep_submodule()\n 3: 34aeb218bf ! 3: 5295c892ee grep: fix racy calls in grep_objects()\n @@ Commit message\n condition with object reading operations (such as the ones internally\n performed by fill_textconv(), called at fill_textconv_grep()). The same\n problem happens with the call to gitmodules_config_oid() which also has\n - parse_object() in its call stack. Fix that protecting both call with the\n - said grep_read_mutex.\n + parse_object() in its call stack. Fix that protecting both calls with\n + the said grep_read_mutex.\n \n Signed-off-by: Matheus Tavares \n \n 4: 5deee3cf11 ! 4: d7f739bc57 replace-object: make replace operations thread-safe\n @@ object-store.h: struct raw_object_store {\n \n ## object.c ##\n @@ object.c: struct raw_object_store *raw_object_store_new(void)\n - \n \tmemset(o, 0, sizeof(*o));\n \tINIT_LIST_HEAD(&o->packed_git_mru);\n + \thashmap_init(&o->pack_map, pack_map_entry_cmp, NULL, 0);\n +\tpthread_mutex_init(&o->replace_mutex, NULL);\n \treturn o;\n }\n 5: 4c5652ab34 ! 5: b72e90f229 object-store: allow threaded access to object reading\n @@ Commit message\n object-store: allow threaded access to object reading\n \n Allow object reading to be performed by multiple threads protecting it\n - with an internal lock. The lock usage can be toggled with\n - enable_obj_read_lock() and disable_obj_read_lock(). Currently, the\n + with an internal lock, the obj_read_mutex. The lock usage can be toggled\n + with enable_obj_read_lock() and disable_obj_read_lock(). Currently, the\n functions which can be safely called in parallel are:\n read_object_file_extended(), repo_read_object_file(),\n read_object_file(), read_object_with_reference(), read_object(),\n - oid_object_info() and oid_object_info_extended(). It's also possible to\n - use obj_read_lock() and obj_read_unlock() to protect other sections that\n - cannot execute in parallel with object reading.\n + oid_object_info() and oid_object_info_extended(). It's also possible\n + to use obj_read_lock() and obj_read_unlock() to protect other sections\n + that cannot execute in parallel with object reading.\n \n Probably there are many spots in the functions listed above that could\n be executed unlocked (and thus, in parallel). But, for now, we are most\n interested in allowing parallel access to zlib inflation. This is one of\n - the sections where object reading spends most of the time and it's\n - already thread-safe. So, to take advantage of that, the respective lock\n - is released when calling git_inflate() and re-acquired right after, for\n - every calling spot in oid_object_info_extended()'s call chain. We may\n - refine the lock to also exploit other possible parallel spots in the\n - future, but threaded zlib inflation should already give great speedups\n - for now.\n + the sections where object reading spends most of the time in (e.g. up to\n + one-third of git-grep's execution time in the chromium repo corresponds\n + to inflation) and it's already thread-safe. So, to take advantage of\n + that, the obj_read_mutex is released when calling git_inflate() and\n + re-acquired right after, for every calling spot in\n + oid_object_info_extended()'s call chain. We may refine this lock to also\n + exploit other possible parallel spots in the future, but for now,\n + threaded zlib inflation should already give great speedups for threaded\n + object reading callers.\n \n Note that add_delta_base_cache() was also modified to skip adding\n already present entries to the cache. This wasn't possible before, but\n - now it is since phase I and phase III of unpack_entry() may execute\n - concurrently.\n + it would be now, with the parallel inflation. Take for example the\n + following situation, where two threads - A and B - are executing the\n + code at unpack_entry():\n \n - Another important thing to notice is that the object reading lock only\n - works in conjunction with the 'struct raw_object_store's replace_mutex.\n - Otherwise, there would still be racy spots in object reading\n - functions.\n + 1. Thread A is performing the decompression of a base O (which is not\n + yet in the cache) at PHASE II. Thread B is simultaneously trying to\n + unpack O, but just starting at PHASE I.\n + 2. Since O is not yet in the cache, B will go to PHASE II to also\n + perform the decompression.\n + 3. When they finish decompressing, one of them will get the object\n + reading mutex and go to PHASE III while the other waits for the\n + mutex. Let’s say A got the mutex first.\n + 4. Thread A will add O to the cache, go throughout the rest of PHASE III\n + and return.\n + 5. Thread B gets the mutex, also add O to the cache (if the check wasn't\n + there) and returns.\n +\n + Finally, it is also important to highlight that the object reading lock\n + can only ensure thread-safety in the mentioned functions thanks to two\n + complementary mechanisms: the use of 'struct raw_object_store's\n + replace_mutex, which guards sections in the object reading machinery\n + that would otherwise be thread-unsafe; and the 'struct pack_window's\n + inuse_cnt, which protects window reading operations (such as the one\n + performed during the inflation of a packed object), allowing them to\n + execute without the acquisition of the obj_read_mutex.\n \n Signed-off-by: Matheus Tavares \n \n @@ packfile.c: unsigned long get_size_from_delta(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n ++\t\t/*\n ++\t\t * Note: the window section returned by use_pack() must be\n ++\t\t * available throughout git_inflate()'s unlocked execution. To\n ++\t\t * ensure no other thread will modify the window in the\n ++\t\t * meantime, we rely on the packed_window.inuse_cnt. This\n ++\t\t * counter is incremented before window reading and checked\n ++\t\t * before window disposal.\n ++\t\t *\n ++\t\t * Other worrying sections could be the call to close_pack_fd(),\n ++\t\t * which can close packs even with in-use windows, and to\n ++\t\t * reprepare_packed_git(). Regarding the former, mmap doc says:\n ++\t\t * \"closing the file descriptor does not unmap the region\". And\n ++\t\t * for the latter, it won't re-open already available packs.\n ++\t\t */\n +\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n +\t\tobj_read_lock();\n @@ packfile.c: static void add_delta_base_cache(struct packed_git *p, off_t base_of\n \tstruct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));\n \tstruct list_head *lru, *tmp;\n \n -+\tif (get_delta_base_cache_entry(p, base_offset))\n ++\t/*\n ++\t * Check required to avoid redundant entries when more than one thread\n ++\t * is unpacking the same object, in unpack_entry() (since its phases I\n ++\t * and III might run concurrently across multiple threads).\n ++\t */\n ++\tif (in_delta_base_cache(p, base_offset))\n +\t\treturn;\n +\n \tdelta_base_cached += base_size;\n @@ packfile.c: static void *unpack_compressed_entry(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n ++\t\t/*\n ++\t\t * Note: we must ensure the window section returned by\n ++\t\t * use_pack() will be available throughout git_inflate()'s\n ++\t\t * unlocked execution. Please refer to the comment at\n ++\t\t * get_size_from_delta() to see how this is done.\n ++\t\t */\n +\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n +\t\tobj_read_lock();\n 6: b439bdeed3 = 6: fc1200bb07 grep: replace grep_read_mutex by internal obj read lock\n 7: d759f1e8c7 ! 7: d39d2ce9c4 submodule-config: add skip_if_read option to repo_read_gitmodules()\n @@ Metadata\n ## Commit message ##\n submodule-config: add skip_if_read option to repo_read_gitmodules()\n \n - Currently, submodule-config.c doesn't have an externally acessible\n + Currently, submodule-config.c doesn't have an externally accessible\n function to read gitmodules only if it wasn't already read. But this\n - exactly behavior is internally implemented by gitmodules_read_check(),\n - to perform a lazy load. Let's merge this function with\n - repo_read_gitmodules() adding an 'skip_if_read' which allow both\n + exact behavior is internally implemented by gitmodules_read_check(), to\n + perform a lazy load. Let's merge this function with\n + repo_read_gitmodules() adding a 'skip_if_read' which allows both\n internal and external callers to access this functionality. This\n - simplifies a little the code. The added option will also be used in the\n - following patch.\n + simplifies a little the code. The added option will also be used in\n + the following patch.\n \n Signed-off-by: Matheus Tavares \n \n @@ submodule-config.h: int option_fetch_parse_recurse_submodules(const struct optio\n -void repo_read_gitmodules(struct repository *repo);\n +void repo_read_gitmodules(struct repository *repo, int skip_if_read);\n void gitmodules_config_oid(const struct object_id *commit_oid);\n - const struct submodule *submodule_from_name(struct repository *r,\n - \t\t\t\t\t const struct object_id *commit_or_tree,\n + \n + /**\n \n ## unpack-trees.c ##\n @@ unpack-trees.c: static void load_gitmodules_file(struct index_state *index,\n 8: 2e76847ec9 ! 8: af8ad95d41 grep: allow submodule functions to run in parallel\n @@ Commit message\n race condition with object reading (for example parse_object() and\n is_promisor_remote()). However, they only call repo_read_gitmodules()\n if it wasn't read before. So let's pre-read it before firing the\n - threads and allow these two functions to safelly be called in\n + threads and allow these two functions to safely be called in\n parallel.\n \n - repo_submodule_init() is already thread-safe, so remove it from the\n @@ Commit message\n protected for now. Let's add a \"NEEDSWORK\" to it, informing why it\n cannot be removed from the critical section yet.\n \n - - Finally, add_to_alternates_memory() must be kept protected by the same\n - reason of the above item.\n + - Finally, add_to_alternates_memory() must be kept protected for the\n + same reason as the item above.\n \n Signed-off-by: Matheus Tavares \n \n 9: 134114b001 ! 9: 0ccf79ba86 grep: protect packed_git [re-]initialization\n @@ Commit message\n happen often, it could be hard to detect. So to prevent future\n headaches, let's force eager initialization of packed_git when setting\n git-grep up. There'll be a small overhead in the cases where we didn't\n - really needed to prepare packed_git during execution but this shouldn't\n - be very noticeable.\n + really need to prepare packed_git during execution but this shouldn't be\n + very noticeable.\n \n Also, packed_git may be re-initialized by\n packfile.c:reprepare_packed_git(). Again, all paths to it in git-grep\n10: 04a4d81ff5 ! 10: 6c09e9169d grep: re-enable threads in non-worktree case\n @@ Commit message\n 8** | 6.2886s | 6.9843s\n \n These are all means of 30 executions after 2 warmup runs. All tests were\n - executed on an i7-7700HQ (quad core w/ hyper-threading), 16GB of RAM and\n + executed on an i7-7700HQ (quad-core w/ hyper-threading), 16GB of RAM and\n SSD, running Manjaro Linux. But to make sure the optimization also\n performs well on HDD, the tests were repeated on another machine with an\n - i5-4210U (dual core w/ hyper-threading), 8GB of RAM and HDD (SATA III,\n + i5-4210U (dual-core w/ hyper-threading), 8GB of RAM and HDD (SATA III,\n 5400 rpm), also running Manjaro Linux:\n \n Threads | Regex 1 | Regex 2\n @@ Commit message\n case when --textconv is used and there're too many text conversions.\n Probably the reason for this is that the object read lock is used to\n protect fill_textconv() and therefore there is a mutual exclusion\n - between textconv execution and object reading. Because both are time\n - consuming operations, not being able to perform them in parallel can\n - cause performance drops. To inform the users about this (and other\n - threading detais), let's also add a \"NOTES ON THREADS\" section to\n + between textconv execution and object reading. Because both are\n + time-consuming operations, not being able to perform them in parallel\n + can cause performance drops. To inform the users about this (and other\n + threading details), let's also add a \"NOTES ON THREADS\" section to\n Documentation/git-grep.txt.\n \n [1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n11: 4f6f8b611c = 11: 2f72f30341 grep: move driver pre-load out of critical section\n -: ---------- > 12: a5891176d7 grep: use no. of cores as the default no. of threads\n-- \n2.24.1\n\n"},{"id":"389843","messageId":"e2f3d377f5408d3d9365b8ac1b785d6d3f0437a9.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 01/12] grep: fix race conditions on userdiff calls","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:49Z","receivedAt":"2020-01-16T02:40:37Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep uses an internal grep_read_mutex to protect object reading\noperations. Similarly, there's a grep_attr_mutex to protect calls to the\ngitattributes machinery. However, two of the three functions protected\nby the last mutex may also perform object reading, as seen below:\n\n- userdiff_get_textconv() > notes_cache_init() >\n notes_cache_match_validity() > lookup_commit_reference_gently() >\n parse_object() > repo_has_object_file() >\n repo_has_object_file_with_flags() > oid_object_info_extended()\n\n- userdiff_find_by_path() > git_check_attr() > collect_some_attrs() >\n prepare_attr_stack() > read_attr() > read_attr_from_index() >\n read_blob_data_from_index() > read_object_file()\n\nAs these calls are not protected by grep_read_mutex, there might be race\nconditions with other threads performing object reading (e.g. threads\ncalling fill_textconv() at grep.c:fill_textconv_grep()). To prevent\nthat, let's make sure to acquire the lock before both of these calls.\n\nNote: this patch might slow down the threaded grep in worktree, for the\nsake of thread-safeness. However, in the following patches, we should\nregain performance by replacing grep_read_mutex for an internal object\nreading lock and allowing parallel inflation during object reading.\n\nSigned-off-by: Matheus Tavares \n---\n grep.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0552b127c1..c028f70aba 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1816,7 +1816,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t * is not thread-safe.\n \t\t */\n \t\tgrep_attr_lock();\n+\t\tgrep_read_lock();\n \t\ttextconv = userdiff_get_textconv(opt->repo, gs->driver);\n+\t\tgrep_read_unlock();\n \t\tgrep_attr_unlock();\n \t}\n \n@@ -2184,8 +2186,11 @@ void grep_source_load_driver(struct grep_source *gs,\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tif (gs->path)\n+\tif (gs->path) {\n+\t\tgrep_read_lock();\n \t\tgs->driver = userdiff_find_by_path(istate, gs->path);\n+\t\tgrep_read_unlock();\n+\t}\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\n-- \n2.24.1\n\n"},{"id":"389844","messageId":"6f0899701b88e255bae68e16e11a978488c0b1cd.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 02/12] grep: fix race conditions at grep_submodule()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:50Z","receivedAt":"2020-01-16T02:40:41Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"There're currently two function calls in builtin/grep.c:grep_submodule()\nwhich might result in race conditions:\n\n- submodule_from_path(): it has config_with_options() in its call stack\n which, in turn, may have read_object_file() in its own. Therefore,\n calling the first function without acquiring grep_read_mutex may end\n up causing a race condition with other object read operations\n performed by worker threads (for example, at the fill_textconv()\n call in grep.c:fill_textconv_grep()).\n- parse_object_or_die(): it falls into the same problem, having\n repo_has_object_file(the_repository, ...) in its call stack. Besides\n that, parse_object(), which is also called by parse_object_or_die(),\n is thread-unsafe and also called by object reading functions.\n\nIt's unlikely to really fall into a data race with these operations as\nthe volume of calls to them is usually very low. But we better protect\nourselves against this possibility, anyway. So, to solve these issues,\nmove both of these function calls into the critical section of\ngrep_read_mutex.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 50ce8d9461..896e7effce 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -407,8 +407,7 @@ static int grep_submodule(struct grep_opt *opt,\n {\n \tstruct repository subrepo;\n \tstruct repository *superproject = opt->repo;\n-\tconst struct submodule *sub = submodule_from_path(superproject,\n-\t\t\t\t\t\t\t &null_oid, path);\n+\tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n \tint hit;\n \n@@ -419,6 +418,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * object.\n \t */\n \tgrep_read_lock();\n+\tsub = submodule_from_path(superproject, &null_oid, path);\n \n \tif (!is_submodule_active(superproject, path)) {\n \t\tgrep_read_unlock();\n@@ -455,9 +455,8 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n-\n \t\tgrep_read_lock();\n+\t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n \t\tdata = read_object_with_reference(&subrepo,\n \t\t\t\t\t\t &object->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-- \n2.24.1\n\n"},{"id":"389845","messageId":"5295c892ee12eb4f8a2fab2cd7e419dc04b18203.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 03/12] grep: fix racy calls in grep_objects()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:51Z","receivedAt":"2020-01-16T02:40:46Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"deref_tag() calls is_promisor_object() and parse_object(), both of which\nperform lazy initializations and other thread-unsafe operations. If it\nwas only called by grep_objects() this wouldn't be a problem as the\nlatter is only executed by the main thread. However, deref_tag() is also\npresent in read_object_file()'s call stack. So calling deref_tag() in\ngrep_objects() without acquiring the grep_read_mutex may incur in a race\ncondition with object reading operations (such as the ones internally\nperformed by fill_textconv(), called at fill_textconv_grep()). The same\nproblem happens with the call to gitmodules_config_oid() which also has\nparse_object() in its call stack. Fix that protecting both calls with\nthe said grep_read_mutex.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 896e7effce..91fc032a32 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -658,13 +658,18 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n+\n+\t\tgrep_read_lock();\n \t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n \t\t\t\t NULL, 0);\n+\t\tgrep_read_unlock();\n \n \t\t/* load the gitmodules file for this rev */\n \t\tif (recurse_submodules) {\n \t\t\tsubmodule_free(opt->repo);\n+\t\t\tgrep_read_lock();\n \t\t\tgitmodules_config_oid(&real_obj->oid);\n+\t\t\tgrep_read_unlock();\n \t\t}\n \t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name,\n \t\t\t\tlist->objects[i].path)) {\n-- \n2.24.1\n\n"},{"id":"389846","messageId":"d7f739bc57b6f59cab7c718300c28b8c6b0a61a8.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 04/12] replace-object: make replace operations thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:52Z","receivedAt":"2020-01-16T02:40:51Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"replace-object functions are very close to being thread-safe: the only\ncurrent racy section is the lazy initialization at\nprepare_replace_object(). The following patches will protect some object\nreading operations to be called threaded, but before that, replace\nfunctions must be protected. To do so, add a mutex to struct\nraw_object_store and acquire it before lazy initializing the\nreplace_map. This won't cause any noticeable performance drop as the\nmutex will no longer be used after the replace_map is initialized.\n\nLater, when the replace functions are called in parallel, thread\ndebuggers might point our use of the added replace_map_initialized flag\nas a data race. However, as this boolean variable is initialized as\nfalse and it's only updated once, there's no real harm. It's perfectly\nfine if the value is updated right after a thread read it in\nreplace-map.h:lookup_replace_object() (there'll only be a performance\npenalty for the affected threads at that moment). We could cease the\ndebugger warning protecting the variable reading at the said function.\nHowever, this would negatively affect performance for all threads\ncalling it, at any time, so it's not really worthy since the warning\ndoesn't represent a real problem. Instead, to make sure we don't get\nfalse positives (at ThreadSanitizer, at least) an entry for the\nrespective function is added to .tsan-suppressions.\n\nSigned-off-by: Matheus Tavares \n---\n .tsan-suppressions | 6 ++++++\n object-store.h | 2 ++\n object.c | 2 ++\n replace-object.c | 11 ++++++++++-\n replace-object.h | 7 ++++++-\n 5 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nindex 8c85014a0a..5ba86d6845 100644\n--- a/.tsan-suppressions\n+++ b/.tsan-suppressions\n@@ -8,3 +8,9 @@\n # in practice it (hopefully!) doesn't matter.\n race:^want_color$\n race:^transfer_debug$\n+\n+# A boolean value, which tells whether the replace_map has been initialized or\n+# not, is read racily with an update. As this variable is written to only once,\n+# and it's OK if the value change right after reading it, this shouldn't be a\n+# problem.\n+race:^lookup_replace_object$\ndiff --git a/object-store.h b/object-store.h\nindex 55ee639350..33739c9dee 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -125,6 +125,8 @@ struct raw_object_store {\n \t * (see git-replace(1)).\n \t */\n \tstruct oidmap *replace_map;\n+\tunsigned replace_map_initialized : 1;\n+\tpthread_mutex_t replace_mutex; /* protect object replace functions */\n \n \tstruct commit_graph *commit_graph;\n \tunsigned commit_graph_attempted : 1; /* if loading has been attempted */\ndiff --git a/object.c b/object.c\nindex 142ef69399..b4e1d3db3c 100644\n--- a/object.c\n+++ b/object.c\n@@ -480,6 +480,7 @@ struct raw_object_store *raw_object_store_new(void)\n \tmemset(o, 0, sizeof(*o));\n \tINIT_LIST_HEAD(&o->packed_git_mru);\n \thashmap_init(&o->pack_map, pack_map_entry_cmp, NULL, 0);\n+\tpthread_mutex_init(&o->replace_mutex, NULL);\n \treturn o;\n }\n \n@@ -507,6 +508,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n \n \toidmap_free(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n+\tpthread_mutex_destroy(&o->replace_mutex);\n \n \tfree_commit_graph(o->commit_graph);\n \to->commit_graph = NULL;\ndiff --git a/replace-object.c b/replace-object.c\nindex e295e87943..7bd9aba6ee 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -34,14 +34,23 @@ static int register_replace_ref(struct repository *r,\n \n void prepare_replace_object(struct repository *r)\n {\n-\tif (r->objects->replace_map)\n+\tif (r->objects->replace_map_initialized)\n \t\treturn;\n \n+\tpthread_mutex_lock(&r->objects->replace_mutex);\n+\tif (r->objects->replace_map_initialized) {\n+\t\tpthread_mutex_unlock(&r->objects->replace_mutex);\n+\t\treturn;\n+\t}\n+\n \tr->objects->replace_map =\n \t\txmalloc(sizeof(*r->objects->replace_map));\n \toidmap_init(r->objects->replace_map, 0);\n \n \tfor_each_replace_ref(r, register_replace_ref, NULL);\n+\tr->objects->replace_map_initialized = 1;\n+\n+\tpthread_mutex_unlock(&r->objects->replace_mutex);\n }\n \n /* We allow \"recursive\" replacement. Only within reason, though */\ndiff --git a/replace-object.h b/replace-object.h\nindex 04ed7a85a2..3fbc32eb7b 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -24,12 +24,17 @@ const struct object_id *do_lookup_replace_object(struct repository *r,\n * name (replaced recursively, if necessary). The return value is\n * either sha1 or a pointer to a permanently-allocated value. When\n * object replacement is suppressed, always return sha1.\n+ *\n+ * Note: some thread debuggers might point a data race on the\n+ * replace_map_initialized reading in this function. However, we know there's no\n+ * problem in the value being updated by one thread right after another one read\n+ * it here (and it should be written to only once, anyway).\n */\n static inline const struct object_id *lookup_replace_object(struct repository *r,\n \t\t\t\t\t\t\t const struct object_id *oid)\n {\n \tif (!read_replace_refs ||\n-\t (r->objects->replace_map &&\n+\t (r->objects->replace_map_initialized &&\n \t r->objects->replace_map->map.tablesize == 0))\n \t\treturn oid;\n \treturn do_lookup_replace_object(r, oid);\n-- \n2.24.1\n\n"},{"id":"389847","messageId":"b72e90f229dbf7d5be016fd6251a9b3ef76f2431.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 05/12] object-store: allow threaded access to object reading","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:53Z","receivedAt":"2020-01-16T02:40:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Allow object reading to be performed by multiple threads protecting it\nwith an internal lock, the obj_read_mutex. The lock usage can be toggled\nwith enable_obj_read_lock() and disable_obj_read_lock(). Currently, the\nfunctions which can be safely called in parallel are:\nread_object_file_extended(), repo_read_object_file(),\nread_object_file(), read_object_with_reference(), read_object(),\noid_object_info() and oid_object_info_extended(). It's also possible\nto use obj_read_lock() and obj_read_unlock() to protect other sections\nthat cannot execute in parallel with object reading.\n\nProbably there are many spots in the functions listed above that could\nbe executed unlocked (and thus, in parallel). But, for now, we are most\ninterested in allowing parallel access to zlib inflation. This is one of\nthe sections where object reading spends most of the time in (e.g. up to\none-third of git-grep's execution time in the chromium repo corresponds\nto inflation) and it's already thread-safe. So, to take advantage of\nthat, the obj_read_mutex is released when calling git_inflate() and\nre-acquired right after, for every calling spot in\noid_object_info_extended()'s call chain. We may refine this lock to also\nexploit other possible parallel spots in the future, but for now,\nthreaded zlib inflation should already give great speedups for threaded\nobject reading callers.\n\nNote that add_delta_base_cache() was also modified to skip adding\nalready present entries to the cache. This wasn't possible before, but\nit would be now, with the parallel inflation. Take for example the\nfollowing situation, where two threads - A and B - are executing the\ncode at unpack_entry():\n\n1. Thread A is performing the decompression of a base O (which is not\n yet in the cache) at PHASE II. Thread B is simultaneously trying to\n unpack O, but just starting at PHASE I.\n2. Since O is not yet in the cache, B will go to PHASE II to also\n perform the decompression.\n3. When they finish decompressing, one of them will get the object\n reading mutex and go to PHASE III while the other waits for the\n mutex. Let’s say A got the mutex first.\n4. Thread A will add O to the cache, go throughout the rest of PHASE III\n and return.\n5. Thread B gets the mutex, also add O to the cache (if the check wasn't\n there) and returns.\n\nFinally, it is also important to highlight that the object reading lock\ncan only ensure thread-safety in the mentioned functions thanks to two\ncomplementary mechanisms: the use of 'struct raw_object_store's\nreplace_mutex, which guards sections in the object reading machinery\nthat would otherwise be thread-unsafe; and the 'struct pack_window's\ninuse_cnt, which protects window reading operations (such as the one\nperformed during the inflation of a packed object), allowing them to\nexecute without the acquisition of the obj_read_mutex.\n\nSigned-off-by: Matheus Tavares \n---\n object-store.h | 35 +++++++++++++++++++++++++++++++\n packfile.c | 32 ++++++++++++++++++++++++++++\n sha1-file.c | 57 +++++++++++++++++++++++++++++++++++++++++++++-----\n 3 files changed, 119 insertions(+), 5 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 33739c9dee..7c80e0d64c 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -6,6 +6,7 @@\n #include \"list.h\"\n #include \"sha1-array.h\"\n #include \"strbuf.h\"\n+#include \"thread-utils.h\"\n \n struct object_directory {\n \tstruct object_directory *next;\n@@ -251,6 +252,40 @@ int has_loose_object_nonlocal(const struct object_id *);\n \n void assert_oid_type(const struct object_id *oid, enum object_type expect);\n \n+/*\n+ * Enabling the object read lock allows multiple threads to safely call the\n+ * following functions in parallel: repo_read_object_file(), read_object_file(),\n+ * read_object_file_extended(), read_object_with_reference(), read_object(),\n+ * oid_object_info() and oid_object_info_extended().\n+ *\n+ * obj_read_lock() and obj_read_unlock() may also be used to protect other\n+ * section which cannot execute in parallel with object reading. Since the used\n+ * lock is a recursive mutex, these sections can even contain calls to object\n+ * reading functions. However, beware that in these cases zlib inflation won't\n+ * be performed in parallel, losing performance.\n+ *\n+ * TODO: oid_object_info_extended()'s call stack has a recursive behavior. If\n+ * any of its callees end up calling it, this recursive call won't benefit from\n+ * parallel inflation.\n+ */\n+void enable_obj_read_lock(void);\n+void disable_obj_read_lock(void);\n+\n+extern int obj_read_use_lock;\n+extern pthread_mutex_t obj_read_mutex;\n+\n+static inline void obj_read_lock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_lock(&obj_read_mutex);\n+}\n+\n+static inline void obj_read_unlock(void)\n+{\n+\tif(obj_read_use_lock)\n+\t\tpthread_mutex_unlock(&obj_read_mutex);\n+}\n+\n struct object_info {\n \t/* Request */\n \tenum object_type *typep;\ndiff --git a/packfile.c b/packfile.c\nindex 7e7c04e4d8..24a73fc33a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1086,7 +1086,23 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\t/*\n+\t\t * Note: the window section returned by use_pack() must be\n+\t\t * available throughout git_inflate()'s unlocked execution. To\n+\t\t * ensure no other thread will modify the window in the\n+\t\t * meantime, we rely on the packed_window.inuse_cnt. This\n+\t\t * counter is incremented before window reading and checked\n+\t\t * before window disposal.\n+\t\t *\n+\t\t * Other worrying sections could be the call to close_pack_fd(),\n+\t\t * which can close packs even with in-use windows, and to\n+\t\t * reprepare_packed_git(). Regarding the former, mmap doc says:\n+\t\t * \"closing the file descriptor does not unmap the region\". And\n+\t\t * for the latter, it won't re-open already available packs.\n+\t\t */\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tcurpos += stream.next_in - in;\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n@@ -1445,6 +1461,14 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,\n \tstruct delta_base_cache_entry *ent = xmalloc(sizeof(*ent));\n \tstruct list_head *lru, *tmp;\n \n+\t/*\n+\t * Check required to avoid redundant entries when more than one thread\n+\t * is unpacking the same object, in unpack_entry() (since its phases I\n+\t * and III might run concurrently across multiple threads).\n+\t */\n+\tif (in_delta_base_cache(p, base_offset))\n+\t\treturn;\n+\n \tdelta_base_cached += base_size;\n \n \tlist_for_each_safe(lru, tmp, &delta_base_cache_lru) {\n@@ -1574,7 +1598,15 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tdo {\n \t\tin = use_pack(p, w_curs, curpos, &stream.avail_in);\n \t\tstream.next_in = in;\n+\t\t/*\n+\t\t * Note: we must ensure the window section returned by\n+\t\t * use_pack() will be available throughout git_inflate()'s\n+\t\t * unlocked execution. Please refer to the comment at\n+\t\t * get_size_from_delta() to see how this is done.\n+\t\t */\n+\t\tobj_read_unlock();\n \t\tst = git_inflate(&stream, Z_FINISH);\n+\t\tobj_read_lock();\n \t\tif (!stream.avail_out)\n \t\t\tbreak; /* the payload is larger than it should be */\n \t\tcurpos += stream.next_in - in;\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..9dc0649748 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1147,6 +1147,8 @@ static int unpack_loose_short_header(git_zstream *stream,\n \t\t\t\t unsigned char *map, unsigned long mapsize,\n \t\t\t\t void *buffer, unsigned long bufsiz)\n {\n+\tint ret;\n+\n \t/* Get the data stream */\n \tmemset(stream, 0, sizeof(*stream));\n \tstream->next_in = map;\n@@ -1155,7 +1157,11 @@ static int unpack_loose_short_header(git_zstream *stream,\n \tstream->avail_out = bufsiz;\n \n \tgit_inflate_init(stream);\n-\treturn git_inflate(stream, 0);\n+\tobj_read_unlock();\n+\tret = git_inflate(stream, 0);\n+\tobj_read_lock();\n+\n+\treturn ret;\n }\n \n int unpack_loose_header(git_zstream *stream,\n@@ -1200,7 +1206,9 @@ static int unpack_loose_header_to_strbuf(git_zstream *stream, unsigned char *map\n \tstream->avail_out = bufsiz;\n \n \tdo {\n+\t\tobj_read_unlock();\n \t\tstatus = git_inflate(stream, 0);\n+\t\tobj_read_lock();\n \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n@@ -1240,8 +1248,11 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\t */\n \t\tstream->next_out = buf + bytes;\n \t\tstream->avail_out = size - bytes;\n-\t\twhile (status == Z_OK)\n+\t\twhile (status == Z_OK) {\n+\t\t\tobj_read_unlock();\n \t\t\tstatus = git_inflate(stream, Z_FINISH);\n+\t\t\tobj_read_lock();\n+\t\t}\n \t}\n \tif (status == Z_STREAM_END && !stream->avail_in) {\n \t\tgit_inflate_end(stream);\n@@ -1411,10 +1422,32 @@ static int loose_object_info(struct repository *r,\n \treturn (status < 0) ? status : 0;\n }\n \n+int obj_read_use_lock = 0;\n+pthread_mutex_t obj_read_mutex;\n+\n+void enable_obj_read_lock(void)\n+{\n+\tif (obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 1;\n+\tinit_recursive_mutex(&obj_read_mutex);\n+}\n+\n+void disable_obj_read_lock(void)\n+{\n+\tif (!obj_read_use_lock)\n+\t\treturn;\n+\n+\tobj_read_use_lock = 0;\n+\tpthread_mutex_destroy(&obj_read_mutex);\n+}\n+\n int fetch_if_missing = 1;\n \n-int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n-\t\t\t struct object_info *oi, unsigned flags)\n+static int do_oid_object_info_extended(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct object_info *oi, unsigned flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n \tstruct pack_entry e;\n@@ -1422,6 +1455,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \tconst struct object_id *real = oid;\n \tint already_retried = 0;\n \n+\n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(r, oid);\n \n@@ -1497,7 +1531,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \trtype = packed_object_info(r, e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, real->hash);\n-\t\treturn oid_object_info_extended(r, real, oi, 0);\n+\t\treturn do_oid_object_info_extended(r, real, oi, 0);\n \t} else if (oi->whence == OI_PACKED) {\n \t\toi->u.packed.offset = e.offset;\n \t\toi->u.packed.pack = e.p;\n@@ -1508,6 +1542,17 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \treturn 0;\n }\n \n+int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n+\t\t\t struct object_info *oi, unsigned flags)\n+{\n+\tint ret;\n+\tobj_read_lock();\n+\tret = do_oid_object_info_extended(r, oid, oi, flags);\n+\tobj_read_unlock();\n+\treturn ret;\n+}\n+\n+\n /* returns enum object_type or negative */\n int oid_object_info(struct repository *r,\n \t\t const struct object_id *oid,\n@@ -1580,6 +1625,7 @@ void *read_object_file_extended(struct repository *r,\n \tif (data)\n \t\treturn data;\n \n+\tobj_read_lock();\n \tif (errno && errno != ENOENT)\n \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n \n@@ -1595,6 +1641,7 @@ void *read_object_file_extended(struct repository *r,\n \tif ((p = has_packed_and_bad(r, repl->hash)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), p->pack_name);\n+\tobj_read_unlock();\n \n \treturn NULL;\n }\n-- \n2.24.1\n\n"},{"id":"389848","messageId":"fc1200bb07f749420dad044d39dfe30ae73ad640.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 06/12] grep: replace grep_read_mutex by internal obj read lock","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:54Z","receivedAt":"2020-01-16T02:41:01Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"git-grep uses 'grep_read_mutex' to protect its calls to object reading\noperations. But these have their own internal lock now, which ensures a\nbetter performance (allowing parallel access to more regions). So, let's\nremove the former and, instead, activate the latter with\nenable_obj_read_lock().\n\nSections that are currently protected by 'grep_read_mutex' but are not\ninternally protected by the object reading lock should be surrounded by\nobj_read_lock() and obj_read_unlock(). These guarantee mutual exclusion\nwith object reading operations, keeping the current behavior and\navoiding race conditions. Namely, these places are:\n\n In grep.c:\n\n - fill_textconv() at fill_textconv_grep().\n - userdiff_get_textconv() at grep_source_1().\n\n In builtin/grep.c:\n\n - parse_object_or_die() and the submodule functions at\n grep_submodule().\n - deref_tag() and gitmodules_config_oid() at grep_objects().\n\nIf these functions become thread-safe, in the future, we might remove\nthe locking and probably get some speedup.\n\nNote that some of the submodule functions will already be thread-safe\n(or close to being thread-safe) with the internal object reading lock.\nHowever, as some of them will require additional modifications to be\nremoved from the critical section, this will be done in its own patch.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 46 ++++++++++++++++------------------------------\n grep.c | 39 +++++++++++++++++++--------------------\n grep.h | 13 -------------\n 3 files changed, 35 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 91fc032a32..4a436d6c99 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -200,12 +200,12 @@ static void start_threads(struct grep_opt *opt)\n \tint i;\n \n \tpthread_mutex_init(&grep_mutex, NULL);\n-\tpthread_mutex_init(&grep_read_mutex, NULL);\n \tpthread_mutex_init(&grep_attr_mutex, NULL);\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n \tgrep_use_locks = 1;\n+\tenable_obj_read_lock();\n \n \tfor (i = 0; i < ARRAY_SIZE(todo); i++) {\n \t\tstrbuf_init(&todo[i].out, 0);\n@@ -257,12 +257,12 @@ static int wait_all(void)\n \tfree(threads);\n \n \tpthread_mutex_destroy(&grep_mutex);\n-\tpthread_mutex_destroy(&grep_read_mutex);\n \tpthread_mutex_destroy(&grep_attr_mutex);\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n \tgrep_use_locks = 0;\n+\tdisable_obj_read_lock();\n \n \treturn hit;\n }\n@@ -295,16 +295,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n \treturn st;\n }\n \n-static void *lock_and_read_oid_file(const struct object_id *oid, enum object_type *type, unsigned long *size)\n-{\n-\tvoid *data;\n-\n-\tgrep_read_lock();\n-\tdata = read_object_file(oid, type, size);\n-\tgrep_read_unlock();\n-\treturn data;\n-}\n-\n static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\t const char *filename, int tree_name_len,\n \t\t const char *path)\n@@ -413,20 +403,20 @@ static int grep_submodule(struct grep_opt *opt,\n \n \t/*\n \t * NEEDSWORK: submodules functions need to be protected because they\n-\t * access the object store via config_from_gitmodules(): the latter\n-\t * uses get_oid() which, for now, relies on the global the_repository\n-\t * object.\n+\t * call config_from_gitmodules(): the latter contains in its call stack\n+\t * many thread-unsafe operations that are racy with object reading, such\n+\t * as parse_object() and is_promisor_object().\n \t */\n-\tgrep_read_lock();\n+\tobj_read_lock();\n \tsub = submodule_from_path(superproject, &null_oid, path);\n \n \tif (!is_submodule_active(superproject, path)) {\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \t\treturn 0;\n \t}\n \n \tif (repo_submodule_init(&subrepo, superproject, sub)) {\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \t\treturn 0;\n \t}\n \n@@ -443,7 +433,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * object.\n \t */\n \tadd_to_alternates_memory(subrepo.objects->odb->path);\n-\tgrep_read_unlock();\n+\tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n \tsubopt.repo = &subrepo;\n@@ -455,13 +445,12 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tgrep_read_lock();\n+\t\tobj_read_lock();\n \t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n+\t\tobj_read_unlock();\n \t\tdata = read_object_with_reference(&subrepo,\n \t\t\t\t\t\t &object->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-\t\tgrep_read_unlock();\n-\n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n \n@@ -586,7 +575,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_oid_file(&entry.oid, &type, &size);\n+\t\t\tdata = read_object_file(&entry.oid, &type, &size);\n \t\t\tif (!data)\n \t\t\t\tdie(_(\"unable to read tree (%s)\"),\n \t\t\t\t oid_to_hex(&entry.oid));\n@@ -624,12 +613,9 @@ 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\tgrep_read_lock();\n \t\tdata = read_object_with_reference(opt->repo,\n \t\t\t\t\t\t &obj->oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n-\t\tgrep_read_unlock();\n-\n \t\tif (!data)\n \t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&obj->oid));\n \n@@ -659,17 +645,17 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \n-\t\tgrep_read_lock();\n+\t\tobj_read_lock();\n \t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n \t\t\t\t NULL, 0);\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \n \t\t/* load the gitmodules file for this rev */\n \t\tif (recurse_submodules) {\n \t\t\tsubmodule_free(opt->repo);\n-\t\t\tgrep_read_lock();\n+\t\t\tobj_read_lock();\n \t\t\tgitmodules_config_oid(&real_obj->oid);\n-\t\t\tgrep_read_unlock();\n+\t\t\tobj_read_unlock();\n \t\t}\n \t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name,\n \t\t\t\tlist->objects[i].path)) {\ndiff --git a/grep.c b/grep.c\nindex c028f70aba..13232a904a 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1540,11 +1540,6 @@ static inline void grep_attr_unlock(void)\n \t\tpthread_mutex_unlock(&grep_attr_mutex);\n }\n \n-/*\n- * Same as git_attr_mutex, but protecting the thread-unsafe object db access.\n- */\n-pthread_mutex_t grep_read_mutex;\n-\n static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n@@ -1741,13 +1736,20 @@ static int fill_textconv_grep(struct repository *r,\n \t}\n \n \t/*\n-\t * fill_textconv is not remotely thread-safe; it may load objects\n-\t * behind the scenes, and it modifies the global diff tempfile\n-\t * structure.\n+\t * fill_textconv is not remotely thread-safe; it modifies the global\n+\t * diff tempfile structure, writes to the_repo's odb and might\n+\t * internally call thread-unsafe functions such as the\n+\t * prepare_packed_git() lazy-initializator. Because of the last two, we\n+\t * must ensure mutual exclusion between this call and the object reading\n+\t * API, thus we use obj_read_lock() here.\n+\t *\n+\t * TODO: allowing text conversion to run in parallel with object\n+\t * reading operations might increase performance in the multithreaded\n+\t * non-worktreee git-grep with --textconv.\n \t */\n-\tgrep_read_lock();\n+\tobj_read_lock();\n \tsize = fill_textconv(r, driver, df, &buf);\n-\tgrep_read_unlock();\n+\tobj_read_unlock();\n \tfree_filespec(df);\n \n \t/*\n@@ -1813,12 +1815,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\tgrep_source_load_driver(gs, opt->repo->index);\n \t\t/*\n \t\t * We might set up the shared textconv cache data here, which\n-\t\t * is not thread-safe.\n+\t\t * is not thread-safe. Also, get_oid_with_context() and\n+\t\t * parse_object() might be internally called. As they are not\n+\t\t * currenty thread-safe and might be racy with object reading,\n+\t\t * obj_read_lock() must be called.\n \t\t */\n \t\tgrep_attr_lock();\n-\t\tgrep_read_lock();\n+\t\tobj_read_lock();\n \t\ttextconv = userdiff_get_textconv(opt->repo, gs->driver);\n-\t\tgrep_read_unlock();\n+\t\tobj_read_unlock();\n \t\tgrep_attr_unlock();\n \t}\n \n@@ -2118,10 +2123,7 @@ static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n \n-\tgrep_read_lock();\n \tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n-\tgrep_read_unlock();\n-\n \tif (!gs->buf)\n \t\treturn error(_(\"'%s': unable to read %s\"),\n \t\t\t gs->name,\n@@ -2186,11 +2188,8 @@ void grep_source_load_driver(struct grep_source *gs,\n \t\treturn;\n \n \tgrep_attr_lock();\n-\tif (gs->path) {\n-\t\tgrep_read_lock();\n+\tif (gs->path)\n \t\tgs->driver = userdiff_find_by_path(istate, gs->path);\n-\t\tgrep_read_unlock();\n-\t}\n \tif (!gs->driver)\n \t\tgs->driver = userdiff_find_by_name(\"default\");\n \tgrep_attr_unlock();\ndiff --git a/grep.h b/grep.h\nindex 811fd274c9..9115db8515 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -220,18 +220,5 @@ int grep_threads_ok(const struct grep_opt *opt);\n */\n extern int grep_use_locks;\n extern pthread_mutex_t grep_attr_mutex;\n-extern pthread_mutex_t grep_read_mutex;\n-\n-static inline void grep_read_lock(void)\n-{\n-\tif (grep_use_locks)\n-\t\tpthread_mutex_lock(&grep_read_mutex);\n-}\n-\n-static inline void grep_read_unlock(void)\n-{\n-\tif (grep_use_locks)\n-\t\tpthread_mutex_unlock(&grep_read_mutex);\n-}\n \n #endif\n-- \n2.24.1\n\n"},{"id":"389849","messageId":"d39d2ce9c4c4975969a7b99cbe1ee6c8abb586c1.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 07/12] submodule-config: add skip_if_read option to repo_read_gitmodules()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:55Z","receivedAt":"2020-01-16T02:41:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Currently, submodule-config.c doesn't have an externally accessible\nfunction to read gitmodules only if it wasn't already read. But this\nexact behavior is internally implemented by gitmodules_read_check(), to\nperform a lazy load. Let's merge this function with\nrepo_read_gitmodules() adding a 'skip_if_read' which allows both\ninternal and external callers to access this functionality. This\nsimplifies a little the code. The added option will also be used in\nthe following patch.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 2 +-\n submodule-config.c | 18 ++++++------------\n submodule-config.h | 2 +-\n unpack-trees.c | 4 ++--\n 4 files changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 4a436d6c99..d3ed05c1da 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -420,7 +420,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\treturn 0;\n \t}\n \n-\trepo_read_gitmodules(&subrepo);\n+\trepo_read_gitmodules(&subrepo, 0);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 85064810b2..bd5e14ab20 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -674,10 +674,13 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \treturn parse_config(var, value, ¶meter);\n }\n \n-void repo_read_gitmodules(struct repository *repo)\n+void repo_read_gitmodules(struct repository *repo, int skip_if_read)\n {\n \tsubmodule_cache_check_init(repo);\n \n+\tif (repo->submodule_cache->gitmodules_read && skip_if_read)\n+\t\treturn;\n+\n \tif (repo_read_index(repo) < 0)\n \t\treturn;\n \n@@ -703,20 +706,11 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tthe_repository->submodule_cache->gitmodules_read = 1;\n }\n \n-static void gitmodules_read_check(struct repository *repo)\n-{\n-\tsubmodule_cache_check_init(repo);\n-\n-\t/* read the repo's .gitmodules file if it hasn't been already */\n-\tif (!repo->submodule_cache->gitmodules_read)\n-\t\trepo_read_gitmodules(repo);\n-}\n-\n const struct submodule *submodule_from_name(struct repository *r,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\tconst char *name)\n {\n-\tgitmodules_read_check(r);\n+\trepo_read_gitmodules(r, 1);\n \treturn config_from(r->submodule_cache, treeish_name, name, lookup_name);\n }\n \n@@ -724,7 +718,7 @@ const struct submodule *submodule_from_path(struct repository *r,\n \t\t\t\t\t const struct object_id *treeish_name,\n \t\tconst char *path)\n {\n-\tgitmodules_read_check(r);\n+\trepo_read_gitmodules(r, 1);\n \treturn config_from(r->submodule_cache, treeish_name, path, lookup_path);\n }\n \ndiff --git a/submodule-config.h b/submodule-config.h\nindex 42918b55e8..c11e22cf50 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -61,7 +61,7 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t\t const char *arg, int unset);\n int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-void repo_read_gitmodules(struct repository *repo);\n+void repo_read_gitmodules(struct repository *repo, int skip_if_read);\n void gitmodules_config_oid(const struct object_id *commit_oid);\n \n /**\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2399b6818b..f5a8051803 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -291,11 +291,11 @@ static void load_gitmodules_file(struct index_state *index,\n \tif (pos >= 0) {\n \t\tstruct cache_entry *ce = index->cache[pos];\n \t\tif (!state && ce->ce_flags & CE_WT_REMOVE) {\n-\t\t\trepo_read_gitmodules(the_repository);\n+\t\t\trepo_read_gitmodules(the_repository, 0);\n \t\t} else if (state && (ce->ce_flags & CE_UPDATE)) {\n \t\t\tsubmodule_free(the_repository);\n \t\t\tcheckout_entry(ce, state, NULL, NULL);\n-\t\t\trepo_read_gitmodules(the_repository);\n+\t\t\trepo_read_gitmodules(the_repository, 0);\n \t\t}\n \t}\n }\n-- \n2.24.1\n\n"},{"id":"389850","messageId":"af8ad95d413aa3d763769eb3ae9544e25ccbe2d1.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:56Z","receivedAt":"2020-01-16T02:41:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Now that object reading operations are internally protected, the\nsubmodule initialization functions at builtin/grep.c:grep_submodule()\nare very close to being thread-safe. Let's take a look at each call and\nremove from the critical section what we can, for better performance:\n\n- submodule_from_path() and is_submodule_active() cannot be called in\n parallel yet only because they call repo_read_gitmodules() which\n contains, in its call stack, operations that would otherwise be in\n race condition with object reading (for example parse_object() and\n is_promisor_remote()). However, they only call repo_read_gitmodules()\n if it wasn't read before. So let's pre-read it before firing the\n threads and allow these two functions to safely be called in\n parallel.\n\n- repo_submodule_init() is already thread-safe, so remove it from the\n critical section without other necessary changes.\n\n- The repo_read_gitmodules(&subrepo) call at grep_submodule() is safe as\n no other thread is performing object reading operations in the subrepo\n yet. However, threads might be working in the superproject, and this\n function calls add_to_alternates_memory() internally, which is racy\n with object readings in the superproject. So it must be kept\n protected for now. Let's add a \"NEEDSWORK\" to it, informing why it\n cannot be removed from the critical section yet.\n\n- Finally, add_to_alternates_memory() must be kept protected for the\n same reason as the item above.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 38 ++++++++++++++++++++++----------------\n 1 file changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d3ed05c1da..ac3d86c2e5 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -401,25 +401,23 @@ static int grep_submodule(struct grep_opt *opt,\n \tstruct grep_opt subopt;\n \tint hit;\n \n-\t/*\n-\t * NEEDSWORK: submodules functions need to be protected because they\n-\t * call config_from_gitmodules(): the latter contains in its call stack\n-\t * many thread-unsafe operations that are racy with object reading, such\n-\t * as parse_object() and is_promisor_object().\n-\t */\n-\tobj_read_lock();\n \tsub = submodule_from_path(superproject, &null_oid, path);\n \n-\tif (!is_submodule_active(superproject, path)) {\n-\t\tobj_read_unlock();\n+\tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n-\t}\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n-\t\tobj_read_unlock();\n+\tif (repo_submodule_init(&subrepo, superproject, sub))\n \t\treturn 0;\n-\t}\n \n+\t/*\n+\t * NEEDSWORK: repo_read_gitmodules() might call\n+\t * add_to_alternates_memory() via config_from_gitmodules(). This\n+\t * operation causes a race condition with concurrent object readings\n+\t * performed by the worker threads. That's why we need obj_read_lock()\n+\t * here. It should be removed once it's no longer necessary to add the\n+\t * subrepo's odbs to the in-memory alternates list.\n+\t */\n+\tobj_read_lock();\n \trepo_read_gitmodules(&subrepo, 0);\n \n \t/*\n@@ -1052,6 +1050,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tpathspec.recursive = 1;\n \tpathspec.recurse_submodules = !!recurse_submodules;\n \n+\tif (recurse_submodules && (!use_index || untracked))\n+\t\tdie(_(\"option not supported with --recurse-submodules\"));\n+\n \tif (list.nr || cached || show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n@@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t && (opt.pre_context || opt.post_context ||\n \t\t\topt.file_break || opt.funcbody))\n \t\t\tskip_first_line = 1;\n+\n+\t\t/*\n+\t\t * Pre-read gitmodules (if not read already) to prevent racy\n+\t\t * lazy reading in worker threads.\n+\t\t */\n+\t\tif (recurse_submodules)\n+\t\t\trepo_read_gitmodules(the_repository, 1);\n+\n \t\tstart_threads(&opt);\n \t} else {\n \t\t/*\n@@ -1105,9 +1114,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (recurse_submodules && (!use_index || untracked))\n-\t\tdie(_(\"option not supported with --recurse-submodules\"));\n-\n \tif (!show_in_pager && !opt.status_only)\n \t\tsetup_pager();\n \n-- \n2.24.1\n\n"},{"id":"389851","messageId":"0ccf79ba863a1a512506cc3aae4cc523d64ab8ae.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 09/12] grep: protect packed_git [re-]initialization","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:57Z","receivedAt":"2020-01-16T02:41:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Some fields in struct raw_object_store are lazy initialized by the\nthread-unsafe packfile.c:prepare_packed_git(). Although this function is\npresent in the call stack of git-grep threads, all paths to it are\ncurrently protected by obj_read_lock() (and the main thread usually\nindirectly calls it before firing the worker threads, anyway). However,\nit's possible that future modifications add new unprotected paths to it,\nintroducing a race condition. Because errors derived from it wouldn't\nhappen often, it could be hard to detect. So to prevent future\nheadaches, let's force eager initialization of packed_git when setting\ngit-grep up. There'll be a small overhead in the cases where we didn't\nreally need to prepare packed_git during execution but this shouldn't be\nvery noticeable.\n\nAlso, packed_git may be re-initialized by\npackfile.c:reprepare_packed_git(). Again, all paths to it in git-grep\nare already protected by obj_read_lock() but it may suffer from the same\nproblem in the future. So let's also internally protect it with\nobj_read_lock() (which is a recursive mutex).\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 8 ++++++--\n packfile.c | 2 ++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ac3d86c2e5..1535fd50f8 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,6 +24,7 @@\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"object-store.h\"\n+#include \"packfile.h\"\n \n static char const * const grep_usage[] = {\n \tN_(\"git grep [] [-e] [...] [[--] ...]\"),\n@@ -1074,11 +1075,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tskip_first_line = 1;\n \n \t\t/*\n-\t\t * Pre-read gitmodules (if not read already) to prevent racy\n-\t\t * lazy reading in worker threads.\n+\t\t * Pre-read gitmodules (if not read already) and force eager\n+\t\t * initialization of packed_git to prevent racy lazy\n+\t\t * reading/initialization once worker threads are started.\n \t\t */\n \t\tif (recurse_submodules)\n \t\t\trepo_read_gitmodules(the_repository, 1);\n+\t\tif (startup_info->have_repository)\n+\t\t\t(void)get_packed_git(the_repository);\n \n \t\tstart_threads(&opt);\n \t} else {\ndiff --git a/packfile.c b/packfile.c\nindex 24a73fc33a..946ca83e7a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1004,12 +1004,14 @@ void reprepare_packed_git(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n+\tobj_read_lock();\n \tfor (odb = r->objects->odb; odb; odb = odb->next)\n \t\todb_clear_loose_cache(odb);\n \n \tr->objects->approximate_object_count_valid = 0;\n \tr->objects->packed_git_initialized = 0;\n \tprepare_packed_git(r);\n+\tobj_read_unlock();\n }\n \n struct packed_git *get_packed_git(struct repository *r)\n-- \n2.24.1\n\n"},{"id":"389853","messageId":"6c09e9169dfb21fc2cd3f69700316d3a87e72019.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 10/12] grep: re-enable threads in non-worktree case","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:58Z","receivedAt":"2020-01-16T02:41:19Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"They were disabled at 53b8d93 (\"grep: disable threading in non-worktree\ncase\", 12-12-2011), due to observable performance drops (to the point\nthat using a single thread would be faster than multiple threads). But\nnow that zlib inflation can be performed in parallel we can regain the\nspeedup, so let's re-enable threads in non-worktree grep.\n\nGrepping 'abcd[02]' (\"Regex 1\") and '(static|extern) (int|double) \\*'\n(\"Regex 2\") at chromium's repository[1] I got:\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 17.2920s | 20.9624s\n 2 | 9.6512s | 11.3184s\n 4 | 6.7723s | 7.6268s\n 8** | 6.2886s | 6.9843s\n\nThese are all means of 30 executions after 2 warmup runs. All tests were\nexecuted on an i7-7700HQ (quad-core w/ hyper-threading), 16GB of RAM and\nSSD, running Manjaro Linux. But to make sure the optimization also\nperforms well on HDD, the tests were repeated on another machine with an\ni5-4210U (dual-core w/ hyper-threading), 8GB of RAM and HDD (SATA III,\n5400 rpm), also running Manjaro Linux:\n\n Threads | Regex 1 | Regex 2\n---------|------------|-----------\n 1 | 18.4035s | 22.5368s\n 2 | 12.5063s | 14.6409s\n 4** | 10.9136s | 12.7106s\n\n** Note that in these cases we relied on hyper-threading, and that's\n probably why we don't see a big difference in time.\n\nUnfortunately, multithreaded git-grep might be slow in the non-worktree\ncase when --textconv is used and there're too many text conversions.\nProbably the reason for this is that the object read lock is used to\nprotect fill_textconv() and therefore there is a mutual exclusion\nbetween textconv execution and object reading. Because both are\ntime-consuming operations, not being able to perform them in parallel\ncan cause performance drops. To inform the users about this (and other\nthreading details), let's also add a \"NOTES ON THREADS\" section to\nDocumentation/git-grep.txt.\n\n[1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/git-grep.txt | 11 +++++++++++\n builtin/grep.c | 2 +-\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex c89fb569e3..de628741fa 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -347,6 +347,17 @@ EXAMPLES\n `git grep solution -- :^Documentation`::\n \tLooks for `solution`, excluding files in `Documentation`.\n \n+NOTES ON THREADS\n+----------------\n+\n+The `--threads` option (and the grep.threads configuration) will be ignored when\n+`--open-files-in-pager` is used, forcing a single-threaded execution.\n+\n+When grepping the object store (with `--cached` or giving tree objects), running\n+with multiple threads might perform slower than single threaded if `--textconv`\n+is given and there're too many text conversions. So if you experience low\n+performance in this case, it might be desirable to use `--threads=1`.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 1535fd50f8..6aaa8d4406 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1054,7 +1054,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (recurse_submodules && (!use_index || untracked))\n \t\tdie(_(\"option not supported with --recurse-submodules\"));\n \n-\tif (list.nr || cached || show_in_pager) {\n+\tif (show_in_pager) {\n \t\tif (num_threads > 1)\n \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n \t\tnum_threads = 1;\n-- \n2.24.1\n\n"},{"id":"389854","messageId":"2f72f3034118432381f3c9378e70a65d27e3dfbb.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 11/12] grep: move driver pre-load out of critical section","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:39:59Z","receivedAt":"2020-01-16T02:41:23Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In builtin/grep.c:add_work() we pre-load the userdiff drivers before\nadding the grep_source in the todo list. This operation is currently\nbeing performed after acquiring the grep_mutex, but as it's already\nthread-safe, we don't need to protect it here. So let's move it out of\nthe critical section which should avoid thread contention and improve\nperformance.\n\nRunning[1] `git grep --threads=8 abcd[02] HEAD` on chromium's\nrepository[2], I got the following mean times for 30 executions after 2\nwarmups:\n\n Original | 6.2886s\n-------------------------|-----------\n Out of critical section | 5.7852s\n\n[1]: Tests performed on an i7-7700HQ with 16GB of RAM and SSD, running\n Manjaro Linux.\n[2]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n builtin/grep.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 6aaa8d4406..a85b710b48 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -92,8 +92,11 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n+static void add_work(struct grep_opt *opt, struct grep_source *gs)\n {\n+\tif (opt->binary != GREP_BINARY_TEXT)\n+\t\tgrep_source_load_driver(gs, opt->repo->index);\n+\n \tgrep_lock();\n \n \twhile ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {\n@@ -101,9 +104,6 @@ static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n \t}\n \n \ttodo[todo_end].source = *gs;\n-\tif (opt->binary != GREP_BINARY_TEXT)\n-\t\tgrep_source_load_driver(&todo[todo_end].source,\n-\t\t\t\t\topt->repo->index);\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n-- \n2.24.1\n\n"},{"id":"389855","messageId":"a5891176d7778b98ac35c756170dd334b8ee21c7.1579141989.git.matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"cover.1579141989.git.matheus.bernardino@usp.br","subject":"[PATCH v3 12/12] grep: use no. of cores as the default no. of threads","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T02:40:00Z","receivedAt":"2020-01-16T02:41:28Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When --threads is not specified, git-grep will use 8 threads by default.\nThis fixed number may be too many for machines with fewer cores and too\nlittle for machines with more cores. So, instead, use the number of\nlogical cores available in the machine, which seems to result in the\nbest overall performance: The following measurements correspond to the\nmean elapsed times for 30 git-grep executions in chromium's\nrepository[1] with a 95% confidence interval (each set of 30 were\nperformed after 2 warmup runs). Regex 1 is 'abcd[02]' and Regex 2 is\n'(static|extern) (int|double) \\*'.\n\n | Working tree | Object Store\n------|-------------------------------|--------------------------------\n #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n------|---------------|---------------|----------------|---------------\n 32 | 2.92s ± 0.01 | 3.72s ± 0.21 | 5.36s ± 0.01 | 6.07s ± 0.01\n 16 | 2.84s ± 0.01 | 3.57s ± 0.21 | 5.05s ± 0.01 | 5.71s ± 0.01\n> 8 | 2.53s ± 0.00 | 3.24s ± 0.21 | 4.86s ± 0.01 | 5.48s ± 0.01\n 4 | 2.43s ± 0.02 | 3.22s ± 0.20 | 5.22s ± 0.02 | 6.03s ± 0.02\n 2 | 3.06s ± 0.20 | 4.52s ± 0.01 | 7.52s ± 0.01 | 9.06s ± 0.01\n 1 | 6.16s ± 0.01 | 9.25s ± 0.02 | 14.10s ± 0.01 | 17.22s ± 0.01\n\nThe above tests were performed in a desktop running Debian 10.0 with\nIntel(R) Xeon(R) CPU E3-1230 V2 (4 cores w/ hyper-threading), 32GB of\nRAM and a 7200 rpm, SATA 3.1 HDD.\n\nBellow, the tests were repeated for a machine with SSD: a Manjaro laptop\nwith Intel(R) i7-7700HQ (4 cores w/ hyper-threading) and 16GB of RAM:\n\n | Working tree | Object Store\n------|--------------------------------|--------------------------------\n #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n------|---------------|----------------|----------------|---------------\n 32 | 3.29s ± 0.21 | 4.30s ± 0.01 | 6.30s ± 0.01 | 7.30s ± 0.02\n 16 | 3.19s ± 0.20 | 4.14s ± 0.02 | 5.91s ± 0.01 | 6.83s ± 0.01\n> 8 | 2.90s ± 0.04 | 3.82s ± 0.20 | 5.70s ± 0.02 | 6.53s ± 0.01\n 4 | 2.84s ± 0.02 | 3.77s ± 0.20 | 6.19s ± 0.02 | 7.18s ± 0.02\n 2 | 3.73s ± 0.21 | 5.57s ± 0.02 | 9.28s ± 0.01 | 11.22s ± 0.01\n 1 | 7.48s ± 0.02 | 11.36s ± 0.03 | 17.75s ± 0.01 | 21.87s ± 0.08\n\n[1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n 04-06-2019), after a 'git gc' execution.\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/git-grep.txt | 4 ++--\n builtin/grep.c | 3 +--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex de628741fa..eb5412724f 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -59,8 +59,8 @@ grep.extendedRegexp::\n \tother than 'default'.\n \n grep.threads::\n-\tNumber of grep worker threads to use. If unset (or set to 0),\n-\t8 threads are used by default (for now).\n+\tNumber of grep worker threads to use. If unset (or set to 0), Git will\n+\tuse as many threads as the number of logical cores available.\n \n grep.fullName::\n \tIf set to true, enable `--full-name` option by default.\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex a85b710b48..629eaf5dbc 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -33,7 +33,6 @@ static char const * const grep_usage[] = {\n \n static int recurse_submodules;\n \n-#define GREP_NUM_THREADS_DEFAULT 8\n static int num_threads;\n \n static pthread_t *threads;\n@@ -1064,7 +1063,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t} else if (num_threads < 0)\n \t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\n \telse if (num_threads == 0)\n-\t\tnum_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;\n+\t\tnum_threads = HAVE_THREADS ? online_cpus() : 1;\n \n \tif (num_threads > 1) {\n \t\tif (!HAVE_THREADS)\n-- \n2.24.1\n\n"},{"id":"389888","messageId":"CAGuA69ujsOBm2+RKEkGu8wLoEVvKxivY762Zokf9MWxDWrwWFQ@mail.gmail.com","threadId":"51621","inReplyTo":"a5891176d7778b98ac35c756170dd334b8ee21c7.1579141989.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v3 12/12] grep: use no. of cores as the default no. of threads","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2020-01-16T13:11:02Z","receivedAt":"2020-01-16T13:11:16Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Grepping bottleneck is not cpu, but IO. Maybe it is more reasonable to\nuse not online_cpus() but online_cpus()*2?\n\n--\nVictor\n\n\nOn Thu, Jan 16, 2020 at 5:41 AM Matheus Tavares\n wrote:\n>\n> When --threads is not specified, git-grep will use 8 threads by default.\n> This fixed number may be too many for machines with fewer cores and too\n> little for machines with more cores. So, instead, use the number of\n> logical cores available in the machine, which seems to result in the\n> best overall performance: The following measurements correspond to the\n> mean elapsed times for 30 git-grep executions in chromium's\n> repository[1] with a 95% confidence interval (each set of 30 were\n> performed after 2 warmup runs). Regex 1 is 'abcd[02]' and Regex 2 is\n> '(static|extern) (int|double) \\*'.\n>\n> | Working tree | Object Store\n> ------|-------------------------------|--------------------------------\n> #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n> ------|---------------|---------------|----------------|---------------\n> 32 | 2.92s ± 0.01 | 3.72s ± 0.21 | 5.36s ± 0.01 | 6.07s ± 0.01\n> 16 | 2.84s ± 0.01 | 3.57s ± 0.21 | 5.05s ± 0.01 | 5.71s ± 0.01\n> > 8 | 2.53s ± 0.00 | 3.24s ± 0.21 | 4.86s ± 0.01 | 5.48s ± 0.01\n> 4 | 2.43s ± 0.02 | 3.22s ± 0.20 | 5.22s ± 0.02 | 6.03s ± 0.02\n> 2 | 3.06s ± 0.20 | 4.52s ± 0.01 | 7.52s ± 0.01 | 9.06s ± 0.01\n> 1 | 6.16s ± 0.01 | 9.25s ± 0.02 | 14.10s ± 0.01 | 17.22s ± 0.01\n>\n> The above tests were performed in a desktop running Debian 10.0 with\n> Intel(R) Xeon(R) CPU E3-1230 V2 (4 cores w/ hyper-threading), 32GB of\n> RAM and a 7200 rpm, SATA 3.1 HDD.\n>\n> Bellow, the tests were repeated for a machine with SSD: a Manjaro laptop\n> with Intel(R) i7-7700HQ (4 cores w/ hyper-threading) and 16GB of RAM:\n>\n> | Working tree | Object Store\n> ------|--------------------------------|--------------------------------\n> #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n> ------|---------------|----------------|----------------|---------------\n> 32 | 3.29s ± 0.21 | 4.30s ± 0.01 | 6.30s ± 0.01 | 7.30s ± 0.02\n> 16 | 3.19s ± 0.20 | 4.14s ± 0.02 | 5.91s ± 0.01 | 6.83s ± 0.01\n> > 8 | 2.90s ± 0.04 | 3.82s ± 0.20 | 5.70s ± 0.02 | 6.53s ± 0.01\n> 4 | 2.84s ± 0.02 | 3.77s ± 0.20 | 6.19s ± 0.02 | 7.18s ± 0.02\n> 2 | 3.73s ± 0.21 | 5.57s ± 0.02 | 9.28s ± 0.01 | 11.22s ± 0.01\n> 1 | 7.48s ± 0.02 | 11.36s ± 0.03 | 17.75s ± 0.01 | 21.87s ± 0.08\n>\n> [1]: chromium’s repo at commit 03ae96f (“Add filters testing at DSF=2”,\n> 04-06-2019), after a 'git gc' execution.\n>\n> Signed-off-by: Matheus Tavares \n> ---\n> Documentation/git-grep.txt | 4 ++--\n> builtin/grep.c | 3 +--\n> 2 files changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index de628741fa..eb5412724f 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -59,8 +59,8 @@ grep.extendedRegexp::\n> other than 'default'.\n>\n> grep.threads::\n> - Number of grep worker threads to use. If unset (or set to 0),\n> - 8 threads are used by default (for now).\n> + Number of grep worker threads to use. If unset (or set to 0), Git will\n> + use as many threads as the number of logical cores available.\n>\n> grep.fullName::\n> If set to true, enable `--full-name` option by default.\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index a85b710b48..629eaf5dbc 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -33,7 +33,6 @@ static char const * const grep_usage[] = {\n>\n> static int recurse_submodules;\n>\n> -#define GREP_NUM_THREADS_DEFAULT 8\n> static int num_threads;\n>\n> static pthread_t *threads;\n> @@ -1064,7 +1063,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> } else if (num_threads < 0)\n> die(_(\"invalid number of threads specified (%d)\"), num_threads);\n> else if (num_threads == 0)\n> - num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1;\n> + num_threads = HAVE_THREADS ? online_cpus() : 1;\n>\n> if (num_threads > 1) {\n> if (!HAVE_THREADS)\n> --\n> 2.24.1\n>\n"},{"id":"389889","messageId":"20200116144729.8033-1-matheus.bernardino@usp.br","threadId":"51621","inReplyTo":"CAGuA69ujsOBm2+RKEkGu8wLoEVvKxivY762Zokf9MWxDWrwWFQ@mail.gmail.com","subject":"Re: [PATCH] grep: use no. of cores as the default no. of threads","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-16T14:47:29Z","receivedAt":"2020-01-16T14:47:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Victor\n\nOn Thu, Jan 16, 2020 at 10:11 AM Victor Leschuk wrote:\n>\n> Grepping bottleneck is not cpu, but IO. Maybe it is more reasonable to\n> use not online_cpus() but online_cpus()*2?\n\nI also tried this approach, but the tests I ran with online_cpus() * 2\nonly showed slowdowns. The results can be seen in the commit message:\n\n> On Thu, Jan 16, 2020 at 5:41 AM Matheus Tavares\n> wrote:\n> >\n[...]\n> > The following measurements correspond to the\n> > mean elapsed times for 30 git-grep executions in chromium's\n> > repository[1] with a 95% confidence interval (each set of 30 were\n> > performed after 2 warmup runs). Regex 1 is 'abcd[02]' and Regex 2 is\n> > '(static|extern) (int|double) \\*'.\n> >\n> > | Working tree | Object Store\n> > ------|-------------------------------|--------------------------------\n> > #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n> > ------|---------------|---------------|----------------|---------------\n> > 32 | 2.92s ± 0.01 | 3.72s ± 0.21 | 5.36s ± 0.01 | 6.07s ± 0.01\n> > 16 | 2.84s ± 0.01 | 3.57s ± 0.21 | 5.05s ± 0.01 | 5.71s ± 0.01\n> > > 8 | 2.53s ± 0.00 | 3.24s ± 0.21 | 4.86s ± 0.01 | 5.48s ± 0.01\n> > 4 | 2.43s ± 0.02 | 3.22s ± 0.20 | 5.22s ± 0.02 | 6.03s ± 0.02\n> > 2 | 3.06s ± 0.20 | 4.52s ± 0.01 | 7.52s ± 0.01 | 9.06s ± 0.01\n> > 1 | 6.16s ± 0.01 | 9.25s ± 0.02 | 14.10s ± 0.01 | 17.22s ± 0.01\n> >\n> > The above tests were performed in a desktop running Debian 10.0 with\n> > Intel(R) Xeon(R) CPU E3-1230 V2 (4 cores w/ hyper-threading), 32GB of\n> > RAM and a 7200 rpm, SATA 3.1 HDD.\n> >\n> > Bellow, the tests were repeated for a machine with SSD: a Manjaro laptop\n> > with Intel(R) i7-7700HQ (4 cores w/ hyper-threading) and 16GB of RAM:\n> >\n> > | Working tree | Object Store\n> > ------|--------------------------------|--------------------------------\n> > #ths | Regex 1 | Regex 2 | Regex 1 | Regex 2\n> > ------|---------------|----------------|----------------|---------------\n> > 32 | 3.29s ± 0.21 | 4.30s ± 0.01 | 6.30s ± 0.01 | 7.30s ± 0.02\n> > 16 | 3.19s ± 0.20 | 4.14s ± 0.02 | 5.91s ± 0.01 | 6.83s ± 0.01\n> > > 8 | 2.90s ± 0.04 | 3.82s ± 0.20 | 5.70s ± 0.02 | 6.53s ± 0.01\n> > 4 | 2.84s ± 0.02 | 3.77s ± 0.20 | 6.19s ± 0.02 | 7.18s ± 0.02\n> > 2 | 3.73s ± 0.21 | 5.57s ± 0.02 | 9.28s ± 0.01 | 11.22s ± 0.01\n> > 1 | 7.48s ± 0.02 | 11.36s ± 0.03 | 17.75s ± 0.01 | 21.87s ± 0.08\n\nI deliberately used somehow complex regexes for these tests. So I\ndecided to do one more test with a very simple fixed string (\"abc\"),\nallowing git-grep to spend less time in the cpu-bound regex searching.\nThe results can be seen bellow (the metodology is the same as described\nabove and the machine is the Manjaro laptop, for which online_cpus()\nreturns 8):\n\n #ths | Working Three | Object Store\n ------|----------------|---------------\n 16 | 3.22s ± 0.20 | 5.96s ± 0.06\n 8 | 2.92s ± 0.01 | 5.73s ± 0.02\n\n"},{"id":"390720","messageId":"20200129112613.GE10482@szeder.dev","threadId":"51621","inReplyTo":"af8ad95d413aa3d763769eb3ae9544e25ccbe2d1.1579141989.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-01-29T11:26:13Z","receivedAt":"2020-01-29T11:26:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Junio, Matheus, Philippe,\n\nthis patch below and a7f3240877 (grep: ignore --recurse-submodules if\n--no-index is given, 2020-01-26) on topic\n'pb/do-not-recurse-grep-no-index' don't work well together, and cause\nfailure of the test 'grep --recurse-submodules --no-index ignores\n--recurse-submodules' in 't7814-grep-recurse-submodules.sh', i.e. in\nthe new test added in a7f3240877.\n\nMore below.\n\nOn Wed, Jan 15, 2020 at 11:39:56PM -0300, Matheus Tavares wrote:\n> Now that object reading operations are internally protected, the\n> submodule initialization functions at builtin/grep.c:grep_submodule()\n> are very close to being thread-safe. Let's take a look at each call and\n> remove from the critical section what we can, for better performance:\n> \n> - submodule_from_path() and is_submodule_active() cannot be called in\n> parallel yet only because they call repo_read_gitmodules() which\n> contains, in its call stack, operations that would otherwise be in\n> race condition with object reading (for example parse_object() and\n> is_promisor_remote()). However, they only call repo_read_gitmodules()\n> if it wasn't read before. So let's pre-read it before firing the\n> threads and allow these two functions to safely be called in\n> parallel.\n> \n> - repo_submodule_init() is already thread-safe, so remove it from the\n> critical section without other necessary changes.\n> \n> - The repo_read_gitmodules(&subrepo) call at grep_submodule() is safe as\n> no other thread is performing object reading operations in the subrepo\n> yet. However, threads might be working in the superproject, and this\n> function calls add_to_alternates_memory() internally, which is racy\n> with object readings in the superproject. So it must be kept\n> protected for now. Let's add a \"NEEDSWORK\" to it, informing why it\n> cannot be removed from the critical section yet.\n> \n> - Finally, add_to_alternates_memory() must be kept protected for the\n> same reason as the item above.\n> \n> Signed-off-by: Matheus Tavares \n> ---\n> builtin/grep.c | 38 ++++++++++++++++++++++----------------\n> 1 file changed, 22 insertions(+), 16 deletions(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index d3ed05c1da..ac3d86c2e5 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -401,25 +401,23 @@ static int grep_submodule(struct grep_opt *opt,\n> \tstruct grep_opt subopt;\n> \tint hit;\n> \n> -\t/*\n> -\t * NEEDSWORK: submodules functions need to be protected because they\n> -\t * call config_from_gitmodules(): the latter contains in its call stack\n> -\t * many thread-unsafe operations that are racy with object reading, such\n> -\t * as parse_object() and is_promisor_object().\n> -\t */\n> -\tobj_read_lock();\n> \tsub = submodule_from_path(superproject, &null_oid, path);\n> \n> -\tif (!is_submodule_active(superproject, path)) {\n> -\t\tobj_read_unlock();\n> +\tif (!is_submodule_active(superproject, path))\n> \t\treturn 0;\n> -\t}\n> \n> -\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n> -\t\tobj_read_unlock();\n> +\tif (repo_submodule_init(&subrepo, superproject, sub))\n> \t\treturn 0;\n> -\t}\n> \n> +\t/*\n> +\t * NEEDSWORK: repo_read_gitmodules() might call\n> +\t * add_to_alternates_memory() via config_from_gitmodules(). This\n> +\t * operation causes a race condition with concurrent object readings\n> +\t * performed by the worker threads. That's why we need obj_read_lock()\n> +\t * here. It should be removed once it's no longer necessary to add the\n> +\t * subrepo's odbs to the in-memory alternates list.\n> +\t */\n> +\tobj_read_lock();\n> \trepo_read_gitmodules(&subrepo, 0);\n> \n> \t/*\n> @@ -1052,6 +1050,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \tpathspec.recursive = 1;\n> \tpathspec.recurse_submodules = !!recurse_submodules;\n> \n> +\tif (recurse_submodules && (!use_index || untracked))\n> +\t\tdie(_(\"option not supported with --recurse-submodules\"));\n\nSo this patch moves this condition here, expecting git to die with \n'--recurse-submodules --no-index'. However, a7f3240877 removes the\n'!use_index' part of the condition, so we won't die here ...\n\n> \tif (list.nr || cached || show_in_pager) {\n> \t\tif (num_threads > 1)\n> \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n> @@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\t && (opt.pre_context || opt.post_context ||\n> \t\t\topt.file_break || opt.funcbody))\n> \t\t\tskip_first_line = 1;\n> +\n> +\t\t/*\n> +\t\t * Pre-read gitmodules (if not read already) to prevent racy\n> +\t\t * lazy reading in worker threads.\n> +\t\t */\n> +\t\tif (recurse_submodules)\n> +\t\t\trepo_read_gitmodules(the_repository, 1);\n\n... and eventually reach this condition, which then reads the\nsubmodules even with '--no-index', which is just what a7f3240877 tried\nto avoid, thus triggering the test failure.\n\nIt might be that all we need is changing this condition to:\n\n if (recurse_submodules && use_index)\n\nOr maybe not, but this change on top of 'pu' makes t7814 succeed\nagain.\n\nHowever, I'm not familiar with the intricacies of either threaded grep\nor submodules, much less the combination of the two... so just an\nidea.\n\n> \t\tstart_threads(&opt);\n> \t} else {\n> \t\t/*\n> @@ -1105,9 +1114,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> -\tif (recurse_submodules && (!use_index || untracked))\n> -\t\tdie(_(\"option not supported with --recurse-submodules\"));\n> -\n> \tif (!show_in_pager && !opt.status_only)\n> \t\tsetup_pager();\n> \n> -- \n> 2.24.1\n> \n"},{"id":"390737","messageId":"xmqq36byf67k.fsf@gitster-ct.c.googlers.com","threadId":"51621","inReplyTo":"20200129112613.GE10482@szeder.dev","subject":"Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-29T18:49:51Z","receivedAt":"2020-01-29T18:49:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> this patch below and a7f3240877 (grep: ignore --recurse-submodules if\n> --no-index is given, 2020-01-26) on topic\n> 'pb/do-not-recurse-grep-no-index' don't work well together, and cause\n> failure of the test 'grep --recurse-submodules --no-index ignores\n> --recurse-submodules' in 't7814-grep-recurse-submodules.sh', i.e. in\n> the new test added in a7f3240877.\n\nYup, I was looking at it and trying to see what was going on.\n\n>> +\tif (recurse_submodules && (!use_index || untracked))\n>> +\t\tdie(_(\"option not supported with --recurse-submodules\"));\n>\n> So this patch moves this condition here, expecting git to die with \n> '--recurse-submodules --no-index'. However, a7f3240877 removes the\n> '!use_index' part of the condition, so we won't die here ...\n>\n>> \tif (list.nr || cached || show_in_pager) {\n>> \t\tif (num_threads > 1)\n>> \t\t\twarning(_(\"invalid option combination, ignoring --threads\"));\n>> @@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>> \t\t && (opt.pre_context || opt.post_context ||\n>> \t\t\topt.file_break || opt.funcbody))\n>> \t\t\tskip_first_line = 1;\n>> +\n>> +\t\t/*\n>> +\t\t * Pre-read gitmodules (if not read already) to prevent racy\n>> +\t\t * lazy reading in worker threads.\n>> +\t\t */\n>> +\t\tif (recurse_submodules)\n>> +\t\t\trepo_read_gitmodules(the_repository, 1);\n>\n> ... and eventually reach this condition, which then reads the\n> submodules even with '--no-index', which is just what a7f3240877 tried\n> to avoid, thus triggering the test failure.\n>\n> It might be that all we need is changing this condition to:\n>\n> if (recurse_submodules && use_index)\n>\n> Or maybe not, but this change on top of 'pu' makes t7814 succeed\n> again.\n\nSounds like a sensible idea.\n"},{"id":"390739","messageId":"xmqqy2tqdr9t.fsf@gitster-ct.c.googlers.com","threadId":"51621","inReplyTo":"20200129112613.GE10482@szeder.dev","subject":"Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-29T18:57:50Z","receivedAt":"2020-01-29T18:57:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Junio, Matheus, Philippe,\n>\n> this patch below and a7f3240877 (grep: ignore --recurse-submodules if\n> --no-index is given, 2020-01-26) on topic\n> 'pb/do-not-recurse-grep-no-index' don't work well together, and cause\n> failure of the test 'grep --recurse-submodules --no-index ignores\n> --recurse-submodules' in 't7814-grep-recurse-submodules.sh', i.e. in\n> the new test added in a7f3240877.\n\nHmph, I wonder if \"ignore --recurse-submodules if --no-index\" should\nhave been done as a single liner patch, something along the lines of\n\"after parse_options() returns, drop recurse_submodules if no-index\nwas given\", i.e.\n\n@@ -958,6 +946,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t/* die the same way as if we did it at the beginning */\n \t\t\tsetup_git_directory();\n \t}\n+\tif (!use_index)\n+\t\trecurse_submodules = 0; /* ignore */\n \n \t/*\n \t * skip a -- separator; we know it cannot be\n"},{"id":"390745","messageId":"CAHd-oW590ZnNnCdD5LLiBQB73LRUVEf41wv7FLJvGMwd2kLYww@mail.gmail.com","threadId":"51621","inReplyTo":"xmqqy2tqdr9t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-01-29T20:42:57Z","receivedAt":"2020-01-29T20:43:11Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jan 29, 2020 at 3:57 PM Junio C Hamano wrote:\n>\n> SZEDER Gábor writes:\n> >\n[...]\n> > @@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> > && (opt.pre_context || opt.post_context ||\n> > opt.file_break || opt.funcbody))\n> > skip_first_line = 1;\n> > +\n> > + /*\n> > + * Pre-read gitmodules (if not read already) to prevent racy\n> > + * lazy reading in worker threads.\n> > + */\n> > + if (recurse_submodules)\n> > + repo_read_gitmodules(the_repository, 1);\n> >\n> > ... and eventually reach this condition, which then reads the\n> > submodules even with '--no-index', which is just what a7f3240877 tried\n> > to avoid, thus triggering the test failure.\n> >\n> > It might be that all we need is changing this condition to:\n> >\n> > if (recurse_submodules && use_index)\n\nYes, I think that would work. I was only worried that, in case of\n!use_index, the path taken could somehow lead to an unprotected call\nto repo_read_gitmodules() (with threads spawned).Then, since the file\nwould not have been pre-loaded by the sequential code, we could\nencounter a race condition. But by what I've inspected, when use_index\nis false, grep_directory() will be called to traverse the files, and\nit does not have repo_read_gitmodules() in its call graph[1]. So the\nsolution should be fine in the point of view of thread-safeness.\n\n> Hmph, I wonder if \"ignore --recurse-submodules if --no-index\" should\n> have been done as a single liner patch, something along the lines of\n> \"after parse_options() returns, drop recurse_submodules if no-index\n> was given\", i.e.\n>\n> @@ -958,6 +946,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> /* die the same way as if we did it at the beginning */\n> setup_git_directory();\n> }\n> + if (!use_index)\n> + recurse_submodules = 0; /* ignore */\n>\n> /*\n> * skip a -- separator; we know it cannot be\n\nYeah, this seems more meaningful, IMHO, as we can easily see that the\nrecurse_submodules option was dropped in favor of using --no-index.\n\n[1]: Well, in fact repo_read_gitmodules() *is* in grep_directory()'s\ncall graph, but the only path to it is through the\nfill_textconv_grep() > fill_textconv() call, which is already guarded\nby the obj_read_mutex. So there is no problem here.\n"},{"id":"390790","messageId":"7DA86BCF-8A20-4B06-BDC0-82EEDFCC0AE5@gmail.com","threadId":"51621","inReplyTo":"CAHd-oW590ZnNnCdD5LLiBQB73LRUVEf41wv7FLJvGMwd2kLYww@mail.gmail.com","subject":"Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-01-30T13:28:01Z","receivedAt":"2020-01-30T13:28:07Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi everyone,\n>> \n>> @@ -958,6 +946,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>> /* die the same way as if we did it at the beginning */\n>> setup_git_directory();\n>> }\n>> + if (!use_index)\n>> + recurse_submodules = 0; /* ignore */\n>> \n>> /*\n>> * skip a -- separator; we know it cannot be\n> \n> Yeah, this seems more meaningful, IMHO, as we can easily see that the\n> recurse_submodules option was dropped in favor of using --no-index.\n> \nI agree. I’ll send a v2 of my patch with this added.\n\nPhilippe.\n\n"}]}