Fix for bug #50333

Hi Anatol,

First of all PHP is mainly used as NTS on Linux (LAMP stack)
It's the reason I tested it first, to check if your patch affects the
performance that is really important for users.

I still think that tsrm_do_alloca() have to be replaced by do_alloca().
They have exactly the same logic, except that the first one fallback to
malloc() and the second one to emalloc(). It must not be affected by
malloc()s and realloc()s if you changed them into emalloc() and erealloc().
May be I missing something...

I'm not sure about alloca() usage on Windows, but you may change zend.h if
you like.
BTW: I don't think we should replace alloca() by _malloca(), because
do_alloca() already does the same in portable way and uses per-request heap.

Thanks. Dmitry.

Hi Dmitry,

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.

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