Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Fix "foreach" behavior »

[RFC] Fix "foreach" behavior

php.internals

Dmitry Stogov

11 years ago

Hi,

I'd like to start discussion about fixing inconsistent "foreach" statement
behavior.
The implementation is almost done. It not only fixes inconsistencies in a
defined by RFC way but also improves performance in most usual cases,
because now foreach by value over array doesn't require array duplication.

https://wiki.php.net/rfc/php7_foreach

https://github.com/php/php-src/pull/1034

I'm going to continue work on implementation improvement and may be fixing
related behavior of some internal functions when they used on array
iterated by foreach by reference (e.g. array_unshift()). However, the
conceptual behavior defined in RFC is not going to be changed anymore.

Thanks. Dmitry.

Julien Pauli

11 years ago

On Fri, Jan 30, 2015 at 8:23 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi,
>
> I'd like to start discussion about fixing inconsistent "foreach" statement
> behavior.
> The implementation is almost done. It not only fixes inconsistencies in a
> defined by RFC way but also improves performance in most usual cases,
> because now foreach by value over array doesn't require array duplication.
>
> https://wiki.php.net/rfc/php7_foreach
>
> https://github.com/php/php-src/pull/1034
>
> I'm going to continue work on implementation improvement and may be fixing
> related behavior of some internal functions when they used on array
> iterated by foreach by reference (e.g. array_unshift()). However, the
> conceptual behavior defined in RFC is not going to be changed anymore.
>
> Thanks. Dmitry.
>

Nice job Dmitry, as usual !

Julien

Alain Williams

11 years ago

On Fri, Jan 30, 2015 at 11:23:19AM +0400, Dmitry Stogov wrote:

> Hi,
>
> I'd like to start discussion about fixing inconsistent "foreach" statement
> behavior.
> The implementation is almost done. It not only fixes inconsistencies in a
> defined by RFC way but also improves performance in most usual cases,
> because now foreach by value over array doesn't require array duplication.
>
> https://wiki.php.net/rfc/php7_foreach
>
> https://github.com/php/php-src/pull/1034
>
> I'm going to continue work on implementation improvement and may be fixing
> related behavior of some internal functions when they used on array
> iterated by foreach by reference (e.g. array_unshift()). However, the
> conceptual behavior defined in RFC is not going to be changed anymore.

I think that I am raising a concern that is not real, but I'll do so anyway.

You write:

This will lead to copy-on-write on attempt of array modification inside the
loop.  As result, we will always iterate over elements of array originally
passed to foreach, ignoring any possible array changes.

I think that you mean modification of the array and that this does not relate to
modification of array elements. Ie will code like the following still work:

foreach($_POST as &$v)
$v = trim($v);

Otherwise - good.

--
Alain Williams
Linux/GNU Consultant - Mail systems, Web sites, Networking, Programmer, IT Lecturer.
+44 (0) 787 668 0256  http://www.phcomp.co.uk/
Parliament Hill Computers Ltd. Registration Information: http://www.phcomp.co.uk/contact.php
#include <std_disclaimer.h>

Dmitry Stogov

11 years ago

That sentence was about "foreach by value".

"foreach by reference" in your example will still work as expected :)

Thanks. Dmitry.

On Fri, Jan 30, 2015 at 2:07 PM, Alain Williams <addw@phcomp.co.uk> wrote:

Dmitry Stogov

11 years ago

Hi,

Nikita got an idea about stopping changing of array/object internal
pointers even in forech by reference.
I've added an additional RFC section:

https://wiki.php.net/rfc/php7_foreach#additional_behavoir_change

Trivial implementation on top of the main patch:

https://gist.github.com/dstogov/63b269207ba0aed8b776

and an additional proposed voting question.

Also, the implementation now must be almost complete.

Thanks. Dmitry.

On Fri, Jan 30, 2015 at 10:23 AM, Dmitry Stogov <dmitry@zend.com> wrote:

Rasmus Lerdorf

11 years ago

On 01/30/2015 09:10 PM, Dmitry Stogov wrote:

> Hi,
>
> Nikita got an idea about stopping changing of array/object internal
> pointers even in forech by reference.
> I've added an additional RFC section:
>
> https://wiki.php.net/rfc/php7_foreach#additional_behavoir_change
>
> Trivial implementation on top of the main patch:
>
> https://gist.github.com/dstogov/63b269207ba0aed8b776
>
> and an additional proposed voting question.
>
> Also, the implementation now must be almost complete.

I built your branch and applied Nikita's patch as well and ran it
through my benchmark box here. Before -> After numbers in requests/sec:

Drupal8-b4        182 ->  184
Wordpress-4.1     661 ->  675
phpbb-3.1.2      1071 -> 1084
Mediawiki-1.24.1  304 ->  306
Opencart-2.0.1.1  458 ->  459
WardrobeCMS-1.2.0 993 ->  999
Geeklog-2.1.0     823 ->  825
Traq-3.5.2       1869 -> 1902

So about a 1% boost across the board except for Opencart, but it spends
most of its time creating sessions on disk and writing to MySQL.

No obvious visible things broke in any of these. Overall, even without
the performance boost, I think making the edge-case foreach behaviour
consistent and predictable is a good thing on its own.

-Rasmus

Laruence

11 years ago

On Sat, Jan 31, 2015 at 4:18 PM, Rasmus Lerdorf <rasmus@lerdorf.com> wrote:

> On 01/30/2015 09:10 PM, Dmitry Stogov wrote:
>> Hi,
>>
>> Nikita got an idea about stopping changing of array/object internal
>> pointers even in forech by reference.
>> I've added an additional RFC section:
>>
>> https://wiki.php.net/rfc/php7_foreach#additional_behavoir_change
>>
>> Trivial implementation on top of the main patch:
>>
>> https://gist.github.com/dstogov/63b269207ba0aed8b776
>>
>> and an additional proposed voting question.
>>
>> Also, the implementation now must be almost complete.
>
> I built your branch and applied Nikita's patch as well and ran it
> through my benchmark box here. Before -> After numbers in requests/sec:
>
> Drupal8-b4        182 ->  184
> Wordpress-4.1     661 ->  675
> phpbb-3.1.2      1071 -> 1084
> Mediawiki-1.24.1  304 ->  306
> Opencart-2.0.1.1  458 ->  459
> WardrobeCMS-1.2.0 993 ->  999
> Geeklog-2.1.0     823 ->  825
> Traq-3.5.2       1869 -> 1902
>
> So about a 1% boost across the board except for Opencart, but it spends
> most of its time creating sessions on disk and writing to MySQL.

Nice~

thanks

>
> No obvious visible things broke in any of these. Overall, even without
> the performance boost, I think making the edge-case foreach behaviour
> consistent and predictable is a good thing on its own.
>
> -Rasmus
>

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Dmitry Stogov

11 years ago

Thanks for benchmarks. This time I didn't see so good results myself :)
I actually, started this work with performance in mind, but after
understanding all existing incosistencies, think that consistency is more
important.

Thanks. Dmitry.
On Jan 31, 2015 11:18 AM, "Rasmus Lerdorf" <rasmus@lerdorf.com> wrote:

Nikita Popov

11 years ago

On Fri, Jan 30, 2015 at 8:23 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> Hi,
>
> I'd like to start discussion about fixing inconsistent "foreach" statement
> behavior.
> The implementation is almost done. It not only fixes inconsistencies in a
> defined by RFC way but also improves performance in most usual cases,
> because now foreach by value over array doesn't require array duplication.
>
> https://wiki.php.net/rfc/php7_foreach
>
> https://github.com/php/php-src/pull/1034
>
> I'm going to continue work on implementation improvement and may be fixing
> related behavior of some internal functions when they used on array
> iterated by foreach by reference (e.g. array_unshift()). However, the
> conceptual behavior defined in RFC is not going to be changed anymore.
>
> Thanks. Dmitry.
>

I think the new behavior introduced by this RFC is much more consistent
than what we have right now. Previously things depended on details of
refcounting and hashing and had very weird interaction with current() etc.
Now the behavior looks clear for all cases.

There's just one thing I'd like to discuss: The object by-value case:

Currently this is handled in much the same way as array by-reference, i.e.
changes to the object will influence iteration. Currently we can't
implement this any differently, however if we always use zend_array instead
of HashTable (and fully support refcounting for this), as has been
suggested in another thread, we could handle this case the same way as
arrays (i.e. just add a refcount to the properties zend_array). Assuming we
can implement it this way, I'm not totally sure which behavior makes more
sense. On one hand it would make arrays and plain objects work the same
way. On the other hand arrays and objects have different copying semantics
in other places as well.

Anyway, big +1 on this, thanks for implementing it.

Nikita

Dmitry Stogov

11 years ago

I also thought about similar approach for "object by value case".
I think, first we should accept the proposed in thic RFC behavior. It's
consistent, and more comatible with PHP5. Later, if we decide to merge
zend_array with HashTable we may try to change it.

Thanks. Dmitry.
On Jan 31, 2015 4:18 PM, "Nikita Popov" <nikita.ppv@gmail.com> wrote:

Lester Caine

11 years ago

On 01/02/15 03:39, Dmitry Stogov wrote:

> I also thought about similar approach for "object by value case".
> I think, first we should accept the proposed in thic RFC behavior. It's
> consistent, and more comatible with PHP5. Later, if we decide to merge
> zend_array with HashTable we may try to change it.

I know I'll get told off for getting off topic, but I think that often a
clean overview helps better understanding of fine detail which currently
goes over my head and I'm sure others as well.

Pre-OO days we created arrays of 'objects' which were and still are
simply organised sets of data. A set of static functions would be called
against a list of 'objects' using foreach to loop through all the
elements. Data would be organised to allow the best performance and
packets of data were passed via reference.

Nowadays we replace the 'set of static functions' with a class and the
data is wrapped up within that object, but IS that really the best way
of handling a list of objects? There is nothing stopping our using a
class to wrap an array of data sets and if the persistent data has been
streamed from a database is it REALLY sensible to build individual OO
objects for each?

With all of the complexity being loaded into PHP7, it just seems that
the main target areas that the majority of users need has been lost.
Fine tuning just how things like foreach works is fine, but has some of
the need been create by having lost site of the simpler functionality
which is all most users require?

--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk