Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:85437 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 97135 invoked from network); 24 Mar 2015 07:50:14 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 24 Mar 2015 07:50:14 -0000 Authentication-Results: pb1.pair.com smtp.mail=pierre.php@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=pierre.php@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.52 as permitted sender) X-PHP-List-Original-Sender: pierre.php@gmail.com X-Host-Fingerprint: 209.85.192.52 mail-qg0-f52.google.com Received: from [209.85.192.52] ([209.85.192.52:35771] helo=mail-qg0-f52.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id A7/F0-25887-43711155 for ; Tue, 24 Mar 2015 02:50:13 -0500 Received: by qgf74 with SMTP id 74so58037816qgf.2 for ; Tue, 24 Mar 2015 00:50:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=WRPwyLr/DPyEViFyXLfXGaMujqxiSAincmTfPHYWHbY=; b=tNOTMXHcRIx5t/aFb2iQbXpIax+kuMgvIDY4mhD7NHTuKkFDHLQFb2a552vBXj3V4n nvJ2nHrOQ7rzK5ascqWVUOhkYMoJ1a8+4XZYQmXirt0aXsIaBNS8Svfzxn6yDB4tuwWO pYJzmPcb7ewCf59stQLKAHZPAAVgkvsrmbp2QBfcgT+8GFtYUqSj4G6Zoc1nKwBrH1Fh R/AovbsGZl9TgB/ev+50bP2F/BeRcljL0uhHhy+uNLdEPdBlCTIdo3KgNmSjdiYsGZWO xXJw1dSwdwQ6BCvssgOZLSiFpCGyVlwbOOV2xvNKWqvKq9UXSk2bAGy3XCu+YqkBHNcR kGfw== MIME-Version: 1.0 X-Received: by 10.55.19.159 with SMTP id 31mr6658214qkt.24.1427183410421; Tue, 24 Mar 2015 00:50:10 -0700 (PDT) Received: by 10.96.39.195 with HTTP; Tue, 24 Mar 2015 00:50:10 -0700 (PDT) In-Reply-To: References: Date: Tue, 24 Mar 2015 14:50:10 +0700 Message-ID: To: Xinchen Hui Cc: Dmitry Stogov , Nikita Popov , Joe Watkins , PHP Internals Content-Type: text/plain; charset=UTF-8 Subject: Re: [PHP-DEV] Use "ropes" for string concatenation From: pierre.php@gmail.com (Pierre Joye) On Tue, Mar 24, 2015 at 1:14 PM, Xinchen Hui wrote: > Hey: > > On Tue, Mar 24, 2015 at 2:04 PM, Pierre Joye wrote: >> On Tue, Mar 24, 2015 at 1:01 PM, Xinchen Hui wrote: >>> Hey: >>> >>> On Tue, Mar 24, 2015 at 1:54 PM, Pierre Joye wrote: >>>> On Tue, Mar 24, 2015 at 12:35 PM, Xinchen Hui wrote: >>>>> Hey: >>>>> >>>>> On Tue, Mar 24, 2015 at 1:31 PM, Pierre Joye wrote: >>>>>> hi! >>>>>> >>>>>> On Tue, Mar 24, 2015 at 5:41 AM, Dmitry Stogov wrote: >>>>>>> Hi, >>>>>>> >>>>>>> Recently, Xinchen and me worked on optimization that eliminates useless >>>>>>> reallocations and copying during string concatenation (ZEND_ADD_STRING and >>>>>>> family + ZEND_CONCAT). >>>>>>> >>>>>>> The idea comes from ropes, but adopted especially for our needs. >>>>>>> Rope is popular data structure in languages with immutable strings. >>>>>>> http://en.wikipedia.org/wiki/Rope_%28data_structure%29 >>>>>>> >>>>>>> We don't try to use ropes everywhere in the engine (at least it's too later >>>>>>> for 7.0), only for concatenation. >>>>>>> >>>>>>> The first branch uses ropes only instead of ZEND_ADD_STRING and family. >>>>>>> This must be safe. The only problem is possible memory leaks on exception >>>>>>> (but we already have this problem anyway). The simplest way to understand >>>>>>> the patch - read code for new opcodes in zend_vm_def.h. >>>>>>> >>>>>>> https://github.com/php/php-src/pull/1194/files >>>>>>> >>>>>>> The second branch in addition uses ropes for series of ZEND_CONCAT. It >>>>>>> disables calls to do_operation(ZEND_CONCAT) handler of custom internal >>>>>>> classes. >>>>>>> >>>>>>> https://github.com/php/php-src/pull/1195/files >>>>>>> >>>>>>> Both make slight speed improvement (first +0.3%, second +0.6% on wordpress >>>>>>> home page). >>>>>>> >>>>>>> We don't currently use ability to override CONCAT behavior in internal >>>>>>> classes, and I'm not sure if it may be useful at all. (For example Lua >>>>>>> doesn't provide concat meta-method). May be remove it? >>>>>>> >>>>>>> Thoughts and comments are welcome. >>>>>> >>>>>> Great work! :) >>>>>> >>>>>> Do you expect similar gain for other ops? >>>>>> >>>>>> I wonder if it would not be better to target 7.1 for that, adding it >>>>>> for other string operations, in one go. Most of the current patch is >>>>>> self contained, it adds quite some complexity to the engine for these >>>>>> operations only and it is not a small change at this stage (post >>>>>> features freeze). It sounds like a possible maintenance pain. Taking >>>>> Actually, it's not. >>>>> >>>>> previously we have ADD_STRING/CHAR/VAR and CONCAT >>>>> >>>>> the 2nd branch cleanup these, and now we only deal with one type concat_list. :) >>>> >>>> Right, it simplifies part of the existing implementation and add some >>>> complexities to other. It only adds this OP and keep other with the >>>> "old" implementation, that's actually the only part where I am not >>>> totally convinced. It may make sense to do it all at once, if that's >>>> the long term plan, easier to maintain. >>>> >>>> But if we do prefer to add this now, then the sooner the better, it >>>> may have some hard to catch bugs. Now, we have one issue, we cannot >>>> have a RFC (deadline behind us) and this is still a rather big change >>>> :/ >>> from user land. this won't change anything.. >> >> Nothing to do with userland or not but code stabilization > In that case, yeah. you might be right. > > but from my opinion, simpler always means easier for maintaining.... > > anyway, I hope this could be merged to 7.0(the second branch) :) Me too :) However I do not want to have double standards. We want a strict time stable to ensure the delivery of 7 final in time. Other RFCs are less intrusive (read, more self contained) and have been rejected. It is not very correct in my opinion. I do not mind much in this case as we can see this patch as part as the white card we gave when we accepted phpng, but I like to hear other voices as well. Cheers, -- Pierre @pierrejoye | http://www.libgd.org