# Improvements to for-each implementation - Thread: https://externals.io/message/80969 - Original archive: https://news-web.php.net/php.internals/80969 - Started by Benjamin Coutu on 2015-01-22T10:38:04+00:00 - Messages: 12 - 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 --- ## #80969 — Benjamin Coutu — 2015-01-22T10:38:04+00:00 URL: https://externals.io/message/80969#80969 Hi Nikita, I would suggest using the proposal for the by-value case and sticking with the current behavior for the by-reference case as you suggested. Granted, we then cannot remove the internal pointer all together, but we would just use it for the less common by-reference case as well as for the legacy reset/next functions, of course. This would still improve most for-each traversals. At least we then wouldn't have to find a replacement solution for rest/prev/next/end. Moreover we can then move the internal position pointer out of the hashtable structure into zend_array because it is only used for userland arrays, not other hash tables (such as object properties or internal structures that build upon the hashtable struct). Thanks, Ben ========== Original ========== From: Nikita Popov To: Benjamin Coutu Date: Thu, 22 Jan 2015 10:53:19 +0100 Subject: Re: [PHP-DEV] Improvements to for-each implementation Doing this was the idea I had in mind as well, i.e. change the semantics of foreach to say that it will always work on a copy for by-value iteration (which ironically avoids having to actually copy it). Note that this will differ from the current behavior in a number of ways. In particular it means that changes to arrays that were references prior to iteration will not influence the iteration. The real question is what we should do in the by-reference case. Given that we need to acquire references to elements of the original array we can't reasonably work with copy-semantics (at least I don't see how). So would we just stick with the previous behavior (using the hash position hack) for that? Nikita --- ## #81249 — Dmitry Stogov — 2015-01-27T16:55:44+00:00 URL: https://externals.io/message/80969#81249 · In reply to: #80969 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. > Hi Nikita, > > I would suggest using the proposal for the by-value case and sticking with > the current behavior for the by-reference case as you suggested. Granted, > we then cannot remove the internal pointer all together, but we would just > use it for the less common by-reference case as well as for the legacy > reset/next functions, of course. This would still improve most for-each > traversals. At least we then wouldn't have to find a replacement solution > for rest/prev/next/end. Moreover we can then move the internal position > pointer out of the hashtable structure into zend_array because it is only > used for userland arrays, not other hash tables (such as object properties > or internal structures that build upon the hashtable struct). > > Thanks, > > Ben > > ========== Original ========== > From: Nikita Popov > To: Benjamin Coutu > Date: Thu, 22 Jan 2015 10:53:19 +0100 > Subject: Re: [PHP-DEV] Improvements to for-each implementation > > Doing this was the idea I had in mind as well, i.e. change the semantics of > foreach to say that it will always work on a copy for by-value iteration > (which ironically avoids having to actually copy it). Note that this will > differ from the current behavior in a number of ways. In particular it > means that changes to arrays that were references prior to iteration will > not influence the iteration. > > The real question is what we should do in the by-reference case. Given that > we need to acquire references to elements of the original array we can't > reasonably work with copy-semantics (at least I don't see how). So would we > just stick with the previous behavior (using the hash position hack) for > that? > > Nikita --- ## #81251 — Rasmus Lerdorf — 2015-01-27T18:14:02+00:00 URL: https://externals.io/message/80969#81251 · In reply to: #81249 > The patch makes ~1% improvement on Wordpress-3.6 (saving duplication and > destruction of 200 arrays on each request) Awesome! --- ## #81253 — Yasuo Ohgaki — 2015-01-27T18:48:46+00:00 URL: https://externals.io/message/80969#81253 · In reply to: #81249 Hi Dmitry, > 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) > Great! -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81265 — Nikita Popov — 2015-01-27T22:36:24+00:00 URL: https://externals.io/message/80969#81265 · In reply to: #81249 > 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 --- ## #81273 — Dmitry Stogov — 2015-01-28T01:54:10+00:00 URL: https://externals.io/message/80969#81273 · In reply to: #81265 > > > > […] https://gist.github.com/dstogov/a311e8b0b2cabee4dab4 > > […] actually they started to behave as HHVM. > > […] be improved to support different array modifications. > > […] destruction of 200 arrays on each request) > > […] > > > I quickly looked over the patch, some notes: > > * 171: Can directly use GET_OP1_ZVAL_PTR_DEREF Right. > * 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 IS_TMP_VAR I try to avoid refcounter incrementation/decrementation. So the OP1 zval has to be freed in ZEND_FREE or in HANDLE_EXCEPTION. Hawever, I'm not 100% sure. It need to be checked more careful. > * 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. I think you are right. I missed this and it may make us problems :( > * 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? Right. Good catch. > * 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. In this case SEPARATE_ARRAY is done in FE_FETCH_RW. > * For RW iterator failures FREE_OP1_VAR_PTR() is used. This probably leaks in the TMP case. (Same for the "invalid argument" case.) The same as above, but needs to be checked. > > 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? Yes. This is the main problem, however, the old implementation wasn't good enough as well. It caught many (but not all cases). It's the reason why we had few tests XFAILEd. This need to be solved in some other way, similar to your old proposal but in more efficient way. Thanks for review, it's very useful. I'll probably able to provide next version of PoC in a couple of days. Dmitry. > > Thanks, > Nikita --- ## #81275 — Xinchen Hui — 2015-01-28T03:22:38+00:00 URL: https://externals.io/message/80969#81275 · In reply to: #81249 Hey: > 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) I just have got an idea: Droping use of ZEND_OP_DATA make it possible to generate better foreach loop opcodes previously FE_RESET and FE_FETCH share one same OP_DATA based on offset. 0 FE_RESET 1 FE_FETCH fail-> 5 2 OP_DATA 3 ..... statements 4 JMP 1 5.... without depending on that OP_DATA, we can change this to 0 FE_RESET 1 JMP 3 2 ....statements 3 FE_FETCH success->2 4 .... that will reduce on JMP in every foreach loop could you please also try this? thanks > > Thanks. Dmitry. > > > > > > […] > -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #81276 — Xinchen Hui — 2015-01-28T03:27:19+00:00 URL: https://externals.io/message/80969#81276 · In reply to: #81275 Hey: > Hey: > > > > […] > > I just have got an idea: > > Droping use of ZEND_OP_DATA make it possible to generate better > foreach loop opcodes > > previously FE_RESET and FE_FETCH share one same OP_DATA based on offset. > > 0 FE_RESET > 1 FE_FETCH fail-> 5 > 2 OP_DATA > 3 ..... statements > 4 JMP 1 > 5.... > > without depending on that OP_DATA, we can change this to > > 0 FE_RESET > 1 JMP 3 > 2 ....statements > 3 FE_FETCH success->2 > 4 .... > > that will reduce on JMP in every foreach loop > > could you please also try this? I mean, dropping both ZEND_FE_RESET and FE_FETCH share one ZEND_OP_DATA. the previously example should be: > 0 FE_RESET > 1 JMP 3 > 2 ....statements > 3 FE_FETCH success->2 4 ZEND_OP_DATA > 5 .... thanks > > thanks > > > […] > > > > -- > Xinchen Hui > @Laruence > http://www.laruence.com/ -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #81278 — Dmitry Stogov — 2015-01-28T03:47:05+00:00 URL: https://externals.io/message/80969#81278 · In reply to: #81275 Hi, I have this in mind. May be It's even possible to remove JMP. FE_RESET op1= op2=