# Improved performance of array_maerge() and func_get_args() - Thread: https://externals.io/message/69802 - Original archive: https://news-web.php.net/php.internals/69802 - Started by Dmitry Stogov on 2013-10-23T12:36:20+00:00 - Messages: 14 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #69802 — Dmitry Stogov — 2013-10-23T12:36:20+00:00 URL: https://externals.io/message/69802#69802 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. --- ## #69804 — Nikita Popov — 2013-10-23T13:52:55+00:00 URL: https://externals.io/message/69802#69804 · In reply to: #69802 > 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 --- ## #69806 — Dmitry Stogov — 2013-10-23T14:00:20+00:00 URL: https://externals.io/message/69802#69806 · In reply to: #69804 makes sense. 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 --- ## #69807 — Levi Morrison — 2013-10-23T14:21:01+00:00 URL: https://externals.io/message/69802#69807 · In reply to: #69802 > 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. --- ## #69812 — Patrick ALLAERT — 2013-10-23T18:10:17+00:00 URL: https://externals.io/message/69802#69812 · In reply to: #69802 2013/10/23 Dmitry Stogov : > 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 --- ## #69816 — Dmitry Stogov — 2013-10-23T19:20:43+00:00 URL: https://externals.io/message/69802#69816 · In reply to: #69807 On Wed, Oct 23, 2013 at 6:21 PM, Levi Morrison wrote: > > I'm proposing two simple patches that eliminate a lot of useless zval > > […] > > Sounds great. Thank you for looking into this. Just to clarify: the > wordpress improvements were from both patches, yes? > Yes, both patches together. > > > […] > > 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. --- ## #69817 — Dmitry Stogov — 2013-10-23T19:23:43+00:00 URL: https://externals.io/message/69802#69817 · In reply to: #69812 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 wrote: > 2013/10/23 Dmitry Stogov : > > […] > > 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 --- ## #69822 — Christopher Jones — 2013-10-24T00:17:06+00:00 URL: https://externals.io/message/69802#69822 · In reply to: #69802 > 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 --- ## #69824 — Pierre Joye — 2013-10-24T05:25:12+00:00 URL: https://externals.io/message/69802#69824 · In reply to: #69822 hi, On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones wrote: > > > > > […] > > 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 --- ## #69825 — Pierre Joye — 2013-10-24T05:26:25+00:00 URL: https://externals.io/message/69802#69825 · In reply to: #69824 fix Julien email > hi, > > On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones > wrote: > > […] > > 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 --- ## #69826 — Dmitry Stogov — 2013-10-24T06:36:35+00:00 URL: https://externals.io/message/69802#69826 · In reply to: #69824 > hi, > > On Thu, Oct 24, 2013 at 2:17 AM, Christopher Jones > wrote: > > […] > on > > […] > result > > […] > > 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" :( "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 --- ## #69836 — Zeev Suraski — 2013-10-24T08:49:50+00:00 URL: https://externals.io/message/69802#69836 · In reply to: #69822 > -----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()` > > > > > > […] > > 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 --- ## #69839 — Julien Pauli — 2013-10-24T11:55:03+00:00 URL: https://externals.io/message/69802#69839 · In reply to: #69836 > > […] > > 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 --- ## #69846 — Christopher Jones — 2013-10-24T16:55:51+00:00 URL: https://externals.io/message/69802#69846 · In reply to: #69836 > > […] > > 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