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
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
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
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/385378Patch for this change can be found here:
https://github.com/php/php-src/pull/420The 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
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/385378Patch for this change can be found here:
https://github.com/php/php-src/pull/420The 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
On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison ellison.terry@gmail.comwrote:
On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov nikita.ppv@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/385378Patch for this change can be found here:
https://github.com/php/php-**src/pull/420https://github.com/php/php-src/pull/420The 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_EXTENSIONor 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";'
fredThis 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
On Fri, Aug 30, 2013 at 2:30 AM, Terry Ellison
<ellison.terry@gmail.com mailto:ellison.terry@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
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
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/385378Patch for this change can be found here:
https://github.com/php/php-src/pull/420The 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
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