-
Notifications
You must be signed in to change notification settings - Fork 210
[JIT] Performance regression with some regexs #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Have you tried with the new Release Candidate (10.38-RC1)? which is available on GitHub? Please do so if you can. If there is still a problem, I'll assign this to the JIT maintainer. |
Yes, I also tried with 10.38-RC1. |
It looks like this commit introduced the issue: 21c40e6 |
@cmb69 thank you for this report. It looks like I accidentally disabled a very important optimization. @PhilipHazel this patch fixes it, but release cycle is started so please decide whether you want it in this release or the next.
Btw I am happy we moved to git, this bug was easier to track down with it. |
As it is a very small patch, I think we should include it in this release. Please go ahead and do the merge and update ChangeLog. |
Fixed in dc5f966. |
That was fast, thank you! I can confirm that the patch solves the issue, and I couldn't detect any regression with the PHP PCRE test suite. |
I'm forwarding this from https://bugs.php.net/81424, but it seems that this is actually a PCRE2 issue. Consider the following (bad) regex:
When run on a large string (e.g. https://pastebin.com/WVBR4f9T), with PCRE2 10.34 JIT this was fast; with PCRE2 10.35 and later it is more than hundred times slower.
If the regex is rewritten to use a lookbehind assertion (
/(?<![{};\/\n]+)\{\}/
), performance with the different PCRE2 versions is on par, so you may not consider this something to be fix-worthy. :)There is no performance regression without JIT, so I wonder whether this regex isn't jitted anymore as of PCRE2 10.35.
The text was updated successfully, but these errors were encountered: