threads / patch / 45956

v2, 7 partsPCRE v2, PCRE v1 JIT, log -P & fixes

Subject: [PATCH v2 0/7] PCRE v2, PCRE v1 JIT, log -P & fixes

## tl;dr

12 messages between May 13, 2017 and May 15, 2017. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · lore

Trivial changes since v1, but I wanted to send a new one for completeness since I re-sent the "Easy to review grep & pre-PCRE changes" series.

For v1 see <20170511170142.15934-1-avarab@gmail.com> (https://public-inbox.org/git/20170511170142.15934-1-avarab@gmail.com/).

Changes noted below & reply to who noted the issue:
Ævar Arnfjörð Bjarmason (7):
  grep: don't redundantly compile throwaway patterns under threading
Brandon: Added a few paragraphs to the commit message about why this
change is being made, i.e. for ease of understanding the code, not
optimization.
  grep: skip pthreads overhead when using one thread
  log: add -P as a synonym for --perl-regexp
  grep: add support for the PCRE v1 JIT API
  grep: un-break building with PCRE < 8.32
  grep: un-break building with PCRE < 8.20
  grep: add support for PCRE v2

No changes in these, except for re-running the performance tests & changing the commit messages accordingly, due to the change in the t/perf code in the previous series, as noted there.

 Documentation/rev-list-options.txt |   1 +
 Makefile                           |  30 +++++--
 builtin/grep.c                     |  16 +++-
 configure.ac                       |  77 +++++++++++++---
 grep.c                             | 180 ++++++++++++++++++++++++++++++++++++-
 grep.h                             |  31 +++++++
 revision.c                         |   2 +-
 t/t4202-log.sh                     |  12 +++
 t/test-lib.sh                      |   2 +-
 9 files changed, 327 insertions(+), 24 deletions(-)
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/7] grep: don't redundantly compile throwaway patterns under threading

Change the pattern compilation logic under threading so that grep doesn't compile a pattern it never ends up using on the non-threaded code path, only to compile it again N times for N threads which will each use their own copy, ignoring the initially compiled pattern.

This redundant compilation dates back to the initial introduction of the threaded grep in commit 5b594f457a ("Threaded grep", 2010-01-25).

There was never any reason for doing this redundant work other than an oversight in the initial commit. Jeff King suggested on-list in <20170414212325.fefrl3qdjigwyitd@sigill.intra.peff.net> that this might be needed to check the pattern for sanity before threaded execution commences.

That's not the case. The pattern is compiled under threading in start_threads() before any concurrent execution has started by calling pthread_create(), so if the pattern contains an error we still do the right thing. I.e. die with one error before any threaded execution has commenced, instead of e.g. spewing out an error for each N threads, which could be a regression a change like this might inadvertently introduce.

This change is not meant as an optimization, any performance gains from this are in the hundreds to thousands of nanoseconds at most. If we wanted more performance here we could just re-use the compiled patterns in multiple threads (regcomp(3) is thread-safe), or partially re-use them and the associated structures in the case of later PCRE JIT changes.

Rather, it's just to make the code easier to reason about. It's confusing to debug this under threading & non-threading when the threading codepaths redundantly compile a pattern which is never used.

The reason the patterns are recompiled is as a side-effect of duplicating the whole grep_opt structure, which is not thread safe, writable, and munged during execution. The grep_opt structure then points to the grep_pat structure where pattern or patterns are stored.

I looked into e.g. splitting the API into some "do & alloc threadsafe stuff", "spawn thread", "do and alloc non-threadsafe stuff", but the execution time of grep_opt_dup() & pattern compilation is trivial compared to actually executing the grep, so there was no point. Even with the more expensive JIT changes to follow the most expensive PCRE patterns take something like 0.0X milliseconds to compile at most[1].

The undocumented --debug mode added in commit 17bf35a3c7 ("grep: teach --debug option to dump the parse tree", 2012-09-13) still works properly with this change. It only emits debugging info during pattern compilation, which is now dumped by the pattern compiled just before the first thread is started.

1. http://sljit.sourceforge.net/pcre.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
Show changes to builtin/grep.c +11 −3
diff --git a/builtin/grep.c b/builtin/grep.c
index b1095362fb..12e62fcbf3 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -224,7 +224,8 @@ static void start_threads(struct grep_opt *opt)
 		int err;
 		struct grep_opt *o = grep_opt_dup(opt);
 		o->output = strbuf_out;
-		o->debug = 0;
+		if (i)
+			o->debug = 0;
 		compile_grep_patterns(o);
 		err = pthread_create(&threads[i], NULL, run, o);
 
@@ -1167,8 +1168,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!opt.fixed && opt.ignore_case)
 		opt.regflags |= REG_ICASE;
 
-	compile_grep_patterns(&opt);
-
 	/*
 	 * We have to find "--" in a separate pass, because its presence
 	 * influences how we will parse arguments that come before it.
@@ -1245,6 +1244,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	num_threads = 0;
 #endif
 
+	if (!num_threads)
+		/*
+		 * The compiled patterns on the main path are only
+		 * used when not using threading. Otherwise
+		 * start_threads() below calls compile_grep_patterns()
+		 * for each thread.
+		 */
+		compile_grep_patterns(&opt);
+
 #ifndef NO_PTHREADS
 	if (num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/7] grep: skip pthreads overhead when using one thread

Skip the administrative overhead of using pthreads when only using one thread. Instead take the non-threaded path which would be taken under NO_PTHREADS.

The threading support was initially added in commit 5b594f457a ("Threaded grep", 2010-01-25) with a hardcoded compile-time number of 8 threads. Later the number of threads was made configurable in commit 89f09dd34e ("grep: add --threads=<num> option and grep.threads configuration", 2015-12-15).

That change did not add any special handling for --threads=1. Now we take a slightly faster path by skipping thread handling entirely when 1 thread is requested.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/grep.c +2 −0
diff --git a/builtin/grep.c b/builtin/grep.c
index 12e62fcbf3..bd008cb100 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1238,6 +1238,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		num_threads = GREP_NUM_THREADS_DEFAULT;
 	else if (num_threads < 0)
 		die(_("invalid number of threads specified (%d)"), num_threads);
