grep: replace grep_read_mutex by internal obj read lock
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Matheus Tavares committed
Jan 15, 2020 at 23:39 UTC
1d1729caebd41b340dd8dd61057f613da4df526c
3 files changed
+35
-63
builtin/grep.c
+16
-30
@@ -200,12 +200,12 @@ static void start_threads(struct grep_opt *opt)
200
int i;
201
202
pthread_mutex_init(&grep_mutex, NULL);
203
- pthread_mutex_init(&grep_read_mutex, NULL);
203
pthread_mutex_init(&grep_attr_mutex, NULL);
204
pthread_cond_init(&cond_add, NULL);
205
pthread_cond_init(&cond_write, NULL);
206
pthread_cond_init(&cond_result, NULL);
207
grep_use_locks = 1;
208
+ enable_obj_read_lock();
209
210
for (i = 0; i < ARRAY_SIZE(todo); i++) {
211
strbuf_init(&todo[i].out, 0);
@@ -257,12 +257,12 @@ static int wait_all(void)
257
free(threads);
258
259
pthread_mutex_destroy(&grep_mutex);
260
- pthread_mutex_destroy(&grep_read_mutex);
260
pthread_mutex_destroy(&grep_attr_mutex);
261
pthread_cond_destroy(&cond_add);
262
pthread_cond_destroy(&cond_write);
263
pthread_cond_destroy(&cond_result);
264
grep_use_locks = 0;
265
+ disable_obj_read_lock();
266
267
return hit;
268
}
@@ -295,16 +295,6 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
295
return st;
296
}
297
298
-static void *lock_and_read_oid_file(const struct object_id *oid, enum object_type *type, unsigned long *size)
299
-{
300
- void *data;
301
-
302
- grep_read_lock();
303
- data = read_object_file(oid, type, size);
304
- grep_read_unlock();
305
- return data;
306
-}
307
-
298
static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
299
const char *filename, int tree_name_len,
300
const char *path)
@@ -413,20 +403,20 @@ static int grep_submodule(struct grep_opt *opt,
403
404
/*
405
* NEEDSWORK: submodules functions need to be protected because they
416
- * access the object store via config_from_gitmodules(): the latter
417
- * uses get_oid() which, for now, relies on the global the_repository
418
- * object.
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
*/
420
- grep_read_lock();
410
+ obj_read_lock();
411
sub = submodule_from_path(superproject, &null_oid, path);
412
413
if (!is_submodule_active(superproject, path)) {
424
- grep_read_unlock();
414
+ obj_read_unlock();
415
return 0;
416
}
417
418
if (repo_submodule_init(&subrepo, superproject, sub)) {
429
- grep_read_unlock();
419
+ obj_read_unlock();
420
return 0;
421
}
422
@@ -443,7 +433,7 @@ static int grep_submodule(struct grep_opt *opt,
433
* object.
434
*/
435
add_to_alternates_memory(subrepo.objects->odb->path);
446
- grep_read_unlock();
436
+ obj_read_unlock();
437
438
memcpy(&subopt, opt, sizeof(subopt));
439
subopt.repo = &subrepo;
@@ -455,13 +445,12 @@ static int grep_submodule(struct grep_opt *opt,
445
unsigned long size;
446
struct strbuf base = STRBUF_INIT;
447
458
- grep_read_lock();
448
+ obj_read_lock();
449
object = parse_object_or_die(oid, oid_to_hex(oid));
450
+ obj_read_unlock();
451
data = read_object_with_reference(&subrepo,
452
&object->oid, tree_type,
453
&size, NULL);
463
- grep_read_unlock();
464
-
454
if (!data)
455
die(_("unable to read tree (%s)"), oid_to_hex(&object->oid));
456
@@ -586,7 +575,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
575
void *data;
576
unsigned long size;
577
589
- data = lock_and_read_oid_file(&entry.oid, &type, &size);
578
+ data = read_object_file(&entry.oid, &type, &size);
579
if (!data)
580
die(_("unable to read tree (%s)"),
581
oid_to_hex(&entry.oid));
@@ -624,12 +613,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
613
struct strbuf base;
614
int hit, len;
615
627
- grep_read_lock();
616
data = read_object_with_reference(opt->repo,
617
&obj->oid, tree_type,
618
&size, NULL);
631
- grep_read_unlock();
632
-
619
if (!data)
620
die(_("unable to read tree (%s)"), oid_to_hex(&obj->oid));
621
@@ -659,17 +645,17 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
645
for (i = 0; i < nr; i++) {
646
struct object *real_obj;
647
662
- grep_read_lock();
648
+ obj_read_lock();
649
real_obj = deref_tag(opt->repo, list->objects[i].item,
650
NULL, 0);
665
- grep_read_unlock();
651
+ obj_read_unlock();
652
653
/* load the gitmodules file for this rev */
654
if (recurse_submodules) {
655
submodule_free(opt->repo);
670
- grep_read_lock();
656
+ obj_read_lock();
657
gitmodules_config_oid(&real_obj->oid);
672
- grep_read_unlock();
658
+ obj_read_unlock();
659
}
660
if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
661
list->objects[i].path)) {
grep.c
+19
-20
@@ -1540,11 +1540,6 @@ static inline void grep_attr_unlock(void)
1540
pthread_mutex_unlock(&grep_attr_mutex);
1541
}
1542
1543
-/*
1544
- * Same as git_attr_mutex, but protecting the thread-unsafe object db access.
1545
- */
1546
-pthread_mutex_t grep_read_mutex;
1547
-
1543
static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
1544
{
1545
xdemitconf_t *xecfg = opt->priv;
@@ -1741,13 +1736,20 @@ static int fill_textconv_grep(struct repository *r,
1736
}
1737
1738
/*
1744
- * fill_textconv is not remotely thread-safe; it may load objects
1745
- * behind the scenes, and it modifies the global diff tempfile
1746
- * structure.
1739
+ * fill_textconv is not remotely thread-safe; it modifies the global
1740
+ * diff tempfile structure, writes to the_repo's odb and might
1741
+ * internally call thread-unsafe functions such as the
1742
+ * prepare_packed_git() lazy-initializator. Because of the last two, we
1743
+ * must ensure mutual exclusion between this call and the object reading
1744
+ * API, thus we use obj_read_lock() here.
1745
+ *
1746
+ * TODO: allowing text conversion to run in parallel with object
1747
+ * reading operations might increase performance in the multithreaded
1748
+ * non-worktreee git-grep with --textconv.
1749
*/
1748
- grep_read_lock();
1750
+ obj_read_lock();
1751
size = fill_textconv(r, driver, df, &buf);
1750
- grep_read_unlock();
1752
+ obj_read_unlock();
1753
free_filespec(df);
1754
1755
/*
@@ -1813,12 +1815,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
1815
grep_source_load_driver(gs, opt->repo->index);
1816
/*
1817
* We might set up the shared textconv cache data here, which
1816
- * is not thread-safe.
1818
+ * is not thread-safe. Also, get_oid_with_context() and
1819
+ * parse_object() might be internally called. As they are not
1820
+ * currenty thread-safe and might be racy with object reading,
1821
+ * obj_read_lock() must be called.
1822
*/
1823
grep_attr_lock();
1819
- grep_read_lock();
1824
+ obj_read_lock();
1825
textconv = userdiff_get_textconv(opt->repo, gs->driver);
1821
- grep_read_unlock();
1826
+ obj_read_unlock();
1827
grep_attr_unlock();
1828
}
1829
@@ -2118,10 +2123,7 @@ static int grep_source_load_oid(struct grep_source *gs)
2123
{
2124
enum object_type type;
2125
2121
- grep_read_lock();
2126
gs->buf = read_object_file(gs->identifier, &type, &gs->size);
2123
- grep_read_unlock();
2124
-
2127
if (!gs->buf)
2128
return error(_("'%s': unable to read %s"),
2129
gs->name,
@@ -2186,11 +2188,8 @@ void grep_source_load_driver(struct grep_source *gs,
2188
return;
2189
2190
grep_attr_lock();
2189
- if (gs->path) {
2190
- grep_read_lock();
2191
+ if (gs->path)
2192
gs->driver = userdiff_find_by_path(istate, gs->path);
2192
- grep_read_unlock();
2193
- }
2193
if (!gs->driver)
2194
gs->driver = userdiff_find_by_name("default");
2195
grep_attr_unlock();
grep.h
-13
@@ -220,18 +220,5 @@ int grep_threads_ok(const struct grep_opt *opt);
220
*/
221
extern int grep_use_locks;
222
extern pthread_mutex_t grep_attr_mutex;
223
-extern pthread_mutex_t grep_read_mutex;
224
-
225
-static inline void grep_read_lock(void)
226
-{
227
- if (grep_use_locks)
228
- pthread_mutex_lock(&grep_read_mutex);
229
-}
230
-
231
-static inline void grep_read_unlock(void)
232
-{
233
- if (grep_use_locks)
234
- pthread_mutex_unlock(&grep_read_mutex);
235
-}
223
224
#endif