-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use nanosleep for non-Windows platforms #6392
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, there are a lot of unknowns/variables when trying to implement this in C#: the native library name, definition of struct timespec
and EINTR
.
Maybe it's better to call SDL3.SDL_DelayNS()
and have a comment explaining that it's just a wrapper for nanosleep()
on Unix platforms.
public long Seconds; | ||
public long NanoSeconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these need to be IntPtr
to work properly on 32-bit Unix platforms. Since time_t
and long
are the underlying types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had also thought about this, but do we have any plans to support 32bit systems in future? I've seen many discussions which suggested 32bit won't be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nint
also works, but I find this pedantry. 32-bit unices are no longer relevant in $CURRENT_YEAR. We don't even ship all binaries for linux-x86 (ffmpeg is missing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android arm32 (armeabi-v7a
) might be relevant. If you don't want to use IntPtr
/nint
here, make this code only work on 64-bit systems by checking Environment.Is64BitProcess
.
[DllImport("libc.so.6", EntryPoint = "nanosleep", CallingConvention = CallingConvention.Cdecl, SetLastError = true)] | ||
private static extern int nanosleep_libc6(in TimeSpec duration, out TimeSpec rem); | ||
|
||
private const int interrupt_error = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://man7.org/linux/man-pages/man3/errno.3.html
The error numbers that correspond to each symbolic name vary
across UNIX systems, and even across different architectures on
Linux. Therefore, numeric values are not included as part of the
list of error names below. The perror(3) and strerror(3)
functions can be used to convert these names to corresponding
textual error messages.
We can't really define a numerical constant for this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EINTR seems to be 4 on all platforms we support. I need confirmation on Apple devices, though.
I also agree that this is not reliable, but anyway..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this is pedantry. As long as it works it's fine to hardcode. If it doesn't work somewhere, then we'll find out eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the decompiled SDL3 code in SDL3-CS/native
and it's always 4.
So do we just try using SDL_DelayNS again? I would prefer using SDL_DelayNS for Unix platforms as targeting different libc/arch in C# doesn't sound good for me, but I also agree that using an SDL function in Clock doesn't match well either (might be good enough with the comment as Susko3 mentioned). |
Where's the testing/benchmarking at? I'd want to see some kind of proof this is beneficial before blindly implementing. I'd hope this is done by the implementor as to not take time away from the core team. |
If this diff can't work then I'd rather have nothing than sdl in threading code paths. |
// Android and some platforms don't have version in lib name. | ||
[DllImport("c", EntryPoint = "nanosleep", CallingConvention = CallingConvention.Cdecl, SetLastError = true)] | ||
private static extern int nanosleep_c(in TimeSpec duration, out TimeSpec rem); | ||
|
||
[DllImport("libc.so.6", EntryPoint = "nanosleep", CallingConvention = CallingConvention.Cdecl, SetLastError = true)] | ||
private static extern int nanosleep_libc6(in TimeSpec duration, out TimeSpec rem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just import "libc"
and let .NET deal with it.
This avoids having two native functions and the delegate. We can probably assume nanosleep()
is always available, so there's no need for the static constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full proposal implementing the 64-bit check:
diff --git a/osu.Framework/Platform/Linux/Native/UnixNativeSleep.cs b/osu.Framework/Platform/Linux/Native/UnixNativeSleep.cs
index d0bf152b3..8418fbff2 100644
--- a/osu.Framework/Platform/Linux/Native/UnixNativeSleep.cs
+++ b/osu.Framework/Platform/Linux/Native/UnixNativeSleep.cs
@@ -8,6 +8,8 @@ namespace osu.Framework.Platform.Linux.Native
{
internal class UnixNativeSleep : INativeSleep
{
+ public static bool Available => Environment.Is64BitProcess;
+
[StructLayout(LayoutKind.Sequential)]
public struct TimeSpec
{
@@ -15,56 +17,13 @@ public struct TimeSpec
public long NanoSeconds;
}
- private delegate int NanoSleepDelegate(in TimeSpec duration, out TimeSpec rem);
-
- private static readonly NanoSleepDelegate? nanosleep;
-
- // Android and some platforms don't have version in lib name.
- [DllImport("c", EntryPoint = "nanosleep", CallingConvention = CallingConvention.Cdecl, SetLastError = true)]
- private static extern int nanosleep_c(in TimeSpec duration, out TimeSpec rem);
-
- [DllImport("libc.so.6", EntryPoint = "nanosleep", CallingConvention = CallingConvention.Cdecl, SetLastError = true)]
- private static extern int nanosleep_libc6(in TimeSpec duration, out TimeSpec rem);
+ [DllImport("libc", CallingConvention = CallingConvention.Cdecl, SetLastError = true)]
+ private static extern int nanosleep(in TimeSpec duration, out TimeSpec rem);
private const int interrupt_error = 4;
- static UnixNativeSleep()
- {
- TimeSpec test = new TimeSpec
- {
- Seconds = 0,
- NanoSeconds = 1,
- };
-
- try
- {
- nanosleep_c(in test, out _);
- nanosleep = nanosleep_c;
- }
- catch
- {
- }
-
- if (nanosleep == null)
- {
- try
- {
- nanosleep_libc6(in test, out _);
- nanosleep = nanosleep_libc6;
- }
- catch
- {
- }
- }
-
- // if nanosleep is null at this point, Thread.Sleep should be used.
- }
-
public bool Sleep(TimeSpan duration)
{
- if (nanosleep == null)
- return false;
-
const int ns_per_second = 1000 * 1000 * 1000;
long ns = (long)duration.TotalNanoseconds;
diff --git a/osu.Framework/Timing/ThrottledFrameClock.cs b/osu.Framework/Timing/ThrottledFrameClock.cs
index 4f827d68c..e0355c33f 100644
--- a/osu.Framework/Timing/ThrottledFrameClock.cs
+++ b/osu.Framework/Timing/ThrottledFrameClock.cs
@@ -33,13 +33,13 @@ public class ThrottledFrameClock : FramedClock, IDisposable
/// </summary>
public double TimeSlept { get; private set; }
- private readonly INativeSleep nativeSleep;
+ private readonly INativeSleep? nativeSleep;
internal ThrottledFrameClock()
{
if (RuntimeInfo.OS == RuntimeInfo.Platform.Windows)
nativeSleep = new WindowsNativeSleep();
- else
+ else if (RuntimeInfo.IsUnix && UnixNativeSleep.Available)
nativeSleep = new UnixNativeSleep();
}
@@ -95,7 +95,7 @@ private double sleepAndUpdateCurrent(double milliseconds)
TimeSpan timeSpan = TimeSpan.FromMilliseconds(milliseconds);
- if (!nativeSleep.Sleep(timeSpan))
+ if (nativeSleep?.Sleep(timeSpan) != true)
Thread.Sleep(timeSpan);
return (CurrentTime = SourceTime) - before;
@@ -103,7 +103,7 @@ private double sleepAndUpdateCurrent(double milliseconds)
public void Dispose()
{
- nativeSleep.Dispose();
+ nativeSleep?.Dispose();
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I noticed your comments after pushing... I think it isn't that bad to use nint if we want to support 32bit Android.
I will revert if anyone thinks otherwise.
I would prefer if this was benchmarked properly using BenchmarkDotNet to provide reproducible empirical results if there's any benefits. These screenshots won't do. |
0.5ms duration:
1ms duration:
1.5ms duration:
I think Also, I assumed screenshots were enough to show near 0.00ms frame time stddev, but I agree that this is more clear. |
SDL_DelayNS
for lower thread jitter on non-Windows platforms #6391This PR lets Unix platforms use
nanosleep
instead ofThread.Sleep
to improve on frame pacing. I will test on Android after some time, but I don't have any Apple devices to test this on.I put
UnixNativeSleep
inPlatform/Linux/Native
, but I don't think it's the best fit... Do I need to create a new folder, or is it good as is?Testing: