Re: [PATCH] More array filling optimizations

For some reason "make test" with the patch reported several broken array_splice() tests. Looking in gdb I saw that init_array() got -1 as a size of new array. I don't think checks for return_value_used for array_splice() have a lot of sense. Thanks. Dmitry. Matt Wilmas wrote:

Hi Dmitry, Yes, I noticed similar results with the compile/vm changes, and was also thinking that it probably wasn't worth doing, so I just left it as a separate patch in case someone else wanted to look into it. :-) So I guess the rest is OK other than array_splice() tests breaking... I didn't know the change to splice() would modify any behavior? I just added a check "if (return_value_used)" to see whether or not to create and return the array of removed elements -- like is done in next/prev/end/reset functions. I thought it would only behave different (internally) when array_splice() is used in a "void context": array_splice(...); // Not used in assignment or function call And the calling code wouldn't be affected? Maybe I don't get it, and that check should be removed. :-) Thanks for your feedback, Matt ----- Original Message ----- From: "Dmitry Stogov" Sent: Wednesday, April 30, 2008

Hi Matt, I've made a review of your patch. At first it has a bug at least in array_psplice() that makes several tests to fail. So I removed the whole ext/standard part and tested only Zend Engine changes. Although the idea is interesting and implementation is perfect, the patch doesn't make any speed improvement on real-life applications (according to callgrind the patch can make 0.2% speedup, but callgrind measure instruction fetches and not the speed itself) and showed small slowdown on bench.php (the slowdown may be not the result of the patch but result of worse code-locality caused by the patch). I think we shouldn't make PHP more complicated without significant result, so I suggest not to apply array_init_size_vm.diff part of the

patch.

Thanks. Dmitry. Matt Wilmas wrote:

Hi all, Expanding on the idea of passing a size other than 0 to

zend_hash_init(),

when possible, which was done awhile ago in a few areas (to save resize/rehash operations), I finally added an "array_init_size()" that

can

be used instead of array_init(), likewise, when a size is known. As an example to start, I updated these functions: array_*: change_key_case, chunk, combine, fill, fill_keys, flip, keys (without $search_value), map, rand, reverse, slice, splice, unique

(since it

copies all entries first), values; and also compact; func_get_args; get_defined_vars; str_split; and a couple internal uses. Those were the simplest, most obvious cases I found. :-) I didn't test every function, but for example, the array functions that don't do much

work

I found to be ~10% faster (with more than 8 elements, otherwise no

resizing

would be needed). If these changes are applied, more can be done where possible, by others, or I can and send further patches, or update them myself if I have CVS access... http://realplain.com/php/array_init_size.diff http://realplain.com/php/array_init_size_5_3.diff I was playing with initializing scripts' array( ... ) constructs with

the

correct size too, but wasn't sure about the changes or what the opinion would be. Runtime creation is improved with >8 elements, though it adds

a

bit of compile overhead. Anyway, separate patches for that: http://realplain.com/php/array_init_size_vm.diff http://realplain.com/php/array_init_size_vm_5_3.diff Thanks, Matt