# [PATCH] - Improving multi-threaded performance by propagating TSRMLS_C - Thread: https://externals.io/message/46249 - Original archive: https://news-web.php.net/php.internals/46249 - Started by Arvind Srinivasan on 2009-11-30T13:32:27+00:00 - Messages: 9 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #46249 — Arvind Srinivasan — 2009-11-30T13:32:27+00:00 URL: https://externals.io/message/46249#46249 When running a benchmarking workload on PHP that was configured with multi-threading support (--enable-maintainer-zts) I noticed that pthread_get_specific is invoked many times during the processing of a request. PHP code invokes TSRMLS_FETCH() (which ends up invoking ts_resource_ex) in a number of places. Caller/callee data from Sun Studio's collector/analyzer showed the following: Attr. Excl. Incl. Name User CPU User CPU User CPU sec. sec. sec. 178.105 185.460 363.564 _emalloc 96.568 114.320 210.888 _efree 27.960 89.232 343.901 _zval_ptr_dtor 19.544 6.685 26.228 php_body_write_wrapper 6.925 36.806 224.617 _zval_dtor_func 4.263 2.902 7.165 safe_free_zval_ptr_rel 4.163 11.898 16.061 zend_get_parameters_ex 4.013 14.690 174.682 my_copy_zval 3.963 6.775 10.738 _erealloc 3.502 12.399 978.444 apc_copy_function_for_execution 2.732 4.143 9.647 do_inherit_method_check 2.592 21.565 225.137 _zval_copy_ctor_func 0.881 22.095 106.535 virtual_file_ex 0.600 1.961 6.855 list_entry_destructor 0.470 1.301 24.397 zend_file_handle_dtor 0.410 1.781 2.712 zend_function_dtor 0.270 0.350 0.620 convert_to_array 0.220 0.991 15.831 apc_search_paths 0.150 0.490 3.362 zend_register_resource 0.140 1.581 10.137 zend_alter_ini_entry 0.130 4.833 9023.272 php5_execute 0.110 0.500 3.502 zend_ini_long 0.070 0.530 0.600 _zend_bailout 0.050 0.320 4.513 zend_error 0.040 0.690 3.913 php_error_cb 0.040 0.560 2.852 zend_alter_ini_entry_ex 0. 3.202 584.369 php_request_shutdown 274.252 274.252 357.910 *ts_resource_ex 83.659 84.749 84.749 pthread_getspecific Propagating the value of TSRMLS_CC will avoid the overhead of having to invoke tsrm_tls_get/pthread_get_specific. The following patch (against trunk) does this: http://bitbucket.org/arvi/arviq/src/tip/svn-TSRM-patch.txt The above patch is mostly the result of making zend_alloc.[ch] TSRMLS_C-aware. While modifying the APIs to add propagate TSRMLS_C, I didn't quite know the best/preferred way to make the changes so I tried to use the following guidelines: (1) If the number of occurrences of the API to be modified are relatively small, then make the change directly. e.g. -int fcgi_accept_request(fcgi_request *req); -int fcgi_finish_request(fcgi_request *req, int force_close); +int fcgi_accept_request(fcgi_request *req TSRMLS_DC); +int fcgi_finish_request(fcgi_request *req, int force_close TSRMLS_DC); (2) If the number of occurrences is large, then rename the function (prefix with _) and add a macro that passes TSRMLS_C e.g. -ZEND_API void zend_hash_destroy(HashTable *ht); +ZEND_API void _zend_hash_destroy(HashTable *ht TSRMLS_DC); +#define zend_hash_destroy(ht) _zend_hash_destroy((ht) TSRMLS_CC) (3) For functions that have varargs, pass TSRMLS_C as the last but one parameter e.g. -ZEND_API int zend_get_parameters(int ht, int param_count, ...); +ZEND_API int zend_get_parameters(int ht TSRMLS_DC, int param_count, ...); (4) For single argument functions that have varags, pass TSRMLS_C as the first parameter e.g. -ZEND_API int zend_get_parameters_ex(int param_count, ...) /* {{{ */ +ZEND_API int zend_get_parameters_ex(TSRMLS_DC1 int param_count, ...) /* {{{ */ where +#define TSRMLS_DC1 TSRMLS_D, +#define TSRMLS_CC1 TSRMLS_C, Is (2) an acceptable thing to do or should I avoid renaming functions and do a mongo search-replace? With respect to (4), I wasn't sure how else to tackle varargs functions that take a single parameter. Any direction on what I should do for (3), (4) would be much appreciated. Out of curiosity, I counted the number of times thread_resources = tsrm_tls_get(); in ts_resource_ex is invoked for a simple command line invocation of hello.php (). Before patch: 22125 times After patch: 119 times Most of this is probably in one-time initialization stuff. I hope to have my trunk tree hooked up to my benchmarking setup soon and then I'll have more meaningful numbers. Thanks, Arvi --- ## #46250 — Pierre Joye — 2009-11-30T14:26:51+00:00 URL: https://externals.io/message/46249#46250 · In reply to: #46249 hi Arvind! Yes, to get rid of TSRMLS_FETCH is a very good thing as it is horribly slow (as you noticed). However it can't be done in 5.3 as it will break the ABI. About the patch itself, as it will break ABI anyway, I would go wit the solution 1) only. That's cleaner and consistent with what we usually do. thanks for your work :) > When running a benchmarking workload on PHP that was configured with > multi-threading support (--enable-maintainer-zts) I noticed that > pthread_get_specific is invoked many times during the processing of a > request. PHP code invokes TSRMLS_FETCH() (which ends up invoking > ts_resource_ex) in a number of places. > (1) If the number of occurrences of the API to be modified are > relatively small, then make the change directly. > e.g. > -int fcgi_accept_request(fcgi_request *req); > -int fcgi_finish_request(fcgi_request *req, int force_close); > +int fcgi_accept_request(fcgi_request *req TSRMLS_DC); > +int fcgi_finish_request(fcgi_request *req, int force_close TSRMLS_DC); Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org --- ## #46252 — Arvind Srinivasan — 2009-11-30T16:21:38+00:00 URL: https://externals.io/message/46249#46252 · In reply to: #46250 Hi Pierre, > slow (as you noticed). However it can't be done in 5.3 as it will > break the ABI. > I noticed that a recent fix (http://bugs.php.net/bug.php?id=49936) added TSRMLS_DC to an API in 5.3. http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/main/streams/php_stream_context.h?r1=290796&r2=290795&pathrev=290796 Wouldn't that need to be reverted as it breaks the ABI? > About the patch itself, as it will break ABI anyway, I would go wit > the solution 1) only. That's cleaner and consistent with what we > usually do. > Ok, I'll get rid of the _ prefix diffs. Do my approaches (3) (4) for varargs look ok? Thanks for your review. arvi --- ## #46253 — Felipe Pena — 2009-11-30T16:29:00+00:00 URL: https://externals.io/message/46249#46253 · In reply to: #46252 2009/11/30 Arvind Srinivasan > Hi Pierre, > > > > […] > > I noticed that a recent fix (http://bugs.php.net/bug.php?id=49936) > added TSRMLS_DC to an API in 5.3. > > http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/main/streams/php_stream_context.h?r1=290796&r2=290795&pathrev=290796 > > Wouldn't that need to be reverted as it breaks the ABI? > > It was reverted. -- Regards, Felipe Pena --- ## #46254 — Arvind Srinivasan — 2009-11-30T16:35:10+00:00 URL: https://externals.io/message/46249#46254 · In reply to: #46253 > It was reverted. ah..i missed that. sorry. --- ## #46255 — jvlad — 2009-11-30T20:03:45+00:00 URL: https://externals.io/message/46249#46255 · In reply to: #46252 > Hi Pierre, > > > > […] > > I noticed that a recent fix (http://bugs.php.net/bug.php?id=49936) > added TSRMLS_DC to an API in 5.3. > http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/main/streams/php_stream_context.h?r1=290796&r2=290795&pathrev=290796 > > Wouldn't that need to be reverted as it breaks the ABI? > How about the idea below: you can introduce new functions/macros with TSRMLSx propagated in, in v5.3 without changing existing API exposed to extensions, and make core using new API. Old functions/macros can be marked as deprecated. This will preseve ABI in v5.3 :) As of v6, you don't have to preserve ABI, so you can do replace the functions with new ones at once (introduce new, remove old, get all parts using new). Hmm and the diff will be big... BTW, did you try to avoid pthread_get_specific() calls now and forever? Something like just one pointer in static TLS would do. http://people.redhat.com/drepper/tls.pdf http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Thread-Local.html Since static TLS storage is quite a limited resource under some OSes, I think it will be wise to use only one thread-specific pointer in TLS and this pointer will point to the thread-specific array of pointers (not in TLS but in regular heap) and these pointers will point to all thread-globals like CG(), EG(), etc. The index of the array will be well-known id returned by ts_allocate_id. These arrays should be big enough to allow new globals to be added in run-time without re-allocating all the arrays in all currently running threads (see how ts_allocate_id() extends the arrays with pointers one by one, locking all threads). Structure and dereferencing will be exactly the same as currently implemented in TSRM.h, only storage will be declared global with __thread prefix and all related calls like TlsAlloc(), pthread_key_create would be removed. -jv --- ## #46257 — johannes@schlueters.de — 2009-12-01T12:05:42+00:00 URL: https://externals.io/message/46249#46257 · In reply to: #46249 Hi, > (1) If the number of occurrences of the API to be modified are > relatively small, then make the change directly. > e.g. > -int fcgi_accept_request(fcgi_request *req); > -int fcgi_finish_request(fcgi_request *req, int force_close); > +int fcgi_accept_request(fcgi_request *req TSRMLS_DC); > +int fcgi_finish_request(fcgi_request *req, int force_close TSRMLS_DC); right > (2) If the number of occurrences is large, then rename the function > (prefix with _) and add a macro that passes TSRMLS_C > e.g. > -ZEND_API void zend_hash_destroy(HashTable *ht); > +ZEND_API void _zend_hash_destroy(HashTable *ht TSRMLS_DC); > +#define zend_hash_destroy(ht) _zend_hash_destroy((ht) TSRMLS_CC) I'm not too happy about hiding the TSRM macro with another macro as it makes reading the code harder. When writing code I want to spot easily whether my function needs TSRM or not. When adding such a macro that should be done for making the code better readable, not worse. > (3) For functions that have varargs, pass TSRMLS_C as the last but one parameter > e.g. > -ZEND_API int zend_get_parameters(int ht, int param_count, ...); > +ZEND_API int zend_get_parameters(int ht TSRMLS_DC, int param_count, ...); right. > (4) For single argument functions that have varags, pass TSRMLS_C as > the first parameter > e.g. > -ZEND_API int zend_get_parameters_ex(int param_count, ...) /* {{{ */ > +ZEND_API int zend_get_parameters_ex(TSRMLS_DC1 int param_count, ...) /* {{{ */ > > where > +#define TSRMLS_DC1 TSRMLS_D, > +#define TSRMLS_CC1 TSRMLS_C, Are there more cases of this? - If zend_get_parameters is the only case I'd keep the TSRMLS_FETCH in it. zend_get_parameters is deprecated and shouldn't be used anymore. But adding a new macro makes it harder for new people to get started with the code. johannes --- ## #46259 — Arvind Srinivasan — 2009-12-01T12:46:34+00:00 URL: https://externals.io/message/46249#46259 · In reply to: #46257 hi johannes, > I'm not too happy about hiding the TSRM macro with another macro as it > makes reading the code harder. When writing code I want to spot easily > whether my function needs TSRM or not. > > When adding such a macro that should be done for making the code better > readable, not worse. > I'm currently working on getting rid of this change and instead explicitly passing TSRMLS_C. > > […] > > Are there more cases of this? - If zend_get_parameters is the only case multi_convert_to_long_ex multi_convert_to_double_ex multi_convert_to_string_ex are the other use cases of this. thanks, arvi --- ## #46267 — Arvind Srinivasan — 2009-12-03T12:30:22+00:00 URL: https://externals.io/message/46249#46267 · In reply to: #46257 > > > […] > With the exception of the APIs mentioned below, I've undone this change and instead done a global search/replace of all the functions that needed to pass TSRMLS_C as a parameter. The only cases that I couldn't quite do this for are: convert_to_long convert_to_double convert_to_null convert_to_boolean convert_to_object convert_to_array in zend_operators.h The problem is that currently convert_to_string is already a macro that calls _convert_to_string passing 2 parameters whereas the above are functions that take only 1 parameter. conv_object_to_type expects all these APIs to have the same signature. > > […] > > Are there more cases of this? - If zend_get_parameters is the only case > I'd keep the TSRMLS_FETCH in it. zend_get_parameters is deprecated and > shouldn't be used anymore. But adding a new macro makes it harder for > new people to get started with the code. I've backed out this change as well and instead used TSRMLS_FETCH() in those APIs that used TSRMLS_DC1/TSRMLS_CC1 (in my previous diff). I'll revisit those if they show up as performance hotspots. http://bitbucket.org/arvi/arviq/src/37f1d6b045b0/svn-TSRM-patch.txt contains the updated diff against trunk. I've compiled and run 'make test' on this after configuring it with the default module set and with/without --enable-maintainer-zts. Thanks, Arvi