-
Notifications
You must be signed in to change notification settings - Fork 38.7k
scheduler: fix sub-second precision with boost < 1.50 #10129
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
Conversation
src/scheduler.cpp
Outdated
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 adds the current time, modulo milliseconds, to the converted time point passed in?
Please add an explanatory comment, it's far from obvious to me why this works.
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.
Ah not the current time, but also t. Makes somewhat more sense now.
What is the reason that this won't work directly?
return boost::posix_time::milliseconds(boost::chrono::duration_cast<boost::chrono::milliseconds>(t.time_since_epoch()).count());
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.
from_time_t() (which seems to be the only reasonable way we have to create the posix_time without involving clocks, strings, or locales) is seconds since the epoch, so we lose the milliseconds there. The change here grabs them off of the input time, and adds them back after creating the posix_time.
What is the reason that this won't work directly?
return boost::posix_time::milliseconds(boost::chrono::duration_cast<boost::chrono::milliseconds>(t.time_since_epoch()).count());
This would return a duration rather than a time point. It would work if it were added to a time point at the epoch, though:
return boost::posix_time::from_time_t(0) + boost::posix_time::milliseconds(boost::chrono::duration_cast<boost::chrono::milliseconds>(t.time_since_epoch()).count());
Tested both, either is fine by me. Do you have a preference?
I'll add a comment either way.
|
Is every 500 too frequent in 735d9b5 (given that it is now, as you point out, the most frequently scheduled task)? Should we also make that a second (independent of this fix)? |
a247894 to
e025246
Compare
|
Updated with @laanwj's suggestion, and added a comment. @JeremyRubin the 500msec matches the previous behavior, I'm unsure where that value came from. Any particular reason to suggest bumping it to a second, or just to reduce the overhead? |
|
utACK e025246 |
e025246 scheduler: fix sub-second precision with boost < 1.50 (Cory Fields) Tree-SHA512: b9d4875406c1a2bf3cb6412d7511c24d871bfba6a2ea5ccfbbf7392f2f8850027b001b776da422fea592878da21d897b1aa56d92bc2239869055dce79fd442ac
… 1.50 e025246 scheduler: fix sub-second precision with boost < 1.50 (Cory Fields) Tree-SHA512: b9d4875406c1a2bf3cb6412d7511c24d871bfba6a2ea5ccfbbf7392f2f8850027b001b776da422fea592878da21d897b1aa56d92bc2239869055dce79fd442ac
For systems with older boost, cpu usage has been ramped up since 735d9b5, due to a time miscalculation. This was never encountered previously because this is the first use of a sub-second repeat interval for a scheduled event.