Hi Dmitry,
On Mon, October 21, 2013 14:30, Dmitry Stogov wrote:
> Hi,
>
>
> I don't have strong opinion about the patch.
> I thought malloc() -> emalloc() change might improve PHP performance in
> general, but unfortunately it doesn't. On the other hand the patch is quite
> big and introduces source level incompatibility.
>
> The patch is not complete. At least it misses this chunk:
>
>
> --- a/sapi/cgi/cgi_main.c
> +++ b/sapi/cgi/cgi_main.c
> @@ -1396,7 +1396,7 @@ static void init_request_info(fcgi_request *request
> TSRMLS_DC)
> } else {
> SG(request_info).request_uri =
> env_script_name; }
> - free(real_path);
> + efree(real_path);
> }
> } else {
> /* pre 4.3 behaviour, shouldn't be used but
> provides BC */
>
> It's also much better to use do_alloca() instead of tsrm_do_alloca() (the
> patch changed them to less efficient emalloc()). May be if you change it,
> we would see improvement :)
>
> As I said, currently, the patch doesn't significantly affect performance
> of non-thread-safe build on Linux.
>
>
> master patched improvement Blog (req/sec) 106.1 105.1 -0.94% drupal
> (req/sec) 1660.5 1668.6 0.49% fw (req/sec) 231.7 227.499 -1.81% hello
> (req/sec) 11828.8 11980.5 1.28% qdig (req/sec) 470 477.3 1.55% typo3
> (req/sec) 580.1 579.3 -0.14% wordpress (req/sec) 185.9 188.5 1.40% xoops
> (req/sec) 130 131.2 0.92% scrum (req/sec) 185.199 185 -0.11% ZF1 Hello
> (req/sec) 1154.7 1155.2 0.04% ZF2 Test (req/sec) 248.7 250.7 0.80%
> Thanks. Dmitry.
>
>
>
>
> On Fri, Oct 18, 2013 at 7:33 PM, Anatol Belski <a...@php.net> wrote:
>
>
>> Hi,
>>
>>
>> the pull request https://github.com/php/php-src/pull/500 fixing the bug
>> #50333 is ready to review. Manual tests done so far on linux and
>> windows in TS and NTS mode with CLI and Apache show no regression. The
>> performance tests are to be done yet.
>>
>> Regards
>>
>>
>> Anatol
>>
thanks for the comments, the CGI part is fixed in the same patch.
While investigating on how to go further with your suggestion, I've yet a
couple of open questions.
The reason for moving files into Zend, as I can see from the original
patch is, that the tsrm_init has to happen after Zend memory manager init
in order to use the e* function family. It has to be true also for the
do_alloca() case as when the stack size was exceeded, it'll fallback to
emalloc(). For that reason, I'd rather implement your suggestion into the
existing patch as it's quicker just to see if it'd make sense at all. If
it would, tsrm files can be moved back into TSRM/ and one can look for
magic to initialize it in the right order.
It'd of course make sense to convert everything for do_alloca() usage,
just a replace for tsrm_do_alloca would already work, but ... there are
some places in the original codelike
http://lxr.php.net/xref/PHP_TRUNK/TSRM/tsrm_virtual_cwd.c#493 where malloc
is used. Or virtual_file_ex which is using realloc(), with do_alloca that
memory should be probably just left unfreed on the stack? Do you generally
think i should touch those places, would it be ok to leave them alone for
now for the tests? As otherwise it might need signature change of some
functions, it's about use_heap when the stack size is exceeded to avoid
memory leaks.
When I look here http://lxr.php.net/xref/PHP_TRUNK/Zend/zend.h#200 , only
some GNU based platform profits from alloca() on both TS and NTS. Some
others are only active with NTS only. Is there a particular reason for
that? I think that macros could be refactored at least for the Windows
part, as this page
http://msdn.microsoft.com/en-us/library/5471dc8s(v=vs.110).aspx deprecates
_alloca nowadays.
Also wondering why you was doing NTS perf tests, as the ticket is about
improvement for multi-threaded envs. Nevertheless the overall improvement
were even better of course :)
Regards
Anatol
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
