FAQ
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

Search Discussions

  • Nikita Popov at Mar 6, 2013 at 3:27 pm

    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
  • Dmitry Stogov at Mar 6, 2013 at 4:41 pm
    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
  • Nikita Popov at Mar 6, 2013 at 5:16 pm

    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
  • Dmitry Stogov at Mar 6, 2013 at 5:28 pm

    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.
  • Reeze Xia at Mar 6, 2013 at 6:14 pm
    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.ppv@gmail.com)> wrote:
    On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov (mailto:dmitry@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.
  • Nikita Popov at Mar 9, 2013 at 9:48 pm
    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
  • Dmitry Stogov at Mar 11, 2013 at 6:35 am
    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
  • Dmitry Stogov at Mar 11, 2013 at 8:50 am
    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
  • Nikita Popov at Mar 11, 2013 at 6:27 pm

    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:

    <?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
  • Dmitry Stogov at Mar 11, 2013 at 7:42 pm

    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:

    <?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.
  • Nikita Popov at Mar 12, 2013 at 4:43 pm
    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:

    <?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
  • Dmitry Stogov at Mar 12, 2013 at 5:18 pm
    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:

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedFeb 27, '13 at 7:33p
activeMar 12, '13 at 5:18p
posts13
users3
websitephp.net

People

Translate

site design / logo © 2022 Grokbase