Re: Re: Fix for bug #50333

I don't think it makes sense to invest into it right now. Lets finish with existing patch. Also, alloca() allocates data on CPU stack, so such data is automatically freed when function returns. In case you change CWD_STATE_COPY() to use alloca(), the "state" couldn't be returned from the function. I'm not sure if it'll work for all use cases. Thanks. Dmitry. On Wed, Oct 30, 2013 at 6:25 AM, Anatol Belski <ab@php.net> wrote: > 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 > > > > > >