Re: Re: Fix for bug #50333

Hi Anatol, I've posted few comments at https://github.com/php/php-src/pull/500/files Otherwise, the patch looks fine. I think it may be accepted even if it doesn't make visible improvement. Of course, when the small issues described in comment are fixed. There were also a minor merging conflict in ext/opcache/ZendAccelerator.c (it's not a problem at all). I hope you testd ZTS PHP with patch well, because I mainly care about non-ZTS builds. Thanks. Dmitry. On Wed, Oct 30, 2013 at 10:49 AM, Dmitry Stogov <dmitry@zend.com> wrote: > 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 >> >> >> >> >> >> >