Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:100970
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain golemon.com does not designate 209.85.216.176 as permitted sender)
MIME-Version: 1.0
Sender: php@golemon.com
In-Reply-To: <CAF+90c_xYPvE72s7pSnKdmrMcUOCTsjyPfr2VoM9t47utbG-EA@mail.gmail.com>
References: <20171027121225.2AAC95FB05@mx.zeyos.com> <CAESVnVrbEYuFLAPFrkssL08KrEFrGyciQwPemaU06SHieT60=w@mail.gmail.com>
 <CAF+90c_xYPvE72s7pSnKdmrMcUOCTsjyPfr2VoM9t47utbG-EA@mail.gmail.com>
Date: Fri, 27 Oct 2017 12:34:15 -0400
Message-ID: <CAESVnVrGUAmE2HD7M=R3vbora4S8-65FtEV0XrfrKBTsiOn--Q@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: Benjamin Coutu <ben.coutu@zeyos.com>, PHP Internals <internals@lists.php.net>, 
	Dmitry Stogov <dmitry@zend.com>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Apply substr() optimization to array_slice()
From: pollita@php.net (Sara Golemon)

On Fri, Oct 27, 2017 at 12:10 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
> On Fri, Oct 27, 2017 at 4:16 PM, Sara Golemon <pollita@php.net> wrote:
>>
>> On Fri, Oct 27, 2017 at 8:12 AM, Benjamin Coutu <ben.coutu@zeyos.com>
>> wrote:
>> > Now, array_slice() could be optimized similarly, but currently
>> > (unless the resulting array is expected to be empty) we always
>> > create a new array no matter if we actually have to.
>> >
>> Pushed, with an additional escape hatch for vector-like arrays (which
>> are implicitly like preserve_keys).  In the future though, please use
>> the PR process. Thanks.
>
>
> Unfortunately these optimizations are subtly incorrect in the current form,
> because arrays have a bunch of additional hidden state. See
> https://bugs.php.net/bug.php?id=75433 for a (not yet fixed) issue that
> resulted from similar optimizations in 7.2. We'll have to review all the
> places where we apply optimizations like these and make sure that we're not
> introducing incorrect behavior wrt the next free element or internal array
> pointer.
>
It took awhile to unwind the repro case given in the bug report, but
this seems to be a simpler example:
https://3v4l.org/rqphD

Basically, that next insert index for the output of array_values()
should be reset, and with the optimization it's not.

For array_values() the quick and dirty fix is adding "&& nextIndex ==
num_elements" psuedocode.  In the case of array_slice, the same will
be true, so I agree we should be careful about applying such
optimizations.

I'll clean up these uses now, and would suggest something like:

zend_array* zend_hash_duplicate(zend_array* input, zend_bool
preserve_keys); type API which can be a certral place for making that
kind of short-circuit versus slow-copy decision when called from
places like array_values() and array_slice().

-Sara