Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2023, 20:08 UTC
Message-ID: <xmqqlelj3hvk.fsf@gitster.g>
In-Reply-To: <xmqq357r4zvk.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

> If we were to keep that "die", it is absolutely required, I would
> think.  Users who got their Git with JIT-enabled pcre2 may be
> viewing JIT merely as "a clever optimization the implementation is
> allowed to use when able", without knowing and more importantly
> without wanting to know how to disable it from within their
> patterns.
>
> But can't we drop that die() if we took the v1 route?

Having said all that, I do not mind queuing v2 if the "use *NO_JIT to disable" is added to the message to help users who are forced to redo the query.

And in practice, it shouldn't make that much difference, because the only scenario (other than the SELinux-like situation where JIT is compiled in but does not work at all) that the difference may matter would happen when a non-trivial portion of the patterns users use are not workable with JIT, but if that were the case, we would have written JIT off as not mature enough and not yet usable long time ago. So, in practice, patterns refused by JIT would be a very tiny minority to matter in real life, and "failing fast to inconvenience users" would not be too bad.

So while I still think v1's simplicity is the right thing to have here, I think it is waste of our braincell to compare v1 vs v2. As v2 gives smaller incremental behaviour change perceived by end users, if somebody really wanted to, I'd expect that a low-hanging fruit #leftoverbit on top of such a patch, after the dust settles, would be to

 (1) rename pcre2_jit_functional() to fall_back_to_interpreter() or
     something,

 (2) add a configuration variable to tell fall_back_to_interpreter()
     that any form of JIT error is allowed to fall back to
     interpreter().

and such a patch will essentially give back the simplicity of v1 to folks who opt into the configuration.

Thanks.