Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: remove boost::thread_group usage #21016

Merged
merged 1 commit into from
Feb 1, 2021
Merged

refactor: remove boost::thread_group usage #21016

merged 1 commit into from
Feb 1, 2021

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jan 27, 2021 •
edited by maflcko
Loading

Post #18710, there isn't much left using boost::thread_group, so should just be able to replace it with the standard library. This also removes the last use of boost::thread_interrupted.

After this change, last piece of Boost Thread we'd be using is boost::shared_mutex. See the commentary here as to why it may be non-trivial to swap that for std::shared_mutex in the near future.

Closes #17307

maflcko
maflcko reviewed Jan 27, 2021
View reviewed changes
src/init.cpp Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 27, 2021 •
edited
Loading

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

Conflicts

Reviewers, 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.

@hebasto
Copy link
Member

hebasto commented Jan 27, 2021

This also removes the last use of boost::thread_interrupted.

Concept ACK on it.

@DrahtBot DrahtBot mentioned this pull request Jan 27, 2021
Merged
@laanwj
Copy link
Member

laanwj commented Jan 27, 2021

This also removes the last use of boost::thread_interrupted.

Concept ACK thank youuu

jonatack
jonatack reviewed Jan 27, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, nice. Debug build is clean.

src/init.cpp Outdated Show resolved Hide resolved
@DrahtBot DrahtBot mentioned this pull request Jan 27, 2021
Merged
@fanquake fanquake mentioned this pull request Jan 28, 2021
Closed
@fanquake
Copy link
Member Author

I've pushed some changes. One thing worth nothing here is that even though the CScheduler is sort of written / documented as if it will have multiple threads running serviceQueue(), the only place we actually do that is in the scheduler tests.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Jan 29, 2021
@fanquake
refactor: move load block thread into ChainstateManager
f538c25 
This is a diff of Marcos from bitcoin#19197, which probably should have just
been used at the time. After this change, and bitcoin#21016, we'll have no more
global thread(Group)s hanging out in init.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
@fanquake fanquake mentioned this pull request Jan 29, 2021
Closed
laanwj
laanwj reviewed Jan 29, 2021
View reviewed changes
src/scheduler.h Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Jan 29, 2021 •
edited
Loading

This is a straightforward change.
Code review ACK 4b45d78

Code review re-ACK dc8be12

@maflcko
Copy link
Member

maflcko commented Jan 29, 2021

review ACK dc8be12 🔁

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK dc8be12510c2fd5a809d9a82d2c14b464b5e5a3f 🔁
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjKMQv6A8rVfG8beOl8DAx4YpJxPMaqL2eEohWtkv4k+F05ZO+qDK/XdyawBQXA
CntP5MQ1zS4/C1kBYE1WIrtgGFzeRzcPOT++Ig6qvLkAgtvQfEayIcpb02GTXDHw
ZcjZrUryf8rj2cxql07QmNnn4X8f41gG2GZ5DsxJsTIYPpluiXuirEBLXLprr+DS
0Bp9eeOG8P/AssBd6oaTdhSWSPivmPk/CSqOqae+X9vPX4VA3g07qyq06QuMYxEw
D5td8Wf/mOtERTLk0PgqqTrhl0IhtcMmk156BYL58BcJtkDuo306TC7SBs8fVAkD
+lgfnvlXWyR/QZJ4EoqIFCff1Sz1XSzFQnsGkx57U1vvgGA+Q3WVC2kuwQT8ldmQ
mE+QiWUeKybZQQianBsmbbLi0G2uwIPaeK9dDKA3czCyZGdvjVg8OqbQOVqyQK+8
jXV9WTtktmV5CWjQuiPU1TM+215xbDU/JoUheDE5EHPhtki5XUUrLYWwlwYODxsT
73771cxI
=mxeP
-----END PGP SIGNATURE-----

Timestamp of file with hash d4a9b161264d0b7adbc481cbca5ceed62333c7b8ce658c5ce2fbf697aa94a943 -

@jonatack
Copy link
Member

Non-expert code review ACK dc8be12, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian

@maflcko
Copy link
Member

maflcko commented Jan 30, 2021

