[VOTE] Allow non-scalar keys in foreach

11 years ago by Nikita Popov — view source

unread

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

Thanks,
Nikita

11 years ago by Nikita Popov — view source

unread

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

11 years ago by Dmitry Stogov — view source

unread

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, Feb 27, 2013 at 8:33 PM, Nikita Popov nikita.ppv@gmail.com
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

11 years ago by Nikita Popov — view source

unread

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

11 years ago by Dmitry Stogov — view source

unread

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.

11 years ago by Reeze Xia — view source

unread

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 写道：

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.

11 years ago by Nikita Popov — view source

unread

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

11 years ago by Dmitry Stogov — view source

unread

Hi Nikita,

Thanks. I'll review it today.

Dmitry.

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

11 years ago by Dmitry Stogov — view source

unread

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.

Hi Nikita,

Thanks. I'll review it today.

Dmitry.

On Sun, Mar 10, 2013 at 1:47 AM, Nikita Popov nikita.ppv@gmail.comwrote:

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

11 years ago by Nikita Popov — view source

unread

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:

<?php
foreach ((object) ['x','y','z'] as $k => $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

11 years ago by Dmitry Stogov — view source

unread

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:

<?php
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.

11 years ago by Nikita Popov — view source

unread

On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov nikita.ppv@gmail.comwrote:

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:

<?php
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

11 years ago by Dmitry Stogov — view source

unread

Hi Nikita,

I suppose it must fine now, but let me take a quick look tomorrow morning.

Thanks. Dmitry.

On Mon, Mar 11, 2013 at 10:27 PM, Nikita Popov nikita.ppv@gmail.comwrote:

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:

<?php
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