Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:37900
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 212.55.154.26 as permitted sender)
Message-ID: <A028FF3C79EE42A88694BCE31CA00A25@pc07654>
To: "Matt Wilmas" <php_lists@realplain.com>,
	<internals@lists.php.net>,
	"Dmitry Stogov" <dmitry@zend.com>
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>
In-Reply-To: <00a901c8bf1a$b46e6dd0$0201a8c0@pc1>
Date: Mon, 26 May 2008 16:20:30 +0100
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="iso-8859-1";
	reply-type=original
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] More array filling optimizations
From: nlopess@php.net ("Nuno Lopes")

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.