scheduler: fix sub-second precision with boost < 1.50 #10129

pull theuni wants to merge 1 commits into bitcoin:master from theuni:fix-scheduler-millisecs changing 1 files +3 −1

theuni commented at 8:01 PM on March 30, 2017: member

For systems with older boost, cpu usage has been ramped up since 735d9b5362aeca34c0e62006986fe9d82c24ca08, 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.
fanquake added the label Bug on Mar 30, 2017
in src/scheduler.cpp:27 in a247894261 outdated
```
  22 | @@ -23,7 +23,8 @@ CScheduler::~CScheduler()
  23 |  #if BOOST_VERSION < 105000
  24 |  static boost::system_time toPosixTime(const boost::chrono::system_clock::time_point& t)
  25 |  {
  26 | -    return boost::posix_time::from_time_t(boost::chrono::system_clock::to_time_t(t));
  27 | +    auto millis = boost::chrono::duration_cast<boost::chrono::milliseconds>(t.time_since_epoch()).count() % 1000;
  28 | +    return boost::posix_time::from_time_t(boost::chrono::system_clock::to_time_t(t)) + boost::posix_time::milliseconds(millis);
```
laanwj commented at 10:26 AM on March 31, 2017:

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.
laanwj commented at 10:55 AM on March 31, 2017:

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());
theuni commented at 3:07 PM on March 31, 2017:

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.
JeremyRubin commented at 3:07 PM on March 31, 2017: contributor

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)?
theuni force-pushed on Mar 31, 2017
scheduler: fix sub-second precision with boost < 1.50 e025246fe2
theuni commented at 5:42 PM on March 31, 2017: member

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?
laanwj commented at 10:24 AM on April 1, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10129/commits/e025246fe27f2d4bcdb6f4e9b5af9084dcc2e0a8
laanwj merged this on Apr 1, 2017
laanwj closed this on Apr 1, 2017
laanwj referenced this in commit 351d0ad404 on Apr 1, 2017
PastaPastaPasta referenced this in commit 5929a2b1ac on May 10, 2019
PastaPastaPasta referenced this in commit bc1c267f58 on May 15, 2019
PastaPastaPasta referenced this in commit b5968cd7a7 on May 20, 2019
PastaPastaPasta referenced this in commit d915eb9655 on May 21, 2019
barrystyle referenced this in commit 1685ae2563 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels