@matheusbsbs / matheus-bagliano / commits / e2e05d619a

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 <rv@rasmusvillemoes.dk> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Rasmus Villemoes committed Feb 23, 2018 at 15:47 UTC e2e05d619a71ae2c06ebd3c661260bf4b5039a93
1 file changed +18 -9
builtin/grep.c
+18 -9
@@ -92,8 +92,7 @@ static pthread_cond_t cond_result;
92
93 static int skip_first_line;
94
95 -static void add_work(struct grep_opt *opt, enum grep_source_type type,
96 - const char *name, const char *path, const void *id)
95 +static void add_work(struct grep_opt *opt, const struct grep_source *gs)
96 {
97 grep_lock();
98
@@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,
100 pthread_cond_wait(&cond_write, &grep_mutex);
101 }
102
104 - grep_source_init(&todo[todo_end].source, type, name, path, id);
103 + todo[todo_end].source = *gs;
104 if (opt->binary != GREP_BINARY_TEXT)
105 grep_source_load_driver(&todo[todo_end].source);
106 todo[todo_end].done = 0;
@@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
316 const char *path)
317 {
318 struct strbuf pathbuf = STRBUF_INIT;
319 + struct grep_source gs;
320
321 if (opt->relative && opt->prefix_length) {
322 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,
325 strbuf_addstr(&pathbuf, filename);
326 }
327
328 + grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
329 +
330 #ifndef NO_PTHREADS
331 if (num_threads) {
330 - add_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);
332 + /*
333 + * add_work() copies gs and thus assumes ownership of
334 + * its fields, so do not call grep_source_clear()
335 + */
336 + add_work(opt, &gs);
337 strbuf_release(&pathbuf);
338 return 0;
339 } else
340 #endif
341 {
336 - struct grep_source gs;
342 int hit;
343
339 - grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
344 strbuf_release(&pathbuf);
345 hit = grep_source(opt, &gs);
346
@@ -348,24 +352,29 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
352 static int grep_file(struct grep_opt *opt, const char *filename)
353 {
354 struct strbuf buf = STRBUF_INIT;
355 + struct grep_source gs;
356
357 if (opt->relative && opt->prefix_length)
358 quote_path_relative(filename, opt->prefix, &buf);
359 else
360 strbuf_addstr(&buf, filename);
361
362 + grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
363 +
364 #ifndef NO_PTHREADS
365 if (num_threads) {
359 - add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
366 + /*
367 + * add_work() copies gs and thus assumes ownership of
368 + * its fields, so do not call grep_source_clear()
369 + */
370 + add_work(opt, &gs);
371 strbuf_release(&buf);
372 return 0;
373 } else
374 #endif
375 {
365 - struct grep_source gs;
376 int hit;
377
368 - grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
378 strbuf_release(&buf);
379 hit = grep_source(opt, &gs);
380