Hi Gustavo,
I didn't look into the code yet (and really don't like to do it), but we
just noticed terrible performance degradation of strtr() function between
5.4.11 and 5.4.15 coming probably after your changes.
$ cat strtr.php
<?php
function foo() {
for ($i = 0; $i < 100000; $i++) {
strtr("abcdefgh", array("a"=>"11", "g"=>"22"));
}
}
foo();
$ time sapi/cli/php.5.4.11 strtr.php
real 0m0.082s
user 0m0.071s
sys 0m0.010s
$ time sapi/cli/php.5.4.15 strtr.php
real 0m0.594s
user 0m0.584s
sys 0m0.007s
7 times slower :(
Could you place take a look.
Thanks. Dmitry.
I didn't look into the code yet (and really don't like to do it), but we
just noticed terrible performance degradation ofstrtr()function between
5.4.11 and 5.4.15 coming probably after your changes.$ cat strtr.php
<?php
function foo() {
for ($i = 0; $i < 100000; $i++) {
strtr("abcdefgh", array("a"=>"11", "g"=>"22"));
}
}
foo();
Your example is basically a worst-case scenario of the new algorithm and a
best-case scenario of the old one. The new algorithm has a more costly
preprocessing step and its gains are the smallest when the minimum pattern
size is 1. That combined with a very short text makes the cost of the
preprocessing step dominate.
For strtr(str_repeat("abcdefgh", 50), ...) the results are very close and
for 100 repeats the new algorithm is already faster. It also does not have
cases of multi-second runs in relatively small inputs like the old
algorithm (see https://bugs.php.net/bug.php?id=63893 ); you're talking
about function calls that still execute in around 7 microseconds.
That said, there certainly is an argument for using the old algorithm for
short inputs, as its preprocessing step is much cheaper. It will require
some experimentation in order to determine the cutoff, but if you think
it's important I can do it (you're welcome to do it as well).
Regards
--
Gustavo Lopes
Hi Gustavo,
This "good" algorithm that doesn't work well for "worse" cases makes about
15% slowdown of ZF2 based applications.
It also slowdowns wordpress and other apps, but not so significantly.
It's definitely doesn't work well for real-life applications :(
Something must be dome (e.g. usage of old or new algorithms depending on
different argument patterns).
Thanks. Dmitry.
On Tue, Jun 4, 2013 at 10:53 PM, Gustavo Lopes glopes@nebm.ist.utl.ptwrote:
I didn't look into the code yet (and really don't like to do it), but we
just noticed terrible performance degradation of
strtr()function between
5.4.11 and 5.4.15 coming probably after your changes.$ cat strtr.php
<?php
function foo() {
for ($i = 0; $i < 100000; $i++) {
strtr("abcdefgh", array("a"=>"11", "g"=>"22"));
}
}
foo();Your example is basically a worst-case scenario of the new algorithm and a
best-case scenario of the old one. The new algorithm has a more costly
preprocessing step and its gains are the smallest when the minimum pattern
size is 1. That combined with a very short text makes the cost of the
preprocessing step dominate.For strtr(str_repeat("abcdefgh", 50), ...) the results are very close and
for 100 repeats the new algorithm is already faster. It also does not have
cases of multi-second runs in relatively small inputs like the old
algorithm (see https://bugs.php.net/bug.php?**id=63893https://bugs.php.net/bug.php?id=63893); you're talking about function calls that still execute in around 7
microseconds.That said, there certainly is an argument for using the old algorithm for
short inputs, as its preprocessing step is much cheaper. It will require
some experimentation in order to determine the cutoff, but if you think
it's important I can do it (you're welcome to do it as well).Regards
--
Gustavo Lopes
This "good" algorithm that doesn't work well for "worse" cases makes
about 15% slowdown of ZF2 based applications.
It also slowdowns wordpress and other apps, but not so significantly.It's definitely doesn't work well for real-life applications :(
Something must be dome (e.g. usage of old or new algorithms depending on
different argument patterns).
All right, I'll take a look at what can be done. It's quite possible we
can even do better than the old algorithm for short texts, but falling
back to it in those cases is always an option.
--
Gustavo Lopes
This "good" algorithm that doesn't work well for "worse" cases makes about
15% slowdown of ZF2 based applications.
It also slowdowns wordpress and other apps, but not so significantly.It's definitely doesn't work well for real-life applications :(
Something must be dome (e.g. usage of old or new algorithms depending on
different argument patterns).All right, I'll take a look at what can be done. It's quite possible we can
even do better than the old algorithm for short texts, but falling back to
it in those cases is always an option.
Hey:
is there any new progress of this ? :)
thanks
--
Gustavo Lopes--
--
Laruence Xinchen Hui
http://www.laruence.com/
>
>>
>>
>>> This "good" algorithm that doesn't work well for "worse" cases makes about
>>> 15% slowdown of ZF2 based applications.
>>> It also slowdowns wordpress and other apps, but not so significantly.
>>>
>>> It's definitely doesn't work well for real-life applications :(
>>> Something must be dome (e.g. usage of old or new algorithms depending on
>>> different argument patterns).
>>>
>>
>> All right, I'll take a look at what can be done. It's quite possible we can
>> even do better than the old algorithm for short texts, but falling back to
>> it in those cases is always an option.
> Hey:
>
> is there any new progress of this ? :)
>
The progress consists of writing of scripts to test the point where to
switch algorithms and trying some improvements on the old one without as
expensive preprocessing steps.
Now, this was in the summer... I'll have some time in the holidays to
pick up this and some other PHP-related backlog; that said, if there's
some impatience (which would be perfectly understandable...), I wouldn't
mind if someone reverted to the previous state in the meantime.
--
Gustavo Lopes
≈
The progress consists of writing of scripts to test the point where to switch algorithms and trying some improvements on the old one without as expensive preprocessing steps.
Now, this was in the summer... I'll have some time in the holidays to pick up this and some other PHP-related backlog; that said, if there's some impatience (which would be perfectly understandable...), I wouldn't mind if someone reverted to the previous state in the meantime.
Sounds like this may take some time to figure out. So if there is absolutely no difference in semantics between the two I would suggest we revert until you are able to dig into this.
Andi
≈
The progress consists of writing of scripts to test the point where to switch algorithms and trying some improvements on the old one without as expensive preprocessing steps.
Now, this was in the summer... I'll have some time in the holidays to pick up this and some other PHP-related backlog; that said, if there's some impatience (which would be perfectly understandable...), I wouldn't mind if someone reverted to the previous state in the meantime.
Sounds like this may take some time to figure out. So if there is absolutely no difference in semantics between the two I would suggest we revert until you are able to dig into this.
This change was included in 5.4.12 and 5.4.22 has been released.
Now you want to revert and maybe release 5.4.23 and then apply again in 5.4.24?
That doesn't seem worth it. What done is done. Had it been reverted
right away then it would have make sense, but not 10 releases later?
-Hannes
At least it must be fixed in PHP-5.6 and measured what kind of performance
degradation we got in 5.4.
Then we may think if it makes sense to apply it to 5.4 and 5.5.
It's always dangerous, but significant speed difference on real life
applications matters.
Thanks. Dmitry.
On Tue, Dec 3, 2013 at 11:33 AM, Hannes Magnusson <
hannes.magnusson@gmail.com> wrote:
On Dec 2, 2013, at 3:02 PM, Gustavo Lopes glopes@nebm.ist.utl.pt
wrote:≈
The progress consists of writing of scripts to test the point where to
switch algorithms and trying some improvements on the old one without as
expensive preprocessing steps.Now, this was in the summer... I'll have some time in the holidays to
pick up this and some other PHP-related backlog; that said, if there's some
impatience (which would be perfectly understandable...), I wouldn't mind if
someone reverted to the previous state in the meantime.Sounds like this may take some time to figure out. So if there is
absolutely no difference in semantics between the two I would suggest we
revert until you are able to dig into this.This change was included in 5.4.12 and 5.4.22 has been released.
Now you want to revert and maybe release 5.4.23 and then apply again in
5.4.24?That doesn't seem worth it. What done is done. Had it been reverted
right away then it would have make sense, but not 10 releases later?-Hannes
-----Original Message-----
From: Hannes Magnusson [mailto:hannes.magnusson@gmail.com]
Sent: Tuesday, December 03, 2013 9:34 AM
To: Andi Gutmans
Cc: Gustavo Lopes; Laruence; Dmitry Stogov; PHP Internals; Zeev Suraski;
Gadi Goldbarg
Subject: Re: [PHP-DEV]strtr()performance degradationOn Dec 2, 2013, at 3:02 PM, Gustavo Lopes glopes@nebm.ist.utl.pt
wrote:≈
The progress consists of writing of scripts to test the point where to
switch
algorithms and trying some improvements on the old one without as
expensive preprocessing steps.Now, this was in the summer... I'll have some time in the holidays to
pick
up this and some other PHP-related backlog; that said, if there's some
impatience (which would be perfectly understandable...), I wouldn't mind
if
someone reverted to the previous state in the meantime.Sounds like this may take some time to figure out. So if there is
absolutely
no difference in semantics between the two I would suggest we revert until
you are able to dig into this.This change was included in 5.4.12 and 5.4.22 has been released.
Now you want to revert and maybe release 5.4.23 and then apply again in
5.4.24?That doesn't seem worth it. What done is done. Had it been reverted right
away then it would have make sense, but not 10 releases later?
Hannes,
To put things in perspective, the work that goes into improving PHP's
performance by 10% is measured in months, sometimes more (from inception to
production). Here, we have a patch that slowed real world apps (not
synthetic benchmarks) by over 10%, and despite the fact it was reported 6
months ago, we've done absolutely nothing about it. If anything in that
story doesn't make sense, that would be it.
As to when we reintroduce it - I think it's absolutely fine that we don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is ready.
We've all experienced first-hand what a complete rewrite of a very popular
piece of code can do, and the fact that compatibility can be broken not just
by changing behavior, but also by radically changing the algorithm in a way
that produces very negative performance side effects. Performance should be
one of the measures by which we weigh compatibility - a major regression in
performance shouldn't be acceptable any more than a major regression in
functionality.
We should revert this patch ASAP; It's unfortunate we haven't done it back
when it was found but better late than never.
Zeev
Hi!
As to when we reintroduce it - I think it's absolutely fine that we don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is ready.
I agree, if it is slowing things down for RL apps - we should remove it
from stable versions and re-introduce it in 5.6 when ready. It shouldn't
matter for how many versions it was slow, since code changes are not
required so once people upgrade to next stable, they get their
performance back.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
As to when we reintroduce it - I think it's absolutely fine that we don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is ready.I agree, if it is slowing things down for RL apps - we should remove it
from stable versions and re-introduce it in 5.6 when ready. It shouldn't
matter for how many versions it was slow, since code changes are not
required so once people upgrade to next stable, they get their
performance back.
I agree too, since revet it won't break any things..
thanks
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
--
Laruence Xinchen Hui
http://www.laruence.com/
It'll break performance improvement for a particular case described in the
bug report.
Thanks. Dmitry.
On Tue, Dec 3, 2013 at 4:37 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:Hi!
As to when we reintroduce it - I think it's absolutely fine that we
don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate
it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is
ready.I agree, if it is slowing things down for RL apps - we should remove it
from stable versions and re-introduce it in 5.6 when ready. It shouldn't
matter for how many versions it was slow, since code changes are not
required so once people upgrade to next stable, they get their
performance back.
I agree too, since revet it won't break any things..thanks
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Laruence Xinchen Hui
http://www.laruence.com/
True, but you still agree that's the right thing to do. Breaking the
common case for the rare case wasn't a good trade...
Sent from my mobile
It'll break performance improvement for a particular case described in the
bug report.Thanks. Dmitry.
On Tue, Dec 3, 2013 at 4:37 PM, Stas Malyshev smalyshev@sugarcrm.com
wrote:Hi!
As to when we reintroduce it - I think it's absolutely fine that we
don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate
it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is
ready.I agree, if it is slowing things down for RL apps - we should remove it
from stable versions and re-introduce it in 5.6 when ready. It shouldn't
matter for how many versions it was slow, since code changes are not
required so once people upgrade to next stable, they get their
performance back.
I agree too, since revet it won't break any things..thanks
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
Laruence Xinchen Hui
http://www.laruence.com/
True, but you still agree that's the right thing to do. Breaking the
common case for the rare case wasn't a good trade...
In addition, this generates a new E_NOTICE in 5.4.12, which IMO
constitutes an unacceptable incompatibility for a minor release:
echo strtr('.', ['a' => []]);
--
Matthew Leverton
To put things in perspective, the work that goes into improving PHP's
performance by 10% is measured in months, sometimes more (from inception to
production). Here, we have a patch that slowed real world apps (not
synthetic benchmarks) by over 10%, and despite the fact it was reported 6
months ago, we've done absolutely nothing about it. If anything in that
story doesn't make sense, that would be it.
I cannot agree more than performance is part of the CI/QA process and
should be taken as blocker on release time (if any regressions are
noticed). We do that on a daily basis for every commit and alerts are
sent if there is a performance impact superior to 1-3%.
However I have hard time to imagine than strstr alone can reduce real
apps performance by 10%, unless we are talking about one single module
using it intensively. Do you have numbers and the way you tested it
pls?
We should revert this patch ASAP; It's unfortunate we haven't done it back
when it was found but better late than never.
I'm not too fund to revert in stable series. But I'm fine to revert it
for 5.6.0 if the performance impact has not been solved by the time we
will release the final release. Once it is out (optimized back or
reverted), we can always consider it for 5.5 or eventually 5.4.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi!
We've all experienced first-hand what a complete rewrite of a very popular
piece of code can do, and the fact that compatibility can be broken not just
by changing behavior, but also by radically changing the algorithm in a way
that produces very negative performance side effects.
What popular piece of code are you referring to?
--
Regards,
Mike
Hi!
We've all experienced first-hand what a complete rewrite of a very popular
piece of code can do, and the fact that compatibility can be broken not just
by changing behavior, but also by radically changing the algorithm in a way
that produces very negative performance side effects.What popular piece of code are you referring to?
strtr(). The algorithm change resulted in a ~10x slowdown for short
simple cases, and given its popularity in PHP apps - that translated
to a ~12% slowdown in real life performance.
Zeev
Hi!
We've all experienced first-hand what a complete rewrite of a very
popular
piece of code can do, and the fact that compatibility can be broken
not just
by changing behavior, but also by radically changing the algorithm in
a way
that produces very negative performance side effects.What popular piece of code are you referring to?
strtr(). The algorithm change resulted in a ~10x slowdown for short
simple cases, and given its popularity in PHP apps - that translated
to a ~12% slowdown in real life performance.
As I agree on the issue, I would really like to see which apps you use to
test and how, as well as the numbers.
Cheers,
-----Original Message-----
From: Hannes Magnusson [mailto:hannes.magnusson@gmail.com]
Sent: Tuesday, December 03, 2013 9:34 AM
To: Andi Gutmans
Cc: Gustavo Lopes; Laruence; Dmitry Stogov; PHP Internals; Zeev Suraski;
Gadi Goldbarg
Subject: Re: [PHP-DEV]strtr()performance degradationOn Dec 2, 2013, at 3:02 PM, Gustavo Lopes glopes@nebm.ist.utl.pt
wrote:≈
The progress consists of writing of scripts to test the point where to
switch
algorithms and trying some improvements on the old one without as
expensive preprocessing steps.Now, this was in the summer... I'll have some time in the holidays to
pick
up this and some other PHP-related backlog; that said, if there's some
impatience (which would be perfectly understandable...), I wouldn't mind
if
someone reverted to the previous state in the meantime.Sounds like this may take some time to figure out. So if there is
absolutely
no difference in semantics between the two I would suggest we revert until
you are able to dig into this.This change was included in 5.4.12 and 5.4.22 has been released.
Now you want to revert and maybe release 5.4.23 and then apply again in
5.4.24?That doesn't seem worth it. What done is done. Had it been reverted right
away then it would have make sense, but not 10 releases later?Hannes,
To put things in perspective, the work that goes into improving PHP's
performance by 10% is measured in months, sometimes more (from inception to
production). Here, we have a patch that slowed real world apps (not
synthetic benchmarks) by over 10%, and despite the fact it was reported 6
months ago, we've done absolutely nothing about it. If anything in that
story doesn't make sense, that would be it.
Agreed. The release manager messed up. He should have been paying
attention and never shipped the release.
And oddly enough, the 5.5.0 release (released just couple of weeks
after the initial mail in this thread) also included this wrong-fix
without anyone caring.
And to this date, I am unable to find a bug report complaining about a slowdown.
As to when we reintroduce it - I think it's absolutely fine that we don't
reintroduce it in the 5.4.x series, or even the 5.5.x series, but slate it
to 5.6.0 - and that too only if it's fully-baked by the time 5.6.0 is ready.
I think optimizations of this kind shouldn't be committed into bugfix
releases anyway.
We are rolling out bugfix releases these days like we are trying to
catch up with browser versions, and minor releases so frequently that
kids use to to learn how to count.
Optimizations should be considered a major feature and only added to
.0 releases so this doesn't happen.
We've all experienced first-hand what a complete rewrite of a very popular
piece of code can do, and the fact that compatibility can be broken not just
by changing behavior, but also by radically changing the algorithm in a way
that produces very negative performance side effects. Performance should be
one of the measures by which we weigh compatibility - a major regression in
performance shouldn't be acceptable any more than a major regression in
functionality.
Completely agree, further strengthening my argument that bugfix
releases should be left out of this.
We should revert this patch ASAP; It's unfortunate we haven't done it back
when it was found but better late than never.
I don't know... Noone cared when it was released, noone has reported a
ticket, can't we leave stable branches stable and fix this in the
upcoming minor release?
-Hannes