(edited OP to say "Closes #17307")

@laanwj laanwj merged commit d0d2565 into bitcoin:master Feb 1, 2021
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 2, 2021
@fanquake
refactor: move load block thread into ChainstateManager
be70656 
This is a diff of Marcos from bitcoin#19197, which probably should have just
been used at the time. After this change, and bitcoin#21016, we'll have no more
global thread(Group)s hanging out in init.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 2, 2021
@laanwj
Merge bitcoin#21016: refactor: remove boost::thread_group usage
d9e8d25
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 2, 2021
@fanquake
refactor: move load block thread into ChainstateManager
e3341df 
This is a diff of Marcos from bitcoin#19197, which probably should have just
been used at the time. After this change, and bitcoin#21016, we'll have no more
global thread(Group)s hanging out in init.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
@fanquake fanquake deleted the remove_boost_threadgroup branch February 2, 2021 09:08
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 2, 2021
@fanquake
refactor: move load block thread into ChainstateManager
63984b3 
This is a diff of Marcos from bitcoin#19197, which probably should have just
been used at the time. After this change, and bitcoin#21016, we'll have no more
global thread(Group)s hanging out in init.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fanquake added a commit to fanquake/bitcoin that referenced this pull request Jul 12, 2021
@fanquake
init: remove straggling boost thread_group code
aa72ffb 
boost::thread_group usage was removed in bitcoin#21016.
@fanquake fanquake mentioned this pull request Jul 12, 2021
Merged
maflcko pushed a commit that referenced this pull request Jul 12, 2021
Merge #22433: init: remove straggling boost thread_group related code
7e1ba37 
aa72ffb init: remove straggling boost thread_group code (fanquake)

Pull request description:

  `boost::thread_group` was removed in #21016.

ACKs for top commit:
  MarcoFalke:
    review ACK aa72ffb

Tree-SHA512: c7ac3c2cde38fb752e0103d563b506732a403aad765a5db6be8d82399df3783044a77b071cc9c71aec3824397b04611894cf115576e63e8ee714eacf62729ab9
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 14, 2021
Merge bitcoin#22433: init: remove straggling boost thread_group relat…
a5e2e68 
…ed code

aa72ffb init: remove straggling boost thread_group code (fanquake)

Pull request description:

  `boost::thread_group` was removed in bitcoin#21016.

ACKs for top commit:
  MarcoFalke:
    review ACK aa72ffb

Tree-SHA512: c7ac3c2cde38fb752e0103d563b506732a403aad765a5db6be8d82399df3783044a77b071cc9c71aec3824397b04611894cf115576e63e8ee714eacf62729ab9
josibake pushed a commit to josibake/bitcoin that referenced this pull request Jul 21, 2021
@fanquake @josibake
init: remove straggling boost thread_group code
d052342 
boost::thread_group usage was removed in bitcoin#21016.
This was referenced Sep 9, 2021
Closed
Closed
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 4, 2022
@PiRK
init: Setup scheduler in tests and init in exactly the same way
1121f6f 
Summary:
This is a partial backport of [[bitcoin/bitcoin#19028 | core#19028]]
bitcoin/bitcoin@fa4ea99

This is backported as a minor dependency of [[bitcoin/bitcoin#21016 | core#21016]]

Test Plan:
With clang and debug:
`ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10991
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 4, 2022
@fanquake @PiRK
refactor: remove boost::thread_group usage
5d1d773 
Summary:
> Post [[bitcoin/bitcoin#18710 | core#18710]], there isn't much left using boost::thread_group, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`.
>
> After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [[bitcoin/bitcoin#16684 (comment)]] as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future.

This is a backport of [[bitcoin/bitcoin#21016 | core#21016]] and [[bitcoin/bitcoin#22433 | core#22433]]

Test Plan:
With clang + debug and (separate run) TSAN
`ninja && ninja check check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10992
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@jonatack jonatack jonatack left review comments

@maflcko maflcko maflcko left review comments

@hebasto hebasto hebasto left review comments

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop using boost::thread_group
6 participants
@fanquake @DrahtBot @hebasto @laanwj @maflcko @jonatack