FreeSWITCH

[patch] Improved timing in soft timer, especially for Windows, but for other systems as well

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.0.7
  • Fix Version/s: 1.0.7
  • Component/s: freeswitch-core
  • Labels:
    None
  • Environment:
    Visual Studio 2008, Windows 7
  • Platform:
    Vista/VS
  • FreeSWITCH GIT Revision:
    883de14f3068d8cc269cccce3895effc750ef108
  • Reproduced with GIT HEAD?:
    Yes
  • Target Version:

Description

This patch makes a few changes to the FS soft timer;

1. Removed a couple of debug log lines I once added to timer_test, it's really not needed anymore, it was more for proof of concept way back. I also enabled another log line which was removed for Windows, I couldn't find a reason why that was disabled.

2. Improved timer_test report accuracy, made it not to include the time it will take for the log to be written (really not much, but still).

3. Changed timer loop to actually sleep the time it needs to, instead of always using runtime.microseconds_per_tick, this will make default 20ms timer work better on OS'es not using timerfd. Since the loop will take some time to actually trigger the waiting timers, using a fixed value the timer will drift more and more, and eventually catch up again, but changing it to sleep a shorter time will give better precision. I used to enable 1ms-timer for this, but after this change it's not needed anymore.

4. Windows: Added QueryPerformanceCounter() reference, insead of timeGetTime(). This is ony activated if it detects Vista or later (sonce some reports on Internet says that it's not always stable on older OS'es), and it also does a basic init check so that it actually returns good values as well. If something seems bad it reverts to the good old timeGetTime(). As for timeGetTime() it also requires that start parameter "-monotonic-clock". This add usec precision to the time reported, and it also removes the need for mutex lock around timeGetTime(), it also has a faster execution time compared to timeGetTime().

5. Windows: Disabled timer calibration on Windows, it's really no use anyway, since we only have ms sleep - and it's not even implemented in the FS code.

/Peter

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Peter Olsson added a comment -
Uploaded updated patch, just to make sure to reinit timing stuff if virtual migration is detected.
Show
Peter Olsson added a comment - Uploaded updated patch, just to make sure to reinit timing stuff if virtual migration is detected.
Hide
Permalink
Jeff Lenk added a comment -
I have been using GetSystemTimeAsFileTime as the time base for my windows systems(Server 2008 and newer) so I'm not sure what benefit the changes for QueryPermanceCounter have. On windows we are only accurate to a 1 millisec sleep anyway.

The code path for GetSystemTimeAsFileTime has always been the simplest and I have not seen timer problems with this so I am not certain that eliminating it from the code is a good idea.

What systems are you running this on and how have you measured the actual improvement you are seeing?

The other changes I dont have any comments for as of yet. Im sure others will too.
Show
Jeff Lenk added a comment - I have been using GetSystemTimeAsFileTime as the time base for my windows systems(Server 2008 and newer) so I'm not sure what benefit the changes for QueryPermanceCounter have. On windows we are only accurate to a 1 millisec sleep anyway. The code path for GetSystemTimeAsFileTime has always been the simplest and I have not seen timer problems with this so I am not certain that eliminating it from the code is a good idea. What systems are you running this on and how have you measured the actual improvement you are seeing? The other changes I dont have any comments for as of yet. Im sure others will too.
Hide
Permalink
Peter Olsson added a comment -
GetSystemTimeAsFileTime is still there, my code only changes the usage when -monotonic-clock is enabled. Also, GetSystemTimeAsFileTime is not that accurate, it has usec resolution, but It's not really updated by the kernel that often. QueryPerformanceCounters is much more reliable, and It's also monotonic, which I personally prefer, since its not affected by time changes in the OS. timeGetTime is also a accurate monotonic clock, but it uses more resources, is only 32-bit, and only ms resolution.
Show
Peter Olsson added a comment - GetSystemTimeAsFileTime is still there, my code only changes the usage when -monotonic-clock is enabled. Also, GetSystemTimeAsFileTime is not that accurate, it has usec resolution, but It's not really updated by the kernel that often. QueryPerformanceCounters is much more reliable, and It's also monotonic, which I personally prefer, since its not affected by time changes in the OS. timeGetTime is also a accurate monotonic clock, but it uses more resources, is only 32-bit, and only ms resolution.
Hide
Permalink
Jeff Lenk added a comment -
oh I will look more carefully thanks for the clarification.
Show
Jeff Lenk added a comment - oh I will look more carefully thanks for the clarification.
Hide
Permalink
Peter Olsson added a comment -
Thanks for looking into it, get back to me if you have further questions. Just to be clear, the goal was to replace timeGetTime where possible, nothing else :)

