Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:69960
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.73.107 as permitted sender)
To: Dmitry Stogov <dmitry@zend.com>
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>
In-Reply-To: <CA+9eiLuYKKc34CnGw+zP4mCHHOm4Z-XxpnoYBExhV9NYZnJ5cA@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>
Content-Type: text/plain; charset="UTF-8"
Date: Wed, 30 Oct 2013 03:25:23 +0100
Message-ID: <1383099923.15492.36.camel@ghost>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: Fix for bug #50333
From: ab@php.net (Anatol Belski)

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