On Jan 28, 2015 2: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
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
