Skip to content

Check if restart is pending before trying to lock SHM #11805

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.

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

Conversation

mikhainin
Copy link
Contributor

This reduces lock contention when Opcache restart is scheduled but not yet started.

@mikhainin
Copy link
Contributor Author

mikhainin commented Jul 27, 2023

Our application consists of 300K of PHP files (~3Gb of code) and the server has 220 FPM workers.

When we tried to enable file_cache, we got an extremely poor performance of the system, each request took over 30-60s when usually it finishes within 0.5s

perf top showed that the system spends most of the time inside the file lock:

  84.28%  [k] __pv_queued_spin_lock_slowpath                                                                                                                                                                         
   1.99%  [k] _raw_spin_unlock_irqrestore                                                                                                                                                                            
   0.92%  [k] finish_task_switch                                                                                                                                                                                     
   0.67%  [k] posix_locks_conflict                                                                                                                                                                                   
   0.24%  [k] posix_lock_inode                                                                                                                                                                                       
   0.21%  [k] copy_user_enhanced_fast_string                                                                                                                                                                         
   0.15%  [k] __raw_callee_save___pv_queued_spin_unlock                                                                                                                                                              
   0.14%  [k] _raw_spin_lock                                                                                                                                                                                         
   0.11%  [k] __do_page_fault                                                                                                                                                                                        
   0.09%  [k] clear_page_erms                                                                                                                                                                                        
   0.08%  [k] __d_lookup_rcu                                                                                                                                                                                         

PHP code wasn't even in the top 30.

The typical stack trace (we're using PHP8.0) for the FPM worker:

#0  0x00007f3778335f07 in fcntl64 () from /lib64/libc.so.6
#1  0x00007f37776d7922 in zend_shared_alloc_lock () at /build/php.git/ext/opcache/zend_shared_alloc.c:481
#2  0x00007f37776d5b7d in zend_file_cache_script_load (file_handle=<optimized out>) at /build/php.git/ext/opcache/zend_file_cache.c:1753
#3  0x00007fffffffe000 in ?? ()
#4  0x00007f3679ba83c8 in ?? ()
#5  0x0000000000000000 in ?? ()

zend_file_cache.c:1753 is zend_shared_alloc_lock(); under this condition.

After we tried this patch, performance was significantly improved (still worse than with shared memory but way better), and p99 of requests fit into 8 seconds.

perf top looks now healthier (when in-memory opcache is inactive):

  18.65%  [kernel]                                                        [k] __pv_queued_spin_lock_slowpath
   8.91%  php-fpm                                                         [.] execute_ex
   4.57%  php-fpm                                                         [.] zend_hash_find
   3.26%  php-fpm                                                         [.] _zend_hash_find_known_hash
   2.11%  [kernel]                                                        [k] _raw_spin_unlock_irqrestore
   1.79%  php-fpm                                                         [.] ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER
   1.57%  opcache.so                                                      [.] zend_accel_load_script
   1.57%  php-fpm                                                         [.] _zend_observe_fcall_begin

I assume that the lock is being held by one of the FPM workers in accel_activate() or it's just at typical contention.

@mikhainin
Copy link
Contributor Author

I can collect more data if you're interested or amend the patch if you feel like something is wrong

@mikhainin mikhainin marked this pull request as ready for review July 27, 2023 13:23
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

This looks fine.
Could you please re-target the pull request to PHP-8.1 branch.

@mikhainin mikhainin changed the base branch from master to PHP-8.1 July 31, 2023 10:01
This reduces lock contention when Opcache restart is scheduled
but not yet started.
@mikhainin mikhainin force-pushed the opcache-filecache-check-restart-pending branch from ad612cc to 570ead3 Compare July 31, 2023 10:19
@mikhainin
Copy link
Contributor Author

Done, it's now based on PHP-8.1

@iluuu1994 iluuu1994 closed this in 3e9792f Jul 31, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
This reduces lock contention when Opcache restart is scheduled
but not yet started.

Closes phpGH-11805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants