@matheusbsbs / matheus-bagliano / commits / c441ea4edc
Fork 0

grep: allow submodule functions to run in parallel

Now that object reading operations are internally protected, the submodule initialization functions at builtin/grep.c:grep_submodule() are very close to being thread-safe. Let's take a look at each call and remove from the critical section what we can, for better performance: - submodule_from_path() and is_submodule_active() cannot be called in parallel yet only because they call repo_read_gitmodules() which contains, in its call stack, operations that would otherwise be in race condition with object reading (for example parse_object() and is_promisor_remote()). However, they only call repo_read_gitmodules() if it wasn't read before. So let's pre-read it before firing the threads and allow these two functions to safely be called in parallel. - repo_submodule_init() is already thread-safe, so remove it from the critical section without other necessary changes. - The repo_read_gitmodules(&subrepo) call at grep_submodule() is safe as no other thread is performing object reading operations in the subrepo yet. However, threads might be working in the superproject, and this function calls add_to_alternates_memory() internally, which is racy with object readings in the superproject. So it must be kept protected for now. Let's add a "NEEDSWORK" to it, informing why it cannot be removed from the critical section yet. - Finally, add_to_alternates_memory() must be kept protected for the same reason as the item above. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Matheus Tavares committed Jan 15, 2020 at 23:39 UTC c441ea4edcba135c60c095b5d8b4517a87890b34
1 file changed +22 -16
builtin/grep.c
+22 -16
@@ -401,25 +401,23 @@ static int grep_submodule(struct grep_opt *opt,
401 struct grep_opt subopt;
402 int hit;
403
404 - /*
405 - * NEEDSWORK: submodules functions need to be protected because they
406 - * call config_from_gitmodules(): the latter contains in its call stack
407 - * many thread-unsafe operations that are racy with object reading, such
408 - * as parse_object() and is_promisor_object().
409 - */
410 - obj_read_lock();
404 sub = submodule_from_path(superproject, &null_oid, path);
405
413 - if (!is_submodule_active(superproject, path)) {
414 - obj_read_unlock();
406 + if (!is_submodule_active(superproject, path))
407 return 0;
416 - }
408
418 - if (repo_submodule_init(&subrepo, superproject, sub)) {
419 - obj_read_unlock();
409 + if (repo_submodule_init(&subrepo, superproject, sub))
410 return 0;
421 - }
411
412 + /*
413 + * NEEDSWORK: repo_read_gitmodules() might call
414 + * add_to_alternates_memory() via config_from_gitmodules(). This
415 + * operation causes a race condition with concurrent object readings
416 + * performed by the worker threads. That's why we need obj_read_lock()
417 + * here. It should be removed once it's no longer necessary to add the
418 + * subrepo's odbs to the in-memory alternates list.
419 + */
420 + obj_read_lock();
421 repo_read_gitmodules(&subrepo, 0);
422
423 /*
@@ -1052,6 +1050,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
1050 pathspec.recursive = 1;
1051 pathspec.recurse_submodules = !!recurse_submodules;
1052
1053 + if (recurse_submodules && (!use_index || untracked))
1054 + die(_("option not supported with --recurse-submodules"));
1055 +
1056 if (list.nr || cached || show_in_pager) {
1057 if (num_threads > 1)
1058 warning(_("invalid option combination, ignoring --threads"));
@@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
1072 && (opt.pre_context || opt.post_context ||
1073 opt.file_break || opt.funcbody))
1074 skip_first_line = 1;
1075 +
1076 + /*
1077 + * Pre-read gitmodules (if not read already) to prevent racy
1078 + * lazy reading in worker threads.
1079 + */
1080 + if (recurse_submodules)
1081 + repo_read_gitmodules(the_repository, 1);
1082 +
1083 start_threads(&opt);
1084 } else {
1085 /*
@@ -1105,9 +1114,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
1114 }
1115 }
1116
1108 - if (recurse_submodules && (!use_index || untracked))
1109 - die(_("option not supported with --recurse-submodules"));
1110 -
1117 if (!show_in_pager && !opt.status_only)
1118 setup_pager();
1119