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

Timer fix #475

Merged
merged 2 commits into from Oct 7, 2013

Conversation

Projects
None yet
4 participants
@binary1248
Member

binary1248 commented Oct 7, 2013

Since this issue popped up again on the forum (http://en.sfml-dev.org/forums/index.php?topic=13181.0) and was added to the tracker (#439), I decided to submit a pull request for my 9 month old patch so we can finally fix this problem. Tell me if any changes need to be made.

Added setting of the timer resolution in the Win32 implementation of …
…Sleep and rewrote the Unix Sleep implementation to use nanosleep instead of pthread_cond_timedwait to prevent spurious wakeups from causing the function to return too early.
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 7, 2013

Member

Thanks.

For Linux, do you have a link to something that explains why this hack is needed?

For Windows, I've read the forum topic again and couldn't find a clear conclusion. The fact that it changes the scheduler resolution system-wide is annoying. And since the typical usage of sf::sleep in for setFramerateLimit, in small apps that consume only a few percents of CPU, the result is that the scheduler period will be set to the smallest possible value most of the time.

As for changes, comments must start with a capital ;)

Member

LaurentGomila commented Oct 7, 2013

Thanks.

For Linux, do you have a link to something that explains why this hack is needed?

For Windows, I've read the forum topic again and couldn't find a clear conclusion. The fact that it changes the scheduler resolution system-wide is annoying. And since the typical usage of sf::sleep in for setFramerateLimit, in small apps that consume only a few percents of CPU, the result is that the scheduler period will be set to the smallest possible value most of the time.

As for changes, comments must start with a capital ;)

@ghost ghost assigned LaurentGomila Oct 7, 2013

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 7, 2013

Member

The Unix implementation isn't really a hack. It is how nanosleep is meant to be used. See: http://pubs.opengroup.org/onlinepubs/009604599/functions/nanosleep.html nanosleep should be supported on all modern Unix based systems.

The current implementation is more of a hack, relying on mutexes and condition variables and a (if you ask me) non-standard way of using pthread_cond_timedwait. Because of that, it is subject to "spurious wakeups". See: http://stackoverflow.com/questions/8594591/why-does-pthread-cond-wait-have-spurious-wakeups or just Google it for many other explanations. This is mentioned in the POSIX specification as well: http://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_cond_wait.html. The current implementation assumes that the only way the call to pthread_cond_timedwait can return is if the timeout elapses. This is not always the case, and recommended practice is to re-evaluate the predicate after returning from the call, as stated in the specification. Since this comes with overhead, I found nanosleep the simpler and more reliable solution.

As for the Windows fix, you need to consider what people have reported in the forum threads, typical applications like Chrome already make use of timeBeginPeriod/timeEndPeriod to get the system into high-resolution mode, and I don't think they take into account the power management policies in effect. Doing the same in SFML probably won't change much unless people like to run SFML applications on their own without any other running applications in the background. Chances are, those applications (if they rely on timers) already called timeBeginPeriod/timeEndPeriod themselves.

It is a tradeoff really... some people can't live with the fact that their FPS values deviate when they run other applications and others would rather have their SFML application save power. As I already said, there is no point to try saving power as a library developer when other applications obviously don't care at all. If people want their laptops to last longer, just don't run any real-time applications at all, including SFML applications, it would be that simple and is something people already understand.

The question of whether to set the timer resolution once at the start of the application or every time sleep is called depends on what you prefer really. The second option (and the one I prefer) is more fault tolerant, because the only way a system can get stuck in high-resolution mode is if the application crashes in between the calls, and that is highly unlikely. I don't know how other applications do it, but judging from Microsoft documentation, it seems like calling those functions often is something they anticipated users to do, so it shouldn't have a huge performance impact, especially since the thread is set to sleep anyway.

A nice side-effect is that sleep would become more precise, at least on Windows, due to the higher resolution. This might be the case with nanosleep as well, I don't know for sure.

Member

binary1248 commented Oct 7, 2013

The Unix implementation isn't really a hack. It is how nanosleep is meant to be used. See: http://pubs.opengroup.org/onlinepubs/009604599/functions/nanosleep.html nanosleep should be supported on all modern Unix based systems.

The current implementation is more of a hack, relying on mutexes and condition variables and a (if you ask me) non-standard way of using pthread_cond_timedwait. Because of that, it is subject to "spurious wakeups". See: http://stackoverflow.com/questions/8594591/why-does-pthread-cond-wait-have-spurious-wakeups or just Google it for many other explanations. This is mentioned in the POSIX specification as well: http://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_cond_wait.html. The current implementation assumes that the only way the call to pthread_cond_timedwait can return is if the timeout elapses. This is not always the case, and recommended practice is to re-evaluate the predicate after returning from the call, as stated in the specification. Since this comes with overhead, I found nanosleep the simpler and more reliable solution.

As for the Windows fix, you need to consider what people have reported in the forum threads, typical applications like Chrome already make use of timeBeginPeriod/timeEndPeriod to get the system into high-resolution mode, and I don't think they take into account the power management policies in effect. Doing the same in SFML probably won't change much unless people like to run SFML applications on their own without any other running applications in the background. Chances are, those applications (if they rely on timers) already called timeBeginPeriod/timeEndPeriod themselves.

It is a tradeoff really... some people can't live with the fact that their FPS values deviate when they run other applications and others would rather have their SFML application save power. As I already said, there is no point to try saving power as a library developer when other applications obviously don't care at all. If people want their laptops to last longer, just don't run any real-time applications at all, including SFML applications, it would be that simple and is something people already understand.

The question of whether to set the timer resolution once at the start of the application or every time sleep is called depends on what you prefer really. The second option (and the one I prefer) is more fault tolerant, because the only way a system can get stuck in high-resolution mode is if the application crashes in between the calls, and that is highly unlikely. I don't know how other applications do it, but judging from Microsoft documentation, it seems like calling those functions often is something they anticipated users to do, so it shouldn't have a huge performance impact, especially since the thread is set to sleep anyway.

A nice side-effect is that sleep would become more precise, at least on Windows, due to the higher resolution. This might be the case with nanosleep as well, I don't know for sure.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 7, 2013

Member

If you ask Google about timeBeginPeriod, you get all kind of results. Some say it makes no difference, some others say it must be avoided at all cost.

So I think we should try it and see what happens. It's just an implementation detail that can be reverted at any time.

Member

LaurentGomila commented Oct 7, 2013

If you ask Google about timeBeginPeriod, you get all kind of results. Some say it makes no difference, some others say it must be avoided at all cost.

So I think we should try it and see what happens. It's just an implementation detail that can be reverted at any time.

LaurentGomila added a commit that referenced this pull request Oct 7, 2013

Merge pull request #475 from binary1248/timer_resolution_fix
Increased the resolution of sf::sleep on Windows, improved the implementation of sf:sleep on Linux

@LaurentGomila LaurentGomila merged commit 5931236 into SFML:master Oct 7, 2013

@LaurentGomila LaurentGomila referenced this pull request Oct 7, 2013

Closed

High Resolution sleep #439

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 7, 2013

Member

Think changing the scheduler is perfectly valid (and in some way expected), but at the same time I'd try to avoid doing so if possible. I've seen other examples like this using a second thread and a "critical" section to block the calling thread, but I assume the scheduler would still run at its default intervall (i.e. nothing changing).

How about adding a static method to control the timer switching in case people want to write low power consumption stuff (e.g. mobile), even though this change right now affects Windows only?

Member

MarioLiebisch commented Oct 7, 2013

Think changing the scheduler is perfectly valid (and in some way expected), but at the same time I'd try to avoid doing so if possible. I've seen other examples like this using a second thread and a "critical" section to block the calling thread, but I assume the scheduler would still run at its default intervall (i.e. nothing changing).

How about adding a static method to control the timer switching in case people want to write low power consumption stuff (e.g. mobile), even though this change right now affects Windows only?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 7, 2013

Member

I don't get what you mean. Mobile as in mobile telephone (Android, IOS) or mobile as in mobile computer (laptop)? This code would not have any effects on mobile telephones (and for those, you probably can't specify timer resolution to start with anyway) and owners of laptops running Windows would expect a real-time graphical application to tax their system a bit. I can't think of any use case where one might run an SFML application and try to save power at the same time, considering merely browsing the internet already consumes "so much" power.

Member

binary1248 commented Oct 7, 2013

I don't get what you mean. Mobile as in mobile telephone (Android, IOS) or mobile as in mobile computer (laptop)? This code would not have any effects on mobile telephones (and for those, you probably can't specify timer resolution to start with anyway) and owners of laptops running Windows would expect a real-time graphical application to tax their system a bit. I can't think of any use case where one might run an SFML application and try to save power at the same time, considering merely browsing the internet already consumes "so much" power.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 7, 2013

Member

Thought of this in a more general way, not directly related to this change specifically. I'm aware that this only affects Windows so far and it wouldn't affect Android or iOS (and probably neither Windows Phone in case it would be supported one day). What I'd suggest is some general thing like setPerformancePolicy(...) with a parameter defining whether you prefer performance or power consumption/system default. In this case this would control whether you change the system's timing. If you do, you get more reliant timing/sleeping, at the cost of higher power consumption. If you don't want to, you don't.

Member

MarioLiebisch commented Oct 7, 2013

Thought of this in a more general way, not directly related to this change specifically. I'm aware that this only affects Windows so far and it wouldn't affect Android or iOS (and probably neither Windows Phone in case it would be supported one day). What I'd suggest is some general thing like setPerformancePolicy(...) with a parameter defining whether you prefer performance or power consumption/system default. In this case this would control whether you change the system's timing. If you do, you get more reliant timing/sleeping, at the cost of higher power consumption. If you don't want to, you don't.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 7, 2013

Member

Let's wait and see if people actually need such an option, before adding it...

Member

LaurentGomila commented Oct 7, 2013

Let's wait and see if people actually need such an option, before adding it...

@Meoo Meoo referenced this pull request Oct 12, 2013

Closed

Fix FPS bug on Windows #1

@eXpl0it3r eXpl0it3r added bug labels May 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment