git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: Carlo Arenas <carenas@gmail.com>, Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, avarab@gmail.com,
	sandals@crustytoothpaste.net, dev+git@drbeat.li,
	Johannes.Schindelin@gmx.de
Subject: Re: [RFC PATCH v2] grep: allow for run time disabling of JIT in PCRE
Date: Tue, 30 Jul 2019 19:55:17 +0200
Message-ID: <22b9cf06-ebbb-a0c1-2cc9-7f09d61d3d9b@web.de> (raw)
In-Reply-To: <CAPUEspjo3Uo8KtSYQ=uh+_gPEjb+dBdSPgsEVE1j1bOMPF=0DA@mail.gmail.com>

Am 30.07.19 um 02:49 schrieb Carlo Arenas:
> On Mon, Jul 29, 2019 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>>> +pcre.jit::
>>>> +    If set to false, disable JIT when using PCRE.  Defaults to
>>>> +    true.
>>>> +    if set to -1 will try first to use JIT and fallback to the
>>>> +    interpreter instead of returning an error.
>>>
>>> Why not implement only -1, without adding this config setting?
>>
>> ... nor command line option.  If we have an auto-fallback, I would
>> think that makes the most sense.  IIRC the first iteration with only
>> the configuration was really about working around the (non-working)
>> pcre-jit---if we can self-detect and skip a non-working case, that
>> would allow us to drop end-user facing knobs, which is ideal.
>
> because that was proposed earlier[1] and wasn't accepted ;)

So you add all those knobs for Ævar?

Users on OpenBSD would get an error whenever they used -P?  And they
are expected to discover an option for turning off said error?  That
could be handled automatically?

This sounds like bullying to me.  I simply wouldn't use -P anymore if
that happened to me.  If that error was returned without -P, I'd
contemplate switching to the Silver Searcher or a similar tool.

> the main pushback though I got was that doing the fallback would degrade
> performance and so it was suggested[2] that keeping the error should be
> possible somehow (with the implication it will add yet another macro)

The slowdown by the fallback should be minimal -- just the extra
wasted time to compile the pattern to machine code.  Compilation time is
probably (hopefully?) dwarfed by search time.

IIUC, Ævar's concern was more about being able to discover when JIT is
not available for some reason.  Printing a warning with --debug or
--verbose could help with that.

Personally I don't care about JIT at all and wouldn't want to see such a
warning and certainly prefer not to get any error message about it,
even more so since there is nothing I can do about it.  (Disabling
security features to get faster search sounds sounds like a no-go.)

> since living without grep -P was a reasonable tradeoff at that time got
> punted, but the need to find a solution for this become more urgent once
> it was announced[3] PCRE2 would be used also used outside -P

Right.

> Obviously I am biased, but I kind of like the knob as it allows the user
> more flexibility to tweak the internals of grep and because we had
> made those internals already visible (ex: not handling any library
> errors ourselves and just aborting with a pcre error), but without any
> flexibility to fix those problems themselves (unless they open the code
> and rebuild, in most cases)
>
> the comment from the user that reported[4] a regression with GNU grep
> because of JIT stack size and that I quote below is representative of how
> that layering violation affect users, and while git users are more likely
> than grep users to do the code tweaking needed, they could use some
> help.

So JIT can cause other problems, and some of them we don't (or can't)
handle in our code?  Turning on an unstable feature without a way to
disable it sounds like a bad idea indeed.  Is it really that bad?  Can
we grow the stack on demand, for example, as GNU grep does now in
response to the bug you mentioned (http://bugs.gnu.org/19833)?  Are
there more such examples?

> making JIT optional at runtime is therefore the title of this patch and as
> I mentioned in some other thread it might be even useful to us for our
> own tests.

Testability is a valid concern, especially if the JIT code is
considered, well, unfinished.

René

  reply	other threads:[~2019-07-30 17:55 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-28 23:54 [RFC PATCH] " Carlo Marcelo Arenas Belón
2019-07-29  0:09 ` Carlo Arenas
2019-07-29  4:57 ` Junio C Hamano
2019-07-29  5:29   ` Carlo Arenas
2019-07-29  8:55 ` Ævar Arnfjörð Bjarmason
2019-07-29 10:26   ` Carlo Arenas
2019-07-29 12:38     ` Ævar Arnfjörð Bjarmason
2019-07-30 13:01       ` Carlo Arenas
2019-07-29 10:59 ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2019-07-29 11:33   ` Carlo Arenas
2019-07-29 15:11   ` René Scharfe
2019-07-29 17:47     ` Junio C Hamano
2019-07-30  0:49       ` Carlo Arenas
2019-07-30 17:55         ` René Scharfe [this message]
2019-07-31 12:36         ` Johannes Schindelin
2019-07-31 16:18           ` Junio C Hamano
2019-07-31 12:32   ` Johannes Schindelin
2019-07-31 14:57     ` Ævar Arnfjörð Bjarmason
2019-08-04  0:25       ` Carlo Arenas
2019-08-04  3:14   ` [RFC PATCH v3] grep: treat PCRE2 jit compilation memory error as non fatal Carlo Marcelo Arenas Belón
2019-08-04  7:43     ` Carlo Arenas
2019-08-05 20:16       ` Junio C Hamano
2019-07-31 12:24 ` [RFC PATCH] grep: allow for run time disabling of JIT in PCRE Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=22b9cf06-ebbb-a0c1-2cc9-7f09d61d3d9b@web.de \
    --to=l.s.r@web.de \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=dev+git@drbeat.li \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [RFC PATCH v2] grep: allow for run time disabling of JIT in PCRE' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git