Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:81265
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.171 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLvLSK3m1PBfS_WY3dQsxbmBHCQ_X6_gh0ynCdOMSypySA@mail.gmail.com>
References: <20150122103807.DD8255F8EE@mx.zeyon.net>
	<CA+9eiLvLSK3m1PBfS_WY3dQsxbmBHCQ_X6_gh0ynCdOMSypySA@mail.gmail.com>
Date: Tue, 27 Jan 2015 23:36:24 +0100
Message-ID: <CAF+90c-Q4ynycLn1+uGHQTOu6KKuu45zu-1tbPgVuwKdb=Q7yQ@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.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=001a11c37c187fdc39050da9e285
Subject: Re: [PHP-DEV] Improvements to for-each implementation
From: nikita.ppv@gmail.com (Nikita Popov)

--001a11c37c187fdc39050da9e285
Content-Type: text/plain; charset=UTF-8

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

--001a11c37c187fdc39050da9e285--