Improved performance of array_maerge() and func_get_args()

12 years ago by Nikita Popov — view source — reply

unread

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

No objection, just quick note on func_get_args patch: You can use
SEPARATE_ARG_IF_REF there, so the code in the loop body is:

zval *arg = *(p-(arg_count-i));
SEPARATE_ARG_IF_REF(arg);
zend_hash_next_index_insert(Z_ARRVAL_P(return_value), &arg, sizeof(zval

*), NULL);

Nikita

12 years ago by Dmitry Stogov — view source — reply

unread

makes sense. thanks :)

Dmitry.

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

No objection, just quick note on func_get_args patch: You can use
SEPARATE_ARG_IF_REF there, so the code in the loop body is:
zval *arg = *(p-(arg_count-i));
SEPARATE_ARG_IF_REF(arg);
zend_hash_next_index_insert(Z_ARRVAL_P(return_value), &arg,
sizeof(zval *), NULL);

Nikita

12 years ago by Levi Morrison — view source — reply

unread

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

Sounds great. Thank you for looking into this. Just to clarify: the
wordpress improvements were from both patches, yes?

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

I don't have an objection; I just want to reiterate that changing code can
introduce BC breaks and to be careful when backporting it. Just be careful.

Again, good work.

12 years ago by Dmitry Stogov — view source — reply

unread

On Wed, Oct 23, 2013 at 6:21 PM, Levi Morrison morrison.levi@gmail.comwrote:

I'm proposing two simple patches that eliminate a lot of useless zval

copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

Sounds great. Thank you for looking into this. Just to clarify: the
wordpress improvements were from both patches, yes?

Yes, both patches together.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

I don't have an objection; I just want to reiterate that changing code can
introduce BC breaks and to be careful when backporting it. Just be careful.

Of course we checked them careful for possible BC breaks, but we may miss
some special cases.

Thanks. Dmitry.

12 years ago by Patrick ALLAERT — view source — reply

unread

2013/10/23 Dmitry Stogov dmitry@zend.com:

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

That sounds really good.

Note that in many performance investigations I made, array_merge() was
often one of the cause of some unnecessary overhead, however, most of
the times this function was used was in fact to merge arrays that were
in fact hashes (dictionary, map,...) and not really arrays. In
those cases, the performance fix is simply to use "$y + $x" instead of
"array_merge($x, $y).

Are all those array_merge() of wordpress really made to merge arrays
rather than hashes?

Is the gain constant for all cases of array_merge(), with numeric
indices vs string ones?

Patrick

12 years ago by Dmitry Stogov — view source — reply

unread

No. the array_merge() patch doesn't change the algorithm.
We just discovered that array_merge() makes a copy of each element of each
array before the actual merging, and in case it's an array or object, it
may take significant time, but this copying is absolutely useless. The
patch just remove it.

Thanks. Dmitry.

On Wed, Oct 23, 2013 at 10:10 PM, Patrick ALLAERT patrickallaert@php.netwrote:

2013/10/23 Dmitry Stogov dmitry@zend.com:

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

That sounds really good.

Note that in many performance investigations I made, array_merge() was
often one of the cause of some unnecessary overhead, however, most of
the times this function was used was in fact to merge arrays that were
in fact hashes (dictionary, map,...) and not really arrays. In
those cases, the performance fix is simply to use "$y + $x" instead of
"array_merge($x, $y).

Are all those array_merge() of wordpress really made to merge arrays
rather than hashes?

Is the gain constant for all cases of array_merge(), with numeric
indices vs string ones?

Patrick

12 years ago by Christopher Jones — view source — reply

unread

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the result was
a performance regression [2].

Chris

References:

[1] http://marc.info/?l=php-internals&m=135820887521861

[2] http://marc.info/?l=php-internals&m=137037681223946

--
christopher.jones@oracle.com http://twitter.com/ghrd
Free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

12 years ago by Pierre Joye — view source — reply

unread

hi,

On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones
christopher.jones@oracle.com wrote:

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the result
was
a performance regression [2].

I tend to agree with Chris here, even for 5.5 :)

However, Let me ask Matt to run our full tests suite against your
patch, that may give us some numbers to discuss or confirm the safety
of this patch.

Matt? Can you run pftt, apps compat and perf tests using this patch pls?

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

12 years ago by Pierre Joye — view source — reply

unread

fix Julien email

hi,

On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones
christopher.jones@oracle.com wrote:

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%) on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the result
was
a performance regression [2].

I tend to agree with Chris here, even for 5.5 :)

However, Let me ask Matt to run our full tests suite against your
patch, that may give us some numbers to discuss or confirm the safety
of this patch.

Matt? Can you run pftt, apps compat and perf tests using this patch pls?

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

--
Pierre

@pierrejoye | http://www.libgd.org

12 years ago by Dmitry Stogov — view source — reply

unread

hi,

On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones
christopher.jones@oracle.com wrote:

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%)
on
each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe to
commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the
result
was
a performance regression [2].

