From 292ea9b1491a510e74d1e3cbea297b66562a7a0c Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 16 Jan 2023 12:25:21 +0100 Subject: [PATCH 1/2] Revert "ext/opcache: use C11 atomics for "restart_in" (#10276)" This reverts commit 061fcdb0a5649572b90cdad1be4d457dd3faa301. While the commit does indeed improve performance, @dstogov complained that it disables the code path calling kill_all_lockers(), and thus hanging workers are never killed and restarted. https://github.com/php/php-src/pull/10276#issuecomment-1383593046 The fact that this feature was not implemented in the existing atomic code path (i.e. Windows) did not mean that the feature was considered not strictly necessary, but that the Windows implementation just did not need the feature because SAPIs that work on Windows do not manage child processes https://github.com/php/php-src/pull/10276#issuecomment-1383868696 https://github.com/php/php-src/pull/10276#issuecomment-1384235011 --- ext/opcache/ZendAccelerator.c | 26 +++++++++++--------------- ext/opcache/ZendAccelerator.h | 16 +--------------- ext/opcache/config.m4 | 2 -- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 48a07760d981d..709a647a31c78 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -135,11 +135,7 @@ static void preload_shutdown(void); static void preload_activate(void); static void preload_restart(void); -#ifdef HAVE_STDATOMIC_H -# define INCREMENT(v) (++ZCSG(v)) -# define DECREMENT(v) (--ZCSG(v)) -# define LOCKVAL(v) atomic_load(&ZCSG(v)) -#elif defined(ZEND_WIN32) +#ifdef ZEND_WIN32 # define INCREMENT(v) InterlockedIncrement64(&ZCSG(v)) # define DECREMENT(v) InterlockedDecrement64(&ZCSG(v)) # define LOCKVAL(v) (ZCSG(v)) @@ -277,7 +273,7 @@ static ZEND_INI_MH(accel_include_path_on_modify) static inline void accel_restart_enter(void) { -#ifdef INCREMENT +#ifdef ZEND_WIN32 INCREMENT(restart_in); #else struct flock restart_in_progress; @@ -296,7 +292,7 @@ static inline void accel_restart_enter(void) static inline void accel_restart_leave(void) { -#ifdef DECREMENT +#ifdef ZEND_WIN32 ZCSG(restart_in_progress) = false; DECREMENT(restart_in); #else @@ -317,9 +313,7 @@ static inline void accel_restart_leave(void) static inline int accel_restart_is_active(void) { if (ZCSG(restart_in_progress)) { -#ifdef LOCKVAL - return LOCKVAL(restart_in) != 0; -#else +#ifndef ZEND_WIN32 struct flock restart_check; restart_check.l_type = F_WRLCK; @@ -337,6 +331,8 @@ static inline int accel_restart_is_active(void) } else { return 1; } +#else + return LOCKVAL(restart_in) != 0; #endif } return 0; @@ -345,7 +341,7 @@ static inline int accel_restart_is_active(void) /* Creates a read lock for SHM access */ static inline zend_result accel_activate_add(void) { -#ifdef INCREMENT +#ifdef ZEND_WIN32 SHM_UNPROTECT(); INCREMENT(mem_usage); SHM_PROTECT(); @@ -368,7 +364,7 @@ static inline zend_result accel_activate_add(void) /* Releases a lock for SHM access */ static inline void accel_deactivate_sub(void) { -#ifdef DECREMENT +#ifdef ZEND_WIN32 if (ZCG(counted)) { SHM_UNPROTECT(); DECREMENT(mem_usage); @@ -391,7 +387,7 @@ static inline void accel_deactivate_sub(void) static inline void accel_unlock_all(void) { -#ifdef DECREMENT +#ifdef ZEND_WIN32 accel_deactivate_sub(); #else struct flock mem_usage_unlock_all; @@ -832,7 +828,7 @@ static void accel_use_shm_interned_strings(void) HANDLE_UNBLOCK_INTERRUPTIONS(); } -#ifndef LOCKVAL +#ifndef ZEND_WIN32 static inline void kill_all_lockers(struct flock *mem_usage_check) { int tries; @@ -899,7 +895,7 @@ static inline void kill_all_lockers(struct flock *mem_usage_check) static inline int accel_is_inactive(void) { -#ifdef LOCKVAL +#ifdef ZEND_WIN32 if (LOCKVAL(mem_usage) == 0) { return SUCCESS; } diff --git a/ext/opcache/ZendAccelerator.h b/ext/opcache/ZendAccelerator.h index 6ecc2c2bc9df1..bc92854c35576 100644 --- a/ext/opcache/ZendAccelerator.h +++ b/ext/opcache/ZendAccelerator.h @@ -55,14 +55,6 @@ #include "zend_accelerator_hash.h" #include "zend_accelerator_debug.h" -#ifdef HAVE_STDATOMIC_H -# ifdef __cplusplus -# include -# else -# include -# endif -#endif // HAVE_STDATOMIC_H - #ifndef PHPAPI # ifdef ZEND_WIN32 # define PHPAPI __declspec(dllimport) @@ -269,13 +261,7 @@ typedef struct _zend_accel_shared_globals { bool restart_pending; zend_accel_restart_reason restart_reason; bool cache_status_before_restart; -#ifdef HAVE_STDATOMIC_H -# ifdef __cplusplus - std::atomic_llong mem_usage, restart_in; -# else - atomic_llong mem_usage, restart_in; -# endif -#elif defined(ZEND_WIN32) +#ifdef ZEND_WIN32 LONGLONG mem_usage; LONGLONG restart_in; #endif diff --git a/ext/opcache/config.m4 b/ext/opcache/config.m4 index c9fa6a7e57e4f..2a83fa2455974 100644 --- a/ext/opcache/config.m4 +++ b/ext/opcache/config.m4 @@ -23,8 +23,6 @@ if test "$PHP_OPCACHE" != "no"; then dnl Always build as shared extension ext_shared=yes - AC_CHECK_HEADERS([stdatomic.h]) - if test "$PHP_HUGE_CODE_PAGES" = "yes"; then AC_DEFINE(HAVE_HUGE_CODE_PAGES, 1, [Define to enable copying PHP CODE pages into HUGE PAGES (experimental)]) fi From c0f0c1960341355080ae559b631cc0670a422536 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 16 Jan 2023 12:32:37 +0100 Subject: [PATCH 2/2] ext/opcache: document lack of kill_all_lockers() on Windows kill_all_lockers() is not implemented on Windows, and does not need to be. --- ext/opcache/ZendAccelerator.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 709a647a31c78..ec33c69eb2f09 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -896,6 +896,13 @@ static inline void kill_all_lockers(struct flock *mem_usage_check) static inline int accel_is_inactive(void) { #ifdef ZEND_WIN32 + /* on Windows, we don't need kill_all_lockers() because SAPIs + that work on Windows don't manage child processes (and we + can't do anything about hanging threads anyway); therefore + on Windows, we can simply manage this counter with atomics + instead of flocks (atomics are much faster but they don't + provide us with the PID of locker processes) */ + if (LOCKVAL(mem_usage) == 0) { return SUCCESS; }