Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:100973
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zeyos.com designates 88.99.153.70 as permitted sender)
Date: Fri, 27 Oct 2017 23:11:20 +0200
To: Sara Golemon <pollita@php.net>,  	Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP Internals <internals@lists.php.net>,  	Dmitry Stogov <dmitry@zend.com>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Message-ID: <20171027211122.06BF55FAFE@mx.zeyos.com>
Subject: Re: [PHP-DEV] Apply substr() optimization to array_slice()
From: ben.coutu@zeyos.com (Benjamin Coutu)

Yes, abstracting such additional checks through something like zend_hash_du=
plicate() would make sense, but IMHO it should be named differently, e.g. z=
end_hash_copy_lazy. So to be analogous to zend_string_copy(), but not to be=
 confused with the existing zend_hash_copy().=0A=0ABy the way: array_pad() =
in L4320, array_unique() in L4476 and array_diff() in L5415 also return the=
 array without those kind of checks (analougous to what I have proposed for=
 the array_slice() case). These existing bugs would have to be fixed as wel=
l.=0A=0A- Ben -=0A=0A=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Original =3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=0AFrom: Sara Golemon <pollita@php.net>=0ATo: Nikita Popo=
v <nikita.ppv@gmail.com>=0ADate: Fri, 27 Oct 2017 18:34:15 +0200=0ASubject:=
 Re: [PHP-DEV] Apply substr() optimization to array_slice()=0A=0A>=20
>=20
> On Fri, Oct 27, 2017 at 12:10 PM, Nikita Popov <nikita.ppv@gmail.com> wro=
te:
> > 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 f=
orm,
> > because arrays have a bunch of additional hidden state. See
> > https://bugs.php.net/bug.php?id=3D75433 for a (not yet fixed) issue tha=
t
> > resulted from similar optimizations in 7.2. We'll have to review all th=
e
> > places where we apply optimizations like these and make sure that we're=
 not
> > introducing incorrect behavior wrt the next free element or internal ar=
ray
> > 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
>=20
> Basically, that next insert index for the output of array_values()
> should be reset, and with the optimization it's not.
>=20
> For array_values() the quick and dirty fix is adding "&& nextIndex =3D=3D
> 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.
>=20
> I'll clean up these uses now, and would suggest something like:
>=20
> 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().
>=20
> -Sara