Re: Re: Fix for bug #50333

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