Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:66498
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain zend.com does not designate 209.85.219.43 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAF+90c92AwjX7Niro_tT5vOv+vHeY-AmgfizL5f9i1FYhKTz2A@mail.gmail.com>
References: <CAF+90c-E7n-WZcqNg28bX+G7zgbzhKoNCwR6hf++tfAgSzmtTw@mail.gmail.com>
	<CAF+90c-wSqeff_2oY8HeY+gDn-ZnwJeXKQ+cz-1W6xcEBjs+xw@mail.gmail.com>
	<CA+9eiLtiOcFuxRXhpGXe5b9SpyVPB1_ixJ4t1GQ3J1bvBURMKA@mail.gmail.com>
	<CAF+90c92AwjX7Niro_tT5vOv+vHeY-AmgfizL5f9i1FYhKTz2A@mail.gmail.com>
Date: Wed, 6 Mar 2013 20:28:41 +0300
Message-ID: <CA+9eiLsWNV7H1zAr1U-wPafz9SzB=eR_C33RQKgqy1Tx1+X8gw@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>, Zeev Suraski <zeev@zend.com>, Andi Gutmans <andi@zend.com>
Content-Type: multipart/alternative; boundary=e89a8fb206f2de288504d744eb0a
Subject: Re: [PHP-DEV] Re: [VOTE] Allow non-scalar keys in foreach
From: dmitry@zend.com (Dmitry Stogov)

--e89a8fb206f2de288504d744eb0a
Content-Type: text/plain; charset=UTF-8

On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov <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.

--e89a8fb206f2de288504d744eb0a--