Sync module: Use std locking primitives instead of boost ones #11199

pull danra wants to merge 1 commits into bitcoin:master from danra:refactor/sync-boost-to-std changing 4 files +37 −31

danra commented at 7:47 pm on August 30, 2017: contributor

Migrate the sync module to use the std-defined (since C++11) condition_variable, mutex and recursive_mutex instead of the boost provided ones.

Boost locking primitives are still used elsewhere in the project. This commit modifies just the sync module itself and the minimum code required to compile and work with its modified interface.

A couple of files were missing boost includes and previously indirectly included them through the sync.h header - added them.

Added TODO comment about changing a polling timeout in mining.cpp to use std::chrono::stable_clock which can’t go back in time due to OS changes instead of std::chrono::system_clock which can. Not making the actual fix in this commit to maintain max compatibility with previous behavior.

Resolves #11166

Sync module: Use std locking primitives instead of boost ones

Migrate the sync module to use the std-defined (since C++11) condition_variable, mutex and recursive_mutex instead of the boost provided ones.

Boost locking primitives are still used elsewhere in the project. This commit modifies just the sync module itself and the minimum code required to compile and work with its modified interface.

A couple of files were missing boost includes and previously indirectly included them through the sync.h header - added them.

Added TODO comment about changing a polling timeout in mining.cpp to use std::chrono::stable_clock which can't go back in time due to OS changes instead of std::chrono::system_clock which can. Not making the actual fix in this commit to maintain max compatibility with previous behavior.

4103397fab

fanquake added the label Refactoring on Sep 3, 2017
sipa commented at 0:26 am on September 4, 2017: member

@theuni Care to have a look?
fanquake added this to the "In progress" column in a project
TheBlueMatt commented at 8:12 pm on September 4, 2017: member

Better to use our sync.h wrappers instead of std::* directly as our wrappers provide for DEBUG_LOCKORDER checks and will hopefully get automated clang locking checks on Travis soon (see commit https://github.com/bitcoin/bitcoin/commit/f01ea9326bc69d4f731a5904881740308c5e4271 in #10866).
sipa commented at 9:04 pm on September 4, 2017: member

@TheBlueMatt I think that’s an orthogonal improvement. There are some places in the codebase where sync.h wrappers are not used (either because nobody thought there was a deadlock risk, or because of performance concerns - sync.h uses recursive locks which are slower than unique locks).
TheBlueMatt commented at 9:53 pm on September 4, 2017: member

@sipa indeed, though luckily the three places boost was switched to std here have no significant performance concerns, so easier to just switch them to sync.h wrappers over switching them to std directly. Generally we should be moving towards a “sync strongly preferred unless you have a really good reason to use std directly” model, though I know I’ve been guilty of not doing that in the past.
fanquake commented at 1:01 am on November 8, 2017: member

Closing now that #10866 has been merged.
fanquake closed this on Nov 8, 2017
fanquake moved this from the "In progress" to the "Later" column in a project
fanquake removed this from the "Later" column in a project
DrahtBot locked this on Sep 8, 2021

Contributors
danra sipa TheBlueMatt fanquake

Labels
Refactoring