Skip to content
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

Fallback to less precise frame pacing on non-Windows platforms #6941

Merged
merged 2 commits into from Dec 5, 2019

Conversation

@mrhelmut
Copy link
Contributor

mrhelmut commented Nov 12, 2019

Workaround for #6925 but reintroduces potential stuttering on non-Windows platforms (assuming it's better to have that issue back than maxing CPU usage).

Fixes #6925

@KakCAT

This comment has been minimized.

Copy link
Contributor

KakCAT commented Nov 12, 2019

Wouldn't it be better that different choices would be offered to the user when using fixed timestep? (i.e. accurate, power friendly)

I think it's one of those cases where a single code path does not work for all requirements, and you'll probably have another issue opened in a few months telling that it's not accurate enough and they want accuracy at the cost of power.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 12, 2019

You're very right. I don't have the time for now to do more, so this would basically be a workaround while we figure out either a real fix or add a switch like you're suggesting.

@MichaelDePiazzi

This comment has been minimized.

Copy link
Contributor

MichaelDePiazzi commented Nov 14, 2019

@mrhelmut Thanks again for addressing this!

As mentioned in the related issue, I did originally keep the sleeps in place for other platforms before it was decided to remove it (this was in my first PR #4207). But I did find that the frame timing accuracy could at least be improved by being more cautious with the use of Sleep. Long story short, this can be done like so:

if (sleepTime >= 1.0)
   System.Threading.Thread.Sleep(1);

So the reasoning for these changes are as follows:

  1. The minimum time we can sleep is 1ms, so there's no point trying to sleep if sleepTime is less than that otherwise it will overshoot the target. Increasing this threshold can actually improve the accuracy further at the cost of increased CPU usage (e.g. only sleeping if the sleepTime is >= 2ms would be even more accurate because the standard timer resolution is likely going to be 1ms in the majority of cases).
  2. Sleep is not accurate and the amount it does sleep is affected by the timer resolution. It can sleep up to the desired time plus the timer resolution (so a sleep time of 5ms with a timer resolution of 10ms means it can sleep anywhere from 5 to 15ms). So to minimise the potential impact of this, it's better to only sleep 1ms at a time until we get as close as possible to the next frame. It can still obviously overshoot the target, but the likelihood of this is reduced at least.

While these changes will still actually spin the CPU a little, my testing of this found the CPU usage to still be very low.

Hope this is useful!

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 14, 2019

Thanks for the insights!

I'm going to test this out. If it is that low in CPU usage, it could be a good way! Worst case scenario I'll propose a switch, though I believe it's going to work! If we're in a situation where the sleep time is close to 1ms, it means that we're already CPU or GPU bound anyway.

You might be right about 1ms resolution being the most widespread for the sleep timer. When I experimented with frame pacing it always snapped to the closest ms almost perfectly (at least on .Net and desktop Mono).

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 14, 2019

Alright, I made some more extensive testing on .Net Core 2.2. Here's where I'm at.

Method Maximum deviation (ms) Average deviation (ms) CPU load (%)
Vsync 0.1637 0.0258 <5
Brute force 0.0106 0.0013 100
Thread.Sleep(sleepTime) 1.0013 0.2728 <5
if (sleepTime >= 1.0) Thread.Sleep(1) 1.0152 0.4779 <5
if (sleepTime >= 2.0) Thread.Sleep(1) 0.1777 0.01108 <5
if (sleepTime >= 2.0) Thread.Sleep(sleepTime - 1) 0.2188 0.0121 <5
SpinWait.SpinOnce() 1.6485 0.8555 <5
TimerHelper.SleepForNoMoreThan(sleepTime) 0.1844 0.0023 <5

So it seems that from a cross-platform perspective, nothing beats Thread.Sleep(sleepTime).

The perfect situation would be to find ways to make TimerHelper cross-platform, but this looks complex in regard to nix systems and knowing which timer the thread scheduler uses (and we'll most likely get different results on each implementation).

I would suggest making TimerHelper compatible with DesktopGL when running on Windows, and in all other cases implementing a switch to choose between Thread.Sleep(sleepTime) and brute force.

Another note: we can slightly improve TimerHelper with NtSetTimerResolution.

@Jjagg

This comment has been minimized.

Copy link
Contributor

Jjagg commented Nov 14, 2019

I would suggest making TimerHelper compatible with DesktopGL when running on Windows, and in all other cases implementing a switch to choose between Thread.Sleep(sleepTime) and brute force.

I'm in favor of this. If we can do better than Thread.Sleep like for Windows that's great, but in case we don't we absolutely need users to be able to choose what's more important to them.

@tomspilman

This comment has been minimized.

Copy link
Member

tomspilman commented Nov 14, 2019

The only thing i have to add here is maybe we should err more on the side of performance and not trying to be as cooperative with other processes. If we do...

if (sleepTime >= SleepThreshold)
  Thread.Sleep(sleepTime /* or SleepThreshold */);

We then tune SleepThreshold... maybe per platform.... to get a solid frame rate and sleep off only when we have a lot more spare performance. So maybe SleepThreshold = 4 or SleepThreshold = 8 or something.... whatever seems to work best.

@MichaelDePiazzi

This comment has been minimized.

Copy link
Contributor

MichaelDePiazzi commented Nov 14, 2019

Nice work @mrhelmut 👍

From your results, I'm guessing that your timer resolution is most likely 1ms while you're running these tests. In this case, Sleep(sleepTime) will actually be fairly close most of the time. However, it's when the timer resolution is coarser (e.g. 10ms, 15.6ms) that you'll find this method becomes very inaccurate. In these cases, you should find that the method I suggested (i.e. Sleep(1) with the threshold check) should perform better.

Just to prove something, could you please try benchmark one other method?

if (sleepTime >= 2.0)
    System.Threading.Thread.Sleep(1);

When the timer resolution is <= 1ms, this should give the same results as SleepForNoMoreThan. The CPU usage will be slightly increased though.

The 2ms threshold comes from the fact that when the timer resolution is 1ms, Sleep(1) will sleep up to 2ms (1ms desired time + 1ms timer resolution).

I would suggest making TimerHelper compatible with DesktopGL when running on Windows, and in all other cases implementing a switch to choose between Thread.Sleep(sleepTime) and brute force.

This would definitely be nice. And I am also in favour of a switch for other platforms, otherwise we are going backwards in regards to accuracy. The user should be able to choose between power-saving and accuracy.

Another note: we can slightly improve TimerHelper with NtSetTimerResolution.

Agree. I'm actually setting the timer resolution to the maximum (which is typically 0.5ms) in my game. While it won't actually increase frame timing accuracy, it can actually reduce CPU usage (because it can sleep more and spin less).

EDIT: Additional note though -- This is only really relevant when using fixed time step. So because increasing the timer resolution increases power usage, it may not be a good default behaviour for all cases.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 15, 2019

@MichaelDePiazzi Ah! I misunderstood your initial thoughts about sleepTime >= 2.0. I re-ran the tests and updated the results in my previous comment. It is indeed pretty close to TimerHelper!

And you're right, the default resolution reported by NtQueryTimerResolution is 0.9989ms on all of my Win10 computers.

Which makes if (sleepTime >= 2.0) Thread.Sleep(sleepTime - 1) relevant too! (I've added it to the table)

I've added Vsync timing to the table for comparison.

I've also tested on macOS (Mono 5.20) and the results are very comparable to Windows (and within Vsync performance range), so I'm assuming that macOS default timer resolution is close to 1ms as well.

@tomspilman This is interesting and pretty close to Michael's proposal. With some testing on all platforms we could empirically define constants for each of them to have a somehow precise default behavior. This is actually close to implementing our own SpinWait with relevant values.

@MichaelDePiazzi

This comment has been minimized.

Copy link
Contributor

MichaelDePiazzi commented Nov 15, 2019

Just be aware that you may not be able to guarantee the timer resolution will be any specific value, unless:

  • You can actually query the current resolution like I've done on Windows with SleepForNoMoreThan
  • You can actually set the timer resolution
  • Or a particular platform uses a specific fixed timer resolution

But based on my experiences (with Windows setups at least), it's typically gonna be 1ms (the video drivers and/or the graphics API seem to be setting it). So this is why stuttering is typically not an issue, as 1ms is good enough to hide noticeable inaccuracies. However, I have come across setups where it doesn't get set to a finer resolution and remains at something like 10ms or 15.6ms. This is when stuttering is reported as a problem as Sleep(sleepTime) can then overshoot by up to 15.6ms which is very noticeable!

So unless you can either query or set the timer resolution, I feel the best trade-off for most platforms will actually be "if (sleepTime >= 2.0) Thread.Sleep(1)". This will be very accurate for the majority of cases where the timer resolution is 1ms. And then for less typical cases where the timer resolution is coarser, Sleep(1) will further help to minimise overshooting.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 16, 2019

I believe that you are very right here. It would probably be complex to figure out a fixed threshold for each platform if the accuracy is so widely varying. We can always query/set that on Windows, but most other platforms most probably won't allow that.
I'm expecting consoles to have a fixed low accuracy, but mobiles are probably going to be a wide range of wilds values, especially on Android.

So TimerHelper + if (sleepTime >= 2.0) Thread.Sleep(1) seems to be the best trade-off for short term resolution of this issue.
Long term would be to find out if it's possible to query/set the timer accuracy bound to Thread.Sleep() for other platforms to make TimerHelper cross-platform.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 16, 2019

I dug into the Mono source, it's using clock_nanosleep when available, or falls back to nanosleep. Those are extremely precise, though it sleeps for at least the amount and then sync on the next system tick, which on Linux (inc. Android) can be between 1 to 16ms (depending on distros) and macOS is about 1ms from my experience.
So the typical accuracy is 1 to 16ms to return, pretty much like Windows.

iOS seems to have a 1ms precision as well. I can't test Android but I'm expecting this to be different on each devices.

@tomspilman

This comment has been minimized.

Copy link
Member

tomspilman commented Nov 16, 2019

I'm expecting consoles to have a fixed low accuracy,

For consoles we should just spinlock and burn CPU time to be 100% accurate.

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Nov 27, 2019

I pushed a revision.
I wasn't very confident with using TimerHelper on DesktopGL so I used the if (sleepTime >= 2.0) Thread.Sleep(1) trick there (which work just as well on Windows, macOS, and Linux), as well as the other platforms.
I made it so consoles spin for accuracy.

This should fix the majority of cases.
There's still room for improvements but it's a per-platform issue I can't tackle myself (I believe it's going to work nicely everywhere as-is, I just don't know about Android, but I'm expecting most devices to enforce Vsync on a system level).

@MichaelDePiazzi

This comment has been minimized.

Copy link
Contributor

MichaelDePiazzi commented Dec 3, 2019

Looks good to me! 👍

@mrhelmut

This comment has been minimized.

Copy link
Contributor Author

mrhelmut commented Dec 5, 2019

@Jjagg @cra0zy Any chance to have a review? :)

@Jjagg

This comment has been minimized.

Copy link
Contributor

Jjagg commented Dec 5, 2019

This is a lot better. Thanks @mrhelmut :)

@Jjagg Jjagg merged commit 1046459 into MonoGame:develop Dec 5, 2019
5 of 6 checks passed
5 of 6 checks passed
Package Mac and Linux Finished TeamCity Build MonoGame / Package Mac and Linux : Exit code 1 (Step: Running NAnt Script (NAnt))
Details
Build Mac, iOS, and Linux Finished TeamCity Build MonoGame / Build Mac : Running
Details
Build Windows, Web, Android, and OUYA Finished TeamCity Build MonoGame / Build Windows : Running
Details
Package Windows SDK Finished TeamCity Build MonoGame / Package Windows : Running
Details
Test Mac Finished TeamCity Build MonoGame / Test Mac : Tests passed: 1150, ignored: 46
Details
Test Windows Finished TeamCity Build MonoGame / Test Windows : Tests passed: 1254, ignored: 20
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.