Remove unused boost/thread by maflcko · Pull Request #18758 · bitcoin/bitcoin

maflcko · 2020-04-24T15:39:56Z

There are predefined interruption points for boost::thread: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points

However, non-boost threads such as std::thread or the main() thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a boost::thread.

Most of them were accompanied by a ShutdownRequested anyway. So even if the current thread was a boost::thread, the interruption point would be redundant. (We only interrupt threads during shutdown)

hebasto · 2020-04-24T15:56:00Z

Concept ACK.

practicalswift · 2020-04-24T17:53:39Z

Concept ACK

DrahtBot · 2020-04-24T18:04:52Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)

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.

hebasto · 2020-04-27T02:30:24Z

fad6f40af271bd6094a70fea8311e5d0d18e7ae7 how about

bitcoin/src/wallet/walletdb.cpp

Lines 782 to 784 in eef90c1

    
           catch (const boost::thread_interrupted&) { 
        
               throw; 
        
           }

?

src/validation.cpp

laanwj · 2020-04-30T08:52:04Z

Concept ACK.

fanquake · 2020-06-02T14:51:29Z

Concept ACK. Not thread related, but could throw in:

diff --git a/src/init.cpp b/src/init.cpp
index 37e625129..438dc3762 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -71,9 +71,7 @@
 #include <sys/stat.h>
 #endif
 
-#include <boost/algorithm/string/classification.hpp>
 #include <boost/algorithm/string/replace.hpp>
-#include <boost/algorithm/string/split.hpp>
 #include <boost/signals2/signal.hpp>
 #include <boost/thread.hpp>

to remove some more Boost includes.

hebasto · 2020-06-02T15:24:22Z

@fanquake

Concept ACK. Not thread related, but could throw in:

diff --git a/src/init.cpp b/src/init.cpp
index 37e625129..438dc3762 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -71,9 +71,7 @@
 #include <sys/stat.h>
 #endif
 
-#include <boost/algorithm/string/classification.hpp>
 #include <boost/algorithm/string/replace.hpp>
-#include <boost/algorithm/string/split.hpp>
 #include <boost/signals2/signal.hpp>
 #include <boost/thread.hpp>

to remove some more Boost includes.

The same cleanup is done in 46838e9 from #18710. Mind reviewing it?

maflcko · 2020-06-04T14:12:55Z

Rebased and cherry-picked one commit by @hebasto

hebasto

ACK 89f9fef, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios.

fanquake

ACK 89f9fef

fad8c89: Checked that the calls to CCoinsViewDB::Upgrade and CBlockTreeDB::LoadBlockIndexGuts only happen in the [init] thread. As mentioned both of these calls were followed by a ShutdownRequested() anyways.

faa958b: Checked that txindex is run in a std::thread.

bitcoin/src/index/base.cpp

Lines 311 to 313 in 01b45b2

    
           m_thread_sync = std::thread(&TraceThread<std::function<void()>>, GetName(), 
        
                                       std::bind(&BaseIndex::ThreadSync, this));

Also followed by a call to ShutdownRequested().

maflcko · 2020-06-05T12:43:21Z

🥳

Thanks everyone for the reviews on this set! Now all boost thread interruption points are removed:

89f9fef refactor: Specify boost/thread/thread.hpp explicitly (Hennadii Stepanov) fad8c89 txdb: Remove unused boost/thread (MarcoFalke) faa958b txindex: Remove unused boost/thread (MarcoFalke) Pull request description: There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points However, non-boost threads such as `std::thread` or the `main()` thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a `boost::thread`. Most of them were accompanied by a `ShutdownRequested` anyway. So even if the current thread was a `boost::thread`, the interruption point would be redundant. (We only interrupt threads during shutdown) ACKs for top commit: fanquake: ACK 89f9fef hebasto: ACK 89f9fef, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios. Tree-SHA512: 17221dadedf2d107e5bda9e4f371cc4f8ffce6ad27cae41aa2b8f1150d8f1adf23d396585ca4a2dd25b1dc6f0d5c81fecd950d8557966ccb45a6d4a85a331d90

