# Always set return_value_ptr? - Thread: https://externals.io/message/68364 - Original archive: https://news-web.php.net/php.internals/68364 - Started by Nikita Popov on 2013-08-03T18:16:42+00:00 - Messages: 10 - 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 --- ## #68364 — Nikita Popov — 2013-08-03T18:16:42+00:00 URL: https://externals.io/message/68364#68364 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 Thanks, Nikita --- ## #68365 — Anatol Belski — 2013-08-03T22:56:39+00:00 URL: https://externals.io/message/68364#68365 · In reply to: #68364 Hi Nikita, On Sat, 2013-08-03 at 20:16 +0200, Nikita Popov wrote: > 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). were it not possible to zval_add_ref() the item one needs to return? Possibly recursive. I can rebember doing similar to array_init(return_value); for(zend_hash_internal_pointer_reset(Z_ARRVAL_P(my_array)); zend_hash_has_more_elements(Z_ARRVAL_P(my_array)) == SUCCESS; zend_hash_move_forward(Z_ARRVAL_P(my_array))) { zval **my_array_item; if(zend_hash_get_current_data(Z_ARRVAL_P(my_array), (void**)&my_array_item) == FAILURE) { continue; } zend_hash_get_current_key(Z_ARRVAL_P(my_array), &string_key, &num_key, 0) zval_add_ref(my_array_item); zend_hash_update(Z_ARRVAL_P(return_value), string_key, strlen(string_key), my_array_item, sizeof(zval *), NULL); } That's still some overhead, but even in case my_array_item was non scalar, that should be working. Unfortunately i've no access to that code anymore. my_array were of course a flat array, for a complex array it should be done recursive. Regards anatol --- ## #68624 — Nikita Popov — 2013-08-27T09:40:35+00:00 URL: https://externals.io/message/68364#68624 · In reply to: #68364 > 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 --- ## #68626 — Julien Pauli — 2013-08-27T13:38:20+00:00 URL: https://externals.io/message/68364#68626 · In reply to: #68624 > > > > […] > > 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 --- ## #68692 — Terry Ellison — 2013-08-30T00:30:06+00:00 URL: https://externals.io/message/68364#68692 · In reply to: #68624 > > > > […] > 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 --- ## #68712 — Julien Pauli — 2013-08-30T09:43:14+00:00 URL: https://externals.io/message/68364#68712 · In reply to: #68692 On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison wrote: > > > > […] > +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 --- ## #68715 — Terry Ellison — 2013-08-30T10:44:18+00:00 URL: https://externals.io/message/68364#68715 · In reply to: #68712 > On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison > > 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 --- ## #68716 — Pierre Joye — 2013-08-30T10:49:55+00:00 URL: https://externals.io/message/68364#68716 · In reply to: #68715 hi, > 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 --- ## #68778 — Nikita Popov — 2013-08-31T13:13:00+00:00 URL: https://externals.io/message/68364#68778 · In reply to: #68624 > > > > […] > > 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 --- ## #68779 — Terry Ellison — 2013-08-31T15:20:30+00:00 URL: https://externals.io/message/68364#68779 · In reply to: #68778 > > […] > 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