+	if (num_threads == 1)
+		num_threads = 0;
 #else
 	if (num_threads)
 		warning(_("no threads support, ignoring --threads"));
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 3/7] log: add -P as a synonym for --perl-regexp

Add a short -P option as a synonym for the longer --perl-regexp, for consistency with the options the corresponding grep invocations accept.

This was intentionally omitted in commit 727b6fc3ed ("log --grep: accept --basic-regexp and --perl-regexp", 2012-10-03) for unspecified future use.

Make it consistent with "grep" rather than to keep it open for future use, and to avoid the confusion of -P meaning different things for grep & log, as is the case with the -G option.

As noted in the aforementioned commit the --basic-regexp option can't have a corresponding -G argument, as the log command already uses that for -G<regex>.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/rev-list-options.txt |  1 +
 revision.c                         |  2 +-
 t/t4202-log.sh                     | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)
Show changes to 3 files +14 −1

Documentation/rev-list-options.txt, revision.c, t/t4202-log.sh

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a46f70c2b1..9c44eae55d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -91,6 +91,7 @@ endif::git-rev-list[]
 	Consider the limiting patterns to be fixed strings (don't interpret
 	pattern as a regular expression).
 
+-P::
 --perl-regexp::
 	Consider the limiting patterns to be Perl-compatible regular
 	expressions.
diff --git a/revision.c b/revision.c
index 7ff61ff5f7..03a3a012de 100644
--- a/revision.c
+++ b/revision.c
@@ -1995,7 +1995,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
 		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;
-	} else if (!strcmp(arg, "--perl-regexp")) {
+	} else if (!strcmp(arg, "--perl-regexp") || !strcmp(arg, "-P")) {
 		revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_PCRE;
 	} else if (!strcmp(arg, "--all-match")) {
 		revs->grep_filter.all_match = 1;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 9680dfe400..5c05c650cd 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -350,8 +350,20 @@ test_expect_success 'log with various grep.patternType configurations & command-
 			--grep="(1|2)" >actual.fixed.short-arg &&
 		git log --pretty=tformat:%s -E \
 			--grep="\|2" >actual.extended.short-arg &&
+		if test_have_prereq PCRE
+		then
+			git log --pretty=tformat:%s -P \
+				--grep="[\d]\|" >actual.perl.short-arg
+		else
+			test_must_fail git log -P \
+				--grep="[\d]\|"
+		fi &&
 		test_cmp expect.fixed actual.fixed.short-arg &&
 		test_cmp expect.extended actual.extended.short-arg &&
+		if test_have_prereq PCRE
+		then
+			test_cmp expect.perl actual.perl.short-arg
+		fi &&
 
 		git log --pretty=tformat:%s --fixed-strings \
 			--grep="(1|2)" >actual.fixed.long-arg &&
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

Change the grep PCRE v1 code to use JIT when available. When PCRE support was initially added in commit 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) PCRE had no JIT support, it was integrated into 8.20 released on 2011-10-21.

Enabling JIT support usually improves performance by more than 40%. The pattern compilation times are relatively slower, but those relative numbers are tiny, and are easily made back in all but the most trivial cases of grep. Detailed benchmarks & overview of compilation times is at: http://sljit.sourceforge.net/pcre.html

With this change the difference in a t/perf/p7820-grep-engines.sh run is, with just the /perl/ tests shown:

    $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~ HEAD p7820-grep-engines.sh
    Test                                           HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep how.to                      0.27(1.18+0.42)   0.19(0.30+0.68) -29.6%
    7820.7: perl grep ^how to                     0.48(2.74+0.41)   0.21(0.64+0.55) -56.2%
    7820.11: perl grep [how] to                   0.47(2.56+0.47)   0.26(0.86+0.56) -44.7%
    7820.15: perl grep (e.t[^ ]*|v.ry) rare       0.88(5.79+0.38)   0.26(1.06+0.54) -70.5%
    7820.19: perl grep m(ú|u)lt.b(æ|y)te          0.32(1.56+0.46)   0.23(0.86+0.48) -28.1%

The conditional support for JIT is implemented as suggested in the pcrejit(3) man page. E.g. defining PCRE_STUDY_JIT_COMPILE to 0 if it's not present.

The implementation is relatively verbose because even if PCRE_CONFIG_JIT is defined only a call to pcre_config() can determine if the JIT is available, and if so the faster pcre_jit_exec() function should be called instead of pcre_exec(), and a different (but not complimentary!) function needs to be called to free pcre1_extra_info.

There's no graceful fallback if pcre_jit_stack_alloc() fails under PCRE_CONFIG_JIT, instead the program will simply abort. I don't think this is worth handling gracefully, it'll only fail in cases where malloc() doesn't work, in which case we're screwed anyway.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 37 ++++++++++++++++++++++++++++++++++++-
 grep.h |  6 ++++++
 2 files changed, 42 insertions(+), 1 deletion(-)
Show changes to 2 files +42 −1

grep.c, grep.h

diff --git a/grep.c b/grep.c
index 1157529115..accf1c45e6 100644
--- a/grep.c
+++ b/grep.c
@@ -351,6 +351,9 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 	const char *error;
 	int erroffset;
 	int options = PCRE_MULTILINE;
+#ifdef PCRE_CONFIG_JIT
+	int canjit;
+#endif
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern))
@@ -365,9 +368,20 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 	if (!p->pcre1_regexp)
 		compile_regexp_failed(p, error);
 
-	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 0, &error);
+	p->pcre1_extra_info = pcre_study(p->pcre1_regexp, PCRE_STUDY_JIT_COMPILE, &error);
 	if (!p->pcre1_extra_info && error)
 		die("%s", error);
+
+#ifdef PCRE_CONFIG_JIT
+	pcre_config(PCRE_CONFIG_JIT, &canjit);
+	if (canjit == 1) {
+		p->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);
+		if (!p->pcre1_jit_stack)
+			die("BUG: Couldn't allocate PCRE JIT stack");
+		pcre_assign_jit_stack(p->pcre1_extra_info, NULL, p->pcre1_jit_stack);
+		p->pcre1_jit_on = 1;
+	}
+#endif
 }
 
 static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
