Re: strtr() performance degradation

On Tue, Dec 3, 2013 at 12:29 AM, Zeev Suraski <zeev@zend.com> wrote: >> -----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 degradation >> >> On Mon, Dec 2, 2013 at 10:54 PM, Andi Gutmans <andi@zend.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, > > 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