| Message ID | 20190724151415.3698-3-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: use custom JIT stack with pcre2 | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Since we've haven't had any reports of users running into > PCRE2_ERROR_JIT_STACKLIMIT in the wild I think we can safely assume > that we can just use the library defaults instead and drop this > code. Does everybody use pcre2 with JIT with Git these days, or only those who want to live near the bleeding edge? > This won't change with the wider use of PCRE v2 in > ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15), a > fixed string search is not a "large or complicated pattern". In any case, if we were not "using" the custom stack anyway for v2, this change does not hurt anybody, possibly other than those who will learn about pcre2 support by reading this message and experiments with larger patterns. And it should be simple to wire it back if it becomes necessary later. Thanks for cleaning up.
On Wed, Jul 24 2019, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Since we've haven't had any reports of users running into >> PCRE2_ERROR_JIT_STACKLIMIT in the wild I think we can safely assume >> that we can just use the library defaults instead and drop this >> code. > > Does everybody use pcre2 with JIT with Git these days, or only those > who want to live near the bleeding edge? My informal survey of various package recipies suggests that all the big *nix distros are using it by default now, so we have a lot of users in the wild, including in the just-released Debian stable. So I'm confidend that if there were issues with e.g. it dying on patterns in practical use we'd have heard about them. >> This won't change with the wider use of PCRE v2 in >> ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15), a >> fixed string search is not a "large or complicated pattern". > > In any case, if we were not "using" the custom stack anyway for v2, > this change does not hurt anybody, possibly other than those who > will learn about pcre2 support by reading this message and experiments > with larger patterns. And it should be simple to wire it back if it > becomes necessary later. *nod*
On Wed, Jul 24, 2019 at 1:20 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Wed, Jul 24 2019, Junio C Hamano wrote: > > > > Does everybody use pcre2 with JIT with Git these days, or only those > > who want to live near the bleeding edge? > > My informal survey of various package recipies suggests that all the big > *nix distros are using it by default now, so we have a lot of users in > the wild, including in the just-released Debian stable. FWIW neither OpenBSD or NetBSD enable JIT, and the git that comes with Xcode (AKA Apple Git) doesn't either, while still using PCRE1 > > In any case, if we were not "using" the custom stack anyway for v2, > > this change does not hurt anybody, possibly other than those who > > will learn about pcre2 support by reading this message and experiments > > with larger patterns. And it should be simple to wire it back if it > > becomes necessary later. > > *nod* the following pattern fails unless 1MB stack is available: '^([/](?!/)|[^/])*~/.*' the workaround implemented in GNU grep (that uses PCRE1) and the related discussion[1] are a very interesting read Carlo [1] https://www.mail-archive.com/bug-grep@gnu.org/msg05763.html
diff --git a/grep.c b/grep.c index be4282fef3..20ce95270a 100644 --- a/grep.c +++ b/grep.c @@ -546,14 +546,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt p->pcre2_jit_on = 0; return; } - - p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL); - if (!p->pcre2_jit_stack) - die("Couldn't allocate PCRE2 JIT stack"); - p->pcre2_match_context = pcre2_match_context_create(NULL); - if (!p->pcre2_match_context) - die("Couldn't allocate PCRE2 match context"); - pcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack); } } @@ -597,8 +589,6 @@ 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) diff --git a/grep.h b/grep.h index 1875880f37..a65f4a1ae1 100644 --- a/grep.h +++ b/grep.h @@ -29,8 +29,6 @@ typedef int pcre_jit_stack; 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" @@ -94,8 +92,6 @@ struct grep_pat { 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; uint32_t pcre2_jit_on; kwset_t kws; unsigned fixed:1;
As reported in [1] the code I added in 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) to use a custom JIT stack has never worked. It was incorrectly copy/pasted from code I added in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25), which did work. Thus our intention of starting with 1 byte of stack at a maximum of 1 MB didn't happen, we'd always use the 32 KB stack provided by PCRE v2's jit_machine_stack_exec()[2]. The reason I allocated a custom stack at all was this advice in pcrejit(3) (same in pcre2jit(3)): "By default, it uses 32KiB on the machine stack. However, some large or complicated patterns need more than this" Since we've haven't had any reports of users running into PCRE2_ERROR_JIT_STACKLIMIT in the wild I think we can safely assume that we can just use the library defaults instead and drop this code. This won't change with the wider use of PCRE v2 in ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15), a fixed string search is not a "large or complicated pattern". For good measure I ran the performance test noted in 94da9193a6, although the command is simpler now due to my 0f50c8e32c ("Makefile: remove the NO_R_TO_GCC_LINKER flag", 2019-05-17): GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE2=Y CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst' ./run HEAD~ HEAD p7820-grep-engines.sh Just the /perl/ results are: Test HEAD~ HEAD --------------------------------------------------------------------------------------- 7820.3: perl grep 'how.to' 0.17(0.27+0.65) 0.17(0.24+0.68) +0.0% 7820.7: perl grep '^how to' 0.16(0.23+0.66) 0.16(0.23+0.67) +0.0% 7820.11: perl grep '[how] to' 0.18(0.35+0.62) 0.18(0.33+0.65) +0.0% 7820.15: perl grep '(e.t[^ ]*|v.ry) rare' 0.17(0.45+0.54) 0.17(0.49+0.50) +0.0% 7820.19: perl grep 'm(ú|u)lt.b(æ|y)te' 0.16(0.33+0.58) 0.16(0.29+0.62) +0.0% So, as expected there's no change, and running with valgrind reveals that we have fewer allocations now. 1. https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/ 2. I didn't really intend to start with 1 byte, looking at the PCRE v2 code again what happened is that I cargo-culted some of PCRE v2's own test code which was meant to test re-allocations. It's more sane to start with say 32 KB with a max of 1 MB, as pcre2grep.c does. Reported-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- grep.c | 10 ---------- grep.h | 4 ---- 2 files changed, 14 deletions(-)