[PATCH v2 0/1] MR6007: ntdll: Don't use CLOCK_REALTIME_COARSE
This reverts commit 5c8ea25014f ("ntdll: Use CLOCK_REALTIME_COARSE for NtQuerySystemTime() if it has sufficient resolution.") CLOCK_*_COARSE only provides up to 1ms resolution at CONFIG_HZ=1000. OTOH, there are several ways to get up to 0.5ms resolution on modern Windows (high resolution waitable timers, NtSetTimerResolution with 0.5ms). This code path therefore has a possibility of behaving worse than native. Since COARSE resolution is HZ dependent, this code path only runs if the kernel is configured with CONFIG_HZ=1000. Most distro ships does not ship with this. Therefore, this code path is rarely tested, and is more of a recipe for surprise. If any application rely on fast NtQuerySystemTime they are likely already broken for majority of Wine users. Given the above reason, don't use CLOCK_REALTIME_COARSE. Use gettimeofday which is internally hooked to CLOCK_REALTIME. -- v2: ntdll: Don't use CLOCK_REALTIME_COARSE https://gitlab.winehq.org/wine/wine/-/merge_requests/6007
From: Tatsuyuki Ishi <ishitatsuyuki(a)gmail.com> This reverts commit 5c8ea25014f ("ntdll: Use CLOCK_REALTIME_COARSE for NtQuerySystemTime() if it has sufficient resolution.") CLOCK_*_COARSE only provides up to 1ms resolution at CONFIG_HZ=1000. OTOH, there are several ways to get up to 0.5ms resolution on modern Windows (high resolution waitable timers, NtSetTimerResolution with 0.5ms). This code path therefore has a possibility of behaving worse than native. Since COARSE resolution is HZ dependent, this code path only runs if the kernel is configured with CONFIG_HZ=1000. Most distro ships does not ship with this. Therefore, this code path is rarely tested, and is more of a recipe for surprise. If any application rely on fast NtQuerySystemTime they are likely already broken for majority of Wine users. Given the above reason, don't use CLOCK_REALTIME_COARSE. Use gettimeofday which is internally hooked to CLOCK_REALTIME. --- dlls/ntdll/unix/sync.c | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index bfbcaf4a851..97a6e2b5ffc 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -1579,35 +1579,10 @@ NTSTATUS WINAPI NtQueryPerformanceCounter( LARGE_INTEGER *counter, LARGE_INTEGER */ NTSTATUS WINAPI NtQuerySystemTime( LARGE_INTEGER *time ) { -#ifdef HAVE_CLOCK_GETTIME - struct timespec ts; - static clockid_t clock_id = CLOCK_MONOTONIC; /* placeholder */ - - if (clock_id == CLOCK_MONOTONIC) - { -#ifdef CLOCK_REALTIME_COARSE - struct timespec res; - - /* Use CLOCK_REALTIME_COARSE if it has 1 ms or better resolution */ - if (!clock_getres( CLOCK_REALTIME_COARSE, &res ) && res.tv_sec == 0 && res.tv_nsec <= 1000000) - clock_id = CLOCK_REALTIME_COARSE; - else -#endif /* CLOCK_REALTIME_COARSE */ - clock_id = CLOCK_REALTIME; - } - - if (!clock_gettime( clock_id, &ts )) - { - time->QuadPart = ticks_from_time_t( ts.tv_sec ) + (ts.tv_nsec + 50) / 100; - } - else -#endif /* HAVE_CLOCK_GETTIME */ - { - struct timeval now; + struct timeval now; - gettimeofday( &now, 0 ); - time->QuadPart = ticks_from_time_t( now.tv_sec ) + now.tv_usec * 10; - } + gettimeofday( &now, 0 ); + time->QuadPart = ticks_from_time_t( now.tv_sec ) + now.tv_usec * 10; return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6007
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=146864 Your paranoid android. === debian11b (64 bit WoW report) === user32: win.c:4037: Test failed: Expected active window 00000000073D0130, got 0000000001AF014C. win.c:4038: Test failed: Expected focus window 00000000073D0130, got 0000000001AF014C.
Show at least one test script that indicates the need to undo a commit that is more than 5 years old. You also just need to delete the desired section of code, instead of completely undoing the commit. Would like to see tests showing the rationality of completely undoing a commit. ``` #ifdef CLOCK_REALTIME_COARSE struct timespec res; /* Use CLOCK_REALTIME_COARSE if it has 1 ms or better resolution */ if (!clock_getres( CLOCK_REALTIME_COARSE, &res ) && res.tv_sec == 0 && res.tv_nsec <= 1000000) clock_id = CLOCK_REALTIME_COARSE; else #endif /* CLOCK_REALTIME_COARSE */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75912
On Thu Jul 11 12:51:07 2024 +0000, Grigory Vasilyev wrote:
Show at least one test script that indicates the need to undo a commit that is more than 5 years old. You also just need to delete the desired section of code or unset `CLOCK_REALTIME_COARSE`, instead of completely undoing the commit. Would like to see tests showing the rationality of completely undoing a commit. ``` #ifdef CLOCK_REALTIME_COARSE struct timespec res; /* Use CLOCK_REALTIME_COARSE if it has 1 ms or better resolution */ if (!clock_getres( CLOCK_REALTIME_COARSE, &res ) && res.tv_sec == 0 && res.tv_nsec <= 1000000) clock_id = CLOCK_REALTIME_COARSE; else #endif /* CLOCK_REALTIME_COARSE */ ``` Both a clean revert or removing the CLOCK_REALTIME coarse path are reasonable ways to achieve the goal. We can go for endless bikeshed but the outcome is the same.
I've provided sufficient justification in the commit message. If you want to craft a test case, you can do one by measuring the accuracy of NtDelayExecution. BTW, one of the reason I want this is because I want count on NtDelayExecution being accurate on Wine (but not on Windows). I'm developing a frame limiter specific for Proton use and that allows me to accurately sleep without all the redundant spinning to compensate for native Windows' poor timer accuracy. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75915
On Thu Jul 11 12:51:07 2024 +0000, Tatsuyuki Ishi wrote:
Both a clean revert or removing the CLOCK_REALTIME coarse path are reasonable ways to achieve the goal. We can go for endless bikeshed but the outcome is the same. I've provided sufficient justification in the commit message. If you want to craft a test case, you can do one by measuring the accuracy of NtDelayExecution. BTW, one of the reason I want this is because I want count on NtDelayExecution being accurate on Wine (but not on Windows). I'm developing a frame limiter specific for Proton use and that allows me to accurately sleep without all the redundant spinning to compensate for native Windows' poor timer accuracy. @ishitatsuyuki Using a real-time clock to limit frame time is not a rational solution. In such cases, use CLOCK_MONOTONIC. CLOCK_MONOTONIC is used by all graphics libraries. The real time clock can change the time abruptly, for example in the case of time adjustment via NTP, hence you can get unpredictable behavior in your code. Also, the real time clock has a higher overhead. In your comment to the commit, I don’t see any tests, I only see that you came up with the need for such a solution in Wine. Therefore, I ask you to provide tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75919
On Thu Jul 11 13:03:05 2024 +0000, Grigory Vasilyev wrote:
@ishitatsuyuki Using a real-time clock to limit frame time is not a rational solution. In such cases, use CLOCK_MONOTONIC. CLOCK_MONOTONIC is used by all graphics libraries. The real time clock can change the time abruptly, for example in the case of time adjustment via NTP, hence you can get unpredictable behavior in your code. Also, the real time clock has a higher overhead. In your comment to the commit, I don’t see any tests, I only see that you came up with the need for such a solution in Wine. Therefore, I ask you to provide tests. Yeah, but did you know that NtQuerySystemTime() is called internally by Wine's NtDelayExecution()?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75920
On Thu Jul 11 13:04:43 2024 +0000, Tatsuyuki Ishi wrote:
Yeah, but did you know that NtQuerySystemTime() is called internally by Wine's NtDelayExecution()? @ishitatsuyuki in this case you are probably better to change the behavior of `NtDelayExecution`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75921
On Thu Jul 11 13:12:09 2024 +0000, Grigory Vasilyev wrote:
@ishitatsuyuki in this case you are probably better to change the behavior of `NtDelayExecution`. Yeah, good luck shaving that yak because now you are risking breaking even more apps.
Anyway, I'd be happy to explain if you weren't being confrontational, but otherwise I'd just leave this to Huw to review since they know how the code works. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75923
On Thu Jul 11 13:12:09 2024 +0000, Tatsuyuki Ishi wrote:
Yeah, good luck shaving that yak because now you are risking breaking even more apps. Anyway, I'd be happy to explain if you weren't being confrontational, but otherwise I'd just leave this to Huw to review since they know how the code works. @ishitatsuyuki you are talking nonsense, you can change the behavior of `NtDelayExecution` so that it does not cause any regressions. So I ask you to provide real tests why we should just uncommit.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75925
On Thu Jul 11 13:18:01 2024 +0000, Grigory Vasilyev wrote:
@ishitatsuyuki you are talking nonsense, you can change the behavior of `NtDelayExecution` so that it does not cause any regressions. So I ask you to provide real tests why we should just uncommit. @ishitatsuyuki Also, if you are writing Proton specific code. Then you are better to implement this as a separate module, and using all the features of *nix, without using Windows system libraries. For example, Proton implements `amd_ags_x64`, `atiadlxx`.
https://github.com/ValveSoftware/wine/tree/bleeding-edge/dlls/atiadlxx https://github.com/ValveSoftware/wine/tree/bleeding-edge/dlls/amd_ags_x64 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75926
On Thu Jul 11 13:36:55 2024 +0000, Grigory Vasilyev wrote:
@ishitatsuyuki Also, if you are writing Proton specific code. Then you are better to implement this as a separate module, and using all the features of *nix, without using Windows system libraries. For example, Proton implements `amd_ags_x64`, `atiadlxx`. https://github.com/ValveSoftware/wine/tree/bleeding-edge/dlls/atiadlxx https://github.com/ValveSoftware/wine/tree/bleeding-edge/dlls/amd_ags_x64 I know about Unixlibs and I'm not using them because they are a PITA to distribute out-of-tree due to Steam runtime compatibility. Also, this is getting off-topic.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6007#note_75928
participants (4)
-
Grigory Vasilyev (＠h0tc0d3) -
Marvin -
Tatsuyuki Ishi
-
Tatsuyuki Ishi (＠ishitatsuyuki)