Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:37344
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.162 as permitted sender)
Message-ID: <48184609.5000804@zend.com>
Date: Wed, 30 Apr 2008 14:12:25 +0400
User-Agent: Thunderbird 2.0.0.12 (Windows/20080213)
MIME-Version: 1.0
To: Matt Wilmas <php_lists@realplain.com>
CC: internals@lists.php.net, Andi Gutmans <andi@zend.com>
References: <014b01c8a6db$57d53ba0$0201a8c0@pc1> <48181C02.3050705@zend.com> <00a701c8aaa9$e0fcd4c0$0201a8c0@pc1>
In-Reply-To: <00a701c8aaa9$e0fcd4c0$0201a8c0@pc1>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] More array filling optimizations
From: dmitry@zend.com (Dmitry Stogov)

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
>>>
>>>
>