Re: Re: 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. On Wed, Oct 23, 2013 at 2:55 PM, Anatol Belski <ab@php.net> wrote: > 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 <ab@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 > >