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
