Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:81280
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.220.176 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAF+90c-Q4ynycLn1+uGHQTOu6KKuu45zu-1tbPgVuwKdb=Q7yQ@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>
Date: Wed, 28 Jan 2015 08:59:16 +0400
Message-ID: <CA+9eiLtpJx3eDuMPYZpa2Yj2ExkMer56dMxqbhvp0h1xTPTc3A@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: Benjamin Coutu <ben.coutu@zeyos.com>, Xinchen Hui <xinchen.h@zend.com>, 
	"internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=bcaec51d2104bed518050daf3b9d
Subject: Re: [PHP-DEV] Improvements to for-each implementation
From: dmitry@zend.com (Dmitry Stogov)

--bcaec51d2104bed518050daf3b9d
Content-Type: text/plain; charset=UTF-8

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
>

--bcaec51d2104bed518050daf3b9d--