Peter
Show
Peter Olsson added a comment - Thanks for looking into it, get back to me if you have further questions. Just to be clear, the goal was to replace timeGetTime where possible, nothing else :) Peter
Hide
Permalink
garmt added a comment -
Peter, is your recommendation to always use -monotonic-clock if MajorVersion > 6?
If so, why not make -monotonic-clock the default on windows if the proper version is detected?
Is there a special problem scenario that makes you need to improve the timer?
---
s/suceeded/succeeded
---
I think that it is better not to log "Clock callibration disabled" at all (to prevent the question: how can I enable it ...), or alternatively use "Clock callibration not supported"
Show
garmt added a comment - Peter, is your recommendation to always use -monotonic-clock if MajorVersion > 6? If so, why not make -monotonic-clock the default on windows if the proper version is detected? Is there a special problem scenario that makes you need to improve the timer? --- s/suceeded/succeeded --- I think that it is better not to log "Clock callibration disabled" at all (to prevent the question: how can I enable it ...), or alternatively use "Clock callibration not supported"
Hide
Permalink
Peter Olsson added a comment -
Hey Garmt!

Yes, my recommendation is to always use monotonic time, since it won't be changed if you have a system time change. I especially recommend it on everything older then Vista, since the actual resolution for GetSystemTimeAsFileTime in these systems is about 2-3 ms (even though the result is in usec). In Vista and later GetSystemTimeAsFileTime works just fine (about 0.5-1ms resolution I think), however, the monotonic clock is better becuase of it's independance to the system time.