@@ -378,8 +392,20 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
 	if (eflags & REG_NOTBOL)
 		flags |= PCRE_NOTBOL;
 
+#ifdef PCRE_CONFIG_JIT
+	if (p->pcre1_jit_on)
+		ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+				    eol - line, 0, flags, ovector,
+				    ARRAY_SIZE(ovector), p->pcre1_jit_stack);
+	else
+		ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+				eol - line, 0, flags, ovector,
+				ARRAY_SIZE(ovector));
+#else
 	ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
 			0, flags, ovector, ARRAY_SIZE(ovector));
+#endif
+
 	if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
 		die("pcre_exec failed with error code %d", ret);
 	if (ret > 0) {
@@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
 static void free_pcre1_regexp(struct grep_pat *p)
 {
 	pcre_free(p->pcre1_regexp);
+#ifdef PCRE_CONFIG_JIT
+	if (p->pcre1_jit_on) {
+		pcre_free_study(p->pcre1_extra_info);
+		pcre_jit_stack_free(p->pcre1_jit_stack);
+	} else {
+		pcre_free(p->pcre1_extra_info);
+	}
+#else
 	pcre_free(p->pcre1_extra_info);
+#endif
 	pcre_free((void *)p->pcre1_tables);
 }
 #else /* !USE_LIBPCRE1 */
diff --git a/grep.h b/grep.h
index 38ac82b638..14f47189f9 100644
--- a/grep.h
+++ b/grep.h
@@ -3,9 +3,13 @@
 #include "color.h"
 #ifdef USE_LIBPCRE1
 #include <pcre.h>
+#ifndef PCRE_STUDY_JIT_COMPILE
+#define PCRE_STUDY_JIT_COMPILE 0
+#endif
 #else
 typedef int pcre;
 typedef int pcre_extra;
+typedef int pcre_jit_stack;
 #endif
 #include "kwset.h"
 #include "thread-utils.h"
@@ -48,7 +52,9 @@ struct grep_pat {
 	regex_t regexp;
 	pcre *pcre1_regexp;
 	pcre_extra *pcre1_extra_info;
+	pcre_jit_stack *pcre1_jit_stack;
 	const unsigned char *pcre1_tables;
+	int pcre1_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
 	unsigned ignore_case:1;
-- 
2.11.0
Simon Ruderich· May 14, 2017, 14:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

On Sat, May 13, 2017 at 11:45:32PM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> [snip]
>
> +#ifdef PCRE_CONFIG_JIT
> +	if (p->pcre1_jit_on)
> +		ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> +				    eol - line, 0, flags, ovector,
> +				    ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> +	else
> +		ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> +				eol - line, 0, flags, ovector,
> +				ARRAY_SIZE(ovector));
> +#else
>  	ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>  			0, flags, ovector, ARRAY_SIZE(ovector));
> +#endif

Wouldn't it be simpler to remove the duplication and unconditionally use the old pcre_exec() call? Something like this:

    +#ifdef PCRE_CONFIG_JIT
    +	if (p->pcre1_jit_on)
    +		ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
    +				    eol - line, 0, flags, ovector,
    +				    ARRAY_SIZE(ovector), p->pcre1_jit_stack);
    +	else
    +#endif
     	ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
     			0, flags, ovector, ARRAY_SIZE(ovector));
Show 17 quoted lines
>  	if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>  		die("pcre_exec failed with error code %d", ret);
>  	if (ret > 0) {
> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
>  static void free_pcre1_regexp(struct grep_pat *p)
>  {
>  	pcre_free(p->pcre1_regexp);
> +#ifdef PCRE_CONFIG_JIT
> +	if (p->pcre1_jit_on) {
> +		pcre_free_study(p->pcre1_extra_info);
> +		pcre_jit_stack_free(p->pcre1_jit_stack);
> +	} else {
> +		pcre_free(p->pcre1_extra_info);
> +	}
> +#else
>  	pcre_free(p->pcre1_extra_info);
> +#endif

Same here. The pcre_free() is the same with and without the ifdef.

Regards Simon

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Ævar Arnfjörð Bjarmason· May 14, 2017, 15:10 UTC · re: Simon Ruderich · lore

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich <simon@ruderich.org> wrote:
Show 51 quoted lines
> On Sat, May 13, 2017 at 11:45:32PM +0000, Ęvar Arnfjörš Bjarmason wrote:
>> [snip]
>>
>> +#ifdef PCRE_CONFIG_JIT
>> +     if (p->pcre1_jit_on)
>> +             ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> +                                 eol - line, 0, flags, ovector,
>> +                                 ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>> +     else
>> +             ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> +                             eol - line, 0, flags, ovector,
>> +                             ARRAY_SIZE(ovector));
>> +#else
>>       ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>>                       0, flags, ovector, ARRAY_SIZE(ovector));
>> +#endif
>
> Wouldn't it be simpler to remove the duplication and
> unconditionally use the old pcre_exec() call? Something like
> this:
>
>     +#ifdef PCRE_CONFIG_JIT
>     +   if (p->pcre1_jit_on)
>     +           ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>     +                               eol - line, 0, flags, ovector,
>     +                               ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>     +   else
>     +#endif
>         ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>                         0, flags, ovector, ARRAY_SIZE(ovector));
>
>>       if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>>               die("pcre_exec failed with error code %d", ret);
>>       if (ret > 0) {
>> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
>>  static void free_pcre1_regexp(struct grep_pat *p)
>>  {
>>       pcre_free(p->pcre1_regexp);
>> +#ifdef PCRE_CONFIG_JIT
>> +     if (p->pcre1_jit_on) {
>> +             pcre_free_study(p->pcre1_extra_info);
>> +             pcre_jit_stack_free(p->pcre1_jit_stack);
>> +     } else {
>> +             pcre_free(p->pcre1_extra_info);
>> +     }
>> +#else
>>       pcre_free(p->pcre1_extra_info);
>> +#endif
>
> Same here. The pcre_free() is the same with and without the
> ifdef.

Yes I could do that, no reason not to, and as you point out it would reduce duplication.

I wrote it like this trying to preserve the indentation with/without the macro being true, thinking someone would have an issue with it otherwise.

I also thought just now that perhaps if it were changed the code like that it would warn under -Wmisleading-indentation, but at least on gcc that's not the case, it knows not to warn in the presence of macros.

Unless someone feel strongly otherwise / can think of a good reason for why not, I'll change it as you suggest in the next version.

Thanks for the review!
Ævar Arnfjörð Bjarmason· May 14, 2017, 15:23 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

On Sun, May 14, 2017 at 5:10 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 68 quoted lines
> On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich <simon@ruderich.org> wrote:
>> On Sat, May 13, 2017 at 11:45:32PM +0000, Ęvar Arnfjörš Bjarmason wrote:
>>> [snip]
>>>
>>> +#ifdef PCRE_CONFIG_JIT
>>> +     if (p->pcre1_jit_on)
>>> +             ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>>> +                                 eol - line, 0, flags, ovector,
>>> +                                 ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>>> +     else
>>> +             ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>>> +                             eol - line, 0, flags, ovector,
>>> +                             ARRAY_SIZE(ovector));
>>> +#else
>>>       ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>>>                       0, flags, ovector, ARRAY_SIZE(ovector));
>>> +#endif
>>
>> Wouldn't it be simpler to remove the duplication and
>> unconditionally use the old pcre_exec() call? Something like
>> this:
>>
>>     +#ifdef PCRE_CONFIG_JIT
>>     +   if (p->pcre1_jit_on)
>>     +           ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>>     +                               eol - line, 0, flags, ovector,
>>     +                               ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>>     +   else
>>     +#endif
>>         ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>>                         0, flags, ovector, ARRAY_SIZE(ovector));
>>
>>>       if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>>>               die("pcre_exec failed with error code %d", ret);
>>>       if (ret > 0) {
>>> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
>>>  static void free_pcre1_regexp(struct grep_pat *p)
>>>  {
>>>       pcre_free(p->pcre1_regexp);
>>> +#ifdef PCRE_CONFIG_JIT
>>> +     if (p->pcre1_jit_on) {
>>> +             pcre_free_study(p->pcre1_extra_info);
>>> +             pcre_jit_stack_free(p->pcre1_jit_stack);
>>> +     } else {
>>> +             pcre_free(p->pcre1_extra_info);
>>> +     }
>>> +#else
>>>       pcre_free(p->pcre1_extra_info);
>>> +#endif
>>
>> Same here. The pcre_free() is the same with and without the
>> ifdef.
>
> Yes I could do that, no reason not to, and as you point out it would
> reduce duplication.
>
> I wrote it like this trying to preserve the indentation with/without
> the macro being true, thinking someone would have an issue with it
> otherwise.
>
> I also thought just now that perhaps if it were changed the code like
> that it would warn under -Wmisleading-indentation, but at least on gcc
> that's not the case, it knows not to warn in the presence of macros.
>
> Unless someone feel strongly otherwise / can think of a good reason
> for why not, I'll change it as you suggest in the next version.
>
> Thanks for the review!

...and if I do change it do others think this is something that warrants a comment & some whitespace padding? I.e.:

Show changes to diff +18 −0
@@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const
char *line, const char *eol,
        if (eflags & REG_NOTBOL)
                flags |= PCRE_NOTBOL;

+#ifdef PCRE_CONFIG_JIT
+       if (p->pcre1_jit_on)
+               ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+                                   eol - line, 0, flags, ovector,
+                                   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
+       else
+#endif
+       /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
        ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
                        0, flags, ovector, ARRAY_SIZE(ovector));
+
        if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
                die("pcre_exec failed with error code %d", ret);
        if (ret > 0) {

and:

@@ -394,7 +417,16 @@ static int pcre1match(struct grep_pat *p, const
char *line, const char *eol,
 static void free_pcre1_regexp(struct grep_pat *p)
 {
        pcre_free(p->pcre1_regexp);
+
+#ifdef PCRE_CONFIG_JIT
+       if (p->pcre1_jit_on) {
+               pcre_free_study(p->pcre1_extra_info);
+               pcre_jit_stack_free(p->pcre1_jit_stack);
+       } else
+#endif
+       /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
        pcre_free(p->pcre1_extra_info);
+
        pcre_free((void *)p->pcre1_tables);
 }
 #else /* !USE_LIBPCRE1 */
Junio C Hamano· May 15, 2017, 02:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 38 quoted lines
>> Yes I could do that, no reason not to, and as you point out it would
>> reduce duplication.
>>
>> I wrote it like this trying to preserve the indentation with/without
>> the macro being true, thinking someone would have an issue with it
>> otherwise.
>>
>> I also thought just now that perhaps if it were changed the code like
>> that it would warn under -Wmisleading-indentation, but at least on gcc
>> that's not the case, it knows not to warn in the presence of macros.
>>
>> Unless someone feel strongly otherwise / can think of a good reason
>> for why not, I'll change it as you suggest in the next version.
>>
>> Thanks for the review!
>
> ...and if I do change it do others think this is something that
> warrants a comment & some whitespace padding? I.e.:
>
> @@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const
> char *line, const char *eol,
>         if (eflags & REG_NOTBOL)
>                 flags |= PCRE_NOTBOL;
>
> +#ifdef PCRE_CONFIG_JIT
> +       if (p->pcre1_jit_on)
> +               ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> +                                   eol - line, 0, flags, ovector,
> +                                   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> +       else
> +#endif
> +       /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
>         ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>                         0, flags, ovector, ARRAY_SIZE(ovector));
> +
>         if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>                 die("pcre_exec failed with error code %d", ret);
>         if (ret > 0) {

If we MUST have this #ifdef/#endif in-line in this function, then tolerating funny indentation in the else clause I think is an accepted common practice that may not need an extra comment.

But I wonder if the resulting code of this function may become simpler to follow if we remove #ifdef/#endif from it, and instead have this function call a helper (which may itself have #ifdef, or maybe #ifdef/#else/#endif may have two implementations)?

Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 6/7] grep: un-break building with PCRE < 8.20

Amend my change earlier in this series ("grep: add support for the PCRE v1 JIT API", 2017-04-11) to un-break the build on PCRE v1 versions earlier than 8.20.

The 8.20 release was the first release to have JIT & pcre_jit_stack in the headers, so a mock type needs to be provided for it on those releases.

Now git should compile with all PCRE versions that it supported before my JIT change.

I've tested it as far back as version 7.5 released on 2008-01-10, once I got down to version 7.0 it wouldn't build anymore with GCC 7.1.1, and I couldn't be bothered to anything older than 7.5 as I'm confident that if the build breaks on those older versions it's not because of my JIT change.

See the "un-break" change in this series ("grep: un-break building with PCRE < 8.32", 2017-05-10) for why this isn't squashed into the main PCRE JIT commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 3 +++
 1 file changed, 3 insertions(+)
Show changes to grep.h +3 −0
diff --git a/grep.h b/grep.h
index 73ef0ef8ec..b7b9d487b0 100644
--- a/grep.h
+++ b/grep.h
@@ -11,6 +11,9 @@
 #ifndef PCRE_STUDY_JIT_COMPILE
 #define PCRE_STUDY_JIT_COMPILE 0
 #endif
+#if PCRE_MAJOR <= 8 && PCRE_MINOR < 20
+typedef int pcre_jit_stack;
+#endif
 #else
 typedef int pcre;
 typedef int pcre_extra;
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 5/7] grep: un-break building with PCRE < 8.32

Amend my change earlier in this series ("grep: add support for the PCRE v1 JIT API", 2017-04-11) to un-break the build on PCRE v1 versions earlier than 8.32.

The JIT support was added in version 8.20 released on 2011-10-21, but it wasn't until 8.32 released on 2012-11-30 that the fast code path to use the JIT via pcre_jit_exec() was added[1] (see also [2]).

This means that versions 8.20 through 8.31 could still use the JIT, but supporting it on those versions would add to the already verbose macro soup around JIT support it, and I don't expect that the use-case of compiling a brand new git against a 5 year old PCRE is particularly common, and if someone does that they can just get the existing pre-JIT slow codepath.

So just take the easy way out and disable the JIT on any version older than 8.32.

The reason this change isn't part of the initial change PCRE JIT support is because possibly slightly annoying someone who's bisecting with an ancient PCRE is worth it to have a cleaner history showing which parts of the implementation are only used for ancient PCRE versions. This also makes it easier to revert this change if we ever decide to stop supporting those old versions.

1. http://www.pcre.org/original/changelog.txt ("28. Introducing a
   native interface for JIT. Through this interface, the
   compiled[...]")
2. https://bugs.exim.org/show_bug.cgi?id=2121
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 8 ++++----
 grep.h | 5 +++++
 2 files changed, 9 insertions(+), 4 deletions(-)
Show changes to 2 files +9 −4

grep.c, grep.h

diff --git a/grep.c b/grep.c
index accf1c45e6..81337638ca 100644
--- a/grep.c
+++ b/grep.c
@@ -351,7 +351,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 	const char *error;
 	int erroffset;
 	int options = PCRE_MULTILINE;
-#ifdef PCRE_CONFIG_JIT
+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT
 	int canjit;
 #endif
 
@@ -372,7 +372,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 	if (!p->pcre1_extra_info && error)
 		die("%s", error);
 
-#ifdef PCRE_CONFIG_JIT
+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT
 	pcre_config(PCRE_CONFIG_JIT, &canjit);
 	if (canjit == 1) {
 		p->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);
@@ -392,7 +392,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
 	if (eflags & REG_NOTBOL)
 		flags |= PCRE_NOTBOL;
 
-#ifdef PCRE_CONFIG_JIT
+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT
 	if (p->pcre1_jit_on)
 		ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
 				    eol - line, 0, flags, ovector,
@@ -420,7 +420,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,
 static void free_pcre1_regexp(struct grep_pat *p)
 {
 	pcre_free(p->pcre1_regexp);
-#ifdef PCRE_CONFIG_JIT
+#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT
 	if (p->pcre1_jit_on) {
 		pcre_free_study(p->pcre1_extra_info);
 		pcre_jit_stack_free(p->pcre1_jit_stack);
diff --git a/grep.h b/grep.h
index 14f47189f9..73ef0ef8ec 100644
--- a/grep.h
+++ b/grep.h
@@ -3,6 +3,11 @@
 #include "color.h"
 #ifdef USE_LIBPCRE1
 #include <pcre.h>
+#ifdef PCRE_CONFIG_JIT
+#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
+#define GIT_PCRE1_CAN_DO_MODERN_JIT
+#endif
+#endif
 #ifndef PCRE_STUDY_JIT_COMPILE
 #define PCRE_STUDY_JIT_COMPILE 0
 #endif
-- 
2.11.0
Ævar Arnfjörð Bjarmason· May 13, 2017, 23:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 7/7] grep: add support for PCRE v2

Add support for v2 of the PCRE API. This is a new major version of PCRE that came out in early 2015[1].

The regular expression syntax is the same, but while the API is similar, pretty much every function is either renamed or takes different arguments. Thus using it via entirely new functions makes sense, as opposed to trying to e.g. have one compile_pcre_pattern() that would call either PCRE v1 or v2 functions.

Git can now be compiled with either USE_LIBPCRE1=YesPlease or USE_LIBPCRE2=YesPlease, with USE_LIBPCRE=YesPlease currently being a synonym for the former. Providing both is a compile-time error.

With earlier patches to enable JIT for PCRE v1 the performance of the release versions of both libraries is almost exactly the same, with PCRE v2 being around 1% slower.

However after I reported this to the pcre-dev mailing list[2] I got a lot of help with the API use from Zoltán Herczeg, he subsequently optimized some of the JIT functionality in v2 of the library.

Running the p7820-grep-engines.sh performance test against the latest Subversion trunk of both, with both them and git compiled as -O3, and the test run against linux.git, gives the following results. Just the /perl/ tests shown:

    $ GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_COMMAND='grep -q LIBPCRE2 Makefile && make -j8 USE_LIBPCRE2=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre2/inst/lib || make -j8 USE_LIBPCRE=YesPlease CC=~/perl5/installed/bin/gcc NO_R_TO_GCC_LINKER=YesPlease CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst LDFLAGS=-Wl,-rpath,/home/avar/g/pcre/inst/lib' ./run HEAD~2 HEAD~ HEAD p7820-grep-engines.sh
    [...]
    Test                                           HEAD~2            HEAD~                    HEAD
    ----------------------------------------------------------------------------------------------------------------
    7820.3: perl grep how.to                      0.19(0.34+0.62)   0.18(0.39+0.57) -5.3%   0.19(0.32+0.61) +0.0%
    7820.7: perl grep ^how to                     0.21(0.68+0.51)   0.21(0.64+0.54) +0.0%   0.19(0.32+0.60) -9.5%
    7820.11: perl grep [how] to                   0.25(0.92+0.51)   0.26(0.93+0.49) +4.0%   0.21(0.39+0.62) -16.0%
    7820.15: perl grep (e.t[^ ]*|v.ry) rare       0.26(1.18+0.42)   0.26(1.14+0.45) +0.0%   0.20(0.49+0.57) -23.1%
    7820.19: perl grep m(ú|u)lt.b(æ|y)te          0.24(0.85+0.48)   0.23(0.92+0.41) -4.2%   0.19(0.36+0.56) -20.8%

See commit ("perf: add a performance comparison test of grep -G, -E and -P", 2017-04-19) for further details on the machine the above test run was executed on.

Here HEAD~2 is git with PCRE v1 without JIT, HEAD~ is PCRE v1 with JIT, and HEAD is PCRE v2 (also with JIT). See previous commits of mine mentioning p7820-grep-engines.sh for more details on the test setup.

For ease of readability, a different run just of HEAD~ (PCRE v1 with JIT v.s. PCRE v2), again with just the /perl/ tests shown:

    Test                                           HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep how.to                      0.19(0.40+0.56)   0.19(0.34+0.59) +0.0%
    7820.7: perl grep ^how to                     0.21(0.64+0.54)   0.19(0.30+0.63) -9.5%
    7820.11: perl grep [how] to                   0.25(0.94+0.48)   0.21(0.38+0.62) -16.0%
    7820.15: perl grep (e.t[^ ]*|v.ry) rare       0.26(1.13+0.46)   0.20(0.48+0.58) -23.1%
    7820.19: perl grep m(ú|u)lt.b(æ|y)te          0.23(0.84+0.50)   0.18(0.29+0.63) -21.7%

I.e. the two are either neck-to-neck, but PCRE v2 usually pulls ahead, when it does it's around 20% faster.

A brief note on thread safety: As noted in pcre2api(3) & pcre2jit(3) the compiled pattern can be shared between threads, but not some of the JIT context, however the grep threading support does all pattern & JIT compilation in separate threads, so this code doesn't need to concern itself with thread safety.

See commit 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) for the initial addition of PCRE v1. This change follows some of the same patterns it did (and which were discussed on list at the time), e.g. mocking up types with typedef instead of ifdef-ing them out when USE_LIBPCRE2 isn't defined. This adds some trivial memory use to the program, but makes the code look nicer.

1. https://lists.exim.org/lurker/message/20150105.162835.0666407a.en.html
2. https://lists.exim.org/lurker/thread/20170419.172322.833ee099.en.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile      |  30 +++++++++---
 configure.ac  |  77 ++++++++++++++++++++++++++-----
 grep.c        | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 grep.h        |  17 +++++++
 t/test-lib.sh |   2 +-
 5 files changed, 250 insertions(+), 19 deletions(-)
Show changes to 5 files +250 −19

Makefile, configure.ac, grep.c, grep.h, t/test-lib.sh

diff --git a/Makefile b/Makefile
index a79274e5e6..d77ca4c1a5 100644
--- a/Makefile
+++ b/Makefile
@@ -29,7 +29,12 @@ all::
 # Perl-compatible regular expressions instead of standard or extended
 # POSIX regular expressions.
 #
-# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in
+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define
+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE
+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by
+# default in future releases.
+#
+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
 # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
@@ -1087,15 +1092,27 @@ ifdef NO_LIBGEN_H
 	COMPAT_OBJS += compat/basename.o
 endif
 
-ifdef USE_LIBPCRE
-	BASIC_CFLAGS += -DUSE_LIBPCRE1
-	ifdef LIBPCREDIR
-		BASIC_CFLAGS += -I$(LIBPCREDIR)/include
-		EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
+USE_LIBPCRE1 ?= $(USE_LIBPCRE)
+
+ifneq (,$(USE_LIBPCRE1))
+	ifdef USE_LIBPCRE2
+$(error Only set USE_LIBPCRE1 (or its alias USE_LIBPCRE) or USE_LIBPCRE2, not both!)
 	endif
+
+	BASIC_CFLAGS += -DUSE_LIBPCRE1
 	EXTLIBS += -lpcre
 endif
 
+ifdef USE_LIBPCRE2
+	BASIC_CFLAGS += -DUSE_LIBPCRE2
+	EXTLIBS += -lpcre2-8
+endif
+
+ifdef LIBPCREDIR
+	BASIC_CFLAGS += -I$(LIBPCREDIR)/include
+	EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
+endif
+
 ifdef HAVE_ALLOCA_H
 	BASIC_CFLAGS += -DHAVE_ALLOCA_H
 endif
@@ -2241,6 +2258,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@+
 	@echo NO_EXPAT=\''$(subst ','\'',$(subst ','\'',$(NO_EXPAT)))'\' >>$@+
 	@echo USE_LIBPCRE1=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@+
+	@echo USE_LIBPCRE2=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE2)))'\' >>$@+
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
 	@echo NO_PTHREADS=\''$(subst ','\'',$(subst ','\'',$(NO_PTHREADS)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
diff --git a/configure.ac b/configure.ac
index deeb968daa..11d083fbe0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -255,21 +255,61 @@ GIT_PARSE_WITH([openssl]))
 # Perl-compatible regular expressions instead of standard or extended
 # POSIX regular expressions.
 #
-# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in
+# Currently USE_LIBPCRE is a synonym for USE_LIBPCRE1, define
+# USE_LIBPCRE2 instead if you'd like to use version 2 of the PCRE
+# library. The USE_LIBPCRE flag will likely be changed to mean v2 by
+# default in future releases.
+#
+# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
 AC_ARG_WITH(libpcre,
-AS_HELP_STRING([--with-libpcre],[support Perl-compatible regexes (default is NO)])
+AS_HELP_STRING([--with-libpcre],[synonym for --with-libpcre1]),
+    if test "$withval" = "no"; then
+	USE_LIBPCRE1=
+    elif test "$withval" = "yes"; then
+	USE_LIBPCRE1=YesPlease
+    else
+	USE_LIBPCRE1=YesPlease
+	LIBPCREDIR=$withval
+	AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])
+        dnl USE_LIBPCRE1 can still be modified below, so don't substitute
+        dnl it yet.
+	GIT_CONF_SUBST([LIBPCREDIR])
+    fi)
+
+AC_ARG_WITH(libpcre1,
+AS_HELP_STRING([--with-libpcre1],[support Perl-compatible regexes via libpcre1 (default is NO)])
+AS_HELP_STRING([],           [ARG can be also prefix for libpcre library and headers]),
+    if test "$withval" = "no"; then
+	USE_LIBPCRE1=
+    elif test "$withval" = "yes"; then
+	USE_LIBPCRE1=YesPlease
+    else
+	USE_LIBPCRE1=YesPlease
+	LIBPCREDIR=$withval
+	AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])
+        dnl USE_LIBPCRE1 can still be modified below, so don't substitute
+        dnl it yet.
+	GIT_CONF_SUBST([LIBPCREDIR])
+    fi)
+
+AC_ARG_WITH(libpcre2,
+AS_HELP_STRING([--with-libpcre2],[support Perl-compatible regexes via libpcre2 (default is NO)])
 AS_HELP_STRING([],           [ARG can be also prefix for libpcre library and headers]),
+    if test -n "$USE_LIBPCRE1"; then
+        AC_MSG_ERROR([Only supply one of --with-libpcre1 or --with-libpcre2!])
+    fi
+
     if test "$withval" = "no"; then
-	USE_LIBPCRE=
+	USE_LIBPCRE2=
     elif test "$withval" = "yes"; then
-	USE_LIBPCRE=YesPlease
+	USE_LIBPCRE2=YesPlease
     else
-	USE_LIBPCRE=YesPlease
+	USE_LIBPCRE2=YesPlease
 	LIBPCREDIR=$withval
 	AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])