I tend to agree with Chris here, even for 5.5 :)

I don't care a lot about 5.4, but think it should be fixed in 5.5.

However, Let me ask Matt to run our full tests suite against your
patch, that may give us some numbers to discuss or confirm the safety
of this patch.

Of course we checked patch with PHPT tests and some real-life apps, but in
case you have any other test suites it would be a great help to run them
too.

I would also look into strtr() implementation that became 6 times slower on
typical usage in comparison to 5.3 after its "improvement" :(

<?php
function foo() {
for ($i = 0; $i < 100000; $i++) {
strtr("abcdefgh", array("a"=>"11", "g"=>"22"));
}
}
foo();
?>

Thanks. Dmitry.

Matt? Can you run pftt, apps compat and perf tests using this patch pls?

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

12 years ago by Zeev Suraski — view source — reply

unread

-----Original Message-----
From: Christopher Jones [mailto:christopher.jones@oracle.com]
Sent: Thursday, October 24, 2013 3:17 AM
To: Dmitry Stogov; Julien Pauli; David Soria Parra; Stas Malyshev; PHP
Internals
Subject: Re: [PHP-DEV] Improved performance of array_maerge() and
func_get_args()

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%)
on each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe
to commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the
result was
a performance regression [2].

I think the two are very different - this isn't some brand new complex
algorithm, but a very localized optimization that provides net gains in some
cases, with no real risk to other cases.

As far as I understand our release rules, as long as we break no APIs and no
ABIs - we can put it into presently shipping versions, as long as we're
confident it won't introduce regressions (which is true for any bugfix).

That said, I'm fine with us only putting it into 5.5 and not 5.4 to give
people a bit more motivation to upgrade; It just doesn't make sense to sit
on it for a whole year, when the risk associated with it isn't that higher
than many other fixes we routinely introduce into bugfix releases all the
time...

Zeev

12 years ago by Julien Pauli — view source — reply

unread

-----Original Message-----
From: Christopher Jones [mailto:christopher.jones@oracle.com]
Sent: Thursday, October 24, 2013 3:17 AM
To: Dmitry Stogov; Julien Pauli; David Soria Parra; Stas Malyshev; PHP
Internals
Subject: Re: [PHP-DEV] Improved performance of array_maerge() and
func_get_args()

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%)
on each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe
to commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the
result was
a performance regression [2].

I think the two are very different - this isn't some brand new complex
algorithm, but a very localized optimization that provides net gains in
some
cases, with no real risk to other cases.

As far as I understand our release rules, as long as we break no APIs and
no
ABIs - we can put it into presently shipping versions, as long as we're
confident it won't introduce regressions (which is true for any bugfix).

That said, I'm fine with us only putting it into 5.5 and not 5.4 to give
people a bit more motivation to upgrade; It just doesn't make sense to sit
on it for a whole year, when the risk associated with it isn't that higher
than many other fixes we routinely introduce into bugfix releases all the
time...

I agree.
I see no objection not to merge a patch that's been proven to give positive
results.
If test suite still pass, and numbers show a real improvement, including on
real life projects, then just go merging it.

I did not follow carefully the strstr patch, so I cannot give advice about
it.

Feel free to merge it to 5.5 and master.

Julien Pauli

12 years ago by Christopher Jones — view source — reply

unread

-----Original Message-----
From: Christopher Jones [mailto:christopher.jones@oracle.com]
Sent: Thursday, October 24, 2013 3:17 AM
To: Dmitry Stogov; Julien Pauli; David Soria Parra; Stas Malyshev; PHP
Internals
Subject: Re: [PHP-DEV] Improved performance of array_maerge() and
func_get_args()

Hi,

I'm proposing two simple patches that eliminate a lot of useless zval
copying.
For example they remove only about 800 calls to zend_hash_copy() (25%)
on each request to wordpress-3.6.0 home page and make it 2-4% faster.

It's not a questions about master branch, but I think it is also safe
to commit them into PHP-5.4 and PHP-5.5.

Any objections?

https://gist.github.com/dstogov/7117623

https://gist.github.com/dstogov/7117649

Thanks. Dmitry.

I'd prefer PHP 5.4 was kept stable.

Last time I suggested keeping PHP 5.4 stable [1], it wasn't, and the
result was
a performance regression [2].

I think the two are very different - this isn't some brand new complex
algorithm, but a very localized optimization that provides net gains in some
cases, with no real risk to other cases.

As far as I understand our release rules, as long as we break no APIs and no
ABIs - we can put it into presently shipping versions, as long as we're
confident it won't introduce regressions (which is true for any bugfix).

That said, I'm fine with us only putting it into 5.5 and not 5.4 to give
people a bit more motivation to upgrade; It just doesn't make sense to sit
on it for a whole year, when the risk associated with it isn't that higher
than many other fixes we routinely introduce into bugfix releases all the
time...

Zeev

I've no issue with it going to PHP 5.5.

Chris

--
christopher.jones@oracle.com http://twitter.com/ghrd
Free PHP & Oracle book:
http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html