From 78db6ea9dc1a872f9d07a36fe7aec700a5c963b9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:18:29 -0500 Subject: grep: make locking flag global The low-level grep code traditionally didn't care about threading, as it doesn't do any threading itself and didn't call out to other non-thread-safe code. That changed with 0579f91 (grep: enable threading with -p and -W using lazy attribute lookup, 2011-12-12), which pushed the lookup of funcname attributes (which is not thread-safe) into the low-level grep code. As a result, the low-level code learned about a new global "grep_attr_mutex" to serialize access to the attribute code. A multi-threaded caller (e.g., builtin/grep.c) is expected to initialize the mutex and set "use_threads" in the grep_opt structure. The low-level code only uses the lock if use_threads is set. However, putting the use_threads flag into the grep_opt struct is not the most logical place. Whether threading is in use is not something that matters for each call to grep_buffer, but is instead global to the whole program (i.e., if any thread is doing multi-threaded grep, every other thread, even if it thinks it is doing its own single-threaded grep, would need to use the locking). In practice, this distinction isn't a problem for us, because the only user of multi-threaded grep is "git-grep", which does nothing except call grep. This patch turns the opt->use_threads flag into a global flag. More important than the nit-picking semantic argument above is that this means that the locking functions don't need to actually have access to a grep_opt to know whether to lock. Which in turn can make adding new locks simpler, as we don't need to pass around a grep_opt. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index 486230b51..7a67c2ff6 100644 --- a/grep.c +++ b/grep.c @@ -807,26 +807,28 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, } #ifndef NO_PTHREADS +int grep_use_locks; + /* * This lock protects access to the gitattributes machinery, which is * not thread-safe. */ pthread_mutex_t grep_attr_mutex; -static inline void grep_attr_lock(struct grep_opt *opt) +static inline void grep_attr_lock(void) { - if (opt->use_threads) + if (grep_use_locks) pthread_mutex_lock(&grep_attr_mutex); } -static inline void grep_attr_unlock(struct grep_opt *opt) +static inline void grep_attr_unlock(void) { - if (opt->use_threads) + if (grep_use_locks) pthread_mutex_unlock(&grep_attr_mutex); } #else -#define grep_attr_lock(opt) -#define grep_attr_unlock(opt) +#define grep_attr_lock() +#define grep_attr_unlock() #endif static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol) @@ -834,9 +836,9 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha xdemitconf_t *xecfg = opt->priv; if (xecfg && !xecfg->find_func) { struct userdiff_driver *drv; - grep_attr_lock(opt); + grep_attr_lock(); drv = userdiff_find_by_path(name); - grep_attr_unlock(opt); + grep_attr_unlock(); if (drv && drv->funcname.pattern) { const struct userdiff_funcname *pe = &drv->funcname; xdiff_set_find_func(xecfg, pe->pattern, pe->cflags); -- cgit v1.2.1 From b3aeb285d0ac1dcb4d578a61a68e08646f96501f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:18:41 -0500 Subject: grep: move sha1-reading mutex into low-level code The multi-threaded git-grep code needs to serialize access to the thread-unsafe read_sha1_file call. It does this with a mutex that is local to builtin/grep.c. Let's instead push this down into grep.c, where it can be used by both builtin/grep.c and grep.c. This will let us safely teach the low-level grep.c code tricks that involve reading from the object db. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'grep.c') diff --git a/grep.c b/grep.c index 7a67c2ff6..db58a29c2 100644 --- a/grep.c +++ b/grep.c @@ -826,6 +826,12 @@ static inline void grep_attr_unlock(void) if (grep_use_locks) 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; + #else #define grep_attr_lock() #define grep_attr_unlock() -- cgit v1.2.1 From e1327023ea22c3bf57e7d28596da356043f073fc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:19:28 -0500 Subject: grep: refactor the concept of "grep source" into an object The main interface to the low-level grep code is grep_buffer, which takes a pointer to a buffer and a size. This is convenient and flexible (we use it to grep commit bodies, files on disk, and blobs by sha1), but it makes it hard to pass extra information about what we are grepping (either for correctness, like overriding binary auto-detection, or for optimizations, like lazily loading blob contents). Instead, let's encapsulate the idea of a "grep source", including the buffer, its size, and where the data is coming from. This is similar to the diff_filespec structure used by the diff code (unsurprising, since future patches will implement some of the same optimizations found there). The diffstat is slightly scarier than the actual patch content. Most of the modified lines are simply replacing access to raw variables with their counterparts that are now in a "struct grep_source". Most of the added lines were taken from builtin/grep.c, which partially abstracted the idea of grep sources (for file vs sha1 sources). Instead of dropping the now-redundant code, this patch leaves builtin/grep.c using the traditional grep_buffer interface (which now wraps the grep_source interface). That makes it easy to test that there is no change of behavior (yet). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 164 insertions(+), 34 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index db58a29c2..8204ca25e 100644 --- a/grep.c +++ b/grep.c @@ -837,13 +837,13 @@ pthread_mutex_t grep_read_mutex; #define grep_attr_unlock() #endif -static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol) +static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol) { xdemitconf_t *xecfg = opt->priv; if (xecfg && !xecfg->find_func) { struct userdiff_driver *drv; grep_attr_lock(); - drv = userdiff_find_by_path(name); + drv = userdiff_find_by_path(gs->name); grep_attr_unlock(); if (drv && drv->funcname.pattern) { const struct userdiff_funcname *pe = &drv->funcname; @@ -866,33 +866,33 @@ static int match_funcname(struct grep_opt *opt, const char *name, char *bol, cha return 0; } -static void show_funcname_line(struct grep_opt *opt, const char *name, - char *buf, char *bol, unsigned lno) +static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs, + char *bol, unsigned lno) { - while (bol > buf) { + while (bol > gs->buf) { char *eol = --bol; - while (bol > buf && bol[-1] != '\n') + while (bol > gs->buf && bol[-1] != '\n') bol--; lno--; if (lno <= opt->last_shown) break; - if (match_funcname(opt, name, bol, eol)) { - show_line(opt, bol, eol, name, lno, '='); + if (match_funcname(opt, gs, bol, eol)) { + show_line(opt, bol, eol, gs->name, lno, '='); break; } } } -static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, +static void show_pre_context(struct grep_opt *opt, struct grep_source *gs, char *bol, char *end, unsigned lno) { unsigned cur = lno, from = 1, funcname_lno = 0; int funcname_needed = !!opt->funcname; - if (opt->funcbody && !match_funcname(opt, name, bol, end)) + if (opt->funcbody && !match_funcname(opt, gs, bol, end)) funcname_needed = 2; if (opt->pre_context < lno) @@ -901,14 +901,14 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, from = opt->last_shown + 1; /* Rewind. */ - while (bol > buf && + while (bol > gs->buf && cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) { char *eol = --bol; - while (bol > buf && bol[-1] != '\n') + while (bol > gs->buf && bol[-1] != '\n') bol--; cur--; - if (funcname_needed && match_funcname(opt, name, bol, eol)) { + if (funcname_needed && match_funcname(opt, gs, bol, eol)) { funcname_lno = cur; funcname_needed = 0; } @@ -916,7 +916,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, /* We need to look even further back to find a function signature. */ if (opt->funcname && funcname_needed) - show_funcname_line(opt, name, buf, bol, cur); + show_funcname_line(opt, gs, bol, cur); /* Back forward. */ while (cur < lno) { @@ -924,7 +924,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, while (*eol != '\n') eol++; - show_line(opt, bol, eol, name, cur, sign); + show_line(opt, bol, eol, gs->name, cur, sign); bol = eol + 1; cur++; } @@ -991,11 +991,10 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size) fwrite(buf, size, 1, stdout); } -static int grep_buffer_1(struct grep_opt *opt, const char *name, - char *buf, unsigned long size, int collect_hits) +static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits) { - char *bol = buf; - unsigned long left = size; + char *bol; + unsigned long left; unsigned lno = 1; unsigned last_hit = 0; int binary_match_only = 0; @@ -1023,13 +1022,16 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, } opt->last_shown = 0; + if (grep_source_load(gs) < 0) + return 0; + switch (opt->binary) { case GREP_BINARY_DEFAULT: - if (buffer_is_binary(buf, size)) + if (buffer_is_binary(gs->buf, gs->size)) binary_match_only = 1; break; case GREP_BINARY_NOMATCH: - if (buffer_is_binary(buf, size)) + if (buffer_is_binary(gs->buf, gs->size)) return 0; /* Assume unmatch */ break; case GREP_BINARY_TEXT: @@ -1043,6 +1045,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, try_lookahead = should_lookahead(opt); + bol = gs->buf; + left = gs->size; while (left) { char *eol, ch; int hit; @@ -1091,14 +1095,14 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, if (opt->status_only) return 1; if (opt->name_only) { - show_name(opt, name); + show_name(opt, gs->name); return 1; } if (opt->count) goto next_line; if (binary_match_only) { opt->output(opt, "Binary file ", 12); - output_color(opt, name, strlen(name), + output_color(opt, gs->name, strlen(gs->name), opt->color_filename); opt->output(opt, " matches\n", 9); return 1; @@ -1107,23 +1111,23 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, * pre-context lines, we would need to show them. */ if (opt->pre_context || opt->funcbody) - show_pre_context(opt, name, buf, bol, eol, lno); + show_pre_context(opt, gs, bol, eol, lno); else if (opt->funcname) - show_funcname_line(opt, name, buf, bol, lno); - show_line(opt, bol, eol, name, lno, ':'); + show_funcname_line(opt, gs, bol, lno); + show_line(opt, bol, eol, gs->name, lno, ':'); last_hit = lno; if (opt->funcbody) show_function = 1; goto next_line; } - if (show_function && match_funcname(opt, name, bol, eol)) + if (show_function && match_funcname(opt, gs, bol, eol)) show_function = 0; if (show_function || (last_hit && lno <= last_hit + opt->post_context)) { /* If the last hit is within the post context, * we need to show this line. */ - show_line(opt, bol, eol, name, lno, '-'); + show_line(opt, bol, eol, gs->name, lno, '-'); } next_line: @@ -1141,7 +1145,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, return 0; if (opt->unmatch_name_only) { /* We did not see any hit, so we want to show this */ - show_name(opt, name); + show_name(opt, gs->name); return 1; } @@ -1155,7 +1159,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, */ if (opt->count && count) { char buf[32]; - output_color(opt, name, strlen(name), opt->color_filename); + output_color(opt, gs->name, strlen(gs->name), opt->color_filename); output_sep(opt, ':'); snprintf(buf, sizeof(buf), "%u\n", count); opt->output(opt, buf, strlen(buf)); @@ -1190,23 +1194,149 @@ static int chk_hit_marker(struct grep_expr *x) } } -int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size) +int grep_source(struct grep_opt *opt, struct grep_source *gs) { /* * we do not have to do the two-pass grep when we do not check * buffer-wide "all-match". */ if (!opt->all_match) - return grep_buffer_1(opt, name, buf, size, 0); + return grep_source_1(opt, gs, 0); /* Otherwise the toplevel "or" terms hit a bit differently. * We first clear hit markers from them. */ clr_hit_marker(opt->pattern_expression); - grep_buffer_1(opt, name, buf, size, 1); + grep_source_1(opt, gs, 1); if (!chk_hit_marker(opt->pattern_expression)) return 0; - return grep_buffer_1(opt, name, buf, size, 0); + return grep_source_1(opt, gs, 0); +} + +int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size) +{ + struct grep_source gs; + int r; + + grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL); + gs.buf = buf; + gs.size = size; + + r = grep_source(opt, &gs); + + grep_source_clear(&gs); + return r; +} + +void grep_source_init(struct grep_source *gs, enum grep_source_type type, + const char *name, const void *identifier) +{ + gs->type = type; + gs->name = name ? xstrdup(name) : NULL; + gs->buf = NULL; + gs->size = 0; + + switch (type) { + case GREP_SOURCE_FILE: + gs->identifier = xstrdup(identifier); + break; + case GREP_SOURCE_SHA1: + gs->identifier = xmalloc(20); + memcpy(gs->identifier, identifier, 20); + break; + case GREP_SOURCE_BUF: + gs->identifier = NULL; + } +} + +void grep_source_clear(struct grep_source *gs) +{ + free(gs->name); + gs->name = NULL; + free(gs->identifier); + gs->identifier = NULL; + grep_source_clear_data(gs); +} + +void grep_source_clear_data(struct grep_source *gs) +{ + switch (gs->type) { + case GREP_SOURCE_FILE: + case GREP_SOURCE_SHA1: + free(gs->buf); + gs->buf = NULL; + gs->size = 0; + break; + case GREP_SOURCE_BUF: + /* leave user-provided buf intact */ + break; + } +} + +static int grep_source_load_sha1(struct grep_source *gs) +{ + enum object_type type; + + grep_read_lock(); + gs->buf = read_sha1_file(gs->identifier, &type, &gs->size); + grep_read_unlock(); + + if (!gs->buf) + return error(_("'%s': unable to read %s"), + gs->name, + sha1_to_hex(gs->identifier)); + return 0; +} + +static int grep_source_load_file(struct grep_source *gs) +{ + const char *filename = gs->identifier; + struct stat st; + char *data; + size_t size; + int i; + + if (lstat(filename, &st) < 0) { + err_ret: + if (errno != ENOENT) + error(_("'%s': %s"), filename, strerror(errno)); + return -1; + } + if (!S_ISREG(st.st_mode)) + return -1; + size = xsize_t(st.st_size); + i = open(filename, O_RDONLY); + if (i < 0) + goto err_ret; + data = xmalloc(size + 1); + if (st.st_size != read_in_full(i, data, size)) { + error(_("'%s': short read %s"), filename, strerror(errno)); + close(i); + free(data); + return -1; + } + close(i); + data[size] = 0; + + gs->buf = data; + gs->size = size; + return 0; +} + +int grep_source_load(struct grep_source *gs) +{ + if (gs->buf) + return 0; + + switch (gs->type) { + case GREP_SOURCE_FILE: + return grep_source_load_file(gs); + case GREP_SOURCE_SHA1: + return grep_source_load_sha1(gs); + case GREP_SOURCE_BUF: + return gs->buf ? 0 : -1; + } + die("BUG: invalid grep_source type"); } -- cgit v1.2.1 From c876d6da88d9bf1f3377d4eed66fc7705e31c30e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:20:10 -0500 Subject: grep: drop grep_buffer's "name" parameter Before the grep_source interface existed, grep_buffer was used by two types of callers: 1. Ones which pulled a file into a buffer, and then wanted to supply the file's name for the output (i.e., git grep). 2. Ones which really just wanted to grep a buffer (i.e., git log --grep). Callers in set (1) should now be using grep_source. Callers in set (2) always pass NULL for the "name" parameter of grep_buffer. We can therefore get rid of this now-useless parameter. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index 8204ca25e..2a3fe7ce6 100644 --- a/grep.c +++ b/grep.c @@ -1215,12 +1215,12 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs) return grep_source_1(opt, gs, 0); } -int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size) +int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size) { struct grep_source gs; int r; - grep_source_init(&gs, GREP_SOURCE_BUF, name, NULL); + grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL); gs.buf = buf; gs.size = size; -- cgit v1.2.1 From 94ad9d9e075d3f7802cf56641ebb342d43fb46e3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:20:43 -0500 Subject: grep: cache userdiff_driver in grep_source Right now, grep only uses the userdiff_driver for one thing: looking up funcname patterns for "-p" and "-W". As new uses for userdiff drivers are added to the grep code, we want to minimize attribute lookups, which can be expensive. It might seem at first that this would also optimize multiple lookups when the funcname pattern for a file is needed multiple times. However, the compiled funcname pattern is already cached in struct grep_opt's "priv" member, so multiple lookups are already suppressed. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index 2a3fe7ce6..bb1856985 100644 --- a/grep.c +++ b/grep.c @@ -841,12 +841,9 @@ static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bo { xdemitconf_t *xecfg = opt->priv; if (xecfg && !xecfg->find_func) { - struct userdiff_driver *drv; - grep_attr_lock(); - drv = userdiff_find_by_path(gs->name); - grep_attr_unlock(); - if (drv && drv->funcname.pattern) { - const struct userdiff_funcname *pe = &drv->funcname; + grep_source_load_driver(gs); + if (gs->driver->funcname.pattern) { + const struct userdiff_funcname *pe = &gs->driver->funcname; xdiff_set_find_func(xecfg, pe->pattern, pe->cflags); } else { xecfg = opt->priv = NULL; @@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type, gs->name = name ? xstrdup(name) : NULL; gs->buf = NULL; gs->size = 0; + gs->driver = NULL; switch (type) { case GREP_SOURCE_FILE: @@ -1340,3 +1338,15 @@ int grep_source_load(struct grep_source *gs) } die("BUG: invalid grep_source type"); } + +void grep_source_load_driver(struct grep_source *gs) +{ + if (gs->driver) + return; + + grep_attr_lock(); + gs->driver = userdiff_find_by_path(gs->name); + if (!gs->driver) + gs->driver = userdiff_find_by_name("default"); + grep_attr_unlock(); +} -- cgit v1.2.1 From 41b59bfcb16abb738e5c95c95fb462e717d47d4d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:21:02 -0500 Subject: grep: respect diff attributes for binary-ness There is currently no way for users to tell git-grep that a particular path is or is not a binary file; instead, grep always relies on its auto-detection (or the user specifying "-a" to treat all binary-looking files like text). This patch teaches git-grep to use the same attribute lookup that is used by git-diff. We could add a new "grep" flag, but that is unnecessarily complex and unlikely to be useful. Despite the name, the "-diff" attribute (or "diff=foo" and the associated diff.foo.binary config option) are really about describing the contents of the path. It's simply historical that diff was the only thing that cared about these attributes in the past. And if this simple approach turns out to be insufficient, we still have a backwards-compatible path forward: we can add a separate "grep" attribute, and fall back to respecting "diff" if it is unset. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index bb1856985..a50d16172 100644 --- a/grep.c +++ b/grep.c @@ -1024,11 +1024,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle switch (opt->binary) { case GREP_BINARY_DEFAULT: - if (buffer_is_binary(gs->buf, gs->size)) + if (grep_source_is_binary(gs)) binary_match_only = 1; break; case GREP_BINARY_NOMATCH: - if (buffer_is_binary(gs->buf, gs->size)) + if (grep_source_is_binary(gs)) return 0; /* Assume unmatch */ break; case GREP_BINARY_TEXT: @@ -1350,3 +1350,15 @@ void grep_source_load_driver(struct grep_source *gs) gs->driver = userdiff_find_by_name("default"); grep_attr_unlock(); } + +int grep_source_is_binary(struct grep_source *gs) +{ + grep_source_load_driver(gs); + if (gs->driver->binary != -1) + return gs->driver->binary; + + if (!grep_source_load(gs)) + return buffer_is_binary(gs->buf, gs->size); + + return 0; +} -- cgit v1.2.1 From 08265798e1ff6abc1b0aaff31c1471f83bd51425 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2012 03:21:11 -0500 Subject: grep: load file data after checking binary-ness Usually we load each file to grep into memory, check whether it's binary, and then either grep it (the default) or not (if "-I" was given). In the "-I" case, we can skip loading the file entirely if it is marked as binary via gitattributes. On my giant 3-gigabyte media repository, doing "git grep -I foo" went from: real 0m0.712s user 0m0.044s sys 0m4.780s to: real 0m0.026s user 0m0.016s sys 0m0.020s Obviously this is an extreme example. The repo is almost entirely binary files, and you can see that we spent all of our time asking the kernel to read() the data. However, with a cold disk cache, even avoiding a few binary files can have an impact. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- grep.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'grep.c') diff --git a/grep.c b/grep.c index a50d16172..382140096 100644 --- a/grep.c +++ b/grep.c @@ -1019,9 +1019,6 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle } opt->last_shown = 0; - if (grep_source_load(gs) < 0) - return 0; - switch (opt->binary) { case GREP_BINARY_DEFAULT: if (grep_source_is_binary(gs)) @@ -1042,6 +1039,9 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle try_lookahead = should_lookahead(opt); + if (grep_source_load(gs) < 0) + return 0; + bol = gs->buf; left = gs->size; while (left) { -- cgit v1.2.1