Skip to content

Prevent regex cache reuse when JIT flag does not match php.ini #11545

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Prevent regex cache reuse when JIT flag does not match php.ini #11545

wants to merge 2 commits into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jun 27, 2023 •
edited
Loading

Fix #11374, replace #11396 and #11511

@mvorisek mvorisek changed the base branch from master to PHP-8.1 June 27, 2023 10:13
@mvorisek mvorisek force-pushed the pcre_jit_cache_key branch 2 times, most recently from 18afe29 to 4f12777 Compare June 27, 2023 10:26
@mvorisek mvorisek marked this pull request as ready for review June 27, 2023 10:28
@mvorisek
Copy link
Contributor Author

@iluuu1994 thank you for your #11374 (comment).

Here is the benchmark comparison before/after this PR:

image

@mvorisek mvorisek changed the title Prevent regex cache reuse when JIT flag does not match Prevent regex cache reuse when JIT flag does not match php.ini Jun 27, 2023
mvorisek
mvorisek commented Jul 7, 2023
View reviewed changes
iluuu1994
iluuu1994 reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is broken, it shows 4435 commits. Please create a new one. Please also target master. The last PR as shown that it's easy to create regressions.

@iluuu1994 iluuu1994 closed this Jul 7, 2023
@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 7, 2023

@iluuu1994 please reopen, this PR is NOT broken, I only merged in master to run the BENCHMARK which is CI job in master only, then I reverted it correctly.

@TimWolla
Copy link
Member

TimWolla commented Jul 7, 2023

then I reverted it correctly.

No you did not. Reverting a merge is not actually possible.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 7, 2023 •
edited
Loading

@TimWolla it is, see the correct diff and feel free to open the branch in GitKraken. If you think I am wrong, please tell me why the result is wrong.

I propose this PR to PHP 8.1 as bugfix.

The mvorisek@660452b commit shows the BENCHMARKING data, one commit later I reverted the merge...

@iluuu1994
Copy link
Member

@mvorisek mvorisek@5d7aa3d Instead of reverting, you should git reset --hard ... and then git push --force(-with-lease). We certainly don't want to merge thousands of commits just to revert them.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 7, 2023

@iluuu1994 reopen this PR and I will squash.

@mvorisek
Copy link
Contributor Author

@iluuu1994 can you please reopen this PR? I will sqush and force push, I do not see any reason to open a new PR. Thank you.

@mvorisek
Copy link
Contributor Author

@iluuu1994 please reopen this PR so I can fix/amend it

@mvorisek
Copy link
Contributor Author

@nielsdos do you have the rights to reopen this PR or who else has?

@nielsdos
Copy link
Member

@mvorisek
I can, but as said earlier this PR is broken, a new one targeting master should be created. The reason to target master is because it's a risky bugfix (previous attempt created a performance issue).

So basically, you should do this:

  • Create a fresh branch, based off master
  • On the new branch, do: git cherry-pick 4f12777863997a58709c5d79ffdacb4bc3e3b5aa and then git cherry-pick 561441451e4dd7d4d7a6cce322bee5b18d473447 to get your changes of this PR into the new branch. Please squash the changes into a single commit after doing so.
  • Open a new PR with your new branch, targeting master.

For future reference, I recommend to use rebasing instead of merging. Undoing merges is a headache. Interactive rebasing give you more freedom.

@mvorisek
Copy link
Contributor Author

@nielsdos please reopen it, I use git daily and I can/will squash and force push the branch to no contain the merge/revert.

@nielsdos nielsdos reopened this Aug 11, 2023
@nielsdos
Copy link
Member

Okay I did, please fix this up and retarget this to master.

@mvorisek mvorisek force-pushed the pcre_jit_cache_key branch from 1d84f5f to 77e72f5 Compare August 11, 2023 21:34
@nielsdos
Copy link
Member

@mvorisek Please also take into account Ilija's feedback: #11545 (comment)
The compiler will be smart enough to optimize this, and Ilija's suggestion makes the code more readable.

Also target this to master please.

@mvorisek mvorisek force-pushed the pcre_jit_cache_key branch from cb73327 to 31197da Compare August 11, 2023 21:50
@mvorisek
Copy link
Contributor Author

@nielsdos git history squashed, about the targetting - can you please review the code? it is a bugfix without ABI break, performance was evaluated here - #11545 (comment), with this said, is the current targed right?

@nielsdos
Copy link
Member

@mvorisek I agree with Ilija that this should target master because it's too easy to create regressions in this component.
I have some fixes that are only in master for the same danger reason: too dangerous to make the changes in stable branches, because of unforeseen consequences a patch may have. Not only for performance.

It's always possible to backport patches from master in stable branches if really necessary, after they've proven their safety in the master branch. Note however that this is not a promise this will happen with this patch.

For now, you should target this to master. It's either that, or no chance at all for merging.

@mvorisek mvorisek changed the base branch from PHP-8.1 to master August 11, 2023 21:59
@mvorisek
Copy link
Contributor Author

ok, retargetted

@nielsdos
Copy link
Member

@mvorisek Also: fetch the master branch, and then rebase (not merging) your branch so that it's properly based on the master branch. This will also re-trigger CI too and fixup the failing targets.

@mvorisek mvorisek force-pushed the pcre_jit_cache_key branch from 31197da to aba6e05 Compare August 11, 2023 22:48
@mvorisek
Copy link
Contributor Author

mvorisek commented Aug 11, 2023 •
edited
Loading

rebased (for GH it is not needed, but for other CI providers it is)

@mvorisek mvorisek requested a review from iluuu1994 August 22, 2023 09:32
@mvorisek mvorisek changed the base branch from master to PHP-8.3 August 30, 2023 12:14
@mvorisek mvorisek force-pushed the pcre_jit_cache_key branch from aba6e05 to 886e6f0 Compare August 30, 2023 20:39
@mvorisek mvorisek mentioned this pull request Oct 10, 2023
Closed
@mvorisek mvorisek changed the base branch from PHP-8.3 to master October 19, 2023 16:26
@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 24, 2023 •
edited
Loading

Hey guys, this PR is here for a while, please let me know if there is anything I can do. The performance impact should be nearly zero this time.

@nielsdos
Copy link
Member

With the bug that prompted the existence of this workaround being fixed, I think this PR is not needed anymore.

@nielsdos nielsdos closed this Oct 27, 2023
@mvorisek
Copy link
Contributor Author

If this PR does not imply any measureable perfomance penalty, it should be landed into master, as updated php.ini value should still be honored in general as much as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees
No one assigned
Labels
Extension: pcre
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Different preg_match result with -d pcre.jit=0
4 participants
@mvorisek @TimWolla @iluuu1994 @nielsdos