ext/opcache: use C11 atomics for "restart_in" #10276
Conversation
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 php#10276 is merged.
Cheaper than fcntl(F_SETLK). The same is done already on Windows, so if that works, why not use it everywhere? (Of course, only if the compiler supports this C11 feature.) As a bonus, the code in this commit also works on C++ via C++11 std::atomic, just in case somebody adds some C++ code to the opcache extension one day.
MaxKellermann
force-pushed
the
opcache_c11_atomic
branch
from
70eed87
to
6471477
Compare
|
Thank you! |
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.
|
This commit is wrong. It completely disables @ iluuu1994 please be more careful reviewing not obvious changes and ask my review in case of doubt. |
|
Hi @dstogov! Sorry. In the past I've felt like I was overly cautious with merging PRs but I probably overcorrected for that here. Would definitely not have hurt to wait a bit longer. I will revert this commit when I get the chance. |
True. This now uses the same code paths as on Windows, wher @dstogov, can you please explain the difference, why it is necessary on most operating systems, but not on Windows? There should be a code comment with this explanation. |
|
This is not my decision. This code was originally developed as a propitiate software before me, and I worked on Open Sourcing it. Anyway, this never worked on Windows, but this doesn't mean you should broke this for others without any comments. |
Okay, this changes everything - the Windows implementation should be considered broken? There was no code comment indicating this. There should be one, to avoid confusing others like it did confuse me. |
This reverts commit 061fcdb. 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. php#10276 (comment) 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 was incomplete and broken. php#10276 (comment)
kill_all_lockers() is not implemented on Windows, thus the code is incomplete and broken. See php#10276 (comment) for the explanation.
|
From the docs:
So yeah, And the fact that OPcache has "limitations" on Windows, is known. That cannot be fixed, though (unless for multithreaded SAPIs, which may not be the best solution wrt. stability). |
This reverts commit 061fcdb. 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. php#10276 (comment) 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 php#10276 (comment) php#10276 (comment)
* Revert "ext/opcache: use C11 atomics for "restart_in" (#10276)" This reverts commit 061fcdb. 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. #10276 (comment) 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 #10276 (comment) #10276 (comment) * ext/opcache: document lack of kill_all_lockers() on Windows kill_all_lockers() is not implemented on Windows, and does not need to be.
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.
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.
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.
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.
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.
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.
Cheaper than fcntl(F_SETLK). The same is done already on Windows, so if that works, why not use it everywhere? (Of course, only if the compiler supports this C11 feature.)
As a bonus, the code in this commit also works on C++ via C++11 std::atomic, just in case somebody adds some C++ code to the opcache extension one day.