Re: com php-src: avoid multiple strlen calls for the same buffer: Zend/zend_virtual_cwd.c

On Fri, September 19, 2014 14:56, Anatol Belski wrote: > On Fri, September 19, 2014 12:57, Nikita Popov wrote: > >> On Fri, Sep 19, 2014 at 12:39 PM, Anatol Belski <ab@php.net> wrote: >> >> >> >>> Commit: 6bbebc60ea0de6ce09ea45094b3bed1823d96cec >>> Author: Anatol Belski <ab@php.net> Fri, 19 Sep 2014 >>> 12:39:17 >>> +0200 >>> Parents: d8de53d498cf75936c9cd6781456c39b01f5af60 >>> Branches: master >>> >>> >>> >>> Link: >>> >>> http://git.php.net/?p=php-src.git;a=commitdiff;h=6bbebc60ea0de6ce09ea4 >>> 50 >>> 94b3bed1823d96cec >>> >>> >>> >>> Log: >>> avoid multiple strlen calls for the same buffer >>> >>> Changed paths: >>> M Zend/zend_virtual_cwd.c >>> >>> >>> >>> >>> Diff: >>> diff --git a/Zend/zend_virtual_cwd.c b/Zend/zend_virtual_cwd.c index >>> 665829d..acc83ec 100644 >>> --- a/Zend/zend_virtual_cwd.c >>> +++ b/Zend/zend_virtual_cwd.c >>> @@ -1442,16 +1442,20 @@ CWD_API char *virtual_realpath(const char >>> *path, >>> char *real_path TSRMLS_DC) /* { if (VCWD_GETCWD(cwd, MAXPATHLEN)) { >>> path = cwd; >>> } >>> - } else if (!IS_ABSOLUTE_PATH(path, strlen(path))) { >>> - CWD_STATE_COPY(&new_state, &CWDG(cwd)); >>> } else { >>> - new_state.cwd = (char*)emalloc(1); >>> - if (new_state.cwd == NULL) { >>> - retval = NULL; >>> - goto end; >>> + size_t path_len = strlen(path); >>> + >>> + if (!IS_ABSOLUTE_PATH(path, path_len)) { >>> + CWD_STATE_COPY(&new_state, &CWDG(cwd)); >>> + } else { >>> + new_state.cwd = (char*)emalloc(1); >>> + if (new_state.cwd == NULL) { >>> + retval = NULL; >>> + goto end; >>> + } >>> + new_state.cwd[0] = '\0'; >>> + new_state.cwd_length = 0; >>> } >>> - new_state.cwd[0] = '\0'; >>> - new_state.cwd_length = 0; >>> } >>> >>> >>> >>> if (virtual_file_ex(&new_state, path, NULL, CWD_REALPATH >>> TSRMLS_CC)==0) >>> { >>> @@ -1967,17 +1971,21 @@ CWD_API char *tsrm_realpath(const char *path, >>> char *real_path TSRMLS_DC) /* {{{ if (VCWD_GETCWD(cwd, MAXPATHLEN)) { >>> path = cwd; } - } else if (!IS_ABSOLUTE_PATH(path, strlen(path)) >>> && >>> - VCWD_GETCWD(cwd, MAXPATHLEN)) >>> { >>> - new_state.cwd = estrdup(cwd); >>> - new_state.cwd_length = strlen(cwd); >>> } else { >>> - new_state.cwd = (char*)emalloc(1); >>> - if (new_state.cwd == NULL) { >>> - return NULL; >>> + size_t path_len = strlen(path); >>> + >>> + if (!IS_ABSOLUTE_PATH(path, strlen(path)) && >>> + VCWD_GETCWD(cwd, MAXPATHLEN)) { >>> + new_state.cwd = estrdup(cwd); >>> + new_state.cwd_length = strlen(cwd); >>> + } else { >>> + new_state.cwd = (char*)emalloc(1); >>> + if (new_state.cwd == NULL) { >>> + return NULL; >>> + } >>> + new_state.cwd[0] = '\0'; >>> + new_state.cwd_length = 0; >>> } >>> - new_state.cwd[0] = '\0'; >>> - new_state.cwd_length = 0; >>> } >>> >>> >>> >>> if (virtual_file_ex(&new_state, path, NULL, CWD_REALPATH TSRMLS_CC)) >>> { >>> >>> >>> >> >> In the second case, you forgot to replace the strlen(path) in the >> IS_ABSOLUTE_PATH invocation. >> >> >> >> Also, these changes will cause unused variable warnings on non-windows >> systems, because IS_ABSOLUTE_PATH only uses the length on windows. I'm >> not sure what's the best way to avoid that - maybe make IS_ABSOLUTE_PATH >> an inline function instead of a macro? >> > Ah, you're right, those will be warned as unreferenced, and even if not - > that's an empty call. > > A grep reports like 34 usage occurrences in the core, not that many. > Those > I've touched was using strlen(), are about 5 AFAIR. In the other cases the > path length is delivered. Though, a couple of places can be optimized. > > I'll try with the inline function, it's cleaner. If it takes too much, > will just go through the places I've touched and #ifdef them. Those macros > are normally not changed/used that often, so should be ok, too. > > Regards > > > Anatol > > > Found another solution to that, just expand the macro :) Anatol