{"thread":{"id":"45573","subject":"Very promising results with libpcre2","startedAt":"2017-03-31T21:24:02Z","lastAt":"2017-04-02T03:45:51Z","messageCount":8,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeffrey Walton"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"315956","messageId":"CACBZZX6FcHcY7cYs6s_pv=E43cHNmzyUY5wrcuhPWWmUixCL+g@mail.gmail.com","threadId":"45573","inReplyTo":null,"subject":"Very promising results with libpcre2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-31T21:23:34Z","receivedAt":"2017-03-31T21:24:02Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The recent libpcre2 got me interested in seeing what the difference in\nv1 and v2 was.\n\nSo I hacked up a *very basic* patch for libpcre2 that passes all\ntests, but obviously isn't ready for inclusion (I searched/replaced\nall the v1 usage with v2). I'm not even bothering sending this to the\nlist since discussing the patch itself isn't the point:\n\nhttps://github.com/avar/git/commit/414647d88dd9c5\n\nBefore that patch, running a test[1] on linux.git where I grep the\nwhole tree for a fixed string / simple regex with POSIX regexes & PCRE\n(all the greps match the same few lines) gives:\n\n s/iter rx prx fixed\nrx 2.20 -- -2% -34%\nprx 2.17 2% -- -32%\nfixed 1.46 51% 48% --\n\nI.e. fixed string is fastest, and both POSIX regcomp() & pcre v1 are\n~30% slower than that, with no difference in performance between the\ntwo. Now with my patch above with pcre v2 there's a notable\nperformance difference:\n\n s/iter rx prx fixed\nrx 2.18 -- -16% -33%\nprx 1.84 19% -- -20%\nfixed 1.47 48% 25% --\n\nWe've gone from ~30% slower to ~20% slower for PCRE with v2. But now\nlet's test that with this patch:\n\nhttps://github.com/avar/git/commit/4b7e5da3606c0b9b12025437de8005f5fa07ff54\n\nThat enables the new JIT support in pcre v2:\n\n s/iter rx fixed prx\nrx 2.19 -- -33% -44%\nfixed 1.47 49% -- -17%\nprx 1.22 79% 20% --\n\nNow it's PCRE that's 20% faster than our currently fastest grep\ncodepath that searches for a fixed string, and in absolute terms it's\naround 50% faster than the current PCRE implementation.\n\nThis is on Debian testing with both PCRE libraries installed via\npackages, 8.35 & 10.22 for v1 and v2, respectively. Both are the\nsecond-latest[2] point releases for their respective versions.\n\nAs far as turning this into a patch goes there's a few open questions:\n\n* PCRE itself supports linking to v1 and v2 in the same program just\nfine. Should we provide the possibility to link to both, or just make\nthe user choose? If these performance numbers hold up preferring v2 is\ndefinitely better.\n\n* The JIT is supposedly a bit slower if you're not doing a lot of\nmatching, although I doubt this matters in practice, but whether to\nuse it & a few other options could be controlled by some config/CLI\noption. I think it probably makes sense just to always use it if it's\nthere pending some cases where it makes performance worse in practice\n\nAs an aside I started looking into this because I'm interested in\neventually hacking up something that makes every user-facing\nregcomp()/regexec() we have now (e.g. log -G) accept PCRE as well.\n\nHow do do this in all cases isn't very obvious, we could just have\nsome global config option, but there's lots of stuff like e.g.\n\"^{/} & \"git show :/\" that takes regexes, and e.g.\nhow to pass a /i flag to some of these isn't obvious at all.\n\nThe solution I'm leaning towards is to just make stuff like thath only\nwork under PCRE, via the native (?:) facility. E.g.\nthis now works:\n\n git grep -P '(?xi: h e l l o)'\n\n1. PF=~/g/git/ perl -MBenchmark=cmpthese -wE 'cmpthese(20, { fixed =>\nsub { system \"$ENV{PF}git grep -F avarasu >/dev/null\" }, rx => sub {\nsystem \"$ENV{PF}git grep avara?su >/dev/null\" }, prx => sub { system\n\"$ENV{PF}git grep -P avara?su >/dev/null\" } })'\n2. https://ftp.pcre.org/pub/pcre/\n"},{"id":"315957","messageId":"xmqqtw69vz9t.fsf@gitster.mtv.corp.google.com","threadId":"45573","inReplyTo":"CACBZZX6FcHcY7cYs6s_pv=E43cHNmzyUY5wrcuhPWWmUixCL+g@mail.gmail.com","subject":"Re: Very promising results with libpcre2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-31T22:48:14Z","receivedAt":"2017-03-31T22:48:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> That enables the new JIT support in pcre v2:\n>\n> s/iter rx fixed prx\n> rx 2.19 -- -33% -44%\n> fixed 1.47 49% -- -17%\n> prx 1.22 79% 20% --\n\nThe numbers with JIT does look \"interesting\". \n\nI couldn't quite tell if there are major incompatibilities in the\nregex language itself between two versions from their documentation,\nbut assuming that there isn't (modulo bugfixes and enhancements) and\nassuming that we are going to use their standard matcher, it may be\nOK to just use the newer one without linking both.\n\n"},{"id":"316002","messageId":"CACBZZX416M3YFDAACGAWCjJ4Xt0g78zpFF+iusMCarKb7SSfVw@mail.gmail.com","threadId":"45573","inReplyTo":"xmqqtw69vz9t.fsf@gitster.mtv.corp.google.com","subject":"Re: Very promising results with libpcre2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-01T08:55:11Z","receivedAt":"2017-04-01T08:55:38Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Apr 1, 2017 at 12:48 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> That enables the new JIT support in pcre v2:\n>>\n>> s/iter rx fixed prx\n>> rx 2.19 -- -33% -44%\n>> fixed 1.47 49% -- -17%\n>> prx 1.22 79% 20% --\n>\n> The numbers with JIT does look \"interesting\".\n>\n> I couldn't quite tell if there are major incompatibilities in the\n> regex language itself between two versions from their documentation,\n> but assuming that there isn't (modulo bugfixes and enhancements) and\n> assuming that we are going to use their standard matcher, it may be\n> OK to just use the newer one without linking both.\n\nThere's no incompatibilities in the regex language itself (modulo bugs\netc). So yeah, I'll prepare some patch to use v2.\n"},{"id":"316018","messageId":"xmqqbmsgugt4.fsf@gitster.mtv.corp.google.com","threadId":"45573","inReplyTo":"CACBZZX416M3YFDAACGAWCjJ4Xt0g78zpFF+iusMCarKb7SSfVw@mail.gmail.com","subject":"Re: Very promising results with libpcre2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T18:24:39Z","receivedAt":"2017-04-01T18:24:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Sat, Apr 1, 2017 at 12:48 AM, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> That enables the new JIT support in pcre v2:\n>>>\n>>> s/iter rx fixed prx\n>>> rx 2.19 -- -33% -44%\n>>> fixed 1.47 49% -- -17%\n>>> prx 1.22 79% 20% --\n>>\n>> The numbers with JIT does look \"interesting\".\n>>\n>> I couldn't quite tell if there are major incompatibilities in the\n>> regex language itself between two versions from their documentation,\n>> but assuming that there isn't (modulo bugfixes and enhancements) and\n>> assuming that we are going to use their standard matcher, it may be\n>> OK to just use the newer one without linking both.\n>\n> There's no incompatibilities in the regex language itself (modulo bugs\n> etc). So yeah, I'll prepare some patch to use v2.\n\nJust to make sure that we are on the same page. While I do not see\nthe need to link with both variants and allow users to choose\nbetween them at runtime, I do not know if the whole world is ready\nto drop pcre1 and use pcre2 (the latter of which has only been\naround for a bit over two years).\n\nSo we'd probably want to do \n\n (1) keep USE_LIBPCRE and enable v1 when set;\n (2) add USE_LIBPCRE2 and enable v2 when set;\n (3) make sure to error out when both are set.\n\nor something like that. It is tempting to allow us to say\n\n make USE_LIBPCRE=2\n\nbut the existing builds are likely to be depending on \"is it set to\nanything? then use PCRE1\" behaviour, so we unfortunately cannot take\nthat route.\n\nThanks.\n"},{"id":"316023","messageId":"CACBZZX5mEMq2ZRKKc-5HxhauNRemPpN4P+wzmihO1YhZq_vJPg@mail.gmail.com","threadId":"45573","inReplyTo":"xmqqbmsgugt4.fsf@gitster.mtv.corp.google.com","subject":"Re: Very promising results with libpcre2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-01T19:11:03Z","receivedAt":"2017-04-01T19:11:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Apr 1, 2017 at 8:24 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Sat, Apr 1, 2017 at 12:48 AM, Junio C Hamano wrote:\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>>> That enables the new JIT support in pcre v2:\n>>>>\n>>>> s/iter rx fixed prx\n>>>> rx 2.19 -- -33% -44%\n>>>> fixed 1.47 49% -- -17%\n>>>> prx 1.22 79% 20% --\n>>>\n>>> The numbers with JIT does look \"interesting\".\n>>>\n>>> I couldn't quite tell if there are major incompatibilities in the\n>>> regex language itself between two versions from their documentation,\n>>> but assuming that there isn't (modulo bugfixes and enhancements) and\n>>> assuming that we are going to use their standard matcher, it may be\n>>> OK to just use the newer one without linking both.\n>>\n>> There's no incompatibilities in the regex language itself (modulo bugs\n>> etc). So yeah, I'll prepare some patch to use v2.\n>\n> Just to make sure that we are on the same page. While I do not see\n> the need to link with both variants and allow users to choose\n> between them at runtime, I do not know if the whole world is ready\n> to drop pcre1 and use pcre2 (the latter of which has only been\n> around for a bit over two years).\n>\n> So we'd probably want to do\n>\n> (1) keep USE_LIBPCRE and enable v1 when set;\n> (2) add USE_LIBPCRE2 and enable v2 when set;\n> (3) make sure to error out when both are set.\n>\n> or something like that. It is tempting to allow us to say\n>\n> make USE_LIBPCRE=2\n>\n> but the existing builds are likely to be depending on \"is it set to\n> anything? then use PCRE1\" behaviour, so we unfortunately cannot take\n> that route.\n\nWe're on the same page, all of this makes sense, there'll be some OS's\n/ environments which for years won't be packaging pcre2 but will have\npcre1.\n\nI am very tempted though to support them in parallel, if only for ease\nof performance testing and to be able to roll out support for\ngrep.patternType=perl meaning pcre1 for now, but add a\ngrep.patternType=pcre2 for testing (and make grep.patternType=pcre1\nwork, with grep.patternType=pcre being synonymous with\ngrep.patternType=perl, i.e. whatever the default is).\n"},{"id":"316031","messageId":"CAH8yC8mxdNFdpo0ZO0+XpnuHGTpp2NCiMCSE71j2Emt8-NRYDg@mail.gmail.com","threadId":"45573","inReplyTo":"xmqqbmsgugt4.fsf@gitster.mtv.corp.google.com","subject":"Re: Very promising results with libpcre2","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2017-04-01T23:33:26Z","receivedAt":"2017-04-01T23:33:34Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"> Just to make sure that we are on the same page. While I do not see\n> the need to link with both variants and allow users to choose\n> between them at runtime, I do not know if the whole world is ready\n> to drop pcre1 and use pcre2 (the latter of which has only been\n> around for a bit over two years).\n>\n> So we'd probably want to do\n>\n> (1) keep USE_LIBPCRE and enable v1 when set;\n> (2) add USE_LIBPCRE2 and enable v2 when set;\n> (3) make sure to error out when both are set.\n>\n> or something like that. It is tempting to allow us to say\n>\n> make USE_LIBPCRE=2\n>\n> but the existing builds are likely to be depending on \"is it set to\n> anything? then use PCRE1\" behaviour, so we unfortunately cannot take\n> that route.\n\nYeah, that's the question I kinda had.\n\n> make USE_LIBPCRE=2\n\nI'd prefer a configure option for consistency. Maybe:\n\n --with-pcre # Original PCRE\n --with-pcre1 # Alias\n --with-pcre2 # PCRE2\n\nI prefer it because I usually do the following to see the interesting\nthings that's going on:\n\n ./configure --help\n\nOften, I find a `--with-ssl` or similar. If `--with-ssl` fails, then I\ngo to the README and INSTALL to fine tune it.\n\nBy the way, if you are tweaking Configure, then consider adding a\n--with-perl=X, too. Its consistent, it side steps the hard coded\n/usr/bin/perl, and it signals to users its tunable.\n\nJeff\n"},{"id":"316034","messageId":"xmqqd1cvtquy.fsf@gitster.mtv.corp.google.com","threadId":"45573","inReplyTo":"CAH8yC8mxdNFdpo0ZO0+XpnuHGTpp2NCiMCSE71j2Emt8-NRYDg@mail.gmail.com","subject":"Re: Very promising results with libpcre2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T03:45:09Z","receivedAt":"2017-04-02T03:45:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeffrey Walton writes:\n\n>> Just to make sure that we are on the same page. While I do not see\n>> the need to link with both variants and allow users to choose\n>> between them at runtime, I do not know if the whole world is ready\n>> to drop pcre1 and use pcre2 (the latter of which has only been\n>> around for a bit over two years).\n>>\n>> So we'd probably want to do\n>>\n>> (1) keep USE_LIBPCRE and enable v1 when set;\n>> (2) add USE_LIBPCRE2 and enable v2 when set;\n>> (3) make sure to error out when both are set.\n>>\n>> or something like that. It is tempting to allow us to say\n>>\n>> make USE_LIBPCRE=2\n>>\n>> but the existing builds are likely to be depending on \"is it set to\n>> anything? then use PCRE1\" behaviour, so we unfortunately cannot take\n>> that route.\n>\n> Yeah, that's the question I kinda had.\n>\n>> make USE_LIBPCRE=2\n>\n> I'd prefer a configure option for consistency. Maybe:\n> ...\n\nIt is way too early to worry about how ./configure support for this\nwill look like to the end user.\n\nBecause our Makefile is designed to be usable without configure, the\norder we do things will be to first decide how we are going to use\nMakefile variables to configure the feature (i.e. what you saw that\nI said to Ævar).\n\nOnce we know the decision, then we arrange autoconf to spit out the\nchosen Makefile variables using --with-pcre or whatever input. This\nstep cannot start before we know the decision of the former. The\none who writes --with-pcre support in ./configure would not know if\nUSE_LIBPCRE=YesPlease or something else is the desired output until\nthen.\n\n"},{"id":"316035","messageId":"xmqq8tnjtqu0.fsf@gitster.mtv.corp.google.com","threadId":"45573","inReplyTo":"CACBZZX5mEMq2ZRKKc-5HxhauNRemPpN4P+wzmihO1YhZq_vJPg@mail.gmail.com","subject":"Re: Very promising results with libpcre2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T03:45:43Z","receivedAt":"2017-04-02T03:45:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I am very tempted though to support them in parallel, if only for ease\n> of performance testing and to be able to roll out support for\n> grep.patternType=perl meaning pcre1 for now, but add a\n> grep.patternType=pcre2 for testing (and make grep.patternType=pcre1\n> work, with grep.patternType=pcre being synonymous with\n> grep.patternType=perl, i.e. whatever the default is).\n\nPerhaps. As long as the code doesn't get too ugly, I think we would\nsurvive ;-)\n\n"}]}