Switched sync.{cpp,h} to std threading primitives. by tjps · Pull Request #11722 · bitcoin/bitcoin

tjps · 2017-11-18T19:38:49Z

Replaced boost threading primitives with the std equivalents.

sipa · 2017-11-18T20:06:37Z

This uses thread_local, a feature that we chose to not use before, though I don't remember why. Perhaps because we needed compatibility with GCC 4.7? Is that still the case, @laanwj @theuni

laanwj · 2017-11-19T10:17:11Z

This uses thread_local, a feature that we chose to not use before, though I don't remember why.

Given that it's only used when lock debugging is used I'd say it's acceptable.

Also FWIW I tried to build bitcoin on g++ 4.7 a while ago and it was a pain; for example the AX_CXX_COMPILE_STDCXX fails because the __cplusplus macro is not set correctly, and that's only the beginning. As it's already effectively broken and no one else seems to even notice it, I wouldn't mind bumping the requirement to g++ 4.8 wholesale. But I'm not sure that discussion belongs here.

promag

Concept ACK.

promag · 2017-11-19T12:42:40Z

src/sync.cpp


 #include <sync.h>

+#include <set>


Amazingly enough the #include tree in sync.cpp did not pull in std::set already.

practicalswift · 2017-11-20T13:02:59Z

Concept ACK

tjps · 2017-11-27T10:26:30Z

Since #11732 was resolved in favor of bumping GCC requirement to 4.8, looks like this PR is good to go?

sipa · 2017-11-27T10:35:19Z

utACK bba9bd0

theuni

thread_local was also off-limits due to missing support in osx's clang. It's been added as of XCode 8 on September 13, 2016. I'm uneasy with that as a minimum requirement.

Additionally, last time I checked this, the use of thread_local causes new symbols to be pulled in from glibc on Linux, meaning that we would lose back-compat with older versions. I'd have to re-test to see where the break lies.

That said, I think it's ok to lessen the requirement for the DEBUG_LOCKCONTENTION case, as long as there's a comment there mentioning that it's problematic for other code.

As osx is probably not the only platform where this will cause headaches, I'd also prefer to add an autoconf link test for thread_local.

TheBlueMatt · 2017-11-27T21:54:18Z

utACK bba9bd0 modulo thread_local concerns. I also think its probably OK in DEBUG_LOCKCONTENTION only even if its not on all supported platforms.

theuni · 2017-11-27T22:14:54Z

Pushed up a commit on top of this PR that adds the configure check for thread_local: theuni@d6df116 . I've verified that it works as intended for me in Linux, and breaks as intended on osx.

@tjps assuming that works for you, mind cherry-picking it here?

tjps · 2017-11-27T22:30:03Z

That looks great to me, will cherry-pick it onto this PR.

theuni

Thanks! utACK f7f7e2c.

f7f7e2c threads: add a thread_local autoconf check (Cory Fields) bba9bd0 Switched sync.{cpp,h} to std threading primitives. (Thomas Snider) Pull request description: Replaced boost threading primitives with the std equivalents. Tree-SHA512: 72d10f9e48bfcf1db87e4a88bc698ef98eba0b29fe904570391b34a6ea1ffad474b7f192e70e3588a30e448f70f244eb4ddc5f24412a0bde2b564e76274160a5

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

Note that this doesn't affect anything unless DEBUG_LOCKCONTENTION is defined. See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

Note that this doesn't affect anything unless DEBUG_LOCKCONTENTION is defined. See discussions here: - bitcoin/bitcoin#11722 (review) - bitcoin/bitcoin#13168 (comment)

f7f7e2c threads: add a thread_local autoconf check (Cory Fields) bba9bd0 Switched sync.{cpp,h} to std threading primitives. (Thomas Snider) Pull request description: Replaced boost threading primitives with the std equivalents. Tree-SHA512: 72d10f9e48bfcf1db87e4a88bc698ef98eba0b29fe904570391b34a6ea1ffad474b7f192e70e3588a30e448f70f244eb4ddc5f24412a0bde2b564e76274160a5

Switched sync.{cpp,h} to std threading primitives.

bba9bd0

fanquake added the Refactoring label Nov 18, 2017

laanwj requested a review from theuni November 19, 2017 10:33

promag reviewed Nov 19, 2017

View reviewed changes

laanwj mentioned this pull request Nov 20, 2017

RFC: bump GCC requirement to 4.8 #11732

Closed

theuni reviewed Nov 27, 2017

View reviewed changes

threads: add a thread_local autoconf check

f7f7e2c

theuni approved these changes Nov 28, 2017

View reviewed changes

laanwj merged commit f7f7e2c into bitcoin:master Nov 28, 2017

tjps deleted the tjps_sync_antiboost branch December 18, 2017 20:23

jamesob mentioned this pull request Apr 26, 2018

Use thread names in logs and deadlock diagnostics #13099

Closed

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 4, 2018

disable thread_local on unreliable platforms

3e1de02

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 13, 2018

disable thread_local on unreliable platforms

789758c

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 13, 2018

disable thread_local on unreliable platforms

b4c026b

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 14, 2018

disable thread_local on unreliable platforms

cdada4f

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 15, 2018

disable thread_local on unreliable platforms

696bdb7

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jun 20, 2018

disable thread_local on unreliable platforms

6f854f6

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jul 11, 2018

disable thread_local on unreliable platforms

99d6a1e

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Aug 20, 2018

disable thread_local on unreliable platforms

558f323

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

Sjors mentioned this pull request Dec 15, 2018

test: Make g_insecure_rand_ctx thread_local #14953

Merged

jamesob added a commit to jamesob/bitcoin that referenced this pull request Jan 17, 2019

disable thread_local on unreliable platforms

315d843

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

jamesob added a commit to jamesob/bitcoin that referenced this pull request Apr 18, 2019

disable thread_local on unreliable platforms

c1d1599

See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)

str4d mentioned this pull request Apr 12, 2021

Sync backports zcash/zcash#5057

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

tjps commented Nov 18, 2017

Uh oh!

sipa commented Nov 18, 2017

Uh oh!

laanwj commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Nov 19, 2017

Choose a reason for hiding this comment

Uh oh!

tjps Nov 19, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 20, 2017

Uh oh!

tjps commented Nov 27, 2017

Uh oh!

sipa commented Nov 27, 2017

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 27, 2017

Uh oh!

theuni commented Nov 27, 2017

Uh oh!

tjps commented Nov 27, 2017

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

laanwj commented Nov 19, 2017 •

edited

Loading