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

Use a single synchronised random pool across all threads #1758

Merged
merged 5 commits into from Feb 21, 2019

Conversation

Projects
4 participants
@wezrule
Copy link
Collaborator

commented Feb 21, 2019

thread_local version seems to cause TSAN error with s_TeFilled variable inside CryptoPP

@wezrule wezrule added the sanitizers label Feb 21, 2019

@wezrule wezrule added this to the V18.0 milestone Feb 21, 2019

@wezrule wezrule self-assigned this Feb 21, 2019

@wezrule wezrule requested review from SergiySW and cryptocode Feb 21, 2019

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2019

Any areas we need to profile with this change, as only one thread can generate random data at a time?

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

store.load creates loads of threads and uses the random_pool but I'm not sure it accurately reflects what will happen on the network. I don't think it should affect it much but I haven't run any benchmarks. However I'm not sure what an appropriate alternative is currently, so not sure we have a choice? @SergiySW wants to remove the cryptopp library entirely but that will probably have to happen at a later date.

@SergiySW
Copy link
Collaborator

left a comment

Possible performance bottlenecks is only concern. Other than that LGTM

wezrule added some commits Feb 21, 2019

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

Encapsulated the random_pool functions
Moved the local static variables from not_an_account () to be a member variables as cryptocode is changing it so that multiple ledger_constants can exist.

wezrule added some commits Feb 21, 2019

@wezrule wezrule merged commit f14e723 into nanocurrency:master Feb 21, 2019

2 checks passed

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

@wezrule wezrule deleted the wezrule:single_random_pool branch Feb 21, 2019

argakiig added a commit that referenced this pull request Feb 21, 2019

Use a single synchronised random pool across all threads (#1758)
* Use a single synchronised random pool across all threads

* Encapsulate mutex

* Remove rpc_secure.cpp changes made from clang-format

* Fix qt build

* Fix nano_wallet build
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.