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....@gmail.com> wrote: > On Tue, Jan 27, 2015 at 5:55 PM, Dmitry Stogov <dmi...@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 >
