[PATCH] - Improving multi-threaded performance by propagating TSRMLS_C View as Markdown

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 (<?php echo "Hello World"?>).

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