From e2e05d619a71ae2c06ebd3c661260bf4b5039a93 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 23 Feb 2018 15:47:56 +0100 Subject: [PATCH] grep: move grep_source_init outside critical section grep_source_init typically does three strdup()s, and in the threaded case, the call from add_work() happens while holding grep_mutex. We can thus reduce the time we hold grep_mutex by moving the grep_source_init() call out of add_work(), and simply have add_work() copy the initialized structure to the available slot in the todo array. This also simplifies the prototype of add_work(), since it no longer needs to duplicate all the parameters of grep_source_init(). In the callers of add_work(), we get to reduce the amount of code duplicated in the threaded and non-threaded cases slightly (avoiding repeating the long "GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent cleanup patch will make that even more so. Signed-off-by: Rasmus Villemoes Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/grep.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 3ca4ac80d8..aad422bb64 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -92,8 +92,7 @@ static pthread_cond_t cond_result; static int skip_first_line; -static void add_work(struct grep_opt *opt, enum grep_source_type type, - const char *name, const char *path, const void *id) +static void add_work(struct grep_opt *opt, const struct grep_source *gs) { grep_lock(); @@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type, pthread_cond_wait(&cond_write, &grep_mutex); } - grep_source_init(&todo[todo_end].source, type, name, path, id); + todo[todo_end].source = *gs; if (opt->binary != GREP_BINARY_TEXT) grep_source_load_driver(&todo[todo_end].source); todo[todo_end].done = 0; @@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid, const char *path) { struct strbuf pathbuf = STRBUF_INIT; + struct grep_source gs; if (opt->relative && opt->prefix_length) { quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf); @@ -325,18 +325,22 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid, strbuf_addstr(&pathbuf, filename); } + grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid); + #ifndef NO_PTHREADS if (num_threads) { - add_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid); + /* + * add_work() copies gs and thus assumes ownership of + * its fields, so do not call grep_source_clear() + */ + add_work(opt, &gs); strbuf_release(&pathbuf); return 0; } else #endif { - struct grep_source gs; int hit; - grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid); strbuf_release(&pathbuf); hit = grep_source(opt, &gs); @@ -348,24 +352,29 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid, static int grep_file(struct grep_opt *opt, const char *filename) { struct strbuf buf = STRBUF_INIT; + struct grep_source gs; if (opt->relative && opt->prefix_length) quote_path_relative(filename, opt->prefix, &buf); else strbuf_addstr(&buf, filename); + grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename); + #ifndef NO_PTHREADS if (num_threads) { - add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename); + /* + * add_work() copies gs and thus assumes ownership of + * its fields, so do not call grep_source_clear() + */ + add_work(opt, &gs); strbuf_release(&buf); return 0; } else #endif { - struct grep_source gs; int hit; - grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename); strbuf_release(&buf); hit = grep_source(opt, &gs);