Re: 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