Skip to content

Commit 00008a8

Browse files
authored
Remove unnecessary memory clearing in virtual_file_ex() (#10963)
I checked a simple Laravel CRUD application's home page under Callgrind and found that the line: char resolved_path[MAXPATHLEN] = {0}; took up about 0.95% of the spent instruction count. This is because when opcache revalidates the timestamps, it has to go through the function virtual_file_ex() which contains that line. That line will memset 4096 bytes on my system to all zeroes. This is bad for the data cache and for the runtime. I found that this memsetting is unnecessary in most cases, and that we can fix the one remaining case: * Lines 1020-1027 don't do anything with resolved_path, so that's okay. * Lines 1033-1098: - The !IS_ABSOLUTE_PATH branch will always result in a memcpy from path to resolved_path (+ sometimes an offset) with the total copied amount equal to path_length+1, so that includes a NUL byte. - The else branch either takes the WIN32 path or the non-WIN32 path. ° WIN32: There's a copy from path+2 with length path_length-1. Note that we chop off the first 2 bytes, so this also includes the NUL byte. ° Non-WIN32: Copies path_length+1 bytes, so that includes a NUL byte. At this point we know that resolved_path ends in a NUL byte. Going further in the code: * Lines 1100-1106 don't write to resolved_path, so no NUL byte is removed. * Lines 1108-1136: - The IS_UNC_PATH branch: ° Lines 1111-1112 don't overwrite the NUL byte, because we know the path length is at least 2 due to the IS_UNC_PATH check. ° Both while loops uppercase the path until a slash is found. If a NUL byte was found then it jumps to verify. Therefore, no NUL byte can be overwritten. Furthermore, Lines 1121 and 1129 cannot overwrite a NUL byte because the check at lines 1115 and 1123 would've jumped to verify when a NUL byte would be encountered. Therefore, the IS_UNC_PATH branch cannot overwrite a NUL byte, so the NUL byte we know we already got stays in place. - The else branch: ° We know the path length is at least 2 due to IS_ABSOLUTE_PATH. That means the earliest NUL byte can be at index 2, which can be overwritten on line 1133. We fix this by adding one byte write if the length is 2. All uses of resolved_path in lines 1139-1141 have a NUL byte at the end now. Lines 1154-1164 do a bunch of post-processing but line 1164 will make sure resolved_path still ends in a NUL byte. So therefore I propose to remove the huge memset, and add a single byte write in that one else branch I mentioned earlier. Looking at Callgrind, the instruction count before this patch for 200 requests is 14,264,569,942; and after the patch it's 14,129,358,195 (averaged over a handful of runs).
1 parent 6df7557 commit 00008a8

File tree

1 file changed

+4
-1
lines changed

1 file changed

+4
-1
lines changed

Zend/zend_virtual_cwd.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,7 @@ static size_t tsrm_realpath_r(char *path, size_t start, size_t len, int *ll, tim
10091009
CWD_API int virtual_file_ex(cwd_state *state, const char *path, verify_path_func verify_path, int use_realpath) /* {{{ */
10101010
{
10111011
size_t path_length = strlen(path);
1012-
char resolved_path[MAXPATHLEN] = {0};
1012+
char resolved_path[MAXPATHLEN];
10131013
size_t start = 1;
10141014
int ll = 0;
10151015
time_t t;
@@ -1131,6 +1131,9 @@ CWD_API int virtual_file_ex(cwd_state *state, const char *path, verify_path_func
11311131
/* skip DRIVE name */
11321132
resolved_path[0] = toupper(resolved_path[0]);
11331133
resolved_path[2] = DEFAULT_SLASH;
1134+
if (path_length == 2) {
1135+
resolved_path[3] = '\0';
1136+
}
11341137
start = 3;
11351138
}
11361139
#endif

0 commit comments

Comments
 (0)