In this case the basic problem I wanted to solve was to be able to use default 20ms timer, instead om 1ms-timer. However, that one is solved bu just changing the sleep time in the timer loop, to the actual time diff, instead of always do 20ms. But while I were at it I also wanted to get more accurate results when doing timer_test, and since QueryRecordCounters() is the most accurate timing source on Windows (and is usec accurate) I choosed that one (also the MS recommendation). However, there have been reports on older systems (older than Vista) that it sometimes reported bad values, and that the frequency could change (even though MS says it can't), but this is supposed to work correctly in Vista and later.

We could probably do monotonic-clock by default, I think most would benefit by it. When I first patched FS and introduced it, I wasn't sure how good it would work, so I did a parameter for it, but since that time I always use it, and it has worked flawlessly :)

/Peter
Show
Peter Olsson added a comment - Hey Garmt! Yes, my recommendation is to always use monotonic time, since it won't be changed if you have a system time change. I especially recommend it on everything older then Vista, since the actual resolution for GetSystemTimeAsFileTime in these systems is about 2-3 ms (even though the result is in usec). In Vista and later GetSystemTimeAsFileTime works just fine (about 0.5-1ms resolution I think), however, the monotonic clock is better becuase of it's independance to the system time. In this case the basic problem I wanted to solve was to be able to use default 20ms timer, instead om 1ms-timer. However, that one is solved bu just changing the sleep time in the timer loop, to the actual time diff, instead of always do 20ms. But while I were at it I also wanted to get more accurate results when doing timer_test, and since QueryRecordCounters() is the most accurate timing source on Windows (and is usec accurate) I choosed that one (also the MS recommendation). However, there have been reports on older systems (older than Vista) that it sometimes reported bad values, and that the frequency could change (even though MS says it can't), but this is supposed to work correctly in Vista and later. We could probably do monotonic-clock by default, I think most would benefit by it. When I first patched FS and introduced it, I wasn't sure how good it would work, so I did a parameter for it, but since that time I always use it, and it has worked flawlessly :) /Peter
Hide
Permalink
Peter Olsson added a comment -
Jeff, I just thought of something that might cause a compiler warning on other systems.

Since all code within win32_init_timers() is ifdef'ed, we might need to have a return in the end of the function - I'm guessing some picky compilers will complain about a completely empty function.

/Peter
Show
Peter Olsson added a comment - Jeff, I just thought of something that might cause a compiler warning on other systems. Since all code within win32_init_timers() is ifdef'ed, we might need to have a return in the end of the function - I'm guessing some picky compilers will complain about a completely empty function. /Peter
Hide
Permalink
its.solution added a comment -
I tried the patch and see the compilation error:


src/switch_time.c: In function 'softtimer_load':
src/switch_time.c:1236: error: 'win32_use_qpc' undeclared (first use in this function)
src/switch_time.c:1236: error: (Each undeclared identifier is reported only once
src/switch_time.c:1236: error: for each function it appears in.)
make[1]: *** [libfreeswitch_la-switch_time.lo] Error 1
make: *** [all] Error 2
Show
its.solution added a comment - I tried the patch and see the compilation error: src/switch_time.c: In function 'softtimer_load': src/switch_time.c:1236: error: 'win32_use_qpc' undeclared (first use in this function) src/switch_time.c:1236: error: (Each undeclared identifier is reported only once src/switch_time.c:1236: error: for each function it appears in.) make[1]: *** [libfreeswitch_la-switch_time.lo] Error 1 make: *** [all] Error 2
Hide
Permalink
Peter Olsson added a comment -
Sorry for that. Placed an ifdef wrong. Now I've tested compiling on Linux as well.

/Peter
Show
Peter Olsson added a comment - Sorry for that. Placed an ifdef wrong. Now I've tested compiling on Linux as well. /Peter
Hide
Permalink
Peter Olsson added a comment -
Jeff, this is the final version of my patch, it has three improvments from the last one;

1. Some minor cleanup

2. Added a time_sync flag, to keep track of when switch_time_sync() is called (for instance when using fsctl sync_time), and not to report errors in the main timer loop during this time, and it also stops the timer loop from calling switch_time_sync() once more if this occurs.

3. When timer tick rolls over, or when samples is greater then 32 bit integer, it now sets private_info->start to one less then private_info->reference. In the current code these cases would cause the same sample value to be sent twice. This rarely happens though, but when it happens it would mean that we loose one RTP frame (since it will send the same timestamp twice).
Show
Peter Olsson added a comment - Jeff, this is the final version of my patch, it has three improvments from the last one; 1. Some minor cleanup 2. Added a time_sync flag, to keep track of when switch_time_sync() is called (for instance when using fsctl sync_time), and not to report errors in the main timer loop during this time, and it also stops the timer loop from calling switch_time_sync() once more if this occurs. 3. When timer tick rolls over, or when samples is greater then 32 bit integer, it now sets private_info->start to one less then private_info->reference. In the current code these cases would cause the same sample value to be sent twice. This rarely happens though, but when it happens it would mean that we loose one RTP frame (since it will send the same timestamp twice).
Hide
Permalink
Peter Olsson added a comment -
Any news on this? Please add a comment if you need more information about the patch.

/Peter
Show
Peter Olsson added a comment - Any news on this? Please add a comment if you need more information about the patch. /Peter
Hide
Permalink
Jeff Lenk added a comment -
These changes look good to me. Tony will have to approve the changes which are mostly related to your item #3.
Show
Jeff Lenk added a comment - These changes look good to me. Tony will have to approve the changes which are mostly related to your item #3.
Hide
Permalink
Anthony Minessale II added a comment -
I think this is ok to try, we should keep our eye on it and get as many people to test it as possible.
Show
Anthony Minessale II added a comment - I think this is ok to try, we should keep our eye on it and get as many people to test it as possible.
Hide
Permalink
Peter Olsson added a comment -
Sounds good. Jeff, if you need any further information, please get back to me. I hope the patch still applies - if not, update the status here and I'll post a new diff for current git head.

/Peter
Show
Peter Olsson added a comment - Sounds good. Jeff, if you need any further information, please get back to me. I hope the patch still applies - if not, update the status here and I'll post a new diff for current git head. /Peter
Hide
Permalink
Peter Olsson added a comment -
Jeff/Tony - if you find this patch usable, please commit it to git. For your information I'm using this patch on both Windows and Linux (and limited testing on Mac OS X) setups, and it works as expected. On Linux it doesn't do much difference, since the timerfd is usually used anyway, but for both Windows and Mac OSX it improves the default timing quite a lot.
Show
Peter Olsson added a comment - Jeff/Tony - if you find this patch usable, please commit it to git. For your information I'm using this patch on both Windows and Linux (and limited testing on Mac OS X) setups, and it works as expected. On Linux it doesn't do much difference, since the timerfd is usually used anyway, but for both Windows and Mac OSX it improves the default timing quite a lot.
Hide
Permalink
Peter Olsson added a comment -
Updated patch to make sure it applies to latest git. Same as before, I only moved an if statement during clock sync - since it's usually only usable when using a monotonic clock. It would be great if you could apply this before 1.2 release candidates.

This patch has been tested on Windows, Linux and Mac OS X. And as I said, I've been using it since december, and it works as it should.

/Peter
Show
Peter Olsson added a comment - Updated patch to make sure it applies to latest git. Same as before, I only moved an if statement during clock sync - since it's usually only usable when using a monotonic clock. It would be great if you could apply this before 1.2 release candidates. This patch has been tested on Windows, Linux and Mac OS X. And as I said, I've been using it since december, and it works as it should. /Peter
Hide
Permalink
Git added a comment -
Repository: freeswitch
Branch: refs/heads/master
Commit: 4276680 http://fisheye.freeswitch.org/changelog/freeswitch.git/?cs=4276680
Closed By: jeff@jefflenk.com

Comment:
FS-3782 --resolve soft timer


FreeSWITCH Support Contracts and Consulting Services available!

Contact us:
Email: consulting@freeswitch.org
Web: http://www.freeswitch.org
Phone: +1-918-420-9266
Tollfree: +1-877-742-2583
Fax: +1-918-420-9267
iNum: +883 5100 1420 9266


Come To ClueCon in August to learn more about FreeSWITCH and Internet Telephony!
http://www.cluecon.com

Show
Git added a comment - Repository: freeswitch Branch: refs/heads/master Commit: 4276680 http://fisheye.freeswitch.org/changelog/freeswitch.git/?cs=4276680 Closed By: jeff@jefflenk.com Comment: FS-3782 --resolve soft timer FreeSWITCH Support Contracts and Consulting Services available! Contact us: Email: consulting@freeswitch.org Web: http://www.freeswitch.org Phone: +1-918-420-9266 Tollfree: +1-877-742-2583 Fax: +1-918-420-9267 iNum: +883 5100 1420 9266 Come To ClueCon in August to learn more about FreeSWITCH and Internet Telephony! http://www.cluecon.com
Hide
Permalink
Peter Olsson added a comment -
Thanks! :)
Show
Peter Olsson added a comment - Thanks! :)
Hide
Permalink
Auto Admin added a comment -
Due to a long period of inactivity (13 or more days), this issue is due to be automatically close within 24 hours.
If this issue is not actually resolved, please reopen it and add appropriate comments to help developers fix the issue.

