git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 06/12] grep: replace grep_read_mutex by internal obj read lock

From
Matheus Tavares <matheus.bernardino@usp.br>
Date
Jan 16, 2020, 02:39 UTC
Message-ID
<fc1200bb07f749420dad044d39dfe30ae73ad640.1579141989.git.matheus.bernardino@usp.br>
In-Reply-To
<cover.1579141989.git.matheus.bernardino@usp.br>

git-grep uses 'grep_read_mutex' to protect its calls to object reading operations. But these have their own internal lock now, which ensures a better performance (allowing parallel access to more regions). So, let's remove the former and, instead, activate the latter with enable_obj_read_lock().

Sections that are currently protected by 'grep_read_mutex' but are not internally protected by the object reading lock should be surrounded by obj_read_lock() and obj_read_unlock(). These guarantee mutual exclusion with object reading operations, keeping the current behavior and avoiding race conditions. Namely, these places are:

  In grep.c:
  - fill_textconv() at fill_textconv_grep().
  - userdiff_get_textconv() at grep_source_1().
  In builtin/grep.c:
  - parse_object_or_die() and the submodule functions at
    grep_submodule().
  - deref_tag() and gitmodules_config_oid() at grep_objects().

If these functions become thread-safe, in the future, we might remove the locking and probably get some speedup.

Note that some of the submodule functions will already be thread-safe (or close to being thread-safe) with the internal object reading lock. However, as some of them will require additional modifications to be removed from the critical section, this will be done in its own patch.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/grep.c | 46 ++++++++++++++++------------------------------
 grep.c         | 39 +++++++++++++++++++--------------------
 grep.h         | 13 -------------
 3 files changed, 35 insertions(+), 63 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 91fc032a32..4a436d6c99 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -200,12 +200,12 @@ static void start_threads(struct grep_opt *opt)
 	int i;
 
 	pthread_mutex_init(&grep_mutex, NULL);
-	pthread_mutex_init(&grep_read_mutex, NULL);
 	pthread_mutex_init(&grep_attr_mutex, NULL);
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
 	pthread_cond_init(&cond_result, NULL);
 	grep_use_locks = 1;
+	enable_obj_read_lock();
 
 	for (i = 0; i < ARRAY_SIZE(todo); i++) {
 		strbuf_init(&todo[i].out, 0);
@@ -257,12 +257,12 @@ static int wait_all(void)
 	free(threads);
 
 	pthread_mutex_destroy(&grep_mutex);
-	pthread_mutex_destroy(&grep_read_mutex);
 	pthread_mutex_destroy(&grep_attr_mutex);
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
 	pthread_cond_destroy(&cond_result);
 	grep_use_locks = 0;
+	disable_obj_read_lock();
 
 	return hit;
 }
@@ -295,16 +295,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
 	return st;
 }
 
-static void *lock_and_read_oid_file(const struct object_id *oid, enum object_type *type, unsigned long *size)
-{
-	void *data;
-
-	grep_read_lock();
-	data = read_object_file(oid, type, size);
-	grep_read_unlock();
-	return data;
-}
-
 static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 		     const char *filename, int tree_name_len,
 		     const char *path)
