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
MaxKellermann wants to merge 1 commit into from
Closed

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

MaxKellermann wants to merge 1 commit into from

Conversation

@MaxKellermann
Copy link
Contributor

@MaxKellermann MaxKellermann commented Jan 10, 2023

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

MaxKellermann commented Jan 10, 2023

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

MaxKellermann commented Jan 10, 2023

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

MaxKellermann commented Jan 11, 2023

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

MaxKellermann commented Jan 13, 2023

Rebased & fixed conflict.

@MaxKellermann
Copy link
Contributor Author

MaxKellermann commented Jan 13, 2023

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

MaxKellermann commented Jan 20, 2023

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

MaxKellermann commented Feb 24, 2023

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