Thanks,
  Jira Admin
Show
Auto Admin added a comment - Due to a long period of inactivity (13 or more days), this issue is due to be automatically close within 24 hours. If this issue is not actually resolved, please reopen it and add appropriate comments to help developers fix the issue. Thanks,   Jira Admin
Hide
Permalink
Auto Admin added a comment -
Since there has been no change to the status of this issue, it is being closed for inactivity

Thanks,
  Jira Admin


FreeSWITCH Support Contracts and Consulting Services available!

Contact us:
Email: consulting@freeswitch.org
Web: http://www.freeswitch.org
Phone: +1-918-420-9266
Tollfree: +1-877-742-2583
Fax: +1-918-420-9267
iNum: +883 5100 1420 9266


Come To ClueCon in August to learn more about FreeSWITCH and Internet Telephony!
http://www.cluecon.com

Show
Auto Admin added a comment - Since there has been no change to the status of this issue, it is being closed for inactivity Thanks,   Jira Admin FreeSWITCH Support Contracts and Consulting Services available! Contact us: Email: consulting@freeswitch.org Web: http://www.freeswitch.org Phone: +1-918-420-9266 Tollfree: +1-877-742-2583 Fax: +1-918-420-9267 iNum: +883 5100 1420 9266 Come To ClueCon in August to learn more about FreeSWITCH and Internet Telephony! http://www.cluecon.com

People

  • Assignee:
    Anthony Minessale II
    Reporter:
    Peter Olsson
Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: