Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:81385
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.220.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLtpJx3eDuMPYZpa2Yj2ExkMer56dMxqbhvp0h1xTPTc3A@mail.gmail.com>
References: <20150122103807.DD8255F8EE@mx.zeyon.net>
	<CA+9eiLvLSK3m1PBfS_WY3dQsxbmBHCQ_X6_gh0ynCdOMSypySA@mail.gmail.com>
	<CAF+90c-Q4ynycLn1+uGHQTOu6KKuu45zu-1tbPgVuwKdb=Q7yQ@mail.gmail.com>
	<CA+9eiLtpJx3eDuMPYZpa2Yj2ExkMer56dMxqbhvp0h1xTPTc3A@mail.gmail.com>
Date: Thu, 29 Jan 2015 23:30:31 +0400
Message-ID: <CA+9eiLvCX9pn++YooG8eJjXEfGyf+RTw_wxZYLdr+ot9=CJhpg@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>, 
	"internals@lists.php.net" <internals@lists.php.net>
Cc: Benjamin Coutu <ben.coutu@zeyos.com>, Xinchen Hui <xinchen.h@zend.com>
Content-Type: multipart/alternative; boundary=001a11c20e5273f503050dcf854e
Subject: Re: [PHP-DEV] Improvements to for-each implementation
From: dmitry@zend.com (Dmitry Stogov)

--001a11c20e5273f503050dcf854e
Content-Type: text/plain; charset=UTF-8

Hi,

I've solved most of the reported problems and started working on RFC.

Nikta, please take a look into:
https://wiki.php.net/rfc/php7_foreach
https://github.com/php/php-src/pull/1034
and especially commit 15a23b1

Only iteration by value over plain objects is not completely consistent now.
However, the implementation may have bugs and other problems (your code
review may be very helpful).
I'm still working...

Thanks. Dmitry.

On Wed, Jan 28, 2015 at 7:59 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> I've created a branch and pull request for easier collaboration and
> tracking.
>
> https://github.com/php/php-src/pull/1034
>
> It's the same diff I published yesterday. Nothing changed or added yet.
>
> Thanks. Dmitry.
>
> On Wed, Jan 28, 2015 at 1:36 AM, Nikita Popov <nikita.ppv@gmail.com>
> wrote:
>
>> On Tue, Jan 27, 2015 at 5:55 PM, Dmitry Stogov <dmitry@zend.com> wrote:
>>
>>> Hi,
>>>
>>> I'm working on a PoC, implementing the proposed behavior -
>>> https://gist.github.com/dstogov/a311e8b0b2cabee4dab4
>>>
>>> Only few PHPT tests had to be changed to confirm new behavior and
>>> actually they started to behave as HHVM.
>>> 2 tests are still unfixed XFAILed. Foreach by reference is still need to
>>> be improved to support different array modifications.
>>>
>>> The patch makes ~1% improvement on Wordpress-3.6 (saving duplication and
>>> destruction of 200 arrays on each request)
>>>
>>> Thanks. Dmitry.
>>>
>>
>> I quickly looked over the patch, some notes:
>>
>> * 171: Can directly use GET_OP1_ZVAL_PTR_DEREF
>> * For iterator failures (exception) you use FREE_OP1_IF_VAR(). Is this
>> enough? If the object is a TMP_VAR, don't we have to free it as well?
>> * For objects it will still be possible to modify the hashtable during
>> iteration even in the _R case, correct? I assume we just don't care about
>> this edge case.
>> * 315: In RESET_RW you now always create a reference for VAR|CV operands.
>> However for the get_iterator case we don't need this, right?
>> * 328: In the non VAR|CV case SEPARTE_ARRAY is not used. As we're going
>> to change the IAP, this is probably necessary in this case as well.
>> * For RW iterator failures FREE_OP1_VAR_PTR() is used. This probably
>> leaks in the TMP case. (Same for the "invalid argument" case.)
>>
>> What concerns me a bit is the new FETCH_RW implementation, because it no
>> longer checks the internal pointer against the external one. This
>> effectively means that foreach by reference behavior will be influenced a
>> lot by details of the hashtable implementation. An example:
>>
>> $array = [0, 1, 2, 3, 4, 5, 6, 7];
>> unset($array[0], $array[1], $array[2], $array[3]);
>> foreach ($array as &$ref) {
>>     var_dump($ref);
>>     //$array[42] = 42;
>> }
>>
>> Without the commented line this will just print the numbers 4, 5, 6, 7.
>> If you uncomment the line it will only print 4. (Because the append caused
>> a rehash and arData was compacted, so the position now points past all
>> elements). The previous output would have been 4, 5, 6, 7, 42, which is
>> closer to what you would expect. Maybe better to keep the pos !=
>> nInternalPointer code?
>>
>> Thanks,
>> Nikita
>>
>
>

--001a11c20e5273f503050dcf854e--