-        dnl USE_LIBPCRE can still be modified below, so don't substitute
+        dnl USE_LIBPCRE2 can still be modified below, so don't substitute
         dnl it yet.
 	GIT_CONF_SUBST([LIBPCREDIR])
     fi)
@@ -501,13 +541,11 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO])
 GIT_CONF_SUBST([NO_OPENSSL])
 
 #
-# Define USE_LIBPCRE if you have and want to use libpcre. Various
-# commands such as log and grep offer runtime options to use
-# Perl-compatible regular expressions instead of standard or extended
-# POSIX regular expressions.
+# Handle the USE_LIBPCRE1 and USE_LIBPCRE2 options potentially set
+# above.
 #
 
-if test -n "$USE_LIBPCRE"; then
+if test -n "$USE_LIBPCRE1"; then
 
 GIT_STASH_FLAGS($LIBPCREDIR)
 
@@ -517,7 +555,22 @@ AC_CHECK_LIB([pcre], [pcre_version],
 
 GIT_UNSTASH_FLAGS($LIBPCREDIR)
 
-GIT_CONF_SUBST([USE_LIBPCRE])
+GIT_CONF_SUBST([USE_LIBPCRE1])
+
+fi
+
+
+if test -n "$USE_LIBPCRE2"; then
+
+GIT_STASH_FLAGS($LIBPCREDIR)
+
+AC_CHECK_LIB([pcre2-8], [pcre2_config_8],
+[USE_LIBPCRE2=YesPlease],
+[USE_LIBPCRE2=])
+
+GIT_UNSTASH_FLAGS($LIBPCREDIR)
+
+GIT_CONF_SUBST([USE_LIBPCRE2])
 
 fi
 
diff --git a/grep.c b/grep.c
index 81337638ca..dfb49a6c59 100644
--- a/grep.c
+++ b/grep.c
@@ -179,22 +179,36 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
 	case GREP_PATTERN_TYPE_BRE:
 		opt->fixed = 0;
 		opt->pcre1 = 0;
+		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_ERE:
 		opt->fixed = 0;
 		opt->pcre1 = 0;
+		opt->pcre2 = 0;
 		opt->regflags |= REG_EXTENDED;
 		break;
 
 	case GREP_PATTERN_TYPE_FIXED:
 		opt->fixed = 1;
 		opt->pcre1 = 0;
+		opt->pcre2 = 0;
 		break;
 
 	case GREP_PATTERN_TYPE_PCRE:
 		opt->fixed = 0;
+#ifdef USE_LIBPCRE2
+		opt->pcre1 = 0;
+		opt->pcre2 = 1;
+#else
+		/* It's important that pcre1 always be assigned to
+		 * even when there's no USE_LIBPCRE* defined. We still
+		 * call the PCRE stub function, it just dies with
+		 * "cannot use Perl-compatible regexes[...]".
+		 */
 		opt->pcre1 = 1;
+		opt->pcre2 = 0;
+#endif
 		break;
 	}
 }
