Re: [PATCH] - Improving multi-threaded performance by propagating TSRMLS_C

Hi, On Mon, 2009-11-30 at 19:02 +0530, Arvind Srinivasan wrote: > (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