Re: [PATCH 1/3] grep: move grep_source_init outside critical section

[Junio C Hamano]

Jeff King  writes:

> I think this makes sense. It does blur the memory ownership lines of the
> grep_source, though. Can we make that more clear with a comment here:
>
>> +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(opt, &gs);
>>  strbuf_release(&pathbuf);
>>  return 0;
>>  } else
>
> like:
>
>   /* leak grep_source, whose fields are now owned by add_work() */
>
> or something? We could even memset() it back to all-zeroes to avoid an
> accidental call to grep_source_clear(), but that's probably unnecessary
> if we have a comment.

I share the same uneasiness about the fuzzy memory ownership this
change brings in.  Thanks for suggesting improvements.

Re: [PATCH 1/3] grep: move grep_source_init outside critical section

[Jeff King]

On Thu, Feb 15, 2018 at 10:56:13PM +0100, Rasmus Villemoes wrote:

> 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
> "GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent
> cleanup patch will make that even more so.

I think this makes sense. It does blur the memory ownership lines of the
grep_source, though. Can we make that more clear with a comment here:

> + 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(opt, &gs);
>   strbuf_release(&pathbuf);
>   return 0;
>   } else

like:

  /* leak grep_source, whose fields are now owned by add_work() */

or something? We could even memset() it back to all-zeroes to avoid an
accidental call to grep_source_clear(), but that's probably unnecessary
if we have a comment.

-Peff

[PATCH 1/3] grep: move grep_source_init outside critical section

[Rasmus Villemoes]

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
"GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent
cleanup patch will make that even more so.

Signed-off-by: Rasmus Villemoes 
---
 builtin/grep.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d..4a4f15172 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,18 @@ 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(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 +348,25 @@ 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(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);
 
-- 
2.15.1