On 8/13/08 1:09 AM, "Dmitry Stogov" <[EMAIL PROTECTED]> wrote:
> I took a look into patch and I still don't like it. > I may miss some things and make mistakes so correct me if I'm wrong. > 1) It makes some slowdown for all SAPIs except Apache1, because it adds > additional block/unblock code (mainly in zend_alloc.c) The additional code was added by Arnaud as he added ZTS support. I'll let him comment as I did not spend any time in the allocator determining if we should add additional critical sections. Technically there are *many* sections of startup where it would make sense to add critical sections, however the likelyhood of getting a signal here is extremely low. > 2) It makes ~10 additional syscalls on each request. I didn't get why do > you save original handlers in MINIT and set new handlers in RINIT. In > general they can be changed between MINIT and RINIT by other apache > module or web server so they should be saved/set in RINIT, but from > performance point of view it might be better to save/set signals in MINIT. Well I can never be sure of what the sapi is doing in regards to it's signal handling. In apache, for example it resets some of it's signal handlers at the beginning of each request. It's possible that we may be able to reduce the number of these in the apache instance but I wouldn't know what to do in any other sapi, thus the safest route was chosen. > 3) The patch cannot solve all issues caused by SIGALARM handling, > because PHP has thousand places which may stay data in inconsistent > state (or data allocated in permanent heap). Wrapping all of them with > block/unblock is not a decision. I don't have anything to add to this observation. My greatest concern is critical sections within the allocator where we do commonly see timeouts and possibly other signals and the critical sectios in APC. When our shared memory locks are frozen because APC relies on critical section protection, all requests can be locked forever. We may also see general corruption here. I would recommend against ever adding APC to core without some sort of protection such as what this patch offers, if that is still the eventual plan. > 4) block/unblock code in estrndup() and some other functions is useless. > It should be removed. > > 5) TSRM_FETCH() in zend_hash and zend_alloc functions will slowdown > Windows ZTS build even if it doesn't use signals. It may be changed to > some other macro that will do TSRM_FETCH only if signals are enabled. I'll let Arnaud respond to these as this is his portion of the patch. #5 may make sense to me though. > May be if we really like safe SIGALARM handling we may use Windows > decision for UNIX too. I mean setup of EG(timed_out) flag in signal > handler and checking it in execute loop. As an optimization we can check > it not on each instruction but only on enter in function and JMP* > opcodes (loops). Anyway, this solution may cause comparable slowdown. In ZTS this may definitely be true, in non-zts it should not be any slower barring the added blocking or TSRM_FETCH's. These functions should be passed TSRM data and not have to fetch, which I assume would perform better. Arnaud, do you have any performance comparisons before/after the patch in ZTS? -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
