Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:37901
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: <483AFF78.5080107@zend.com>
Date: Mon, 26 May 2008 22:20:40 +0400
User-Agent: Thunderbird 2.0.0.14 (X11/20080501)
MIME-Version: 1.0
To: Nuno Lopes <nlopess@php.net>
CC: Matt Wilmas <php_lists@realplain.com>, internals@lists.php.net
References: <014b01c8a6db$57d53ba0$0201a8c0@pc1> <48181C02.3050705@zend.com> <00a701c8aaa9$e0fcd4c0$0201a8c0@pc1> <48184609.5000804@zend.com> <00cf01c8aab0$0be2fb50$0201a8c0@pc1> <48185599.1030308@zend.com> <00a901c8bf1a$b46e6dd0$0201a8c0@pc1> <A028FF3C79EE42A88694BCE31CA00A25@pc07654>
In-Reply-To: <A028FF3C79EE42A88694BCE31CA00A25@pc07654>
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)

I have no objections about this patch if it breaks nothing.
Double check it and commit.

Thanks. Dmitry.

Nuno Lopes wrote:
> Hi,
> 
> This patch looks pretty good. I didn't check all the init sizes, but 
> seem ok. Just go ahead and commit!
> Note to Dmitry: this patch doesn't have the VM part that you didn't like 
> in the previous patch (didn't perform very well..). This just touches 
> the core and it seems ok.
> 
> Nuno
> 
> 
> ----- Original Message ----- From: "Matt Wilmas" <php_lists@realplain.com>
> To: <internals@lists.php.net>; "Dmitry Stogov" <dmitry@zend.com>
> Sent: Monday, May 26, 2008 11:22 AM
> Subject: Re: [PHP-DEV] [PATCH] More array filling optimizations
> 
> 
>> Hi Dmitry, all,
>>
>> I looked into and fixed the -1 size and memory overflow error a couple 
>> weeks
>> or so ago, and finally updated the patches now. :-)  I made the 
>> mistake of
>> assuming the code that checks offset/length (in array_splice, etc.) set
>> length correctly, but it can still be negative, which broke
>> array_init_size...  Should be OK now there, and also in array_slice and
>> array_chunk.
>>
>> So if it works correctly now, it sounded like the patch could be used?
>> (This doesn't have the compile/VM part, of course.)  I would be able to
>> commit these things myself now, but wouldn't unless I was given the OK
>> first! :-)
>>
>> http://realplain.com/php/array_init_size.diff
>> http://realplain.com/php/array_init_size_5_3.diff
>>
>>
>> Thanks,
>> Matt
>>
>>
>> ----- Original Message -----
>> From: "Dmitry Stogov"
>> Sent: Wednesday, April 30, 2008
>>
>>>
>>> Matt Wilmas wrote:
>>> > Hi again Dmitry,
>>> >
>>> > Hmm, if a -1 size was passed, I thought it would just make a huge 
>>> array
>> size
>>> > (bad, obviously), but the tests would still work.  (Or I guess it 
>>> might
>>> > exhaust memory_limit.)  Anyway, the code for checking the size (length
>>> > variable) in array_splice() is copied from php_splice(), so maybe
>> there's
>>> > some other error if it's becoming -1...
>>>
>>> The bug causes memory overflow error, but the size definitely shouldn't
>>> be -1, as the same tests work fine before the patch.
>>>
>>> > You don't think it makes sense to check return_value_used like
>>> > next/prev/end/reset do?  They would only create a 1 element array, but
>>> > array_splice() could waste time filling a large array that isn't used.
>> I
>>> > think the function is used a lot to only remove/replace elements, not
>> using
>>> > the return_value:
>>> > http://www.google.com/codesearch?q=lang%3Aphp+array_splice
>>>
>>> Ah. I see. My mistake. Of course it makes sense.
>>>
>>> Thanks. Dmitry.
>>>
>>> > If there's a -1 size bug, the array_splice changes can be reverted and
>>> > simply have:
>>> >
>>> > if (return_value_used) {
>>> >     /* Initialize return value */
>>> >     array_init(return_value);
>>> >     rem_hash = &Z_ARRVAL_P(return_value);
>>> > }
>>> >
>>> > - Matt
>>> >
>>> >
>>> > ----- Original Message -----
>>> > From: "Dmitry Stogov"
>>> > Sent: Wednesday, April 30, 2008
>>> >
>>> >> 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. 
>