Summary: > txindex: Remove unused boost/thread > txdb: Remove unused boost/thread > refactor: Specify boost/thread/thread.hpp explicitly > There are predefined interruption points for boost::thread: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points > > However, non-boost threads such as std::thread or the main() thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a boost::thread. > > Most of them were accompanied by a ShutdownRequested anyway. So even if the current thread was a boost::thread, the interruption point would be redundant. (We only interrupt threads during shutdown) To make it work for our codebase, I had to add an addition include in system.cpp and core_io_tests.cpp. This is a backport of [[bitcoin/bitcoin#18758 | core#18758]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta, deadalnix Reviewed By: #bitcoin_abc, majcosta, deadalnix Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9522

maflcko added the Refactoring label Apr 24, 2020

maflcko force-pushed the 2004-noBoostThread branch from faef463 to fa8122c Compare April 24, 2020 15:52

DrahtBot mentioned this pull request Apr 24, 2020

Add local thread pool to CCheckQueue #18710

Merged

hebasto reviewed Apr 27, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

This was referenced Apr 27, 2020

init: Remove boost from ThreadImport #18786

Merged

refactor: Make CCheckQueue RAII-styled #18731

Closed

maflcko marked this pull request as draft April 29, 2020 01:38

maflcko mentioned this pull request May 26, 2020

wallet: Remove boost from PeriodicFlush #18792

Merged

maflcko force-pushed the 2004-noBoostThread branch 2 times, most recently from fae70ac to 1123846 Compare June 2, 2020 14:39

maflcko force-pushed the 2004-noBoostThread branch from 1123846 to 830c07e Compare June 2, 2020 18:33

MarcoFalke and others added 3 commits June 4, 2020 10:05

txindex: Remove unused boost/thread

faa958b

txdb: Remove unused boost/thread

fad8c89

refactor: Specify boost/thread/thread.hpp explicitly

89f9fef

maflcko force-pushed the 2004-noBoostThread branch from 830c07e to 89f9fef Compare June 4, 2020 14:06

maflcko marked this pull request as ready for review June 4, 2020 14:08

hebasto approved these changes Jun 4, 2020

View reviewed changes

fanquake approved these changes Jun 5, 2020

View reviewed changes

fanquake merged commit 4ede05d into bitcoin:master Jun 5, 2020

maflcko deleted the 2004-noBoostThread branch June 5, 2020 12:38

This was referenced Sep 9, 2021

Boost usage reduction DeFiCh/ain#714

Closed

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

str4d mentioned this pull request Dec 6, 2021

Backport upstream PRs that remove Boost usage zcash/zcash#4819

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused boost/thread#18758

Remove unused boost/thread#18758
fanquake merged 3 commits intobitcoin:masterfrom
maflcko:2004-noBoostThread

maflcko commented Apr 24, 2020 •

edited

Loading

Uh oh!

hebasto commented Apr 24, 2020

Uh oh!

practicalswift commented Apr 24, 2020

Uh oh!

DrahtBot commented Apr 24, 2020 •

edited

Loading

Uh oh!

hebasto commented Apr 27, 2020

Uh oh!

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

fanquake commented Jun 2, 2020

Uh oh!

hebasto commented Jun 2, 2020 •

edited

Loading

Uh oh!

maflcko commented Jun 4, 2020

Uh oh!

hebasto left a comment

Uh oh!

fanquake left a comment

Uh oh!

maflcko commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants


	m_thread_sync = std::thread(&TraceThread<std::function<void()>>, GetName(),
	std::bind(&BaseIndex::ThreadSync, this));

Conversation

maflcko commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 24, 2020

Uh oh!

practicalswift commented Apr 24, 2020

Uh oh!

DrahtBot commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Apr 27, 2020

Uh oh!

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

fanquake commented Jun 2, 2020

Uh oh!

hebasto commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 4, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maflcko commented Apr 24, 2020 •

edited

Loading

DrahtBot commented Apr 24, 2020 •

edited

Loading

hebasto commented Jun 2, 2020 •

edited

Loading