[PATCH 0/1] MR10892: ntdll/unix: use localtime_r() instead just localtime()

[Benjamin Fischer (＠bfis)]

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

[PATCH 1/1] ntdll/unix: use localtime_r() instead just localtime()

[Benjamin Fischer]

From: Benjamin Fischer <[e-mail]>

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

[Alfred Agrell (＠Alcaro)]

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

[Benjamin Fischer (＠bfis)]

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