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.

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

test: Make g_insecure_rand_ctx thread_local #14953

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Dec 13, 2018

Some tests might spin up several threads and FastRandomContext is not thread safe.

Fix that by giving each thread their own randomness context (as opposed to e.g. making FastRandomContext thread safe or add locks elsewhere).

Also, add the g_ prefix to it (according to developer notes), since I am touching it anyway.

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

This seems overkill, as most tests are single-threaded. The multi-threaded test could just have one RNG per thread?

By that I mean having an explicit "FastRandomContext rng(true);" in each of the threads.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

The alternative you suggest (only give each thread their own randomness context when they are multithreaded) would involve (re)writing the InsecureRandRange helpers. I'd prefer to just add the thread_local keyword unless there are observable performance regressions.

@MarcoFalke MarcoFalke added the Tests label Dec 13, 2018

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

Oh, I forgot about the wrappers for the global test RNG. Objection withdrawn.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

utACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14464 (refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) by ken2812221)
  • #14193 (validation: Add missing mempool locks by MarcoFalke)
  • #13804 (WIP: Transaction Pool Layer by MarcoFalke)

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.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 13, 2018

Merge bitcoin#14953: test: Make g_insecure_rand_ctx thread_local
faead93 test: Make g_insecure_rand_ctx thread_local (MarcoFalke)

Pull request description:

  Some tests might spin up several threads and `FastRandomContext` is not thread safe.

  Fix that by giving each thread their own randomness context (as opposed to e.g. making `FastRandomContext` thread safe or add locks elsewhere).

  Also, add the `g_` prefix to it (according to developer notes), since I am touching it anyway.

Tree-SHA512: c6b61375636dfbb2f8311efe8b47e9fe7c4f8bee9804871243f877545f3117cb6aa8556a2d9b1d1673e46e2e585b695a8ddd235b746b583c3eab962435efe2d1

@MarcoFalke MarcoFalke merged commit faead93 into bitcoin:master Dec 13, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1812-testThreadLocal branch Dec 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

No idea if this is significant, but mentioning just in case:

Thread-local support used to be optional before this (was only used in sync.h in debug mode optionally). This change ignores HAVE_THREAD_LOCAL and makes it mandatory.

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

Should those tests perhaps fail explicitly, like in sync.cpp?

#if !defined(HAVE_THREAD_LOCAL)
static_assert(false, "thread_local is not supported");
#endif

Or should we make it mandatory in ./configure?

Some earlier related discussion:

We bumped the requirement from g++ 4.7 to g++ 4.8 in #11755

@theuni wrote there:

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.

From Apple docs: "Xcode 8.1 requires a Mac running macOS 10.11.5 or later". We still support macOS 10.10. We could consider bumping that to 10.11.5, or accept that some tests won't run.

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.

So perhaps this depends on if we drop e.g. Ubuntu Trusty 14.04 in the next release.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2018

thread_local was added in C++11, which we require right now and we are about to switch to C++14 soon, so a compiler not supporting it shouldn't be a problem.

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

@MarcoFalke That reasoning seems backwards. We identify what C++ version (and features) we can use based on the platforms we want to support. I don't mean to say that this particular change is a problem, but just because we're considering C++14 does not mean we should drop support for every platform that doesn't support every feature from C++11 or C++14.

Does this mean we have now removed support for OSX 10.10 for the unit tests with this change?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

It would be easier to decide which C++11 (or C++14) features are acceptable to use when there was a guideline/process that defines compatibility requirements we want to achieve.

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

We identify what C++ version (and features) we can use based on the platforms we want to support.

It's probably a bit of both. It's easier to support older platforms if it only causes minor inconveniences.

This change indeed breaks macOS 10.10 support for the unit tests. Afaik it takes a patch with an ifdef to restore support for the majority of tests. @jonasschnelli might be able to confirm since he has VMs with older macOS versions.

MarcoFalke added a commit that referenced this pull request Dec 18, 2018

Merge #14985: test: Remove thread_local from test_bitcoin
fa61202 test: Add comment to g_insecure_rand_ctx (MarcoFalke)
fa0d3c4 test: Undo thread_local g_insecure_rand_ctx (MarcoFalke)

Pull request description:

  `thread_local` seems to be highly controversial according to the discussion in #14953, so remove it again from the tests.

  Also remove boost::thread_group in the test that uses it, since I am touching it anyway.

Tree-SHA512: 977c1f597e3cfbd0e97d0b037d998fdbc701f62e9a2f57e02dbe1727b63ae8ff478dbd9d3d6dc4ffdfa23f2058b331f04949d51f23a8f55b41ecb75f088f1cbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.