@@ -449,6 +463,126 @@ static void free_pcre1_regexp(struct grep_pat *p)
 }
 #endif /* !USE_LIBPCRE1 */
 
+#ifdef USE_LIBPCRE2
+static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
+{
+	int error;
+	PCRE2_UCHAR errbuf[256];
+	PCRE2_SIZE erroffset;
+	int options = PCRE2_MULTILINE;
+	const uint8_t *character_tables = NULL;
+	uint32_t canjit;
+	int jitret;
+
+	assert(opt->pcre2);
+
+	p->pcre2_compile_context = NULL;
+
+	if (opt->ignore_case) {
+		if (has_non_ascii(p->pattern)) {
+			character_tables = pcre2_maketables(NULL);
+			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+		}
+		options |= PCRE2_CASELESS;
+	}
+	if (is_utf8_locale() && has_non_ascii(p->pattern))
+		options |= PCRE2_UTF;
+
+	p->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,
+					 p->patternlen, options, &error, &erroffset,
+					 p->pcre2_compile_context);
+
+	if (p->pcre2_pattern) {
+		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		if (!p->pcre2_match_data)
+			die("BUG: Couldn't allocate PCRE2 match data");
+	} else {
+		pcre2_get_error_message(error, errbuf, sizeof(errbuf));
+		compile_regexp_failed(p, (const char *)&errbuf);
+	}
+
+	pcre2_config(PCRE2_CONFIG_JIT, &canjit);
+	if (canjit == 1) {
+		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
+		if (!jitret)
+			p->pcre2_jit_on = 1;
+		else
+			die("BUG: Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		if (!p->pcre2_jit_stack)
+			die("BUG: Couldn't allocate PCRE2 JIT stack");
+		p->pcre2_match_context = pcre2_match_context_create(NULL);
+		if (!p->pcre2_jit_stack)
+			die("BUG: Couldn't allocate PCRE2 match context");
+		pcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);
+	}
+}
+
+static int pcre2match(struct grep_pat *p, const char *line, const char *eol,
+		regmatch_t *match, int eflags)
+{
+	int ret, flags = 0;
+	PCRE2_SIZE *ovector;
+	PCRE2_UCHAR errbuf[256];
+
+	if (eflags & REG_NOTBOL)
+		flags |= PCRE2_NOTBOL;
+
+	if (p->pcre2_jit_on)
+		ret = pcre2_jit_match(p->pcre2_pattern, (unsigned char *)line,
+				      eol - line, 0, flags, p->pcre2_match_data,
+				      NULL);
+	else
+		ret = pcre2_match(p->pcre2_pattern, (unsigned char *)line,
+				  eol - line, 0, flags, p->pcre2_match_data,
+				  NULL);
+
+	if (ret < 0 && ret != PCRE2_ERROR_NOMATCH) {
+		pcre2_get_error_message(ret, errbuf, sizeof(errbuf));
+		die("%s failed with error code %d: %s",
+		    (p->pcre2_jit_on ? "pcre2_jit_match" : "pcre2_match"), ret,
+		    errbuf);
+	}
+	if (ret > 0) {
+		ovector = pcre2_get_ovector_pointer(p->pcre2_match_data);
+		ret = 0;
+		match->rm_so = (int)ovector[0];
+		match->rm_eo = (int)ovector[1];
+	}
+
+	return ret;
+}
+
+static void free_pcre2_pattern(struct grep_pat *p)
+{
+	pcre2_compile_context_free(p->pcre2_compile_context);
+	pcre2_code_free(p->pcre2_pattern);
+	pcre2_match_data_free(p->pcre2_match_data);
+	pcre2_jit_stack_free(p->pcre2_jit_stack);
+	pcre2_match_context_free(p->pcre2_match_context);
+}
+#else /* !USE_LIBPCRE2 */
+static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
+{
+	/* Unreachable until USE_LIBPCRE2 becomes synonymous with
+	 * USE_LIBPCRE. See the sibling comment in
+	 * grep_set_pattern_type_option().
+	 */
+	die("cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE");
+}
+
+static int pcre2match(struct grep_pat *p, const char *line, const char *eol,
+		regmatch_t *match, int eflags)
+{
+	return 1;
+}
+
+static void free_pcre2_pattern(struct grep_pat *p)
+{
+}
+#endif /* !USE_LIBPCRE2 */
+
 static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -512,6 +646,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 		return;
 	}
 
