Re: Apply substr() optimization to array_slice()

Yes, abstracting such additional checks through something like zend_hash_duplicate() would make sense, but IMHO it should be named differently, e.g. zend_hash_copy_lazy. So to be analogous to zend_string_copy(), but not to be confused with the existing zend_hash_copy(). By 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 well. - Ben - ========== Original ========== From: Sara Golemon <pollita@php.net> To: Nikita Popov <nikita.ppv@gmail.com> Date: Fri, 27 Oct 2017 18:34:15 +0200 Subject: Re: [PHP-DEV] Apply substr() optimization to array_slice() > > > 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