util: Replace boost sleep with std sleep #16117
Conversation
faa8ccf to
1bbbbb3
Compare
|
Removed unused configure checks |
|
Are the std:: versions compatible with our use of boost threadgroup interrupts? EDIT: ping @theuni |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Oh crap I knew there was a reason we hadn't got rid of the boost::sleep madness yet 😢 I don't think the I think the non-boost approach would be to have them timed-wait on a condition variable. |
There is a very observable effect of this PR on 32-bit systems: Prior to this PR After this PR |
|
Perhaps we should add a test making sure Related: RFC: Improving testing under the remaining supported 32 bit platforms |
|
How do other programs handle this? I though Linux switched to 64 bit timer values on 32-bit systems internally a while ago (and Linux is the only 32 bit platform supported), but I see on ARM I mean it must be possible to handle time correctly on 32-bit systems post-2038—right? Edit: no, apparently this is not possible?!? Threads from 2015 made my hopeful: https://lwn.net/Articles/643234/ but I don't think this was actually addressed yet, I think. |
Can you please explain a bit more why this happens. Be reminded that the predefined |
As long as the syscall used returns 32-bit time (since epoch) on 32-bit systems this cannot be solved no matter what integer types are used to abstract time in the standard library, right? :-) |
|
Yeah, and the you can't run the underlying system anyway. There is no need to try to figure out how to make Bitcoin Core work on a broken system. |
|
@MarcoFalke Perhaps we should start thinking about a deprecation plan for 32-bit systems (see #16096)? Even if this is 19 years away software tends to live longer than originally anticipated :-) |
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
1bbbbb3 to
facf344
Compare
|
Postmortem ACK. |
fae86c3 util: Remove unused MilliSleep (MarcoFalke) fa9af06 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke) fa4620b util: Add UnintrruptibleSleep (MarcoFalke) Pull request description: We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread` ACKs for top commit: ajtowns: ACK fae86c3 quick code review practicalswift: ACK fae86c3 -- patch looks correct sipa: Concept and code review ACK fae86c3 fanquake: ACK fae86c3 - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later. Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
Summary: bitcoin/bitcoin@fa4620b --- Partial backport of Core [[bitcoin/bitcoin#16117 | PR16117]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6517
…leSleep
Summary:
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main
thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e
's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g'
$(git grep -l MilliSleep)
-END VERIFY SCRIPT-
bitcoin/bitcoin@fa9af06
---
Depends on D6517
Partial backport of Core [[bitcoin/bitcoin#16117 | PR16117]]
Test Plan:
ninja check-all
Reviewers: #bitcoin_abc, deadalnix
Reviewed By: #bitcoin_abc, deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D6518
Summary: bitcoin/bitcoin@fae86c3 --- Depends on D6518 Concludes backport of Core [[bitcoin/bitcoin#16117 | PR16117]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6519
Summary: bitcoin/bitcoin@fa4620b --- Partial backport of Core [[bitcoin/bitcoin#16117 | PR16117]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6517
fae86c3 util: Remove unused MilliSleep (MarcoFalke) fa9af06 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke) fa4620b util: Add UnintrruptibleSleep (MarcoFalke) Pull request description: We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread` ACKs for top commit: ajtowns: ACK fae86c3 quick code review practicalswift: ACK fae86c3 -- patch looks correct sipa: Concept and code review ACK fae86c3 fanquake: ACK fae86c3 - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later. Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
merge bitcoin#16117, bitcoin#18358, bitcoin#17383, bitcoin#21052, bitcoin#14424, bitcoin#15159, bitcoin#14689, bitcoin#14978, partial bitcoin#16908, bitcoin#14978, bitcoin#13932: Auxillary Backports
… std equivalents in scheduler b2ac70f [Cleanup] Drop unused reverselock.h (random-zebra) 55ac6de scheduler: switch from boost to std (Anthony Towns) 15f292b sync.h: add REVERSE_LOCK (Anthony Towns) a8705a1 scheduler: don't rely on boost interrupt on shutdown (random-zebra) 004c064 util: Add UnintrruptibleSleep (MarcoFalke) Pull request description: Backports bitcoin#18234 [Anthony Towns] > Motivated by 18227, but should stand alone. Changing from boost::condition_var to std::condition_var means threadGroup.interrupt_all isn't enough to interrupt serviceQueue anymore, so that means calling stop() before join_all() is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from g_lockstack which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour. Fixes 16027, Fixes 14200, Fixes 18227 First commit coming from bitcoin#16117 This should fix #2515 ACKs for top commit: furszy: code review ACK b2ac70f Fuzzbawls: utACK b2ac70f Tree-SHA512: 5a39f8047948d02f75bb3cce99b8a9ce8d31e30c2e12a8ebfe2a871ed8e0fbe552c6e9deaad5d3e9c0830e7286f2cb627f0605d5887c4407f91cc2f9d667e8b1
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in
std::thread