Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:85437
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.52 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CABBJUpeR7FW7S_ygUt0SfVJvXy0sb16BuqcjVni=yO9dyOmpvw@mail.gmail.com>
References: <CA+9eiLsAQPTkVuR=D0wXMFXhBNM8DQzGq0DRXzSuL+hjPsUpgA@mail.gmail.com>
	<CAEZPtU7yj6cYpBd_1hfZkr5F9S2Jw_JhuGKK=M=w5qS3o7uTqw@mail.gmail.com>
	<CABBJUpe4zmwzK3PJ+6DhAvwjLr_pfAcNdkouChNVV-OaRjWpgw@mail.gmail.com>
	<CAEZPtU4h2RnGJ7GuNU4uvD8dZz97Eo2k8OiOQYVF3kz-_VdQAQ@mail.gmail.com>
	<CABBJUpe+CgQVMJeh7pa93BBtCXkP-4oXDHVR0xvd-D9FpeRyRQ@mail.gmail.com>
	<CAEZPtU6s4_-dJJqeFgjS3nsVqB+K3zy=g-ge74en7tG7Ga0tBw@mail.gmail.com>
	<CABBJUpeR7FW7S_ygUt0SfVJvXy0sb16BuqcjVni=yO9dyOmpvw@mail.gmail.com>
Date: Tue, 24 Mar 2015 14:50:10 +0700
Message-ID: <CAEZPtU6zOXiqF1bTe-TbZKESg3zbdHe6J5=aTCt1+U+uzKX71A@mail.gmail.com>
To: Xinchen Hui <xinchen.h@zend.com>
Cc: Dmitry Stogov <dmitry@zend.com>, Nikita Popov <nikita.ppv@gmail.com>, 
	Joe Watkins <pthreads@pthreads.org>, PHP Internals <internals@lists.php.net>
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 <xinchen.h@zend.com> wrote:
> Hey:
>
> On Tue, Mar 24, 2015 at 2:04 PM, Pierre Joye <pierre.php@gmail.com> wrote:
>> On Tue, Mar 24, 2015 at 1:01 PM, Xinchen Hui <xinchen.h@zend.com> wrote:
>>> Hey:
>>>
>>> On Tue, Mar 24, 2015 at 1:54 PM, Pierre Joye <pierre.php@gmail.com> wrote:
>>>> On Tue, Mar 24, 2015 at 12:35 PM, Xinchen Hui <xinchen.h@zend.com> wrote:
>>>>> Hey:
>>>>>
>>>>> On Tue, Mar 24, 2015 at 1:31 PM, Pierre Joye <pierre.php@gmail.com> wrote:
>>>>>> hi!
>>>>>>
>>>>>> On Tue, Mar 24, 2015 at 5:41 AM, Dmitry Stogov <dmitry@zend.com> 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