Skip to content

ext/opcache: use pthread_mutex_t instead of a fcntl(F_SETLK) #10277

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 1 commit into from
Closed

ext/opcache: use pthread_mutex_t instead of a fcntl(F_SETLK) #10277

wants to merge 1 commit into from

Conversation

MaxKellermann
Copy link
Contributor

pthread mutexes are lighter than fcntl(F_SETLK) because non-contending operations can avoid the system call.

Note that this commit preserves the "lock_file" variable because it is used by ZendAccelerator.c - to be removed after
#10276 is merged.

@cmb69
Copy link
Member

cmb69 commented Jan 10, 2023

Wouldn't that introduce a pthreads requirement even for NTS builds?

@MaxKellermann
Copy link
Contributor Author

Wouldn't that introduce a pthreads requirement even for NTS builds?

This PR does add a dependency, but it's optional - it will fall back to flock as before if pthreads is not available.

(That (optional) dependency already exists in the PHP core (zend_call_stack.c) and in the JIT (zend_jit.c) and in the pcre extension.)

@MaxKellermann
Copy link
Contributor Author

LINUX_X64_RELEASE_ZTS fails due to segfault in ext/mysqli/tests/mysqli_fork.phpt - forking the PHP process could indeed reveal a bug in my code, but I can't reproduce it (not even with valgrind), and this is the only CI target that fails - anybody got an idea? Or is this just yet another flaky CI target? Or is this an unrelated PHP heisenbug?

@cmb69
Copy link
Member

cmb69 commented Jan 11, 2023

I've triggered a re-run. Let's see.

@MaxKellermann
Copy link
Contributor Author

All green. So the segfault appears to be an existing PHP bug, and unrelated to my code changes.

@MaxKellermann MaxKellermann force-pushed the opcache_pthread_pshared_mutex branch from e077ffd to 71968c5 Compare January 13, 2023 09:35
@MaxKellermann
Copy link
Contributor Author

Rebased & fixed conflict.

@MaxKellermann
Copy link
Contributor Author

LINUX_X32_DEBUG_ZTS failure unrelated to this PR.

@MaxKellermann MaxKellermann mentioned this pull request Jan 17, 2023
Merged
@MaxKellermann MaxKellermann force-pushed the opcache_pthread_pshared_mutex branch from 71968c5 to 27f456f Compare January 20, 2023 10:41
@MaxKellermann
Copy link
Contributor Author

Rebased & fixed conflict again.

@MaxKellermann MaxKellermann force-pushed the opcache_pthread_pshared_mutex branch 4 times, most recently from 2d6905e to a6d76df Compare February 13, 2023 13:30
@MaxKellermann MaxKellermann mentioned this pull request Feb 24, 2023
Closed
@MaxKellermann
ext/opcache: use pthread_mutex_t instead of a fcntl(F_SETLK)
6e9f69f 
pthread mutexes are lighter than fcntl(F_SETLK) because non-contending
operations can avoid the system call.

Note that this commit preserves the "lock_file" variable because it is
used by ZendAccelerator.c - now that
php#10276 is merged, we may consider
removing it.
@MaxKellermann MaxKellermann force-pushed the opcache_pthread_pshared_mutex branch from a6d76df to 6e9f69f Compare February 24, 2023 09:17
@MaxKellermann
Copy link
Contributor Author

Rebased & fixed conflict.

@MaxKellermann MaxKellermann deleted the opcache_pthread_pshared_mutex branch March 6, 2023 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MaxKellermann @cmb69