Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:69965
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain zend.com from 209.85.212.174 cause and error)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLsUAs96LquVm+7MHLmM=ij53_ci2vucMcEYfVGygCAudw@mail.gmail.com>
References: <c35dcb527aee65fdc17f1a29aadd8d21.squirrel@webmail.klapt.com>
	<CA+9eiLskBJGsMuW_-Pc1Scf3s7edxvEAeoz-kjj95Ui7xZu89g@mail.gmail.com>
	<63d89379efdd7f2ba0bcf0b62d5025af.squirrel@webmail.klapt.com>
	<CA+9eiLt+HDQVcD5kc2DE0w+R=AYPt8PHQ9Tbdg0EwJRH52+RuA@mail.gmail.com>
	<7c830ad80d33e82eb5f1553d03a44c6d.squirrel@webmail.klapt.com>
	<CA+9eiLuYKKc34CnGw+zP4mCHHOm4Z-XxpnoYBExhV9NYZnJ5cA@mail.gmail.com>
	<1383099923.15492.36.camel@ghost>
	<CA+9eiLsUAs96LquVm+7MHLmM=ij53_ci2vucMcEYfVGygCAudw@mail.gmail.com>
Date: Wed, 30 Oct 2013 16:00:12 +0400
Message-ID: <CA+9eiLsp+UazB8ZBAdbrQ6DMNDAJGGFBc5EMzAiN--p6=JTzQQ@mail.gmail.com>
To: Anatol Belski <ab@php.net>
Cc: Pierre Joye <pierre.php@gmail.com>, PHP Internals <internals@lists.php.net>, 
	Andi Gutmans <andi@zend.com>, Zeev Suraski <zeev@zend.com>, Xinchen Hui <xinchen.h@zend.com>
Content-Type: multipart/alternative; boundary=047d7ba975c853a4b204e9f4138c
Subject: Re: [PHP-DEV] Re: Fix for bug #50333
From: dmitry@zend.com (Dmitry Stogov)

--047d7ba975c853a4b204e9f4138c
Content-Type: text/plain; charset=UTF-8

Hi Anatol,

I've posted few comments at https://github.com/php/php-src/pull/500/files
Otherwise, the patch looks fine.
I think it may be accepted even if it doesn't make visible improvement.
Of course, when the small issues described in comment are fixed.

There were also a minor merging conflict in ext/opcache/ZendAccelerator.c
(it's not a problem at all).

I hope you testd ZTS PHP with patch well, because I mainly care about
non-ZTS builds.

Thanks. Dmitry.


On Wed, Oct 30, 2013 at 10:49 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> I don't think it makes sense to invest into it right now.
> Lets finish with existing patch.
>
> Also, alloca() allocates data on CPU stack, so such data is automatically
> freed when function returns.
> In case you change CWD_STATE_COPY() to use alloca(), the "state" couldn't
> be returned from the function.
> I'm not sure if it'll work for all use cases.
>
> Thanks. Dmitry.
>
>
> On Wed, Oct 30, 2013 at 6:25 AM, Anatol Belski <ab@php.net> wrote:
>
>> Hi Dmitry,
>>
>> On Tue, 2013-10-29 at 11:45 +0400, Dmitry Stogov wrote:
>> > Hi Anatol,
>> >
>> >
>> > Thank you for update.
>> >
>> > I'm surprised you don't see speed difference even with ZTS :(
>> >
>> >
>> > I didn't get what you propose to change in virtual_file_ex().
>> >
>> >
>> > I'll do a more careful patch review later on this week.
>> >
>> >
>>
>> the story is that
>>
>> - the original patch had replacements for all tsrm_do_alloca() and
>> malloc() to emaloc()
>> - after that, i've turned the places originally having tsrm_do_alloca()
>> to do_alloca()
>>
>> Just as we discussed, and that's the current state of the patch. Still,
>> there are many places using the e*() family of functions, like
>>
>>
>> https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L151
>>
>> https://github.com/weltling/php-src/blob/bug50333/Zend/zend_virtual_cwd.c#L1340
>>
>> Alone for that two I find 21 occurrences of each elsewhere in the file.
>> As they are used very often in other filesystem functions like
>> virtual_copy(), virtual_rename(), etc. That's why I thought it could
>> make sense to turn any emalloc() usage into do_alloca(). But as the
>> do_alloca(size, use_heap) requires that use_heap argument to determine
>> the fallback situation, the signatures in at least that two cases for
>> virtual_file_ex() or CWD_STATE_COPY() will need to be extended, so the
>> correct freeing can happen. A simple snippet in pseudo code, just to
>> illustrate the idea
>>
>> currently:
>>
>> some_function()
>> {
>> CWD_STATE_COPY(&new_state, &CWDG(cwd));
>> virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC);
>> CWD_STATE_FREE(&new_state);
>> }
>>
>> should be like:
>>
>> some_function()
>> {
>> ALLOCA_FLAG(use_heap)
>> CWD_STATE_COPY(&new_state, &CWDG(cwd), use_heap);
>> virtual_file_ex(&new_state, path, NULL, CWD_REALPATH, use_heap
>> TSRMLS_CC);
>> CWD_STATE_FREE(&new_state, use_heap);
>> }
>>
>> That's of course much deeper change and would affect more code outside
>> just the scope of that one file, still that's manageable. I'd expect
>> some more functions needing that, so then we'd have do_alloca() usage
>> everywhere and drop all e*() invocations. Maybe then we see some
>> effect :)
>>
>> Regards
>>
>> Anatol
>>
>>
>>
>>
>>
>>
>

--047d7ba975c853a4b204e9f4138c--