Skip to content

Conversation

@theuni
Copy link
Member

@theuni theuni commented Mar 30, 2017

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.

@fanquake fanquake added the Bug label Mar 30, 2017
Copy link
Member

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.

Copy link
Member

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());

Copy link
Member Author

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.

@JeremyRubin
Copy link
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 theuni force-pushed the fix-scheduler-millisecs branch from a247894 to e025246 Compare March 31, 2017 15:54
@theuni
Copy link
Member Author

theuni commented Mar 31, 2017

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
Copy link
Member

laanwj commented Apr 1, 2017

utACK e025246

@laanwj laanwj merged commit e025246 into bitcoin:master Apr 1, 2017
laanwj added a commit that referenced this pull request Apr 1, 2017
e025246 scheduler: fix sub-second precision with boost < 1.50 (Cory Fields)

Tree-SHA512: b9d4875406c1a2bf3cb6412d7511c24d871bfba6a2ea5ccfbbf7392f2f8850027b001b776da422fea592878da21d897b1aa56d92bc2239869055dce79fd442ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 20, 2019
… 1.50

e025246 scheduler: fix sub-second precision with boost < 1.50 (Cory Fields)

Tree-SHA512: b9d4875406c1a2bf3cb6412d7511c24d871bfba6a2ea5ccfbbf7392f2f8850027b001b776da422fea592878da21d897b1aa56d92bc2239869055dce79fd442ac
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants