# [RFC] Fix "foreach" behavior - Thread: https://externals.io/message/81406 - Original archive: https://news-web.php.net/php.internals/81406 - Started by Dmitry Stogov on 2015-01-30T07:23:19+00:00 - Messages: 11 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #81406 — Dmitry Stogov — 2015-01-30T07:23:19+00:00 URL: https://externals.io/message/81406#81406 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. --- ## #81420 — Julien Pauli — 2015-01-30T10:52:07+00:00 URL: https://externals.io/message/81406#81420 · In reply to: #81406 > 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 --- ## #81422 — Alain Williams — 2015-01-30T11:07:16+00:00 URL: https://externals.io/message/81406#81422 · In reply to: #81406 > 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 > > […] > statement > > […] > duplication. > > […] > fixing > > […] > > 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 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. --- ## #81472 — Rasmus Lerdorf — 2015-01-31T08:18:10+00:00 URL: https://externals.io/message/81406#81472 · In reply to: #81468 > 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 --- ## #81473 — Xinchen Hui — 2015-01-31T08:27:51+00:00 URL: https://externals.io/message/81406#81473 · In reply to: #81472 > > > […] > > 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/ --- ## #81485 — Nikita Popov — 2015-01-31T13:18:08+00:00 URL: https://externals.io/message/81406#81485 · In reply to: #81406 > 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 --- ## #81517 — Dmitry Stogov — 2015-02-01T03:32:32+00:00 URL: https://externals.io/message/81406#81517 · In reply to: #81472 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. > > > […] > > 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 --- ## #81519 — Dmitry Stogov — 2015-02-01T03:39:34+00:00 URL: https://externals.io/message/81406#81519 · In reply to: #81485 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. > > > > […] > > 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 --- ## #81529 — Lester Caine — 2015-02-01T12:34:43+00:00 URL: https://externals.io/message/81406#81529 · In reply to: #81519 > 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