[PHP-DEV] Re: Always set return_value_ptr?
On Tue, Aug 27, 2013 at 11:40 AM, Nikita Popov nikita@gmail.com wrote: On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita@gmail.com wrote: Hi internals! Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/17844379/385378 Patch for this change can be found here: https://github.com/php/php-src/pull/420 The patch also adds new macros to allow easy use of this feature called RETVAL_ZVAL_FAST/RETURN_ZVAL_FAST (anyone got a better name?) If no one objects I'll merge this sometime soon. Changes merged. Small benchmark to verify that this indeed avoids the copy: https://gist.github.com/nikic/6398090 :) Nikita
Re: [PHP-DEV] Re: Always set return_value_ptr?
On 31/08/13 14:13, Nikita Popov wrote: Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/17844379/385378 Changes merged. Small benchmark to verify that this indeed avoids the copy: https://gist.github.com/nikic/6398090 :) Nikita, IMO, this is a material performance optimisation of the PHP internals, as it removes one of the most common unnecessary (expensive) copies. So thanks for this. It will be interesting to see the benefit on real apps such as MediaWiki. I'll pull a 5.5 snapshot and compare it to 5.5.2 :-) Regards Terry -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: Always set return_value_ptr?
On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison ellison.te...@gmail.comwrote: On 27/08/13 10:40, Nikita Popov wrote: On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita@gmail.com wrote: Hi internals! Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/**17844379/385378http://stackoverflow.com/q/17844379/385378 Patch for this change can be found here: https://github.com/php/php-**src/pull/420https://github.com/php/php-src/pull/420 The patch also adds new macros to allow easy use of this feature called RETVAL_ZVAL_FAST/RETURN_ZVAL_**FAST (anyone got a better name?) If no one objects I'll merge this sometime soon. +1 Though looking through the ext uses, most functions returning an array build it directly in return_value and thus avoid the copy. I also see that you've picked up all of the cases in ext/standard/array.c where these macros can be applied. There's another one in string.c, in PHP_FUNCTION(pathinfo), that could be applied as well, though there's little performance gain in avoiding the copy of a 4 element string array. BTW, looking at this pathinfo code, it doesn't do what the documentation says it does -- or at least this states that the optional argument if present should be _one_ of PATHINFO_DIRNAME, PATHINFO_BASENAME, PATHINFO_EXTENSION or PATHINFO_FILENAME. However, if a bitmask is supplied then this function returns the element corresponding to the lowest bit value rather than an error return, for example: $ php -r 'echo pathinfo(/tmp/x.fred, PATHINFO_FILENAME|PATHINFO_** EXTENSION),\n;' fred This is a bizarre behaviour. At a minimum the documentation should actually state what the function does. Or do we bother to raise a patch to fix this sort of thing, given that returning an empty string (or more consistently with other functions, NULL) in this case could create a BC break with existing buggy code? This is weird, yes. It's not the lowest bit value that is returned, but the first element put in the array (as zend_hash_get_current_data() is used with no HashPosition) , which is even more confusing. How to explain that in the documentation ? :| Julien.Pauli
Re: [PHP-DEV] Re: Always set return_value_ptr?
On 30/08/13 10:43, Julien Pauli wrote: On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison ellison.te...@gmail.com mailto:ellison.te...@gmail.com wrote: There's another one in string.c, in PHP_FUNCTION(pathinfo), that could be applied as well, though there's little performance gain in avoiding the copy of a 4 element string array. BTW, looking at this pathinfo code, it doesn't do what the documentation says it does -- or at least this states that the optional argument if present should be _one_ of PATHINFO_DIRNAME, PATHINFO_BASENAME, PATHINFO_EXTENSION or PATHINFO_FILENAME. However, if a bitmask is supplied then this function returns the element corresponding to the lowest bit value rather than an error return, for example: $ php -r 'echo pathinfo(/tmp/x.fred, PATHINFO_FILENAME|PATHINFO_EXTENSION),\n;' fred This is a bizarre behaviour. At a minimum the documentation should actually state what the function does. Or do we bother to raise a patch to fix this sort of thing, given that returning an empty string (or more consistently with other functions, NULL) in this case could create a BC break with existing buggy code? This is weird, yes. It's not the lowest bit value that is returned, but the first element put in the array (as zend_hash_get_current_data() is used with no HashPosition) , which is even more confusing. How to explain that in the documentation ? :| Yes I understand that, but the code processes the elements in this dirname, basename, filename, extension order so the two statements are equivalent in implementation. I am an experienced developer but a newbie-ish to the PHP developer community, and I come back to my Q. What do we typically do if we come across such weird functional behaviour outside the documented use of a standard function? * Shrug our shoulders and say That's PHP for you. BC rules * Fix the documentation to say what the code actually does * Fix the code at the next major release, say 5.6 to have sensible error behaviour. Just interested in understanding the consensus policy here. Do I post a fix to the doc; post a fix to the code; or move on to other issues? Regards Terry
Re: [PHP-DEV] Re: Always set return_value_ptr?
hi, On Fri, Aug 30, 2013 at 12:44 PM, Terry Ellison ellison.te...@gmail.com wrote: Yes I understand that, but the code processes the elements in this dirname, basename, filename, extension order so the two statements are equivalent in implementation. I am an experienced developer but a newbie-ish to the PHP developer community, and I come back to my Q. What do we typically do if we come across such weird functional behaviour outside the documented use of a standard function? * Shrug our shoulders and say That's PHP for you. BC rules As of now, this is the rule. BC has a much higher priority than some weird edge cases bug fixes. * Fix the documentation to say what the code actually does That should be the case as much as possible :) * Fix the code at the next major release, say 5.6 to have sensible error behaviour. This is always a gray zone, especially if changes behaviors and then introduces BC issues. BC breaks are the biggest problems in php. Obviously I am not referring to new notices or warnings but actual behaviors changes. Cheers, -- Pierre @pierrejoye | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: Always set return_value_ptr?
On 27/08/13 10:40, Nikita Popov wrote: On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita@gmail.com wrote: Hi internals! Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/17844379/385378 Patch for this change can be found here: https://github.com/php/php-src/pull/420 The patch also adds new macros to allow easy use of this feature called RETVAL_ZVAL_FAST/RETURN_ZVAL_FAST (anyone got a better name?) If no one objects I'll merge this sometime soon. +1 Though looking through the ext uses, most functions returning an array build it directly in return_value and thus avoid the copy. I also see that you've picked up all of the cases in ext/standard/array.c where these macros can be applied. There's another one in string.c, in PHP_FUNCTION(pathinfo), that could be applied as well, though there's little performance gain in avoiding the copy of a 4 element string array. BTW, looking at this pathinfo code, it doesn't do what the documentation says it does -- or at least this states that the optional argument if present should be _one_ of PATHINFO_DIRNAME, PATHINFO_BASENAME, PATHINFO_EXTENSION or PATHINFO_FILENAME. However, if a bitmask is supplied then this function returns the element corresponding to the lowest bit value rather than an error return, for example: $ php -r 'echo pathinfo(/tmp/x.fred, PATHINFO_FILENAME|PATHINFO_EXTENSION),\n;' fred This is a bizarre behaviour. At a minimum the documentation should actually state what the function does. Or do we bother to raise a patch to fix this sort of thing, given that returning an empty string (or more consistently with other functions, NULL) in this case could create a BC break with existing buggy code? Regards Terry -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: Always set return_value_ptr?
On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita@gmail.com wrote: Hi internals! Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/17844379/385378 Patch for this change can be found here: https://github.com/php/php-src/pull/420 The patch also adds new macros to allow easy use of this feature called RETVAL_ZVAL_FAST/RETURN_ZVAL_FAST (anyone got a better name?) If no one objects I'll merge this sometime soon. Nikita
Re: [PHP-DEV] Re: Always set return_value_ptr?
On Tue, Aug 27, 2013 at 11:40 AM, Nikita Popov nikita@gmail.com wrote: On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita@gmail.com wrote: Hi internals! Is there any particular reason why we only pass return_value_ptr to internal functions if they have the ACC_RETURN_REFERENCE flag set? Why can't we always provide the retval ptr, even for functions that don't return by reference? This would allow returning zvals without having to copy them first (what RETVAL_ZVAL does). Motivation for this is the following SO question: http://stackoverflow.com/q/17844379/385378 Patch for this change can be found here: https://github.com/php/php-src/pull/420 The patch also adds new macros to allow easy use of this feature called RETVAL_ZVAL_FAST/RETURN_ZVAL_FAST (anyone got a better name?) If no one objects I'll merge this sometime soon. After discussing this point, I'm +1 with this patch. Julien.Pauli