+	if (opt->pcre2) {
+		compile_pcre2_pattern(p, opt);
+		return;
+	}
+
 	if (opt->pcre1) {
 		compile_pcre1_regexp(p, opt);
 		return;
@@ -871,6 +1010,8 @@ void free_grep_patterns(struct grep_opt *opt)
 				kwsfree(p->kws);
 			else if (p->pcre1_regexp)
 				free_pcre1_regexp(p);
+			else if (p->pcre2_pattern)
+				free_pcre2_pattern(p);
 			else
 				regfree(&p->regexp);
 			free(p->pattern);
@@ -951,6 +1092,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,
 		hit = !fixmatch(p, line, eol, match);
 	else if (p->pcre1_regexp)
 		hit = !pcre1match(p, line, eol, match, eflags);
+	else if (p->pcre2_pattern)
+		hit = !pcre2match(p, line, eol, match, eflags);
 	else
 		hit = !regexec_buf(&p->regexp, line, eol - line, 1, match,
 				   eflags);
diff --git a/grep.h b/grep.h
index b7b9d487b0..b40afc2e2f 100644
--- a/grep.h
+++ b/grep.h
@@ -19,6 +19,16 @@ typedef int pcre;
 typedef int pcre_extra;
 typedef int pcre_jit_stack;
 #endif
+#ifdef USE_LIBPCRE2
+#define PCRE2_CODE_UNIT_WIDTH 8
+#include <pcre2.h>
+#else
+typedef int pcre2_code;
+typedef int pcre2_match_data;
+typedef int pcre2_compile_context;
+typedef int pcre2_match_context;
+typedef int pcre2_jit_stack;
+#endif
 #include "kwset.h"
 #include "thread-utils.h"
 #include "userdiff.h"
@@ -63,6 +73,12 @@ struct grep_pat {
 	pcre_jit_stack *pcre1_jit_stack;
 	const unsigned char *pcre1_tables;
 	int pcre1_jit_on;
+	pcre2_code *pcre2_pattern;
+	pcre2_match_data *pcre2_match_data;
+	pcre2_compile_context *pcre2_compile_context;
+	pcre2_match_context *pcre2_match_context;
+	pcre2_jit_stack *pcre2_jit_stack;
+	int pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
 	unsigned ignore_case:1;
@@ -126,6 +142,7 @@ struct grep_opt {
 	int extended;
 	int use_reflog_filter;
 	int pcre1;
+	int pcre2;
 	int relative;
 	int pathname;
 	int null_following_name;
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ab92c0ebaa..44d4679384 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1011,7 +1011,7 @@ esac
 test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
-test -n "$USE_LIBPCRE1" && test_set_prereq PCRE
+test -n "$USE_LIBPCRE1$USE_LIBPCRE2" && test_set_prereq PCRE
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 
 # Can we rely on git's output in the C locale?
-- 
2.11.0

← back to recent threads