[PATCH 0/1] MR10892: ntdll/unix: use localtime_r() instead just localtime()
At least for glibc: localtime() calls always check for timezone configuration changes which usually includes a stat syscall, localtime_r() does not. # Explanation At least for glibc, every call of `localtime()` will internally check for (and react to) changes of the timezone: either the `TZ` environment variable (if set) or changes to a used file, typically `/etc/localtime` - the latter causes a 'stat' syscall. In contrast, calls to `localtime_r()` are faster because they don't check for such changes (and also wont update `tzname`, `timezone`, `daylight` global variables). This behavior is extremely poorly documented (on glibc side) such that people regularly rediscover it, often because of the performance impact. # Consequences of this change - for `weekday_to_mday()` and `find_dst_change()`: - this may actually prevent potential issues, if the timezone were to be changed during these search loops - performance may improve, but is likely irrelevant/unnoticeable since this is rarely running code (as far as i can tell) - for `get_timezone_info()`: - it massively improves performance: `ftime` and `time` calls (of MSVCRT) will eventuall call this function, thus incur the massive syscall overhead - after the first use, the timezone is not *automatically* updated when changed via either: - changing the `TZ` environment variable of the running process - changing the timezone information file (usually `/etc/localtime`) and it was actually used e.g. typically when `TZ` is unset Personally, the potential performance gains are worth the limitation that a timezone change of the system will only be effective after application restart. But his could understandably be a point of contention. # Alternative Workarounds The performance issues of the `localtime()` call in the hotpath of `get_timezone_info()` can be: - mitigate for `time()` calls: by not querying timezone info - see upcoming MR TBD - partially mitigate by setting the `TZ` environment variable, but: - in many distros it is not set by default - the value of the variable is used by both the Unix C runtime (e.g. glibc) and MSVCRT and their supported format are not identical, so setting the correct value may be more difficult than usual or even impossible - when using `TZ=:/etc/localtime`, a special format for glibc, the information is still initally read from `/etc/localtime` but not checked for freshness on every subsequent call via stat - but this format is not understood by MSVCRT, thus breaking stuff there. # Origin of Discovery I've a private application that typically is running fairly well, but during certain workloads it becomes sluggish and downright unusable. I took some profiles with `perf` and found fairly clean culprit with `localtime()` and its stat syscall, which I measure the rate for: Under normal conditions I have about 200 per second and during the problematic phases it staturated at about 100k per second. I tracked the origin of call down to `_time64` -> `_ftime64` -> ... -> `get_timezone_info()` by matching callstack information with the list of exported symbols of the involved *.dll files. # Beyond `localtime_r()` Generally speaking, there are also a few opportunities to use _r variants of these time related functions on the Unix side of wine, e.g. for `gmtime()` in `get_zomezone_info()`. Some of such changes could also improve performance, but likely not to the same degree as getting rid of a entire syscall by switching from `localtime()` to `localtime_r()`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10892
From: Benjamin Fischer <bfis@gmx.net> At least for glibc: localtime() calls always check for timezone configuration changes which usually includes a stat syscall, localtime_r() does not. --- dlls/ntdll/unix/system.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/unix/system.c b/dlls/ntdll/unix/system.c index 148624ff405..d3de54deb1a 100644 --- a/dlls/ntdll/unix/system.c +++ b/dlls/ntdll/unix/system.c @@ -2694,12 +2694,12 @@ static int weekday_to_mday(int year, int day, int mon, int day_of_week) wday = 1; /* 1 - 1st, ...., 5 - last */ while (wday < day) { - struct tm *tm; + struct tm *tm, tmbuf; date.tm_mday += 7; date.tm_isdst = -1; tmp = mktime(&date); - tm = localtime(&tmp); + tm = localtime_r(&tmp, &tmbuf); if (tm->tm_mon != mon) break; mday = tm->tm_mday; @@ -2879,18 +2879,18 @@ static void find_reg_tz_info(RTL_DYNAMIC_TIME_ZONE_INFORMATION *tzi, int year) static time_t find_dst_change(time_t start, time_t end, int *is_dst) { - struct tm *tm; + struct tm *tm, tmbuf; ULONGLONG min = (sizeof(time_t) == sizeof(int)) ? (ULONG)start : start; ULONGLONG max = (sizeof(time_t) == sizeof(int)) ? (ULONG)end : end; time_t pos; - tm = localtime(&start); + tm = localtime_r(&start, &tmbuf); *is_dst = !tm->tm_isdst; TRACE("starting date isdst %d, %s", !*is_dst, ctime(&start)); for (pos = min; pos <= max; pos += 30 * 24 * 3600) { - tm = localtime(&pos); + tm = localtime_r(&pos, &tmbuf); if (tm->tm_isdst == *is_dst) { max = pos; @@ -2901,7 +2901,7 @@ static time_t find_dst_change(time_t start, time_t end, int *is_dst) while (min <= max) { pos = (min + max) / 2; - tm = localtime(&pos); + tm = localtime_r(&pos, &tmbuf); if (tm->tm_isdst != *is_dst) min = pos + 1; @@ -3000,7 +3000,7 @@ static void get_timezone_info( RTL_DYNAMIC_TIME_ZONE_INFORMATION *tzi ) static RTL_DYNAMIC_TIME_ZONE_INFORMATION cached_tzi; static int current_year = -1, current_bias = 65535; RTL_DYNAMIC_TIME_ZONE_INFORMATION reg_tzi; - struct tm *tm, tm1, tm2; + struct tm *tm, tmbuf, tm1, tm2; time_t year_start, year_end, tmp, dlt = 0, std = 0; int is_dst, bias; BOOL inverted_dst; @@ -3011,7 +3011,7 @@ static void get_timezone_info( RTL_DYNAMIC_TIME_ZONE_INFORMATION *tzi ) tm = gmtime(&year_start); bias = (LONG)(mktime(tm) - year_start) / 60; - tm = localtime(&year_start); + tm = localtime_r(&year_start, &tmbuf); if (current_year == tm->tm_year && current_bias == bias) { *tzi = cached_tzi; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10892
sorry about that, clicked wrong button, still not used to gitlab's redesign that moved mark as draft to where subscribe used to be -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10892#note_139818
Mhmm, I can't reproduce the 7 failures reported above - for me all these test succeed. I've looked into whether the windowing function are somehow touching the time stuff, but found no such connection - which makes sense since these are totally different concerns. Could it be that they are flaky, since they are working with window state across different threads? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10892#note_139865
participants (3)
-
Alfred Agrell (@Alcaro) -
Benjamin Fischer -
Benjamin Fischer (@bfis)