[PATCH] More array filling optimizations

17 years ago by Matt Wilmas — view source

unread

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

17 years ago by Andrey Hristov — view source

unread

Hi,
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.

heh, this optimisitations was one of the pluses mysqlnd had over
mysqli/libmysql. I am using the following macro:

#if PHP_MAJOR_VERSION < 6
#define mysqlnd_array_init(arg, field_count)
{
ALLOC_HASHTABLE_REL(Z_ARRVAL_P(arg));
zend_hash_init(Z_ARRVAL_P(arg), (field_count), NULL, ZVAL_PTR_DTOR, 0);
Z_TYPE_P(arg) = IS_ARRAY;
}
#else
#define mysqlnd_array_init(arg, field_count)
{
ALLOC_HASHTABLE_REL(Z_ARRVAL_P(arg));
zend_u_hash_init(Z_ARRVAL_P(arg), (field_count), NULL, ZVAL_PTR_DTOR,
0, 0);
Z_TYPE_P(arg) = IS_ARRAY;
}
#endif

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

17 years ago by Matt Wilmas — view source

unread

Hi Andrey,

----- Original Message -----
From: "Andrey Hristov"
Sent: Friday, April 25, 2008

Hi,
[...]
heh, this optimisitations was one of the pluses mysqlnd had over
mysqli/libmysql. I am using the following macro:

#if PHP_MAJOR_VERSION < 6
#define mysqlnd_array_init(arg, field_count)
{
ALLOC_HASHTABLE_REL(Z_ARRVAL_P(arg));
zend_hash_init(Z_ARRVAL_P(arg), (field_count), NULL, ZVAL_PTR_DTOR, 0);
Z_TYPE_P(arg) = IS_ARRAY;
}
#else
#define mysqlnd_array_init(arg, field_count)
{
ALLOC_HASHTABLE_REL(Z_ARRVAL_P(arg));
zend_u_hash_init(Z_ARRVAL_P(arg), (field_count), NULL, ZVAL_PTR_DTOR,
0, 0);
Z_TYPE_P(arg) = IS_ARRAY;
}
#endif

Ah yes, I see. :-) Yeah, any of the database functions (in all DB exts.)
that create/return arrays are one of the obvious things I had in mind to
change, but hadn't come across your mysqlnd "workaround," heh. Just knew
that plain array_init() is used in mysql/mysqli, pgsql, sqlite, etc...

Matt

17 years ago by Dmitry Stogov — view source

unread

Hi Matt,

I like the idea (especially ZEND_INIT_ARRAY optimization) and from the
first look patch seems proper. I'll need to look into it more careful
before commit (probably on next week).

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

17 years ago by Dmitry Stogov — view source

unread

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

17 years ago by Matt Wilmas — view source

unread

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

17 years ago by Dmitry Stogov — view source

unread

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

17 years ago by Matt Wilmas — view source

unread

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...

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

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.

17 years ago by Dmitry Stogov — view source

unread

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.

17 years ago by Matt Wilmas — view source

unread

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.

17 years ago by Dmitry Stogov — view source

unread

Hi Matt,

Sorry, but I'm very busy in the following two weeks (have to travel a
lot). I'll try not to forget about you patch, but it would be better if
you remember me after June 7.

Thanks. Dmitry.

Matt Wilmas wrote:

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.

17 years ago by Nuno Lopes — view source

unread

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.

17 years ago by Dmitry Stogov — view source

unread

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.