Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
Hi Nikita,
I suppose it must fine now, but let me take a quick look tomorrow morning.
Thanks. Dmitry.
On Tue, Mar 12, 2013 at 8:43 PM, Nikita Popov wrote:
> On Mon, Mar 11, 2013 at 8:42 PM, Dmitry Stogov wrote:
>
>>
>>
>> On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov wrote:
>>
>>> On Mon, Mar 11, 2013 at 9:50 AM, Dmitry Stogov wrote:
>>>
Hi Nikita,
The patch looks good. I have just few comments
- In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I
didn't get why you added unreachable code for INT and NULL.
>>>
>>> You are right about the NULL case, that can indeed not occur. But INT is
>>> possible, e.g. consider this:
>>>
>>> >> foreach ((object) ['x','y','z'] as $k => $v) {
>>> var_dump($k);
>>> }
>>>
>>> this will give you keys int(0), int(1), int(2).
>>>
>>
>> Agree. I missed it.
>>
>>
>>> I'll remove the check for NULL.
>>>
>>>
>>> - At first, I fought, that it might be a good idea to change
zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of
returning IS_NULL that has a special meaning. But after looking into the
FE_FETCH code before the commit I understood where this NULL came from. I
know that NULL key may not appear for plain array and objects but I'm not
sure about iterators and generators. Now IS_NULL keys may mean that
iterator returned it directly IS_NULL or may be it was returned because of
some error conditions. Probably it's not a problem. What do you think?
>>>
>>> In foreach IS_NULL can't occur in an error condition, because the loop
>>> is already aborted when get_current_data provides NULL. IS_NULL can only
>>> happen when its explicitly provided (or handlers are incorrectly coded). I
>>> think the IS_NULL fallback is mainly important when the iterator is used
>>> for other things (like a dual it), to be sure that it'll always work. I
>>> don't think that it's important to distinguish between explicit NULL and
>>> failure NULL.
>>>
>>
>> Agree as well.
>>
>>
>>> I have one more question: Right now I added the
>>> zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
>>> because it has a zval dependency (unlike all the other code in there) and
>>> requires me to forward-declare the zval struct. Would it be better to move
>>> this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
>>> name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
>>> able to do the IS_CONSISTENT check anymore, is that a problem?
>>>
>>
>> I think we can move zval forward declaration (typedef struct _zval_struct
>> zval;) from zend.h into zend_type.h.
>>
>
> I now merged the changeset in
> https://github.com/php/php-src/commit/fcc6611de9054327441786e52444b5f8eecdd525(for
> PHP-5.5 and master) with the forward declaration moved. Also updated
> some array functions to use the new get_current_key_zval function :)
>
> Nikita
>
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Mon, Mar 11, 2013 at 8:42 PM, Dmitry Stogov wrote:
>
>
> On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov wrote:
>
>> On Mon, Mar 11, 2013 at 9:50 AM, Dmitry Stogov wrote:
>>
>>> Hi Nikita,
>>>
>>> The patch looks good. I have just few comments
>>>
>>> - In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I
>>> didn't get why you added unreachable code for INT and NULL.
>>>
>>
>> You are right about the NULL case, that can indeed not occur. But INT is
>> possible, e.g. consider this:
>>
>> > foreach ((object) ['x','y','z'] as $k => $v) {
>> var_dump($k);
>> }
>>
>> this will give you keys int(0), int(1), int(2).
>>
>
> Agree. I missed it.
>
>
>> I'll remove the check for NULL.
>>
>>
>> - At first, I fought, that it might be a good idea to change
>>> zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of
>>> returning IS_NULL that has a special meaning. But after looking into the
>>> FE_FETCH code before the commit I understood where this NULL came from. I
>>> know that NULL key may not appear for plain array and objects but I'm not
>>> sure about iterators and generators. Now IS_NULL keys may mean that
>>> iterator returned it directly IS_NULL or may be it was returned because of
>>> some error conditions. Probably it's not a problem. What do you think?
>>>
>>
>> In foreach IS_NULL can't occur in an error condition, because the loop is
>> already aborted when get_current_data provides NULL. IS_NULL can only
>> happen when its explicitly provided (or handlers are incorrectly coded). I
>> think the IS_NULL fallback is mainly important when the iterator is used
>> for other things (like a dual it), to be sure that it'll always work. I
>> don't think that it's important to distinguish between explicit NULL and
>> failure NULL.
>>
>
> Agree as well.
>
>
>> I have one more question: Right now I added the
>> zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
>> because it has a zval dependency (unlike all the other code in there) and
>> requires me to forward-declare the zval struct. Would it be better to move
>> this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
>> name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
>> able to do the IS_CONSISTENT check anymore, is that a problem?
>>
>
> I think we can move zval forward declaration (typedef struct _zval_struct
> zval;) from zend.h into zend_type.h.
>
I now merged the changeset in
https://github.com/php/php-src/commit/fcc6611de9054327441786e52444b5f8eecdd525(for
PHP-5.5 and master) with the forward declaration moved. Also updated
some array functions to use the new get_current_key_zval function :)
Nikita
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov wrote:
> On Mon, Mar 11, 2013 at 9:50 AM, Dmitry Stogov wrote:
>
>> Hi Nikita,
>>
>> The patch looks good. I have just few comments
>>
>> - In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I
>> didn't get why you added unreachable code for INT and NULL.
>>
>
> You are right about the NULL case, that can indeed not occur. But INT is
> possible, e.g. consider this:
>
> foreach ((object) ['x','y','z'] as $k => $v) {
> var_dump($k);
> }
>
> this will give you keys int(0), int(1), int(2).
>
Agree. I missed it.
> I'll remove the check for NULL.
>
>
> - At first, I fought, that it might be a good idea to change
>> zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of
>> returning IS_NULL that has a special meaning. But after looking into the
>> FE_FETCH code before the commit I understood where this NULL came from. I
>> know that NULL key may not appear for plain array and objects but I'm not
>> sure about iterators and generators. Now IS_NULL keys may mean that
>> iterator returned it directly IS_NULL or may be it was returned because of
>> some error conditions. Probably it's not a problem. What do you think?
>>
>
> In foreach IS_NULL can't occur in an error condition, because the loop is
> already aborted when get_current_data provides NULL. IS_NULL can only
> happen when its explicitly provided (or handlers are incorrectly coded). I
> think the IS_NULL fallback is mainly important when the iterator is used
> for other things (like a dual it), to be sure that it'll always work. I
> don't think that it's important to distinguish between explicit NULL and
> failure NULL.
>
Agree as well.
> I have one more question: Right now I added the
> zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
> because it has a zval dependency (unlike all the other code in there) and
> requires me to forward-declare the zval struct. Would it be better to move
> this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
> name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
> able to do the IS_CONSISTENT check anymore, is that a problem?
>
I think we can move zval forward declaration (typedef struct _zval_struct
zval;) from zend.h into zend_type.h.
Thanks. Dmitry.
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Mon, Mar 11, 2013 at 9:50 AM, Dmitry Stogov wrote:
> Hi Nikita,
>
> The patch looks good. I have just few comments
>
> - In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I
> didn't get why you added unreachable code for INT and NULL.
>
You are right about the NULL case, that can indeed not occur. But INT is
possible, e.g. consider this:
$v) {
var_dump($k);
}
this will give you keys int(0), int(1), int(2).
I'll remove the check for NULL.
- At first, I fought, that it might be a good idea to change
> zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of
> returning IS_NULL that has a special meaning. But after looking into the
> FE_FETCH code before the commit I understood where this NULL came from. I
> know that NULL key may not appear for plain array and objects but I'm not
> sure about iterators and generators. Now IS_NULL keys may mean that
> iterator returned it directly IS_NULL or may be it was returned because of
> some error conditions. Probably it's not a problem. What do you think?
>
In foreach IS_NULL can't occur in an error condition, because the loop is
already aborted when get_current_data provides NULL. IS_NULL can only
happen when its explicitly provided (or handlers are incorrectly coded). I
think the IS_NULL fallback is mainly important when the iterator is used
for other things (like a dual it), to be sure that it'll always work. I
don't think that it's important to distinguish between explicit NULL and
failure NULL.
I have one more question: Right now I added the
zend_hash_get_current_key_zval API in zend_hash.c/h, which is a bit ugly
because it has a zval dependency (unlike all the other code in there) and
requires me to forward-declare the zval struct. Would it be better to move
this somewhere else, e.g. zend_API.c/h? If so, can I still keep the same
name (with the zend_hash_ prefix)? If I move it to zend_API, I won't be
able to do the IS_CONSISTENT check anymore, is that a problem?
Thanks,
Nikita
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
Hi Nikita, The patch looks good. I have just few comments - In ZEND_FE_FETCH handler PLAIN_OBJECT may have only STRING keys. I didn't get why you added unreachable code for INT and NULL. - At first, I fought, that it might be a good idea to change zend_user_it_get_current_key() to return SUCCESS/FAILURE instead of returning IS_NULL that has a special meaning. But after looking into the FE_FETCH code before the commit I understood where this NULL came from. I know that NULL key may not appear for plain array and objects but I'm not sure about iterators and generators. Now IS_NULL keys may mean that iterator returned it directly IS_NULL or may be it was returned because of some error conditions. Probably it's not a problem. What do you think? I personally, irrelevant to this patch. I didn't found serious technical issues, so I think it may be accepted. Thanks. Dmitry. On Mon, Mar 11, 2013 at 10:35 AM, Dmitry Stogov wrote: > Hi Nikita, > > Thanks. I'll review it today. > > Dmitry. > > > On Sun, Mar 10, 2013 at 1:47 AM, Nikita Popov wrote: > >> On Wed, Mar 6, 2013 at 6:28 PM, Dmitry Stogov wrote: >> >>> I wonder what would be a good way to avoid allocating a temporary zval for the key and freeing it again. Do you think it would be okay to pass &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value can be written directly into it without doing extra allocs/frees? >>> I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't >>> be referenced or marked as a possible root of garbage. >>> I took only a very quick look over the patch and didn't understand all >>> the details, but probably it must be possible to copy iterator key into >>> TMP_VAR >>> and call copy_ctor(). >>> >>> Please, let me review the patch when it's ready (I won't be available on >>> March 8 and weekend). >>> >>> Thanks. Dmitry. >>> >> >> Here is the new patch: >> https://github.com/nikic/php-src/commit/a1bfc8105713eeb4e66e852b81884b567ad56020 >> It passes in the tmp_var in as a zval*, which can then be set using the >> ZVAL_* macros (basically the same way as it's done with return_value). This >> way we don't need any further zval allocs and frees. It also turned out >> that doing it this way is more convenient to use in the respective key >> handlers. >> >> Nikita >> > >
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
Hi Nikita, Thanks. I'll review it today. Dmitry. On Sun, Mar 10, 2013 at 1:47 AM, Nikita Popov wrote: > On Wed, Mar 6, 2013 at 6:28 PM, Dmitry Stogov wrote: > >> >>> I wonder what would be a good way to avoid allocating a temporary zval >>> for the key and freeing it again. Do you think it would be okay to pass >>> &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value >>> can be written directly into it without doing extra allocs/frees? >>> >>> >> I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't >> be referenced or marked as a possible root of garbage. >> I took only a very quick look over the patch and didn't understand all >> the details, but probably it must be possible to copy iterator key into >> TMP_VAR >> and call copy_ctor(). >> >> Please, let me review the patch when it's ready (I won't be available on >> March 8 and weekend). >> >> Thanks. Dmitry. >> > > Here is the new patch: > https://github.com/nikic/php-src/commit/a1bfc8105713eeb4e66e852b81884b567ad56020 > It passes in the tmp_var in as a zval*, which can then be set using the > ZVAL_* macros (basically the same way as it's done with return_value). This > way we don't need any further zval allocs and frees. It also turned out > that doing it this way is more convenient to use in the respective key > handlers. > > Nikita >
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Wed, Mar 6, 2013 at 6:28 PM, Dmitry Stogov wrote: > >> I wonder what would be a good way to avoid allocating a temporary zval >> for the key and freeing it again. Do you think it would be okay to pass >> &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value >> can be written directly into it without doing extra allocs/frees? >> >> > I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't be > referenced or marked as a possible root of garbage. > I took only a very quick look over the patch and didn't understand all the > details, but probably it must be possible to copy iterator key into TMP_VAR > and call copy_ctor(). > > Please, let me review the patch when it's ready (I won't be available on > March 8 and weekend). > > Thanks. Dmitry. > Here is the new patch: https://github.com/nikic/php-src/commit/a1bfc8105713eeb4e66e852b81884b567ad56020 It passes in the tmp_var in as a zval*, which can then be set using the ZVAL_* macros (basically the same way as it's done with return_value). This way we don't need any further zval allocs and frees. It also turned out that doing it this way is more convenient to use in the respective key handlers. Nikita
[PHP-DEV] 回复： [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
Hi Nikita, I got some test failure with the patch, one test failure and some memory leaks. see: https://gist.github.com/reeze/5101596 -- reeze | reeze.cn 已使用 Sparrow (http://www.sparrowmailapp.com/?sig) 在 2013年3月7日星期四，上午1:28，Dmitry Stogov 写道： > On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov (mailto:nikita@gmail.com)> wrote: > > > On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov > (mailto:dmi...@zend.com)> wrote: > > > > > Hi Nikita, > > > > > > few notes about the patch... > > > > > > - you may avoid estrndup() in zend_hash_current_key_zval_ex() for > > > interned strings. > > > > > > > > > Good idea, I'll do that. > > > > > > > > - I didn't completely get why did you change the "key" operand type from > > > IS_TMP_VAR to IS_VAR and how it affects performance > > > As I understood now you need to allocate new zval on each loop iteration > > > even for foreach over plain arrays. :( > > > > > > > > > Good point, I didn't consider the performance implication the type change > > would have. The intent behind that was to avoid copying the get_current_key > > zval into the temporary (and destroying it then), but I didn't consider how > > it affects normal arrays. This should be changed back to TMP_VAR. > > > > > It would be great. I can agree that new features may work slower, but > really don't like when they slowdown existing and mach more usual cases. > > > > > > I wonder what would be a good way to avoid allocating a temporary zval for > > the key and freeing it again. Do you think it would be okay to pass > > &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value > > can be written directly into it without doing extra allocs/frees? > > > > I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't be > referenced or marked as a possible root of garbage. > I took only a very quick look over the patch and didn't understand all the > details, but probably it must be possible to copy iterator key into TMP_VAR > and call copy_ctor(). > > Please, let me review the patch when it's ready (I won't be available on > March 8 and weekend). > > Thanks. Dmitry.
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov wrote: > On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov wrote: > >> Hi Nikita, >> >> few notes about the patch... >> >> - you may avoid estrndup() in zend_hash_current_key_zval_ex() for >> interned strings. >> > > Good idea, I'll do that. > >> >> - I didn't completely get why did you change the "key" operand type from >> IS_TMP_VAR to IS_VAR and how it affects performance >> As I understood now you need to allocate new zval on each loop iteration >> even for foreach over plain arrays. :( >> > > Good point, I didn't consider the performance implication the type change > would have. The intent behind that was to avoid copying the get_current_key > zval into the temporary (and destroying it then), but I didn't consider how > it affects normal arrays. This should be changed back to TMP_VAR. > It would be great. I can agree that new features may work slower, but really don't like when they slowdown existing and mach more usual cases. > > I wonder what would be a good way to avoid allocating a temporary zval for > the key and freeing it again. Do you think it would be okay to pass > &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value > can be written directly into it without doing extra allocs/frees? > > I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't be referenced or marked as a possible root of garbage. I took only a very quick look over the patch and didn't understand all the details, but probably it must be possible to copy iterator key into TMP_VAR and call copy_ctor(). Please, let me review the patch when it's ready (I won't be available on March 8 and weekend). Thanks. Dmitry.
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov wrote: > Hi Nikita, > > few notes about the patch... > > - you may avoid estrndup() in zend_hash_current_key_zval_ex() for interned > strings. > Good idea, I'll do that. > > - I didn't completely get why did you change the "key" operand type from > IS_TMP_VAR to IS_VAR and how it affects performance > As I understood now you need to allocate new zval on each loop iteration > even for foreach over plain arrays. :( > Good point, I didn't consider the performance implication the type change would have. The intent behind that was to avoid copying the get_current_key zval into the temporary (and destroying it then), but I didn't consider how it affects normal arrays. This should be changed back to TMP_VAR. I wonder what would be a good way to avoid allocating a temporary zval for the key and freeing it again. Do you think it would be okay to pass &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value can be written directly into it without doing extra allocs/frees? Nikita
Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
Hi Nikita, few notes about the patch... - you may avoid estrndup() in zend_hash_current_key_zval_ex() for interned strings. - I didn't completely get why did you change the "key" operand type from IS_TMP_VAR to IS_VAR and how it affects performance As I understood now you need to allocate new zval on each loop iteration even for foreach over plain arrays. :( Thanks. Dmitry. On Wed, Mar 6, 2013 at 7:27 PM, Nikita Popov wrote: > On Wed, Feb 27, 2013 at 8:33 PM, Nikita Popov > wrote: > > > Hi internals! > >interned strings > > I've opened the voting the the foreach-keys RFC: > > > > https://wiki.php.net/rfc/foreach-non-scalar-keys#vote > > > > The discussion for this RFC can be found here: > > http://markmail.org/message/rzoacqaesxbd67lu > > > > The RFC was accepted unanimously, with 21 votes in favor. I'll merge the > patch sometime soon. > > Thanks, > Nikita >
[PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
On Wed, Feb 27, 2013 at 8:33 PM, Nikita Popov wrote: > Hi internals! > > I've opened the voting the the foreach-keys RFC: > > https://wiki.php.net/rfc/foreach-non-scalar-keys#vote > > The discussion for this RFC can be found here: > http://markmail.org/message/rzoacqaesxbd67lu > The RFC was accepted unanimously, with 21 votes in favor. I'll merge the patch sometime soon. Thanks, Nikita