@@ -413,20 +403,20 @@ static int grep_submodule(struct grep_opt *opt,
 
 	/*
 	 * NEEDSWORK: submodules functions need to be protected because they
-	 * access the object store via config_from_gitmodules(): the latter
-	 * uses get_oid() which, for now, relies on the global the_repository
-	 * object.
+	 * call config_from_gitmodules(): the latter contains in its call stack
+	 * many thread-unsafe operations that are racy with object reading, such
+	 * as parse_object() and is_promisor_object().
 	 */
-	grep_read_lock();
+	obj_read_lock();
 	sub = submodule_from_path(superproject, &null_oid, path);
 
 	if (!is_submodule_active(superproject, path)) {
-		grep_read_unlock();
+		obj_read_unlock();
 		return 0;
 	}
 
 	if (repo_submodule_init(&subrepo, superproject, sub)) {
-		grep_read_unlock();
+		obj_read_unlock();
 		return 0;
 	}
 
@@ -443,7 +433,7 @@ static int grep_submodule(struct grep_opt *opt,
 	 * object.
 	 */
 	add_to_alternates_memory(subrepo.objects->odb->path);
-	grep_read_unlock();
+	obj_read_unlock();
 
 	memcpy(&subopt, opt, sizeof(subopt));
 	subopt.repo = &subrepo;
@@ -455,13 +445,12 @@ static int grep_submodule(struct grep_opt *opt,
 		unsigned long size;
 		struct strbuf base = STRBUF_INIT;
 
-		grep_read_lock();
+		obj_read_lock();
 		object = parse_object_or_die(oid, oid_to_hex(oid));
+		obj_read_unlock();
 		data = read_object_with_reference(&subrepo,
 						  &object->oid, tree_type,
 						  &size, NULL);
-		grep_read_unlock();
-
 		if (!data)
 			die(_("unable to read tree (%s)"), oid_to_hex(&object->oid));
 
@@ -586,7 +575,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 			void *data;
 			unsigned long size;
 
-			data = lock_and_read_oid_file(&entry.oid, &type, &size);
+			data = read_object_file(&entry.oid, &type, &size);
 			if (!data)
 				die(_("unable to read tree (%s)"),
 				    oid_to_hex(&entry.oid));
@@ -624,12 +613,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		struct strbuf base;
 		int hit, len;
 
-		grep_read_lock();
 		data = read_object_with_reference(opt->repo,
 						  &obj->oid, tree_type,
 						  &size, NULL);
-		grep_read_unlock();
-
 		if (!data)
 			die(_("unable to read tree (%s)"), oid_to_hex(&obj->oid));
 
@@ -659,17 +645,17 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
 	for (i = 0; i < nr; i++) {
 		struct object *real_obj;
 
-		grep_read_lock();
+		obj_read_lock();
 		real_obj = deref_tag(opt->repo, list->objects[i].item,
 				     NULL, 0);
-		grep_read_unlock();
+		obj_read_unlock();
 
 		/* load the gitmodules file for this rev */
 		if (recurse_submodules) {
 			submodule_free(opt->repo);
-			grep_read_lock();
+			obj_read_lock();
 			gitmodules_config_oid(&real_obj->oid);
-			grep_read_unlock();
+			obj_read_unlock();
 		}
 		if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
 				list->objects[i].path)) {
diff --git a/grep.c b/grep.c
index c028f70aba..13232a904a 100644
--- a/grep.c
+++ b/grep.c
@@ -1540,11 +1540,6 @@ static inline void grep_attr_unlock(void)
 		pthread_mutex_unlock(&grep_attr_mutex);
 }
 
-/*
- * Same as git_attr_mutex, but protecting the thread-unsafe object db access.
- */
-pthread_mutex_t grep_read_mutex;
-
 static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
@@ -1741,13 +1736,20 @@ static int fill_textconv_grep(struct repository *r,
 	}
 
 	/*
-	 * fill_textconv is not remotely thread-safe; it may load objects
-	 * behind the scenes, and it modifies the global diff tempfile
-	 * structure.
+	 * fill_textconv is not remotely thread-safe; it modifies the global
+	 * diff tempfile structure, writes to the_repo's odb and might
+	 * internally call thread-unsafe functions such as the
+	 * prepare_packed_git() lazy-initializator. Because of the last two, we
+	 * must ensure mutual exclusion between this call and the object reading
+	 * API, thus we use obj_read_lock() here.
+	 *
+	 * TODO: allowing text conversion to run in parallel with object
+	 * reading operations might increase performance in the multithreaded
+	 * non-worktreee git-grep with --textconv.
 	 */
-	grep_read_lock();
+	obj_read_lock();
 	size = fill_textconv(r, driver, df, &buf);
-	grep_read_unlock();
+	obj_read_unlock();
 	free_filespec(df);
 
 	/*
@@ -1813,12 +1815,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
 		grep_source_load_driver(gs, opt->repo->index);
 		/*
 		 * We might set up the shared textconv cache data here, which
-		 * is not thread-safe.
+		 * is not thread-safe. Also, get_oid_with_context() and
+		 * parse_object() might be internally called. As they are not
+		 * currenty thread-safe and might be racy with object reading,
+		 * obj_read_lock() must be called.
 		 */
 		grep_attr_lock();
-		grep_read_lock();
+		obj_read_lock();
 		textconv = userdiff_get_textconv(opt->repo, gs->driver);
-		grep_read_unlock();
+		obj_read_unlock();
 		grep_attr_unlock();
 	}
 
@@ -2118,10 +2123,7 @@ static int grep_source_load_oid(struct grep_source *gs)
 {
 	enum object_type type;
 
-	grep_read_lock();
 	gs->buf = read_object_file(gs->identifier, &type, &gs->size);
-	grep_read_unlock();
-
 	if (!gs->buf)
 		return error(_("'%s': unable to read %s"),
 			     gs->name,
@@ -2186,11 +2188,8 @@ void grep_source_load_driver(struct grep_source *gs,
 		return;
 
 	grep_attr_lock();
-	if (gs->path) {
-		grep_read_lock();
+	if (gs->path)
 		gs->driver = userdiff_find_by_path(istate, gs->path);
-		grep_read_unlock();
-	}
 	if (!gs->driver)
 		gs->driver = userdiff_find_by_name("default");
 	grep_attr_unlock();
diff --git a/grep.h b/grep.h
index 811fd274c9..9115db8515 100644
--- a/grep.h
+++ b/grep.h
@@ -220,18 +220,5 @@ int grep_threads_ok(const struct grep_opt *opt);
  */
 extern int grep_use_locks;
 extern pthread_mutex_t grep_attr_mutex;
-extern pthread_mutex_t grep_read_mutex;
-
-static inline void grep_read_lock(void)
-{
-	if (grep_use_locks)
-		pthread_mutex_lock(&grep_read_mutex);
-}
-
-static inline void grep_read_unlock(void)
-{
-	if (grep_use_locks)
-		pthread_mutex_unlock(&grep_read_mutex);
-}
 
 #endif

-- 
2.24.1
Previous: Matheus TavaresNext: Matheus Tavares
Message 34 of 47 in “grep: re-enable threads when cached, w/ parallel inflation”
  1. Matheus TavaresAug 10, 2019
  2. [GSoC][PATCH 1/4] object-store: add lock to read_object_file_extended()Matheus Tavares, Aug 10, 2019
  3. [GSoC][PATCH 2/4] grep: allow locks to be enabled individuallyMatheus Tavares, Aug 10, 2019
  4. [GSoC][PATCH 3/4] grep: disable grep_read_mutex when possibleMatheus Tavares, Aug 10, 2019
  5. [GSoC][PATCH 4/4] grep: re-enable threads in some non-worktree casesMatheus Tavares, Aug 10, 2019
  6. 00/11 grep: improve threading and fix race conditionsMatheus Tavares, Sep 30, 2019
  7. 01/11 grep: fix race conditions on userdiff callsMatheus Tavares, Sep 30, 2019
  8. 02/11 grep: fix race conditions at grep_submodule()Matheus Tavares, Sep 30, 2019
  9. 03/11 grep: fix racy calls in grep_objects()Matheus Tavares, Sep 30, 2019
  10. 04/11 replace-object: make replace operations thread-safeMatheus Tavares, Sep 30, 2019
  11. 05/11 object-store: allow threaded access to object readingMatheus Tavares, Sep 30, 2019
  12. Jonathan TanNov 12, 2019
  13. Jeff KingNov 13, 2019
  14. Matheus Tavares BernardinoNov 14, 2019
  15. Jeff KingNov 14, 2019
  16. Jonathan TanNov 14, 2019
  17. Jeff KingNov 15, 2019
  18. Matheus Tavares BernardinoDec 19, 2019
  19. Matheus Tavares BernardinoJan 9, 2020
  20. Christian CouderJan 10, 2020
  21. 06/11 grep: replace grep_read_mutex by internal obj read lockMatheus Tavares, Sep 30, 2019
  22. squash! grep: replace grep_read_mutex by internal obj read lockMatheus Tavares, Oct 1, 2019
  23. 07/11 submodule-config: add skip_if_read option to repo_read_gitmodules()Matheus Tavares, Sep 30, 2019
  24. 08/11 grep: allow submodule functions to run in parallelMatheus Tavares, Sep 30, 2019
  25. 09/11 grep: protect packed_git [re-]initializationMatheus Tavares, Sep 30, 2019
  26. 10/11 grep: re-enable threads in non-worktree caseMatheus Tavares, Sep 30, 2019
  27. 11/11 grep: move driver pre-load out of critical sectionMatheus Tavares, Sep 30, 2019
  28. 00/12 grep: improve threading and fix race conditionsMatheus Tavares, Jan 16, 2020
  29. 01/12 grep: fix race conditions on userdiff callsMatheus Tavares, Jan 16, 2020
  30. 02/12 grep: fix race conditions at grep_submodule()Matheus Tavares, Jan 16, 2020
  31. 03/12 grep: fix racy calls in grep_objects()Matheus Tavares, Jan 16, 2020
  32. 04/12 replace-object: make replace operations thread-safeMatheus Tavares, Jan 16, 2020
  33. 05/12 object-store: allow threaded access to object readingMatheus Tavares, Jan 16, 2020
  34. 06/12 grep: replace grep_read_mutex by internal obj read lockMatheus Tavares, Jan 16, 2020
  35. 07/12 submodule-config: add skip_if_read option to repo_read_gitmodules()Matheus Tavares, Jan 16, 2020
  36. 08/12 grep: allow submodule functions to run in parallelMatheus Tavares, Jan 16, 2020
  37. SZEDER GáborJan 29, 2020
  38. Junio C HamanoJan 29, 2020
  39. Junio C HamanoJan 29, 2020
  40. Matheus Tavares BernardinoJan 29, 2020
  41. Philippe BlainJan 30, 2020
  42. 09/12 grep: protect packed_git [re-]initializationMatheus Tavares, Jan 16, 2020
  43. 10/12 grep: re-enable threads in non-worktree caseMatheus Tavares, Jan 16, 2020
  44. 11/12 grep: move driver pre-load out of critical sectionMatheus Tavares, Jan 16, 2020
  45. 12/12 grep: use no. of cores as the default no. of threadsMatheus Tavares, Jan 16, 2020
  46. Victor LeschukJan 16, 2020
  47. Matheus TavaresJan 16, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.