refactor: remove boost::thread_group usage #21016
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maflcko
reviewed
Jan 27, 2021
Contributor
|
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. |
fanquake
force-pushed
the
remove_boost_threadgroup
branch
from
11cff38 to
59d53f3
Compare
Member
Concept ACK on it. |
fanquake
force-pushed
the
remove_boost_threadgroup
branch
from
59d53f3 to
58b54e1
Compare
Member
Concept ACK thank youuu |
jonatack
reviewed
Jan 27, 2021
Member
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Concept ACK, nice. Debug build is clean.
fanquake
force-pushed
the
remove_boost_threadgroup
branch
from
58b54e1 to
4b45d78
Compare
Member
Author
|
I've pushed some changes. One thing worth nothing here is that even though the |
laanwj
reviewed
Jan 29, 2021
Member
|
This is a straightforward change. Code review re-ACK dc8be12 |
fanquake
force-pushed
the
remove_boost_threadgroup
branch
from
4b45d78 to
dc8be12
Compare
Member
|
review ACK dc8be12 🔁 Show signature and timestampSignature: Timestamp of file with hash |
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 |
Member
|
(edited OP to say "Closes #17307") |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 2, 2021
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Feb 2, 2021
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 <[email protected]>
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Jul 12, 2021
boost::thread_group usage was removed in bitcoin#21016.
maflcko
pushed a commit
that referenced
this pull request
Jul 12, 2021
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
…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
This was referenced Sep 9, 2021
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 4, 2022
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
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 [[https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696|here]] 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ofboost::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 forstd::shared_mutexin the near future.Closes #17307